Guaranteed use-after-free in js_FinishJSONParse

RESOLVED FIXED

Status

()

defect
--
critical
RESOLVED FIXED
10 years ago
2 years ago

People

(Reporter: Waldo, Assigned: sayrer)

Tracking

({crash, csectype-uaf, fixed1.9.1})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
wanted1.9.0.x -
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] fixed-in-tracemonkey, )

Attachments

(1 attachment)

Reporter

Description

10 years ago
Look six lines up.
Flags: blocking1.9.1?
Whiteboard: [sg:critical?]
Assignee

Comment 1

10 years ago
lame, sorry. OS X debug didn't catch this. Valgrind does.
Flags: blocking1.9.1? → blocking1.9.1+
Assignee

Comment 2

10 years ago
Posted patch fixSplinter Review
Attachment #366984 - Flags: review?
Assignee

Updated

10 years ago
Attachment #366984 - Flags: review? → review?(jwalden+bmo)
Reporter

Updated

10 years ago
Attachment #366984 - Flags: review?(jwalden+bmo) → review-
Reporter

Comment 3

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

Comment 4

10 years ago
(In reply to comment #3)
> 
> jp->rootVal isn't a rooted value, is it?  

It's designed to be rooted by the caller.
Reporter

Updated

10 years ago
Attachment #366984 - Flags: review- → review+
Reporter

Comment 5

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

Comment 6

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

Comment 7

10 years ago
http://hg.mozilla.org/tracemonkey/rev/cf269afe3d21
Whiteboard: [sg:critical?] → [sg:critical?] fixed-in-tracemonkey
Assignee

Comment 8

10 years ago
http://hg.mozilla.org/mozilla-central/rev/cf269afe3d21
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Tardy nit: v is canonical name for jsval; vp for jsval*. Next time...

/be
Assignee

Comment 10

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

Comment 11

10 years ago
hint for a test?
Reporter

Comment 12

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

Comment 13

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

Comment 14

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

Updated

10 years ago
Duplicate of this bug: 483350

Updated

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