Bug 816842 (CVE-2013-0746)

Quickstubs returning jsval should JS_WrapValue

RESOLVED FIXED in Firefox 18

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

({sec-high})

unspecified
mozilla20
x86
Mac OS X
sec-high
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox17- wontfix, firefox18+ fixed, firefox19+ fixed, firefox20+ fixed, firefox-esr1018+ fixed, firefox-esr1718+ fixed)

Details

(Whiteboard: [adv-main18+][adv-esr17+][adv-esr10+])

Attachments

(2 attachments)

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.
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
Created attachment 686925 [details] [diff] [review]
Clean up jsval return values in quickstubs a bit.
Attachment #686925 - Flags: review?(bobbyholley+bmo)
Note that unlike the specific issue in bug 812744 this affects 18 and 17 too.  We may want to land on those, and esr17...
Assignee: nobody → bzbarsky
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?
tracking-firefox19: --- → ?
tracking-firefox20: --- → ?
tracking-firefox-esr17: --- → ?
Whiteboard: [need review]

Comment 4

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

Updated

5 years ago
status-firefox17: --- → wontfix
tracking-firefox17: ? → -
tracking for now - still would like to get a sec-rating here.
tracking-firefox18: ? → +
tracking-firefox19: ? → +
tracking-firefox20: ? → +
status-firefox18: --- → affected
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox-esr17: --- → affected
Keywords: sec-high
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.
Attachment #686925 - Flags: review?(jorendorff)
Attachment #686925 - Flags: review?(bobbyholley+bmo)
Attachment #686925 - Flags: review+
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 on attachment 686925 [details] [diff] [review]
Clean up jsval return values in quickstubs a bit.

Peter, are you comfortable reviewing the codegen bits here?
Attachment #686925 - Flags: review?(peterv)
Attachment #686925 - Flags: review?(peterv) → review+
Attachment #686925 - Flags: review?(jorendorff) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfa4758bd564
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla20
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.
Attachment #686925 - Flags: approval-mozilla-beta?
Attachment #686925 - Flags: approval-mozilla-aurora?
Attachment #686925 - Flags: approval-mozilla-esr17?

Updated

4 years ago
tracking-firefox-esr17: ? → 18+
https://hg.mozilla.org/mozilla-central/rev/bfa4758bd564
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox20: affected → fixed
Resolution: --- → FIXED
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.
Attachment #686925 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Lukas, thanks.

http://hg.mozilla.org/releases/mozilla-aurora/rev/706b49b96b1e
status-firefox19: affected → fixed
(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?
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 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
Attachment #686925 - Flags: approval-mozilla-esr17?
Attachment #686925 - Flags: approval-mozilla-esr17+
Attachment #686925 - Flags: approval-mozilla-beta?
Attachment #686925 - Flags: approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/90d12ab3ccac
https://hg.mozilla.org/releases/mozilla-esr17/rev/69f1c373450d
status-firefox18: affected → fixed
status-firefox-esr17: affected → fixed
Is this a problem on the ESR10 branch or was it introduced later?
status-firefox-esr10: --- → ?
tracking-firefox-esr10: --- → ?
Flags: needinfo?(bzbarsky)
It's been a problem ever since we turned on jsval-returning quickstubs.  I'm pretty sure it's a problem in esr10.
Flags: needinfo?(bzbarsky)
status-firefox-esr10: ? → affected

Updated

4 years ago
tracking-firefox-esr10: ? → 18+
(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!
Created attachment 694719 [details] [diff] [review]
ESR10 patch (same as esr17 patch)
Attachment #694719 - Flags: approval-mozilla-esr10?

Updated

4 years ago
Attachment #694719 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
https://hg.mozilla.org/releases/mozilla-esr10/rev/d5d599c86132
status-firefox-esr10: affected → fixed
Whiteboard: [adv-main18+][adv-esr17+][adv-esr10+]
Alias: CVE-2013-0746
Group: core-security
You need to log in before you can comment on or make changes to this bug.