Closed Bug 429739 Opened 14 years ago Closed 14 years ago

Crash [@ js_ValueToString]

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: gkw, Assigned: igor)

References

Details

(4 keywords, Whiteboard: [sg:critical?] post 1.8-branch)

Crash Data

Attachments

(2 files, 4 obsolete files)

I found this crash in latest js opt shell, and reduced it a little. Jesse Ruderman further reduced it till

js> var o = { __noSuchMethod__: Function }; new o.y;

This crash occurs in KERN_INVALID_ADDRESS at 0x0000000080080005. at js_ValueToString.

The line causes an assertion in debug js shell as well:

Assertion failure: OBJ_GET_CLASS(cx, obj)->flags & JSCLASS_HAS_PRIVATE, at jsapi.c:2876
Flags: blocking1.9?
Assignee: general → igor
For notes:

This crash was found after running ~2-3 billion individual js testcases using jsfunfuzz.js on js opt builds after the report of bug 427191, which had been discovered after ~1 billion individual js testcases.
The reason for the bug is that in 

   var o = { __noSuchMethod__: Function }; new o.y;

Function will be called as a constructor with "this" set to an instance of "Object" class. This is bad since the Function, when called as a constructor, expects its "this" parameter to have "Function" class.

This happens because NoSuchMethod just calls js_InternalInvoke even weh called with JSINVOKE_CONSTRUCT, not js_InvokeConstructor, bypassing the "this" object initialization done by the latter.
Can we get a better explanation as to why this should block?  How often will
people hit this?  Exploitable? etc.  

Please re-nom once we have more info.

  
Flags: blocking1.9? → blocking1.9-
Since the crash address is 0x0000000080080005, I think it's likely to be exploitable.

Re-nominating blocking-1.9, unless someone thinks otherwise and feels that it should end up wanted-1.9.0.x instead.
Flags: blocking1.9- → blocking1.9?
This bug is easy to find by fuzzing, too.
(In reply to comment #4)
> Exploitable? etc.  

The consequence of the bug is writing of 5 words beyond the allocated memory when the code misrepresents JSObject as JSFunction and tries to initialize the extra JSFunction fields. The context of these words are partially controllable by a script. Since the objects are allocated sequentially from a special GC arena, most likely the overwritten bytes would belong to another object containing various pointers to function maps.

So I guess this is exploitable.

Igor, will you be able to fix this soon?  I told other browser vendors I'd be releasing the new version of jsfunfuzz around now, but I also don't want to delay Firefox 3 by turning this bug into a firedrill.
I plan to have a patch ready today.
There are other fuzzer bugs that are likely exploitable as well that we're not holding back the release for.  At this point, wouldn't hold back the release unless the exploit is public.

So, don't release any new fuzzers, Jessie!  :-)
Flags: blocking1.9? → blocking1.9-
This is the only security-sensitive dependency of the jsfunfuzz bug.
The bug is regression from bug 420399.
Blocks: 420399
+'ing this per a direct request from Brendan.  Let's see a patch and how much of a risk it is.
Flags: blocking1.9- → blocking1.9+
Depends on: 430871
I added dependancy on bug 430871 as fixing that bug simplifies the patch here.
Attached patch v1 (obsolete) — Splinter Review
[This patch depends on the patch from bug 430871 comment 1]

The essence of the patch is NoSuchMethod to call js_InvokeConstructor when the method is called in the constructor context. The rest is naming of anonymous constants and reodering js_InvokeConstructor signature to use (uintN argc, jsval *vp) ordering from the rest of code.
Attachment #316492 - Attachment is obsolete: true
Attachment #317783 - Flags: review?(brendan)
Attachment #317783 - Attachment is patch: true
Attachment #317783 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 317783 [details] [diff] [review]
v1

>     JS_ASSERT(STOBJ_GET_CLASS(obj) == &js_NoSuchMethodClass);
> 
>+    invokevp[0] = OBJ_GET_SLOT(cx, obj, JSSLOT_FOUND_FUNCTION);
>+    invokevp[1] = vp[1];
>+    invokevp[2] = OBJ_GET_SLOT(cx, obj, JSSLOT_SAVED_ID);

The js_NoSuchMethodClass instance can't leak into script, IIRC, so it doesn't need to be thread-safe. Or does it? If not, then STOBJ_GET_SLOT, etc. Oh, but those slots are within obj->fslots, so you really don't need any veneer here. I think we prefer obj->fslots[JSSLOT_XXX] for maximum clarity and explicitude.

>+    if (flags & JSINVOKE_CONSTRUCT)
>+        ok = js_InvokeConstructor(cx, 2, invokevp);
>+    else
>+        ok = js_Invoke(cx, 2, invokevp, flags);

Uber-nit: house style commons LHS using ?:, still aligning multiline consequent and alternate -- expression language ftw.

r=me with these picked or addressed somehow.

/be
Attachment #317783 - Flags: review?(brendan) → review+
Attached patch v2 (obsolete) — Splinter Review
In v1 I have early return in NoSuchMethod that bypassed a call to js_FreeStack. The new version fixes that besides addressing the nits. Here is a delta:

@@ -943,6 +943,6 @@ js_OnUnknownMethod(JSContext *cx, jsval 
             goto out;
         }
-        STOBJ_SET_SLOT(obj, JSSLOT_FOUND_FUNCTION, tvr.u.value);
-        STOBJ_SET_SLOT(obj, JSSLOT_SAVED_ID, vp[0]);
+        obj->fslots[JSSLOT_FOUND_FUNCTION] = tvr.u.value;
+        obj->fslots[JSSLOT_SAVED_ID] = vp[0];
         vp[0] = OBJECT_TO_JSVAL(obj);
     }
