Closed
Bug 488995
Opened 16 years ago
Closed 16 years ago
Exploitable crash with watch and __defineSetter__ on nsIDOMSVGAnimatedLength::animVal
Categories
(Core :: JavaScript Engine, defect, P2)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
People
(Reporter: ladamski, Assigned: igor)
Details
(Keywords: verified1.9.0.12, verified1.9.1, Whiteboard: [sg:critical] fixed-in-tracemonkey [found by "PenPal"])
Attachments
(5 files, 4 obsolete files)
695 bytes,
text/html
|
Details | |
3.64 KB,
patch
|
Details | Diff | Splinter Review | |
6.43 KB,
patch
|
mrbkap
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
2.69 KB,
text/plain
|
Details | |
6.88 KB,
patch
|
igor
:
review+
samuel.sidler+old
:
approval1.9.0.12+
|
Details | Diff | Splinter Review |
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".
Reporter | ||
Updated•16 years ago
|
Whiteboard: [sg:critical]
Summary: Exploitable crash → Exploitable crash with watch and __defineSetter__ on nsIDOMSVGAnimatedLength::animVal
valgrind didn't have anything informative to say that gdb didn't. Its first relevant warning is basically the stack of the crash.
(Oh, and what gdb says is that wp->setter on line 657 of jsdbgapi.cpp isn't a healthy function pointer.)
Assignee: nobody → general
Component: General → JavaScript Engine
OS: Mac OS X → All
Product: Firefox → Core
QA Contact: general → general
Hardware: x86 → All
Assignee | ||
Updated•16 years ago
|
Assignee: general → igor
Assignee | ||
Comment 3•16 years ago
|
||
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.
Updated•16 years ago
|
Flags: wanted1.9.1?
Flags: wanted1.9.0.x?
Flags: blocking1.9.1?
Flags: blocking1.9.0.10?
Assignee | ||
Comment 4•16 years ago
|
||
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.
Assignee | ||
Comment 5•16 years ago
|
||
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.
Assignee | ||
Comment 6•16 years ago
|
||
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.
Attachment #373525 -
Flags: superreview?(mrbkap)
Attachment #373525 -
Flags: review?(jst)
Assignee | ||
Comment 7•16 years ago
|
||
Comment on attachment 373525 [details] [diff] [review]
v1
The patch is wrong - null is not appropriate here.
Assignee | ||
Updated•16 years ago
|
Attachment #373525 -
Attachment is obsolete: true
Attachment #373525 -
Flags: superreview?(mrbkap)
Attachment #373525 -
Flags: review?(jst)
Assignee | ||
Comment 8•16 years ago
|
||
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.
Assignee | ||
Comment 9•16 years ago
|
||
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.
Attachment #373536 -
Flags: superreview?(jst)
Attachment #373536 -
Flags: review?(mrbkap)
Comment 10•16 years ago
|
||
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.
Attachment #373536 -
Flags: superreview?(jst)
Attachment #373536 -
Flags: review?(mrbkap)
Attachment #373536 -
Flags: review-
Assignee | ||
Comment 11•16 years ago
|
||
The new version fixes the missing setter assignment and indentation. I will ask for a review after a try server run.
Attachment #373536 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Attachment #373539 -
Flags: superreview?(jst)
Attachment #373539 -
Flags: review?(mrbkap)
Assignee | ||
Comment 12•16 years ago
|
||
(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.
Updated•16 years ago
|
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.11?
Flags: blocking1.9.0.10?
Updated•16 years ago
|
Attachment #373539 -
Flags: review?(mrbkap) → review+
Comment 13•16 years ago
|
||
Comment on attachment 373539 [details] [diff] [review]
v3
Looks good, thanks for tracking this down.
Updated•16 years ago
|
Attachment #373539 -
Flags: superreview?(jst) → superreview+
Updated•16 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Assignee | ||
Comment 14•16 years ago
|
||
Whiteboard: [sg:critical] → [sg:critical] fixed-in-tracemonkey
Comment 15•16 years ago
|
||
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 16•16 years ago
|
||
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
Assignee | ||
Comment 17•16 years ago
|
||
(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.
Updated•16 years ago
|
Flags: wanted1.9.1?
Flags: blocking1.9.0.12?
Flags: blocking1.9.0.12+
Comment 18•16 years ago
|
||
Keywords: fixed1.9.1
Comment 19•16 years ago
|
||
should be a normal crash test but this works for now too. browser only.
Updated•16 years ago
|
Flags: in-testsuite+
Updated•16 years ago
|
Whiteboard: [sg:critical] fixed-in-tracemonkey → [sg:critical][needs 1.9.0 patch] fixed-in-tracemonkey
Assignee | ||
Comment 20•16 years ago
|
||
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.
Attachment #384215 -
Flags: approval1.9.0.12?
Assignee | ||
Comment 21•16 years ago
|
||
Assignee | ||
Comment 22•16 years ago
|
||
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.
Updated•16 years ago
|
Whiteboard: [sg:critical][needs 1.9.0 patch] fixed-in-tracemonkey → [sg:critical] fixed-in-tracemonkey
Comment 23•16 years ago
|
||
Comment on attachment 384215 [details] [diff] [review]
backport to 1.9.0
Approved for 1.9.0.12, a=dveditz for release-drivers
Attachment #384215 -
Flags: approval1.9.0.12? → approval1.9.0.12+
Assignee | ||
Comment 24•16 years ago
|
||
(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.
Assignee | ||
Comment 25•16 years ago
|
||
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
Updated•16 years ago
|
Keywords: fixed1.9.0.12
Assignee | ||
Comment 26•16 years ago
|
||
To ss: thanks for adding fixed1.9.0.12 that I have missed.
Assignee | ||
Comment 27•16 years ago
|
||
I backed out the patch due to build failure on Windows.
Keywords: fixed1.9.0.12
Assignee | ||
Comment 28•16 years ago
|
||
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.
Attachment #384215 -
Attachment is obsolete: true
Attachment #384216 -
Attachment is obsolete: true
Attachment #384696 -
Flags: review+
Assignee | ||
Updated•16 years ago
|
Attachment #384696 -
Flags: approval1.9.0.12?
Comment 29•16 years ago
|
||
Comment on attachment 384696 [details] [diff] [review]
backport to 1.9.0
Approved for 1.9.0.12. a=ss
Attachment #384696 -
Flags: approval1.9.0.12? → approval1.9.0.12+
Assignee | ||
Comment 30•16 years ago
|
||
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
Keywords: fixed1.9.0.12
Comment 31•16 years ago
|
||
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).
Keywords: fixed1.9.0.12 → verified1.9.0.12
1.8.1 crashes (seamonkey 1.1.17), 1.8.0 survives.
Updated•16 years ago
|
Whiteboard: [sg:critical] fixed-in-tracemonkey → [sg:critical] fixed-in-tracemonkey [found by "PenPal"]
Updated•16 years ago
|
Attachment #373489 -
Attachment is private: true
Updated•16 years ago
|
Group: core-security
Comment 33•15 years ago
|
||
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•15 years ago
|
||
v 1.9.3, 1.9.2, 1.9.1, 1.9.0
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Updated•15 years ago
|
Attachment #373489 -
Attachment is private: false
You need to log in
before you can comment on or make changes to this bug.
Description
•