Last Comment Bug 816842 - (CVE-2013-0746) Quickstubs returning jsval should JS_WrapValue
(CVE-2013-0746)
: Quickstubs returning jsval should JS_WrapValue
Status: RESOLVED FIXED
[adv-main18+][adv-esr17+][adv-esr10+]
: sec-high
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla20
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-29 20:19 PST by Boris Zbarsky [:bz] (Out June 25-July 6)
Modified: 2013-04-30 18:40 PDT (History)
12 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
+
fixed
+
fixed
+
fixed
18+
fixed
18+
fixed


Attachments
Clean up jsval return values in quickstubs a bit. (2.58 KB, patch)
2012-11-29 20:21 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
bobbyholley: review+
jorendorff: review+
peterv: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
bajaj.bhavana: approval‑mozilla‑beta+
bajaj.bhavana: approval‑mozilla‑esr17+
Details | Diff | Review
ESR10 patch (same as esr17 patch) (2.59 KB, patch)
2012-12-20 23:26 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
akeybl: approval‑mozilla‑esr10+
Details | Diff | Review

Description Boris Zbarsky [:bz] (Out June 25-July 6) 2012-11-29 20:19:11 PST
I ran into this in bug 812744 but didn't realize until later that this was also a problem.

Patch coming up.  Example codegen before the patch:

    nsresult rv;
    rv = self->GetOnmouseenter(cx, vp);
    if (NS_FAILED(rv))
        return xpc_qsThrowGetterSetterFailed(cx, rv, JSVAL_TO_OBJECT(*vp), id);
    return JS_TRUE;

and after:

    nsresult rv;
    jsval result;
    rv = self->GetOnmouseenter(cx, &result);
    if (NS_FAILED(rv))
        return xpc_qsThrowGetterSetterFailed(cx, rv, JSVAL_TO_OBJECT(*vp), id);
    *vp = result;
    return JS_WrapValue(cx, vp);

Note that before the patch we had two problems:

1)  If the native method threw, we passed an already-clobbered thing to the third argument of xpc_qsThrowGetterSetterFailed.  In fact, what we passed might not be an object at all, and might not be in the compartment of cx even if it is, and so forth.

2)  We never ensured that the return value is in the compartment of cx.

The patch fixes both issues.
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-11-29 20:20:27 PST
Oh, and a testcase is something like this:

  var div = document.createElement("div");
  div.onmouseenter = function() {};
  // adopt div into another document
  div.onmouseenter; // compartment mismatch assert
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-11-29 20:21:20 PST
Created attachment 686925 [details] [diff] [review]
Clean up jsval return values in quickstubs a bit.
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-11-29 20:22:45 PST
Note that unlike the specific issue in bug 812744 this affects 18 and 17 too.  We may want to land on those, and esr17...
Comment 4 Alex Keybl [:akeybl] 2012-11-30 09:51:50 PST
Hey bz - what's the security rating of this bug? sec-critical like bug 812744? Are you concerned about the discoverability of this issue in the FF17 timeframe? We've already spun up a 17.0.1 without this fix that we're releasing today, so unless this is critical, it's unlikely to be fixed until FF18.
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-11-30 10:11:30 PST
I don't know.  It's a compartment mismatch, but how critical that is is unclear to me...  Bobby?

I don't think we should hold 17.0.1 for this in any case.
Comment 6 Lukas Blakk [:lsblakk] use ?needinfo 2012-12-03 14:38:42 PST
tracking for now - still would like to get a sec-rating here.
Comment 8 Bobby Holley (busy) 2012-12-05 12:24:53 PST
Comment on attachment 686925 [details] [diff] [review]
Clean up jsval return values in quickstubs a bit.

Wrapping jsval return values in quickstubs is definitely the right thing to do here, per our previous discussion, and per the fact that XPCConvert wraps.

