Closed Bug 482861 Opened 11 years ago Closed 11 years ago

Guaranteed use-after-free in js_FinishJSONParse

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: Waldo, Assigned: sayrer)

References

()

Details

(Keywords: crash, csectype-uaf, fixed1.9.1, Whiteboard: [sg:critical?] fixed-in-tracemonkey)

Attachments

(1 file)

Look six lines up.
Flags: blocking1.9.1?
Whiteboard: [sg:critical?]
lame, sorry. OS X debug didn't catch this. Valgrind does.
Flags: blocking1.9.1? → blocking1.9.1+
Attached patch fixSplinter Review
Attachment #366984 - Flags: review?
Attachment #366984 - Flags: review? → review?(jwalden+bmo)
Attachment #366984 - Flags: review?(jwalden+bmo) → review-
Comment on attachment 366984 [details] [diff] [review]
fix

>diff --git a/js/src/json.cpp b/js/src/json.cpp
>--- a/js/src/json.cpp
>+++ b/js/src/json.cpp
>@@ -586,23 +586,24 @@ js_FinishJSONParse(JSContext *cx, JSONPa
> 
>     if (jp->buffer)
>         js_FinishStringBuffer(jp->buffer);
>     JS_free(cx, jp->buffer);
> 
>     if (!js_RemoveRoot(cx->runtime, &jp->objectStack))
>         return JS_FALSE;
>     JSBool ok = *jp->statep == JSON_PARSE_STATE_FINISHED;
>+    jsval *v = jp->rootVal;
>     JS_free(cx, jp);
> 
>     if (!ok)
>         JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_JSON_BAD_PARSE);
> 
>     if (!JSVAL_IS_PRIMITIVE(reviver) && js_IsCallable(JSVAL_TO_OBJECT(reviver), cx))
>-        ok = Revive(cx, reviver, jp->rootVal);
>+        ok = Revive(cx, reviver, v);

jp->rootVal isn't a rooted value, is it?  In the case it's called from js_json_parse jp->rootVal is the address of the return location for the function, but in the JSAPI case it could be anything.  It looks to me like the rootVal field needs to be a root, and the value you grab here has to be rooted while revive does its work.
(In reply to comment #3)
> 
> jp->rootVal isn't a rooted value, is it?  

It's designed to be rooted by the caller.
Attachment #366984 - Flags: review- → review+
Comment on attachment 366984 [details] [diff] [review]
fix

The current way things are set up smells funny, but yeah, as discussed on IRC this is a separate bug -- please file a followup.
(In reply to comment #5)
> (From update of attachment 366984 [details] [diff] [review])
> The current way things are set up smells funny, but yeah, as discussed on IRC
> this is a separate bug -- please file a followup.

482895
http://hg.mozilla.org/tracemonkey/rev/cf269afe3d21
Whiteboard: [sg:critical?] → [sg:critical?] fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/cf269afe3d21
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Tardy nit: v is canonical name for jsval; vp for jsval*. Next time...

/be
(In reply to comment #9)
> Tardy nit: v is canonical name for jsval; vp for jsval*. Next time...

fixed in bug 482899
Depends on: 482899
hint for a test?
Every call to JSON.parse that doesn't hit a rare error condition like OOM will hit this; I found this by running xpcshell tests, in particular <http://mxr.mozilla.org/mozilla-central/source/dom/src/json/test/unit/test_reviver.js>, in a Windows debug build.
If this problem wasn't introduced in time for 3.1b3 and hadn't yet made its way into 191 (or as soon as this fix does make it in there), I think we should open it, otherwise wait for b4 to open it.  I don't know the checkin/release timelines well enough to confidently and deliberately open or not open it, tho.
(In reply to comment #12)
> Every call to JSON.parse that doesn't hit a rare error condition like OOM will
> hit this; 

Only calls to JSON.parse with the optional reviver argument will hit this.

>I found this by running xpcshell tests, in particular
> <http://mxr.mozilla.org/mozilla-central/source/dom/src/json/test/unit/test_reviver.js>,
> in a Windows debug build.

Doesn't seem to bite in opt somehow, but we should wait till b4 to open this bug, imho.
Duplicate of this bug: 483350
Flags: in-testsuite-
(In reply to comment #12)
> Every call to JSON.parse that doesn't hit a rare error condition like OOM will
> hit this; I found this by running xpcshell tests, in particular
> <http://mxr.mozilla.org/mozilla-central/source/dom/src/json/test/unit/test_reviver.js>,
> in a Windows debug build.
Can we get this on 1.9.1? Our unit test box is red :-(
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1238422048.1238425884.10078.gz
Group: core-security
Flags: wanted1.9.0.x-
You need to log in before you can comment on or make changes to this bug.