816842 - (CVE-2013-0746) Quickstubs returning jsval should JS_WrapValue

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

Attachment: Clean up jsval return values in quickstubs a bit.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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]]

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 [:bzbarsky]]

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]

tracking for now - still would like to get a sec-rating here.

Comment 8

[Bobby Holley (:bholley)]

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 (:bholley)]

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 [:bzbarsky]]

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 [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bfa4758bd564

Comment 12

[Boris Zbarsky [:bzbarsky]]

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]]

https://hg.mozilla.org/mozilla-central/rev/bfa4758bd564

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

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 [:bzbarsky]]

Lukas, thanks.

http://hg.mozilla.org/releases/mozilla-aurora/rev/706b49b96b1e

Comment 16

[Alex Keybl [:akeybl]]

(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 [:bzbarsky]]

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]]

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 19

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/releases/mozilla-beta/rev/90d12ab3ccac
https://hg.mozilla.org/releases/mozilla-esr17/rev/69f1c373450d

Comment 20

[Daniel Veditz [:dveditz]]

Is this a problem on the ESR10 branch or was it introduced later?

Comment 21

[Boris Zbarsky [:bzbarsky]]

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]]

(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 [:bzbarsky]]

Attachment: ESR10 patch (same as esr17 patch)

Comment 24

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/d5d599c86132