Closed Bug 830614 (CVE-2013-0765) Opened 11 years ago Closed 11 years ago

Wrapping a WebIDL object should beware WrapNativeParent reentering itself

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox18 --- wontfix
firefox19 --- fixed
firefox20 --- fixed
firefox21 --- fixed
firefox-esr17 --- wontfix
b2g18 --- wontfix

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: csectype-uaf, sec-critical, Whiteboard: [adv-main19+])

Attachments

(1 file)

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.  ;)
Group: core-security
Peter, do you think this is safe to not worry about on branches?
Attachment #702129 - Flags: review?(peterv)
Nasty!

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

to avoid the extra braces.  Or name obj something else. ;)
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 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.
Attachment #702129 - Flags: review?(peterv) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8efa257d99b
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla21
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.
Attachment #702129 - Flags: approval-mozilla-beta?
Attachment #702129 - Flags: approval-mozilla-aurora?
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.
Attachment #702129 - Flags: approval-mozilla-beta?
Attachment #702129 - Flags: approval-mozilla-beta+
Attachment #702129 - Flags: approval-mozilla-aurora?
Attachment #702129 - Flags: approval-mozilla-aurora+
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.
https://hg.mozilla.org/mozilla-central/rev/e8efa257d99b
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(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.
Flags: needinfo?(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.
Flags: needinfo?(peterv)
Flags: needinfo?(longsonr)
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(longsonr)
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?
That's correct.
(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.
Flags: needinfo?(peterv)
Given comment 13-15 marking this wontfix for esr/b2g18
Whiteboard: [adv-main19+]
Alias: CVE-2013-0765
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.