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

Issue 416526 link

Starred by 1 user

Issue metadata

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

Blocking:
issue 416449



Sign in to add a comment

V8 slow/fast properties confusion

Project Member Reported by jww@chromium.org, Sep 22 2014

Issue description

Exploit meta bug:  issue 416449 

A carefully constructed property and getter combination can allow an attacker to transform an object to fast properties, confusing ReplaceSlowProperty() into corrupting memory.

Summary breakdown in 3 parts:

(1) If “1” is a property and there’s also a getter for “1”, confusion occurs as SetOwnPropertyIgnoreAttributes determines it’s a property so it calls GetPropertyOrElement but GetPropertyOrElement realizes there’s a getter, and calls the getter.

(2) A property named “1” rather than an element can be created in the JSON parser. This has already been fixed in https://codereview.chromium.org/592813002/, but we may want to merge it.

(3) Runtime_DefineDataPropertyUnchecked doesn’t check if |name| is an integer. As Juri points out, this is really only a problem with the JSON bug above, but we just probably put an ASSERT_WITH_SECURITY_IMPLICATIONS in there (or whatever the V8 equivalent is).

More details, from Juri's exploit writeup:

The bug was fixed by verwaest@chromium.org in M39 V8 refactoring:
• e61f7277 - Aug 18, Rewriting SetOwnPropertyIgnoreAttributes using the LookupIterator
• 49e4aebb – Aug 21, Remove last LookupOwnRealNamedProperty usage from runtime.cc
Current stable and beta (M37, M38) are still vulnerable.

