830614 - (CVE-2013-0765) Wrapping a WebIDL object should beware WrapNativeParent reentering itself

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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

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

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

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

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

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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]

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

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

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

Comment 10

[Alex Keybl [:akeybl]]

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

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

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

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

That's correct.

Comment 15

[Peter Van der Beken [:peterv]]

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

Given comment 13-15 marking this wontfix for esr/b2g18