Bug 830614 (CVE-2013-0765)

Wrapping a WebIDL object should beware WrapNativeParent reentering itself

RESOLVED FIXED in Firefox 19

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

({csectype-uaf, sec-critical})

unspecified
mozilla21
x86
Mac OS X
csectype-uaf, sec-critical
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox18 wontfix, firefox19 fixed, firefox20 fixed, firefox21 fixed, firefox-esr17 wontfix, b2g18 wontfix)

Details

(Whiteboard: [adv-main19+])

Attachments

(1 attachment)

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
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?
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+
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox21: --- → affected
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?
status-b2g18: --- → affected
status-firefox18: --- → wontfix
status-firefox-esr17: --- → affected
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.
status-firefox19: affected → fixed
status-firefox20: affected → fixed
Keywords: csec-uaf, sec-critical

Comment 9

4 years ago
https://hg.mozilla.org/mozilla-central/rev/e8efa257d99b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox21: affected → fixed
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)

Comment 12

4 years ago
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
status-b2g18: affected → wontfix
status-firefox-esr17: affected → wontfix
Whiteboard: [adv-main19+]
Alias: CVE-2013-0765
Group: core-security
You need to log in before you can comment on or make changes to this bug.