488995 - Exploitable crash with watch and __defineSetter__ on nsIDOMSVGAnimatedLength::animVal

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Lucas Adamski [:ladamski]]

Attachment: exploit (no payload included)

I haven't had time to analyze this but I'm assured its exploitable and the underlying issue apparently is related to the watch method not SVG.  Please credit to "PenPal".

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: stack from valgrind

valgrind didn't have anything informative to say that gdb didn't.  Its first relevant warning is basically the stack of the crash.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Oh, and what gdb says is that wp->setter on line 657 of jsdbgapi.cpp isn't a healthy function pointer.)

Comment 3

[Igor Bukanov]

The example contains:

var o=document.createElementNS("http://www.w3.org/2000/svg", "svg");
var p=o.y; 
p.watch('animVal', function(id, oldvar, newval) {} );
p.type='xxx';
p.__defineSetter__('animVal', function() {});
p.animVal=0;

Here is that p.animVal has a native setter so during p.watch it will be stored in the watch point. Then a new property will be defined for p.animVal with that setter replaced by one implementing the watchpoint functionality. 

Then comes p.__defineSetter__('animValue'). That eventually calls js_DefineNativePropertyWithFlags. The function searches for the property and finds one with the watchpoint setter. It confirms that it is possible to replace the setter and calls js_AddScopeProperty to alter the property.

Now, that function contains:

/*
 * Check for a watchpoint on a deleted property; if one exists, change                                                                                             
 * setter to js_watch_set.
 * XXXbe this could get expensive with lots of watchpoints...
 */
