Guaranteed use-after-free in js_FinishJSONParse

RESOLVED FIXED

Status

()

--
critical
RESOLVED FIXED
10 years ago
2 years ago

People

(Reporter: Waldo, Assigned: sayrer)

Tracking

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

Trunk
crash, csectype-uaf, fixed1.9.1
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, URL)

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
Created attachment 366984 [details] [diff] [review]
fix
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

Updated

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

Comment 16

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; 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-
Keywords: csectype-uaf
You need to log in before you can comment on or make changes to this bug.