Closed
Bug 816842
(CVE-2013-0746)
Opened 12 years ago
Closed 12 years ago
Quickstubs returning jsval should JS_WrapValue
Categories
(Core :: XPConnect, defect)
Tracking
()
People
(Reporter: bzbarsky, Assigned: bzbarsky)
Details
(Keywords: sec-high, Whiteboard: [adv-main18+][adv-esr17+][adv-esr10+])
Attachments
(2 files)
2.58 KB,
patch
|
bholley
:
review+
jorendorff
:
review+
peterv
:
review+
lsblakk
:
approval-mozilla-aurora+
bajaj
:
approval-mozilla-beta+
bajaj
:
approval-mozilla-esr17+
|
Details | Diff | Splinter Review |
2.59 KB,
patch
|
akeybl
:
approval-mozilla-esr10+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
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
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #686925 -
Flags: review?(bobbyholley+bmo)
Assignee | ||
Comment 3•12 years ago
|
||
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•12 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.
Assignee | ||
Comment 5•12 years ago
|
||
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•12 years ago
|
status-firefox17:
--- → wontfix
Comment 6•12 years ago
|
||
tracking for now - still would like to get a sec-rating here.
Updated•12 years ago
|
status-firefox18:
--- → affected
status-firefox19:
--- → affected
status-firefox20:
--- → affected
status-firefox-esr17:
--- → affected
Keywords: sec-high
Comment 8•12 years ago
|
||
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+
Comment 9•12 years ago
|
||
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.
Assignee | ||
Comment 10•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #686925 -
Flags: review?(peterv) → review+
Updated•12 years ago
|
Attachment #686925 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 11•12 years ago
|
||
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla20
Assignee | ||
Comment 12•12 years ago
|
||
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?
Assignee | ||
Updated•12 years ago
|
Attachment #686925 -
Flags: approval-mozilla-esr17?
Updated•12 years ago
|
Comment 13•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 14•12 years ago
|
||
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+
Assignee | ||
Comment 15•12 years ago
|
||
Comment 16•12 years ago
|
||
(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?
Assignee | ||
Comment 17•12 years ago
|
||
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•12 years ago
|
||
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+
Assignee | ||
Comment 19•12 years ago
|
||
Comment 20•12 years ago
|
||
Is this a problem on the ESR10 branch or was it introduced later?
status-firefox-esr10:
--- → ?
tracking-firefox-esr10:
--- → ?
Updated•12 years ago
|
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 21•12 years ago
|
||
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)
Updated•12 years ago
|
Updated•12 years ago
|
Comment 22•12 years ago
|
||
(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!
Assignee | ||
Comment 23•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #694719 -
Flags: approval-mozilla-esr10?
Updated•12 years ago
|
Attachment #694719 -
Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Assignee | ||
Comment 24•12 years ago
|
||
Updated•12 years ago
|
Whiteboard: [adv-main18+][adv-esr17+][adv-esr10+]
Updated•12 years ago
|
Alias: CVE-2013-0746
Updated•12 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•