if (!JS_CLIST_IS_EMPTY(&cx->runtime->watchPointList) &&
    js_FindWatchPoint(cx->runtime, scope, id)) {
...
    setter = js_WrapWatchedSetter(cx, id, attrs, setter);                                                                                                          
...

The comment here are somewhat misleading - the intention here is make sure that for a property with a watchpoint its setter will always be rewrapped for the watch point. 

Now, the bug happens because js_WrapWatchedSetter does not wrap the watch setter itself. Rather it provides a special setter that matches JSPROP_SETTER attribute. If it is not set, it return a native callback. If it set, it returns a function object. The latter happens in this case because __defineSetter uses JSPROP_SETTER. But after this the code does not replace the setter in the watchpoint which continues to point to the old native setter.

Thus, when p.animVal = 0 is executed, the watchpoint calls that native setter as JSPROP_SETTER one reinterpreting a pointer to a native code as JS function object.

Comment 4

[Igor Bukanov]

I was wrong in the previous comment. There is no problem with de-synchronization between JSPROP_SETTER and wp->setter as the watchpoint uses wp->sprop, not the current sprop, when reading the attributes. 

I a back to the debugger.

Comment 5

[Igor Bukanov]

The problem is that when the JS_SetWatchPoint calls js_LookupProperty, it gets sprop with missing JSPROP_SETTER attribute while the setter stored in the property is a function object. That function object matches the value stored for sprop->getter which has JSPROP_GETTER attribute. The class of that object is XPC_WN_ModsAllowed_NoCall_Proto_JSClass and it is the prototype for the object stored in p in the fragment below:

var o=document.createElementNS("http://www.w3.org/2000/svg", "svg");
var p=o.y; 
p.watch('animVal', function(id, oldvar, newval) {} );

So the bug comes from reinterpreting a pointer to GC-allocated function object as a pointer to the native code.

Comment 6

[Igor Bukanov]

Attachment: v1

The missing JSPROP_SETTER attribute comes from DefinePropertyIfFound from mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp. According to CVS blame, the bug were introduced in 2001:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp , line 472 and below.

The patch changes that code to use null for the setter argument when the attribute is not writable.

Comment 7

[Igor Bukanov]

Comment on attachment 373525 [details] [diff] [review]
v1

The patch is wrong - null is not appropriate here.

Comment 8

[Igor Bukanov]

Some more info on the need for __defineSetter__ to trigger the bug:

p.watch('animVal', function(id, oldvar, newval) {} );
p.type='xxx';
p.__defineSetter__('animVal', function() {});

The attributes for sprop here are JSPROP_ENUM | JSPROP_GETTER | JSPROP_SHARED. Since the setter is not stub one, js_SetSprop from js/src/jsscope.h will report error on any assignment to the property without calling the setter. Setting the watchpoint on the property does not change this since that preserve the attributes. So effectively the watchpoint is never invoked. 

__defineSetter__ changes this since it replaces the property with a new one having an extra JSPROP_SETTER attribute. That property also gets a watchpoint which continue to point to the old setter (an old "feature"). So now js_SetSprop on an assignment does call the watchpoint which in turn executes the setter.

Note that to fix the bug it is not sufficient to pass null or JSPropertyStub to the JS_DefinePropertyById. The former means to use the getter form the class. The later means that js_NativeSet would just ignore the write without throwing an error.

Comment 9

[Igor Bukanov]

Attachment: v2

The new patch introduces convenience stub, js_GetterOnlyPropertyStub, that is used in DefinePropertyIfFound and XPCIDispatchExtension::DefineProp when the property does not have a setter but wants to generate errors on any attempt to write to such property.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 373536 [details] [diff] [review]
v2

>Index: tm/js/src/xpconnect/src/xpcwrappednativejsops.cpp
>-    JSObject* funobj = JSVAL_TO_OBJECT(funval);
>     return JS_ValueToId(ccx, idval, &id) &&
>-           JS_DefinePropertyById(ccx, obj, id, JSVAL_VOID,
>-                                 JS_DATA_TO_FUNC_PTR(JSPropertyOp, funobj),
>-                                 JS_DATA_TO_FUNC_PTR(JSPropertyOp, funobj),
>+        JS_DefinePropertyById(ccx, obj, id, JSVAL_VOID, getter, setter,

Nit: This looks misindented.

>Index: tm/js/src/xpconnect/src/XPCIDispatchExtension.cpp
>+    JSPropertyOp getter = JS_DATA_TO_FUNC_PTR(JSPropertyOp, funobj);
>+    JSPropertyOp setter;
>     if(member->IsSetter())
>     {
>         propFlags |= JSPROP_SETTER;
>         propFlags &= ~JSPROP_READONLY;

Non-nit: don't you need to set setter here?

>     return JS_ValueToId(ccx, idval, &id) &&
>-           JS_DefinePropertyById(ccx, obj, id, JSVAL_VOID,
>-                                 JS_DATA_TO_FUNC_PTR(JSPropertyOp, funobj),
>-                                 JS_DATA_TO_FUNC_PTR(JSPropertyOp, funobj),
>+        JS_DefinePropertyById(ccx, obj, id, JSVAL_VOID, getter, setter,

Same indentation problem here.

I'll r+ the next one with that fixed.

Comment 11

[Igor Bukanov]

Attachment: v3

The new version fixes the missing setter assignment and indentation. I will ask for a review after a try server run.

Comment 12

[Igor Bukanov]

(In reply to comment #0)
> I haven't had time to analyze this but I'm assured its exploitable 

An exploit should only work without NX protection. The pointer that is reinterpreted as a native function pointer comes from the GC heap and points to a live GC thing. So if OS makes heap non-executable, the pointer dereference will lead just to a crash.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 373539 [details] [diff] [review]
v3

Looks good, thanks for tracking this down.

Comment 14

[Igor Bukanov]

https://hg.mozilla.org/tracemonkey/rev/faf08a026ab8

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/faf08a026ab8

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 373539 [details] [diff] [review]
v3

>+++ tm/js/src/jsobj.h    2009-04-19 03:24:08.000000000 +0200
>@@ -834,16 +834,22 @@ js_GetWrappedObject(JSContext *cx, JSObj
> extern const char *
> js_ComputeFilename(JSContext *cx, JSStackFrame *caller,
>                    JSPrincipals *principals, uintN *linenop);
> 
> /* Infallible, therefore cx is last parameter instead of first. */
> extern JSBool
> js_IsCallable(JSObject *obj, JSContext *cx);
> 
>+void
>+js_ReportGetterOnlyAssignment(JSContext *cx);

Any reason to lose the customary (C required? maybe only old C) extern? With C++, we can lose all externs I guess. Fodder for another bug.

/be

Comment 17

[Igor Bukanov]

(In reply to comment #16)
> >+void
> >+js_ReportGetterOnlyAssignment(JSContext *cx);
> 
> Any reason to lose the customary (C required? maybe only old C) extern?

The reason is hacking without paying attention to details :( For 1.9.0 port we should add the missing extern.

Comment 18

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2b9f8950330c

Comment 19

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-488995.js

should be a normal crash test but this works for now too. browser only.

Comment 20

[Igor Bukanov]

Attachment: backport to 1.9.0

Here is straightforward backport of the patch to 1.9.0. The differences (see the following plain diff between patches) comes from changing the SPROP_SET macro, not js_SetSprop inline, in jsscope.h, and using the (JSPropertyOp) cast, not the warning-suppressing JS_DATA_TO_FUNC_PTR macro in xpconnect files.

Given that the nature of the changes is not alter the semantic of the patch I am not asking for an extra review here.

Comment 21

[Igor Bukanov]

Attachment: plain diff beween 1.9.1 and 1.9.0 patches

Comment 22

[Igor Bukanov]

If the patch for 1.9.0 would get an approval, I could land it only on Monday as I most likely will be away for the whole weekend.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 384215 [details] [diff] [review]
backport to 1.9.0

Approved for 1.9.0.12, a=dveditz for release-drivers

Comment 24

[Igor Bukanov]

(In reply to comment #23)
> (From update of attachment 384215 [details] [diff] [review])
> Approved for 1.9.0.12, a=dveditz for release-drivers

I plan to land this tomorrow, 2009-06-23 as I do not have time for this today.

Comment 25

[Igor Bukanov]

landed to 1.9.0:

Checking in js/src/jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.483; previous revision: 3.482
done
Checking in js/src/jsobj.h;
/cvsroot/mozilla/js/src/jsobj.h,v  <--  jsobj.h
new revision: 3.105; previous revision: 3.104
done
Checking in js/src/jsscope.h;
/cvsroot/mozilla/js/src/jsscope.h,v  <--  jsscope.h
new revision: 3.62; previous revision: 3.61
done
Checking in js/src/xpconnect/src/XPCIDispatchExtension.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/XPCIDispatchExtension.cpp,v  <--  XPCIDispatchExtension.cpp
new revision: 1.21; previous revision: 1.20
done
Checking in js/src/xpconnect/src/xpcwrappednativejsops.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,v  <--  xpcwrappednativejsops.cpp
new revision: 1.89; previous revision: 1.88
done

Comment 26

[Igor Bukanov]

To ss: thanks for adding fixed1.9.0.12 that I have missed.

Comment 27

[Igor Bukanov]

I backed out the patch due to build failure on Windows.

Comment 28

[Igor Bukanov]

Attachment: backport to 1.9.0

The reason for the compilation error is the usage of the JS_DATA_TO_FUNC_PTR macro. I missed to replace it in XPCIDispatchExtension.cpp. The new patch fixes this.

Comment 29

[Samuel Sidler (old account; do not CC)]

Comment on attachment 384696 [details] [diff] [review]
backport to 1.9.0

Approved for 1.9.0.12. a=ss

Comment 30

[Igor Bukanov]

landed to 1.9.0:

Checking in js/src/jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.486; previous revision: 3.485
done
Checking in js/src/jsobj.h;
/cvsroot/mozilla/js/src/jsobj.h,v  <--  jsobj.h
new revision: 3.107; previous revision: 3.106
done
Checking in js/src/jsscope.h;
/cvsroot/mozilla/js/src/jsscope.h,v  <--  jsscope.h
new revision: 3.64; previous revision: 3.63
done
Checking in js/src/xpconnect/src/XPCIDispatchExtension.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/XPCIDispatchExtension.cpp,v  <--  XPCIDispatchExtension.cpp
new revision: 1.23; previous revision: 1.22
done
Checking in js/src/xpconnect/src/xpcwrappednativejsops.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,v  <--  xpcwrappednativejsops.cpp
new revision: 1.91; previous revision: 1.90
done

Comment 31

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.12 with testcase. Crashes 1.9.0.11 but not Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.12pre) Gecko/2009063005 GranParadiso/3.0.12pre (.NET CLR 3.5.30729).

Comment 32

[Martin Stránský [:stransky] (ni? me)]

1.8.1 crashes (seamonkey 1.1.17), 1.8.0 survives.

Comment 33

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/tracemonkey/rev/7bb71d2e16a7
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-488995.js,v  <--  regress-488995.js
initial revision: 1.1

Comment 34

[Bob Clary [:bc] (inactive)]

v 1.9.3, 1.9.2, 1.9.1, 1.9.0