@@ -966,4 +966,5 @@ NoSuchMethod(JSContext *cx, uintN argc, 
         return JS_FALSE;
 
+    /* From this point the control must flow through the label out. */
     JS_ASSERT(!JSVAL_IS_PRIMITIVE(vp[0]));
     JS_ASSERT(!JSVAL_IS_PRIMITIVE(vp[1]));
@@ -971,16 +972,19 @@ NoSuchMethod(JSContext *cx, uintN argc, 
     JS_ASSERT(STOBJ_GET_CLASS(obj) == &js_NoSuchMethodClass);
 
-    invokevp[0] = OBJ_GET_SLOT(cx, obj, JSSLOT_FOUND_FUNCTION);
+    invokevp[0] = obj->fslots[JSSLOT_FOUND_FUNCTION];
     invokevp[1] = vp[1];
-    invokevp[2] = OBJ_GET_SLOT(cx, obj, JSSLOT_SAVED_ID);
+    invokevp[2] = obj->fslots[JSSLOT_SAVED_ID];
     argsobj = js_NewArrayObject(cx, argc, vp + 2);
-    if (!argsobj)
-        return JS_FALSE;
+    if (!argsobj) {
+        ok = JS_FALSE;
+        goto out;
+    }
     invokevp[3] = OBJECT_TO_JSVAL(argsobj);
-    if (flags & JSINVOKE_CONSTRUCT)
-        ok = js_InvokeConstructor(cx, 2, invokevp);
-    else
-        ok = js_Invoke(cx, 2, invokevp, flags);
+    ok = (flags & JSINVOKE_CONSTRUCT)
+         ? js_InvokeConstructor(cx, 2, invokevp)
+         : js_Invoke(cx, 2, invokevp, flags);
     vp[0] = invokevp[0];
+
+  out:
     js_FreeStack(cx, mark);
     return ok;
Attachment #317783 - Attachment is obsolete: true
Attachment #317799 - Flags: review?(brendan)
OS: Mac OS X → All
Hardware: PC → All
Comment on attachment 317799 [details] [diff] [review]
v2

Sorry, should have caught that failure to free stack on error.

>     argsobj = js_NewArrayObject(cx, argc, vp + 2);
>+    if (!argsobj) {
>+        ok = JS_FALSE;
>+        goto out;
>+    }
>+    invokevp[3] = OBJECT_TO_JSVAL(argsobj);
>+    ok = (flags & JSINVOKE_CONSTRUCT)
>+         ? js_InvokeConstructor(cx, 2, invokevp)
>+         : js_Invoke(cx, 2, invokevp, flags);
>+    vp[0] = invokevp[0];
>+
>+  out:
>+    js_FreeStack(cx, mark);
>+    return ok;

Avoid a goto, an angel gets its wings. Just else the success path and r=me.

/be
Attachment #317799 - Flags: review?(brendan) → review+
Attached patch v3 (obsolete) — Splinter Review
The new version avoids goto, here is a delta against v2:

@@ -967,3 +967,2 @@ NoSuchMethod(JSContext *cx, uintN argc, 
 
-    /* From this point the control must flow through the label out. */
     JS_ASSERT(!JSVAL_IS_PRIMITIVE(vp[0]));
@@ -979,11 +978,9 @@ NoSuchMethod(JSContext *cx, uintN argc, 
         ok = JS_FALSE;
-        goto out;
+    } else {
+        invokevp[3] = OBJECT_TO_JSVAL(argsobj);
+        ok = (flags & JSINVOKE_CONSTRUCT)
+             ? js_InvokeConstructor(cx, 2, invokevp)
+             : js_Invoke(cx, 2, invokevp, flags);
+        vp[0] = invokevp[0];
     }
-    invokevp[3] = OBJECT_TO_JSVAL(argsobj);
-    ok = (flags & JSINVOKE_CONSTRUCT)
-         ? js_InvokeConstructor(cx, 2, invokevp)
-         : js_Invoke(cx, 2, invokevp, flags);
-    vp[0] = invokevp[0];
-
-  out:
     js_FreeStack(cx, mark);
Attachment #317799 - Attachment is obsolete: true
Attachment #317807 - Flags: review+
Attachment #317807 - Attachment description: v2 → v3
Attached patch v3 (cvs diff)Splinter Review
This is a normal CVS diff that I generated after landing the bug 430871.
Attachment #317807 - Attachment is obsolete: true
Attachment #318115 - Flags: review+
Attachment #318115 - Flags: approval1.9?
Attachment #318115 - Attachment is patch: true
Attachment #318115 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 318115 [details] [diff] [review]
v3 (cvs diff)

a1.9+=damons
Attachment #318115 - Flags: approval1.9? → approval1.9+
Whiteboard: [needs landing]
I checked in the patch from the comment 20 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1209503799&maxdate=1209503844&who=igor%25mir2.org
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Flags: in-testsuite?
no need to cc me on js bugs. I watch the component and see _all_. ;-) I also regularly check fixed bugs for unset in-testsuite and add tests as they are fixed. If you see a works for me or duplicate bug that you think needs a test though, please do add in-testsuite?
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus-
v 1.9.0
Status: RESOLVED → VERIFIED
We're not planning on taking perf bug 420399 on the 1.8 branch, doesn't look like we need this fix.
Flags: wanted1.8.1.x-
Keywords: regression
Whiteboard: [sg:critical?] post 1.8-branch
Group: core-security
test checked into 1.9.0, 1.9.1, 1.9.2, tracemonkey. 1.9.3 will get picked up in the next merge.
Crash Signature: [@ js_ValueToString]
You need to log in before you can comment on or make changes to this bug.