Exploitable crash with watch and __defineSetter__ on nsIDOMSVGAnimatedLength::animVal

VERIFIED FIXED

Status

()

Core
JavaScript Engine
P2
normal
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: ladamski, Assigned: Igor Bukanov)

Tracking

({verified1.9.0.12, verified1.9.1})

unspecified
verified1.9.0.12, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
blocking1.9.0.12 +
wanted1.9.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] fixed-in-tracemonkey [found by "PenPal"])

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Description

8 years ago
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".
(Reporter)

Updated

8 years ago
Whiteboard: [sg:critical]
Summary: Exploitable crash → Exploitable crash with watch and __defineSetter__ on nsIDOMSVGAnimatedLength::animVal
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.
(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

8 years ago
Assignee: general → igor
(Assignee)

Comment 3

8 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.
Flags: wanted1.9.1?
Flags: wanted1.9.0.x?
Flags: blocking1.9.1?
Flags: blocking1.9.0.10?
(Assignee)

Comment 4

8 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

8 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

8 years ago
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.
Attachment #373525 - Flags: superreview?(mrbkap)
Attachment #373525 - Flags: review?(jst)
(Assignee)

Comment 7

8 years ago
Comment on attachment 373525 [details] [diff] [review]
v1

The patch is wrong - null is not appropriate here.
(Assignee)

Updated

8 years ago
Attachment #373525 - Attachment is obsolete: true
Attachment #373525 - Flags: superreview?(mrbkap)
Attachment #373525 - Flags: review?(jst)
(Assignee)

Comment 8

8 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

8 years ago
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.
Attachment #373536 - Flags: superreview?(jst)
Attachment #373536 - Flags: review?(mrbkap)
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

8 years ago
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.
Attachment #373536 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #373539 - Flags: superreview?(jst)
Attachment #373539 - Flags: review?(mrbkap)
(Assignee)

Comment 12

8 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.
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.11?
Flags: blocking1.9.0.10?

Updated

8 years ago
Attachment #373539 - Flags: review?(mrbkap) → review+
Comment on attachment 373539 [details] [diff] [review]
v3

Looks good, thanks for tracking this down.

Updated

8 years ago
Attachment #373539 - Flags: superreview?(jst) → superreview+
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
(Assignee)

Comment 14

8 years ago
https://hg.mozilla.org/tracemonkey/rev/faf08a026ab8
Whiteboard: [sg:critical] → [sg:critical] fixed-in-tracemonkey

Comment 15

8 years ago
http://hg.mozilla.org/mozilla-central/rev/faf08a026ab8
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
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

8 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.
Flags: wanted1.9.1?
Flags: blocking1.9.0.12?
Flags: blocking1.9.0.12+

Comment 18

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2b9f8950330c
Keywords: fixed1.9.1

Comment 19

8 years ago
Created attachment 381459 [details]
js1_5/extensions/regress-488995.js

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

Updated

8 years ago
Flags: in-testsuite+
Whiteboard: [sg:critical] fixed-in-tracemonkey → [sg:critical][needs 1.9.0 patch] fixed-in-tracemonkey
(Assignee)

Comment 20

8 years ago
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.
Attachment #384215 - Flags: approval1.9.0.12?
(Assignee)

Comment 21

8 years ago
Created attachment 384216 [details]
plain diff beween 1.9.1 and 1.9.0 patches
(Assignee)

Comment 22

8 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.
Whiteboard: [sg:critical][needs 1.9.0 patch] fixed-in-tracemonkey → [sg:critical] fixed-in-tracemonkey
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

8 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

8 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
Keywords: fixed1.9.0.12
(Assignee)

Comment 26

8 years ago
To ss: thanks for adding fixed1.9.0.12 that I have missed.
(Assignee)

Comment 27

8 years ago
I backed out the patch due to build failure on Windows.
Keywords: fixed1.9.0.12
(Assignee)

Comment 28

8 years ago
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.
Attachment #384215 - Attachment is obsolete: true
Attachment #384216 - Attachment is obsolete: true
Attachment #384696 - Flags: review+
(Assignee)

Updated

8 years ago
Attachment #384696 - Flags: approval1.9.0.12?
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

8 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
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

Comment 32

8 years ago
1.8.1 crashes (seamonkey 1.1.17), 1.8.0 survives.
Whiteboard: [sg:critical] fixed-in-tracemonkey → [sg:critical] fixed-in-tracemonkey [found by "PenPal"]
Attachment #373489 - Attachment is private: true
Group: core-security

Comment 33

8 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

8 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
Attachment #373489 - Attachment is private: false
You need to log in before you can comment on or make changes to this bug.