MaybeHandle<Object> Object::GetPropertyOrElement(Handle<Object> object, 
                                                 Handle<Name> name) { 
  uint32_t index; 
  Isolate* isolate = name­>GetIsolate(); 
  if (name­>AsArrayIndex(&index)) return GetElement(isolate, object, index); 
  return GetProperty(object, name); 
} 
Notice that GetPropertyOrElement() returns GetElement() if name is an integer. The main issue is 
in the following code, that sets a property of an object.
src/objects.cc:4089:
MaybeHandle<Object> JSObject::SetOwnPropertyIgnoreAttributes(
  ..
  object­>LookupOwn(name, &lookup, true);
  ..
  if (is_observed && lookup.IsProperty()) { 
    if (lookup.IsDataProperty()) { 
      old_value = Object::GetPropertyOrElement(object, name).ToHandleChecked(); 
    } 
    old_attributes = lookup.GetAttributes(); 
  } 
  ..
  switch (lookup.type()) { 
    case NORMAL: 
      ReplaceSlowProperty(object, name, value, attributes);

Object.observe() is a new JavaScript feature, which allows property changes to be observed. If the object is observed then the old value is obtained first by GetPropertyOrElement() but only if the 
property is an actual data property, not a getter.

What would happen if we had a data property named "1" and an element getter named "1"? Well, the 
lookup.IsDataProperty() check succeeds, because the "1" property is not a getter. And then 
GetPropertyOrElement() finds that "1" is actually an element and calls its getter. The getter can can 
do evil stuff, like transform slow properties to fast, so ReplaceSlowProperty() gets confused later on.

There is a problem though. How to create a property named "1"? Normally when setting a property 
with an integer name, it will become an element. At first glance it doesn't even look like we can call 
SetOwnPropertyIgnoreAttributes(obj, "1", ...). There are some ways though.
src/json­parser.h:297:
Handle<Object> JsonParser<seq_ascii>::ParseJsonObject() { 
  ...
  if (c0_ >= '0' && c0_ <= '9') { 
    ... 
    // Successfully parsed index, parse and store element. 
    JSObject::SetOwnElement(json_object, index, value, SLOPPY).Assert();
    continue;
  }
  ...
  key = ParseJsonInternalizedString(); 
  ...
  JSObject::SetOwnPropertyIgnoreAttributes( 
      json_object, key, value, NONE).Assert();

This is the JSON parser. If a key looks like a number then SetOwnElement() is called, otherwise a key 
string is parsed and SetOwnPropertyIgnoreAttributes() is called.

src/json­parser.h:584:
Handle<String> JsonParser<seq_ascii>::SlowScanJsonString(
  ...
  case 'u': { 
    uc32 value = 0; 
    for (int i = 0; i < 4; i++) { 
...
      int digit = HexValue(c0_); 
...
      value = value * 16 + digit; 
    ...
      SeqStringSet(seq_string, count++, value);

String parser handles escape sequences. So parsing JSON with a property named "\\u0031" will call 
SetOwnPropertyIgnoreAttributes(json_object, "1", ...).
There is a problem though. As the object is being built by JSON parser, we don't have a reference to it 
yet. So we can't Object.observe() it. Once the JSON parser gives us the object, we can observe it, 
but then we can't call SetOwnPropertyIgnoreAttributes(obj, "1", ...) on it anymore. How to trigger the bug? Turns out there is a way.

src/runtime.cc:5002:
RUNTIME_FUNCTION(Runtime_DefineDataPropertyUnchecked) {
  ...
  LookupResult lookup(isolate); 
  js_object­>LookupOwnRealNamedProperty(name, &lookup); 
  ...
  if (lookup.IsFound() && 
      (attr != lookup.GetAttributes() || lookup.IsPropertyCallbacks())) { 
    ...
    JSObject::SetOwnPropertyIgnoreAttributes(
        js_object, name, obj_value, attr, 

This code handles Object.defineProperty(). If the property exists and attributes are modified, then 
SetOwnPropertyIgnoreAttributes(js_object, name, ...) is called. It hasn't been checked yet 
whether name is an integer. Normally this would be fine, because integer-named properties shouldn't 
exist, but we have this JSON parser bug.
So the exploit should do:
  obj = JSON.parse('{"\\u0031": 0}')
  Object.observe(obj, function() {})
  obj.__defineGetter__(1, EvilGetter)
  Object.defineProperty(obj, "1", {writable: false})
 
Blocking: chromium:416449

Comment 2 by wfh@chromium.org, Sep 22 2014

Cc: danno@chromium.org
Labels: Security_Impact-Stable
Owner: verwa...@chromium.org
Cc: mbarbe...@chromium.org machenb...@chromium.org
Labels: -Security_Impact-Stable
Status: Assigned
V8 team, can you evaluate (1), (2), (3). Is there any more work pending on this bug.

Marty, what updates does you fuzzer need to catch this ?
Labels: Security_Impact-Stable
Cc: -infe...@chromium.org aedla@chromium.org

Comment 6 by wfh@chromium.org, Sep 22 2014

Labels: M-38
Cc: jkummerow@chromium.org
Adding sheriff.
Project Member

Comment 8 by ClusterFuzz, Sep 22 2014

Labels: -Pri-0 Pri-1

Comment 9 by jww@chromium.org, Sep 22 2014

Cc: adamk@chromium.org
Labels: Merge-Requested
Is there any work pending other than https://code.google.com/p/v8/source/detail?r=24125. If not, please mark bug as fixed and merge to m38 branch asap.
Which cl(s) are we requesting?
Cc: verwa...@chromium.org
Owner: yangguo@chromium.org
Reassigning to Yang since I'm unexpectedly OOO.
Labels: -Merge-Requested Merge-Merged Release-0-M38
Status: Fixed
#20 verwaest@chromium.org
I don't think anything needs to be done on the O.o side. It was just a simple step along the way to provoke the bug.

The JSON part (entry-point) is fixed, and Yang backmerged a CHECK (which will crash) in SetNormalizedProperty, the backend function that was used to corrupt the heap.

Reassigning to yangguo@ since I'm unexpectedly OOO.
Owner: yangguo@chromium.org 
Today (6 hours ago) Delete comment #21 yangguo@chromium.org
This has been merged back to M37 and M38 with additional CHECK to prevent slow/fast mode confusion when setting a property. Marking this as fixed.
Status: Fixed 
Today (6 hours ago) Delete comment #22 amineer@google.com
Is there a merge required here?
Labels: Merge-TBD 
Today (6 hours ago) Delete comment #23 yangguo@chromium.org
The fix has been merged to older V8 branches:
V8 3.28.71.12 (r24162) for M38
V8 3.27.34.21 (r24125) for M37
I guess the DEPS file will need to be updated to pick up the changes in Chromium.
Cc: matthewyuan@chromium.org
Matthew, what DEPS changed are needed on M38 to pick up V8 3.28.71.12 (r24162) for M38 ? Can you please do that.
I don't think a DEPS change is needed, V8 is currently pointing to the head of 3.28 branch, so as long as the change gets onto branch we should have it.
Thanks Matthew.
Project Member

Comment 17 by ClusterFuzz, Sep 24 2014

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 18 by ClusterFuzz, Dec 31 2014

Labels: -Restrict-View-SecurityNotify
Bulk update: removing view restriction from closed bugs.
Project Member

Comment 19 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 20 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

Sign in to add a comment