I don't know the quickstubs codegen that well, so jorendorff can review the other small change easier than I can.
Comment 9 Bobby Holley (busy) 2012-12-05 15:34:30 PST
Hm, based on my outstanding reviews in his queue, jorendorff is off somewhere. I can always go dig through the quickstubs code and finish the review, but it might be a bit (I'm pretty swamped right now). If there's anyone else who knows can give a 1-minute r+ that would be better.
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-12-05 19:30:01 PST
Comment on attachment 686925 [details] [diff] [review]
Clean up jsval return values in quickstubs a bit.

Peter, are you comfortable reviewing the codegen bits here?
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-12-06 13:40:11 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfa4758bd564
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-12-06 13:41:11 PST
Comment on attachment 686925 [details] [diff] [review]
Clean up jsval return values in quickstubs a bit.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Been a bug all along in this code.
User impact if declined: No known impact yet, but there's always a risk someone
   will find this.
Testing completed (on m-c, etc.): Passes automated tests, manual testing, code
   inspection.
Risk to taking this patch (and alternatives if risky): Low risk, I believe.  The
   alternative is to not take this patch at all and let it ride the trains.
String or UUID changes made by this patch: None.
Comment 13 Ed Morley [:emorley] 2012-12-07 06:48:47 PST
https://hg.mozilla.org/mozilla-central/rev/bfa4758bd564
Comment 14 Lukas Blakk [:lsblakk] use ?needinfo 2012-12-07 09:59:17 PST
Comment on attachment 686925 [details] [diff] [review]
Clean up jsval return values in quickstubs a bit.

Let's uplift to Aurora for now and get a little bake time before we assess uplift to Beta.
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-12-07 10:02:07 PST
Lukas, thanks.

http://hg.mozilla.org/releases/mozilla-aurora/rev/706b49b96b1e
Comment 16 Alex Keybl [:akeybl] 2012-12-10 16:19:19 PST
(In reply to Boris Zbarsky (:bz) from comment #12)
> Risk to taking this patch (and alternatives if risky): Low risk, I believe. 
> The
>    alternative is to not take this patch at all and let it ride the trains.

Where would you expect to see regressions, if any?
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-12-10 17:12:21 PST
The most likely place would be the quickstubbed properties that return jsval (i.e. the things whose behavior we actually changed).  A quick look suggests that these are:

nsIDOMWindow/Document/HTMLElement.onmouseenter
nsIDOMWindow/Document/HTMLElement.onmouseleave
nsIDOMDocument.onreadystatechange
nsIDOMMessageEvent.data
nsIDOMFileReader.result/onloadstart/onprogress/onload/onabort/onerror/onloadend
nsIDOMNotifyAudioAvailableEvent.frameBuffer
nsIIDBCursor.key/primaryKey
nsIIDBCursorWithValue.value
nsIIDBDatabase.onabort/onerror/onversionchange/
nsIIDBIndex.keyPath
nsIIDBKeyRange.lower/upper
nsIIDBObjectStore.keyPath
nsIIDBRequest.result/onsuccess/onerror
nsIIDBTransaction.onerror/oncomplete/onabort
nsIIDBOpenDBRequest.onblocked/onupgradeended
nsIIDBVersionChangeEvent.newVersion
nsIDOMFileHandle.onabort/onerror
nSIDOMFileRequest.onprogress
nsIDOMLockedFile.location/oncomplete/onabort/onerror

Furthermore, to really cause problems we'd have to be doing some sort of cross-compartment access, probably from chrome to content (so Xrays are involved).

So extensions touching IDB inside a webpage seems like the most likely place to look for regressions.  Not that I expect any, in this case.
Comment 18 bhavana bajaj [:bajaj] 2012-12-11 13:40:44 PST
Comment on attachment 686925 [details] [diff] [review]
Clean up jsval return values in quickstubs a bit.

Approving on beta & esr branch considering the testing completed and the limit towards expected regression's vs the user impact
Comment 20 Daniel Veditz [:dveditz] 2012-12-13 16:36:04 PST
Is this a problem on the ESR10 branch or was it introduced later?
Comment 21 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-12-13 18:21:30 PST
It's been a problem ever since we turned on jsval-returning quickstubs.  I'm pretty sure it's a problem in esr10.
Comment 22 bhavana bajaj [:bajaj] 2012-12-20 16:54:25 PST
(In reply to Boris Zbarsky (:bz) from comment #21)
> It's been a problem ever since we turned on jsval-returning quickstubs.  I'm
> pretty sure it's a problem in esr10.

bz, can you please help with the esr10 patch needed here ? Thanks!
Comment 23 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-12-20 23:26:13 PST
Created attachment 694719 [details] [diff] [review]
ESR10 patch (same as esr17 patch)
Comment 24 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-12-21 18:57:27 PST
https://hg.mozilla.org/releases/mozilla-esr10/rev/d5d599c86132

Note You need to log in before you can comment on or make changes to this bug.