Last Comment Bug 830614 - (CVE-2013-0765) Wrapping a WebIDL object should beware WrapNativeParent reentering itself
(CVE-2013-0765)
: Wrapping a WebIDL object should beware WrapNativeParent reentering itself
Status: RESOLVED FIXED
[adv-main19+]
: csectype-uaf, sec-critical
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla21
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 824589
  Show dependency treegraph
 
Reported: 2013-01-14 19:51 PST by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2014-11-19 19:35 PST (History)
9 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
fixed
fixed
fixed
wontfix
wontfix


Attachments
Wrapping a wrappercached WebIDL object should watch out for reentry via GetParentObject. (2.65 KB, patch)
2013-01-14 19:54 PST, Boris Zbarsky [:bz] (still a bit busy)
peterv: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2013-01-14 19:51:41 PST
XPCWrappedNative::GetNewOrUsed has comments like:

540     // Take the performance hit of checking the hashtable again in case
541     // the preCreate call caused the wrapper to get created through some
542     // interesting path (the DOM code tends to make this happen sometimes).

The point is that if our parent is a node, WrapNativeParent can end up running XBL constructors for it, which can try to wrap us, manage to do that, and then unwind back to the generated Wrap() with the object already wrapped.  And possibly preserved.  And then we'll go and SetWrapper() after that, clobbering bits and triggering asserts.

I think we can just get away with checking for a cached object after WrapNativeParent and returning it.  That certainly fixes my minimal testcase, at least.

The bad news is that I think we may want this on Aurora and maybe Beta.  I ran into this while working on XULElement, but I can't guarantee that we have no cases in which some WebIDL binding is parented to a node but can be wrapped before that node.

If we _can_ prove we have no such cases, great.  Then we should open this up.  ;)
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2013-01-14 19:54:23 PST
Created attachment 702129 [details] [diff] [review]
Wrapping a wrappercached WebIDL object should watch out for reentry via GetParentObject.

Peter, do you think this is safe to not worry about on branches?
Comment 2 Andrew McCreight [:mccr8] 2013-01-14 20:39:03 PST
Nasty!

You could do:
if (JSObject* obj = aCache->GetWrapper()) {
  return obj;
}

to avoid the extra braces.  Or name obj something else. ;)
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2013-01-14 20:44:02 PST
Either way.  I'm not unhappy with the extra scope, honestly.  It makes things nice and clear.  But yeah, the "declare in the if condition" thing would work too.
Comment 4 Peter Van der Beken [:peterv] 2013-01-15 05:42:19 PST
Comment on attachment 702129 [details] [diff] [review]
Wrapping a wrappercached WebIDL object should watch out for reentry via GetParentObject.

I'd rather take this on branch too.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2013-01-15 11:06:31 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8efa257d99b
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2013-01-15 11:11:37 PST
Comment on attachment 702129 [details] [diff] [review]
Wrapping a wrappercached WebIDL object should watch out for reentry via GetParentObject.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): WebIDL bindings landing
User impact if declined: Probably nothing, but possibly a security issue in some
   corner case.
Testing completed (on m-c, etc.): Passes tests.
Risk to taking this patch (and alternatives if risky): Very very low risk.
String or UUID changes made by this patch: None.
Comment 7 Lukas Blakk [:lsblakk] use ?needinfo 2013-01-15 16:50:16 PST
Comment on attachment 702129 [details] [diff] [review]
Wrapping a wrappercached WebIDL object should watch out for reentry via GetParentObject.

Low risk, nice and early in the beta cycle, approving.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2013-01-15 19:33:11 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/3f9116d9a244
https://hg.mozilla.org/releases/mozilla-beta/rev/6176bb500beb

I'm not sure how much we care about esr17 and b2g18 here... In particular, not much is on WebIDL bindings there, iirc.
Comment 9 Ed Morley [:emorley] 2013-01-16 13:32:49 PST
https://hg.mozilla.org/mozilla-central/rev/e8efa257d99b
Comment 10 Alex Keybl [:akeybl] 2013-01-23 15:37:49 PST
(In reply to Boris Zbarsky (:bz) from comment #8)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/3f9116d9a244
> https://hg.mozilla.org/releases/mozilla-beta/rev/6176bb500beb
> 
> I'm not sure how much we care about esr17 and b2g18 here... In particular,
> not much is on WebIDL bindings there, iirc.

Who can we verify with? We'd like to keep unnecessary landings to those branches to a minimum.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2013-01-24 18:04:20 PST
So on esr17, it looks like the following are on WebIDL bindings:

CanvasRenderingContext2D, CSSStyleDeclaration, Function, EventListener, EventTarget, Performance, PerformanceNavigation, PerformanceTiming, XMLHttpRequest, XMLHttpRequestEventTarget, XMLHttpRequestUpload.

On Firefox 18 (the FIREFOX_18_0b7_RELEASE, but I would expect b2g18 is identical), it's those plus:

AudioBuffer, AudioBufferSourceNode, AudioContext, AudioDestination, AudioNode, AudioSourceNode, Blob, ClientRectList, DOMTokenList, DOMSettableTokenList, FileList, FileReaderSync, HTMLCollection, HTMLOptionsCollection, HTMLPropertiesCollection, NodeList, PaintRequestList, SVGLengthList, SVGNumberList, SVGPathSegList, SVGPointList, SVGTransformList, TextDecoder, TextEncoder, WebSocket.

Of those, the ones that have elements as native parent should be: 
CanvasRenderingContext2D, CSSStyleDeclaration, DOMTokenList, DOMSettableTokenList, HTMLCollection, HTMLOptionsCollection, HTMLPropertiesCollection, NodeList, SVG*List.

The question is whether any of those can be created before a wrapper is created for the element they use as native parent.

I'm pretty certain that can't happen for CSSStyleDeclaration or CanvasRenderingContext2D or the token lists or HTML*Collection or NodeList.  Peter, agreed?

For the SVG bits, going to defer to Robert.
Comment 12 Robert Longson 2013-01-25 10:51:14 PST
All the SVG list stuff comes from elements so you need to have a DOM wrapped element before you get to have a SVG*List

I think that answers the question for SVG.
Comment 13 Daniel Veditz [:dveditz] 2013-01-31 16:54:51 PST
I believe comment 11 is saying that if Peterv agrees with bz's assessment, and given Robert's comment 12, that we do not need this fix on the ESR-17 or b2g18 branches. Is that correct?
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2013-01-31 17:39:40 PST
That's correct.
Comment 15 Peter Van der Beken [:peterv] 2013-02-08 02:54:28 PST
(In reply to Boris Zbarsky (:bz) from comment #11)
> I'm pretty certain that can't happen for CSSStyleDeclaration or
> CanvasRenderingContext2D or the token lists or HTML*Collection or NodeList. 
> Peter, agreed?

We just discussed this in person, I agree with Boris.
Comment 16 bhavana bajaj [:bajaj] 2013-02-12 11:58:35 PST
Given comment 13-15 marking this wontfix for esr/b2g18

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