Last Comment Bug 488995 - Exploitable crash with watch and __defineSetter__ on nsIDOMSVGAnimatedLength::animVal
: Exploitable crash with watch and __defineSetter__ on nsIDOMSVGAnimatedLength:...
Status: VERIFIED FIXED
[sg:critical] fixed-in-tracemonkey [f...
: verified1.9.0.12, verified1.9.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-18 09:43 PDT by Lucas Adamski [:ladamski]
Modified: 2009-11-19 12:20 PST (History)
14 users (show)
mbeltzner: blocking1.9.1+
dveditz: blocking1.9.0.12+
dveditz: wanted1.9.0.x+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
exploit (no payload included) (695 bytes, text/html)
2009-04-18 09:43 PDT, Lucas Adamski [:ladamski]
no flags Details
stack from valgrind (3.64 KB, patch)
2009-04-18 10:45 PDT, David Baron :dbaron: ⌚️UTC-8
no flags Details | Diff | Splinter Review
v1 (1.68 KB, patch)
2009-04-18 17:25 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
v2 (6.40 KB, patch)
2009-04-18 18:37 PDT, Igor Bukanov
mrbkap: review-
Details | Diff | Splinter Review
v3 (6.43 KB, patch)
2009-04-18 19:04 PDT, Igor Bukanov
mrbkap: review+
jst: superreview+
Details | Diff | Splinter Review
js1_5/extensions/regress-488995.js (2.69 KB, text/plain)
2009-06-03 20:13 PDT, Bob Clary [:bc:]
no flags Details
backport to 1.9.0 (6.90 KB, patch)
2009-06-19 21:27 PDT, Igor Bukanov
dveditz: approval1.9.0.12+
Details | Diff | Splinter Review
plain diff beween 1.9.1 and 1.9.0 patches (8.61 KB, text/plain)
2009-06-19 21:28 PDT, Igor Bukanov
no flags Details
backport to 1.9.0 (6.88 KB, patch)
2009-06-23 12:38 PDT, Igor Bukanov
igor: review+
samuel.sidler+old: approval1.9.0.12+
Details | Diff | Splinter Review

Description Lucas Adamski [:ladamski] 2009-04-18 09:43:07 PDT
Created attachment 373489 [details]
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-8 2009-04-18 10:45:05 PDT
Created attachment 373496 [details] [diff] [review]
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-8 2009-04-18 10:46:19 PDT
(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 2009-04-18 14:59:25 PDT
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 2009-04-18 16:35:52 PDT
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 2009-04-18 16:59:16 PDT
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 2009-04-18 17:25:54 PDT
Created attachment 373525 [details] [diff] [review]
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 2009-04-18 17:40:52 PDT
Comment on attachment 373525 [details] [diff] [review]
v1

The patch is wrong - null is not appropriate here.
Comment 8 Igor Bukanov 2009-04-18 17:59:20 PDT
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 2009-04-18 18:37:34 PDT
Created attachment 373536 [details] [diff] [review]
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) 2009-04-18 18:55:07 PDT
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 2009-04-18 19:04:12 PDT
Created attachment 373539 [details] [diff] [review]
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 2009-04-19 08:16:07 PDT
(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) 2009-04-20 12:03:03 PDT
Comment on attachment 373539 [details] [diff] [review]
v3

Looks good, thanks for tracking this down.
Comment 16 Brendan Eich [:brendan] 2009-04-22 11:17:00 PDT
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 2009-04-22 11:28:59 PDT
(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 19 Bob Clary [:bc:] 2009-06-03 20:13:42 PDT
Created attachment 381459 [details]
js1_5/extensions/regress-488995.js

should be a normal crash test but this works for now too. browser only.
Comment 20 Igor Bukanov 2009-06-19 21:27:18 PDT
Created attachment 384215 [details] [diff] [review]
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 2009-06-19 21:28:14 PDT
Created attachment 384216 [details]
plain diff beween 1.9.1 and 1.9.0 patches
Comment 22 Igor Bukanov 2009-06-19 21:30:31 PDT
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] 2009-06-22 11:45:15 PDT
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 2009-06-22 11:57:51 PDT
(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 2009-06-23 07:46:48 PDT
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 2009-06-23 08:28:58 PDT
To ss: thanks for adding fixed1.9.0.12 that I have missed.
Comment 27 Igor Bukanov 2009-06-23 12:26:26 PDT
I backed out the patch due to build failure on Windows.
Comment 28 Igor Bukanov 2009-06-23 12:38:15 PDT
Created attachment 384696 [details] [diff] [review]
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) 2009-06-23 13:14:56 PDT
Comment on attachment 384696 [details] [diff] [review]
backport to 1.9.0

Approved for 1.9.0.12. a=ss
Comment 30 Igor Bukanov 2009-06-23 13:28:22 PDT
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] 2009-06-30 12:59:26 PDT
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ý 2009-07-10 02:43:05 PDT
1.8.1 crashes (seamonkey 1.1.17), 1.8.0 survives.
Comment 33 Bob Clary [:bc:] 2009-08-18 01:59:01 PDT
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:] 2009-10-14 17:18:38 PDT
v 1.9.3, 1.9.2, 1.9.1, 1.9.0

Note You need to log in before you can comment on or make changes to this bug.