Closed Bug 571014 Opened 11 years ago Closed 11 years ago

Assertion when accessing ArrayBuffer properties on non-ArrayBuffer objects

Categories

(Core :: JavaScript Engine, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: joachim.kuebart, Assigned: vlad)

Details

(Whiteboard: [sg:critical] fixed-in-tracemonkey)

Attachments

(5 files, 1 obsolete file)

ArrayBuffer::fromJSObject() asserts that it's called on objects of class ArrayBuffer, but that's incorrect when an ArrayBuffer acts as a prototype to an object of a different class (see attached test).

Possible fixes are to throw an exception (I think some Array members do that), return NULL (to treat it as unset, like on the uninitialised prototype), or to search up the prototype chain to find the ArrayBuffer that triggered the invocation in the first place...
blocking2.0: --- → ?
the third option in comment #1.
Group: core-security
This is likely exploitable. At the very least its a dos, but probably worse.
Whiteboard: [sg:critical]
Do we need the same code in the other fromJSObject?
Comment on attachment 450159 [details] [diff] [review]
Search the prototype chain for ArrayBuffer object

We don't assert non-null -- if obj goes null in the loop body you'll get a clean null+4 death in obj->getClass() on the next evaluation of the loop condition.

/be
Joachim, thanks for finding this (I certainly should have! :-().

It looks like there's a similar bug in fun_slice, which can be called on any class of object with arbitrary prototype chain members.

/be
Remove NULL-assert, relying on NULL dereference, as per comment #4.
Attachment #450159 - Attachment is obsolete: true
(In reply to comment #3)
> Do we need the same code in the other fromJSObject?

If it's used only by object ops (obj_* functions) and fun_slice, then you want something else for the object ops: assertion that we have the exact class; and for fun_slice you need a JS_InstanceOf test passing vp + 2, propagating failure.

/be
(In reply to comment #3)
> Do we need the same code in the other fromJSObject?

Yes, because TypedArray::fromJSObject() is used from, for example, TypedArray::prop_getBuffer.

This creates a JSCLASS_HAS_PRIVATE assertion when the object that has the TypedArray on the prototype chain has no private, or returns a bogus foreight private pointer otherwise.

What the right solution is in each case w.r.t comment #7 I'm not sure yet.
Just for the sake of unit testing, the assertion mentioned in comment #7 triggered in Int32Array.slice().
Attached patch like so?Splinter Review
This look right?
Assignee: general → vladimir
Ignore the "return true" -> "return false" change; it's bogus, I've put it back to "return true".
Comment on attachment 450240 [details] [diff] [review]
like so?

>@@ -823,9 +826,12 @@ class TypedArrayTemplate
>         argv = JS_ARGV(cx, vp);
>         obj = JS_THIS_OBJECT(cx, vp);
> 
>+        if (!JS_InstanceOf(cx, obj, ThisTypeArray::fastClass(), vp+2))
>+            return false;
>+
>         ThisTypeArray *tarray = ThisTypeArray::fromJSObject(obj);
>         if (!tarray)
>             return true;

The JS_InstanceOf check applies only to obj, it does not search up the proto chain for an object of the given class. So the fromJSObject is pointless, you could just reinterpret_cast obj->getPrivate(). If someone makes an object delegating to a typed array, do they want slice to operate on the proto? If so, then you really want fromJSObject to be null-defensive as it walks up, since it could walk right off. If it finds no obj of the given class then fun_slice could call JS_InstanceOf or (better) just report the same error it reports, the same way.

/be
(In reply to comment #13)
> (From update of attachment 450240 [details] [diff] [review])
> >@@ -823,9 +826,12 @@ class TypedArrayTemplate
> >         argv = JS_ARGV(cx, vp);
> >         obj = JS_THIS_OBJECT(cx, vp);
> > 
> >+        if (!JS_InstanceOf(cx, obj, ThisTypeArray::fastClass(), vp+2))
> >+            return false;
> >+
> >         ThisTypeArray *tarray = ThisTypeArray::fromJSObject(obj);
> >         if (!tarray)
> >             return true;
> 
> The JS_InstanceOf check applies only to obj, it does not search up the proto
> chain for an object of the given class. So the fromJSObject is pointless, you

I think the patch is ok as it is. The quoted code in fun_slice calls TypeArrayTemplate<>::fromJSObject, which still JS_ASSERTs that getClass() == fastClass() without walking the proto chain. It is TypedArray::fromJSObject which has the prototype walking code added because it is used from the prop_* methods.

> could just reinterpret_cast obj->getPrivate(). If someone makes an object
> delegating to a typed array, do they want slice to operate on the proto? If so,

I think it is counter-intuitive to modify the prototype with a delegated method, and since fun_slice in general probably can't do anything useful with the delegating object itself, it's best to throw an exception using JS_InstanceOf as in the above patch.

This patch works for me.

Joachim
(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 450240 [details] [diff] [review] [details])
> > >@@ -823,9 +826,12 @@ class TypedArrayTemplate
> > >         argv = JS_ARGV(cx, vp);
> > >         obj = JS_THIS_OBJECT(cx, vp);
> > > 
> > >+        if (!JS_InstanceOf(cx, obj, ThisTypeArray::fastClass(), vp+2))
> > >+            return false;
> > >+
> > >         ThisTypeArray *tarray = ThisTypeArray::fromJSObject(obj);
> > >         if (!tarray)
> > >             return true;
> > 
> > The JS_InstanceOf check applies only to obj, it does not search up the proto
> > chain for an object of the given class. So the fromJSObject is pointless, you
> 
> I think the patch is ok as it is. The quoted code in fun_slice calls
> TypeArrayTemplate<>::fromJSObject, which still JS_ASSERTs that getClass() ==
> fastClass() without walking the proto chain. It is TypedArray::fromJSObject
> which has the prototype walking code added because it is used from the prop_*
> methods.

Oh right -- thanks.

> > could just reinterpret_cast obj->getPrivate(). If someone makes an object
> > delegating to a typed array, do they want slice to operate on the proto? If so,
> 
> I think it is counter-intuitive to modify the prototype with a delegated
> method, and since fun_slice in general probably can't do anything useful with
> the delegating object itself, it's best to throw an exception using
> JS_InstanceOf as in the above patch.

fun_slice does not modify its |this| parameter, it extracts a slice. But your point above means we don't care, so the patch is good (with the return true; fix for the case of the class prototype).

/be
Comment on attachment 450240 [details] [diff] [review]
like so?

With return true fix of course.

/be
Attachment #450240 - Flags: review?(brendan) → review+
http://hg.mozilla.org/tracemonkey/rev/b1cd46b52868
Whiteboard: [sg:critical] → [sg:critical] fixed-in-tracemonkey
To keep myself from filing dups, here are the assertion failures in question.

Testcase in comment 0:

Assertion failure: obj->getClass() == &ArrayBuffer::jsclass, at ../jstypedarray.cpp:80

Testcase in comment 10:

Assertion failure: obj->getClass() == fastClass(), at ../jstypedarray.cpp:886
Object.create allows a more straightforward testcase:

var o = Object.create(new ArrayBuffer(1));
o.byteLength;
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
blocking2.0: ? → betaN+
Note that these properties have now been changed to accessors and, following WebIDL, will now throw a TypeError.
Group: core-security
You need to log in before you can comment on or make changes to this bug.