If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Crash [@ js_ValueToString]

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: gkw, Assigned: Igor Bukanov)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
assertion, crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
wanted1.8.1.x -
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] post 1.8-branch, crash signature)

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

10 years ago
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?
(Reporter)

Comment 1

10 years ago
Created attachment 316492 [details]
stack trace from latest js shell
(Assignee)

Updated

10 years ago
Assignee: general → igor
(Reporter)

Comment 2

10 years ago
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.
(Assignee)

Comment 3

10 years ago
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.

  

Updated

10 years ago
Flags: blocking1.9? → blocking1.9-
(Reporter)

Comment 5

10 years ago
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?

Comment 6

10 years ago
This bug is easy to find by fuzzing, too.
(Assignee)

Comment 7

10 years ago
(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.

Comment 8

10 years ago
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.
(Assignee)

Comment 9

10 years ago
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-

Comment 11

10 years ago
This is the only security-sensitive dependency of the jsfunfuzz bug.
(Assignee)

Comment 12

10 years ago
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+
(Assignee)

Updated

10 years ago
Depends on: 430871
(Assignee)

Comment 14

10 years ago
I added dependancy on bug 430871 as fixing that bug simplifies the patch here.
(Assignee)

Comment 15

10 years ago
Created attachment 317783 [details] [diff] [review]
v1

[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)

Updated

10 years ago
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+
(Assignee)

Comment 17

10 years ago
Created attachment 317799 [details] [diff] [review]
v2

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)

Updated

10 years ago
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+
(Assignee)

Comment 19

10 years ago
Created attachment 317807 [details] [diff] [review]
v3

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+
(Assignee)

Updated

10 years ago
Attachment #317807 - Attachment description: v2 → v3
(Assignee)

Comment 20

10 years ago
Created attachment 318115 [details] [diff] [review]
v3 (cvs diff)

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?
(Assignee)

Updated

10 years ago
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]
(Assignee)

Comment 22

10 years ago
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
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
(Reporter)

Updated

10 years ago
Flags: in-testsuite?

Comment 23

10 years ago
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?

Comment 24

10 years ago
Created attachment 318599 [details]
js1_5/extensions/regress-429739.js

Updated

10 years ago
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus-

Comment 25

10 years ago
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

Comment 27

8 years ago
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.