Bug 794158 (CVE-2013-0745)

Investigate if AutoWrapperChanger causes security problems

RESOLVED FIXED in Firefox 18

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

({csectype-uaf, sec-critical})

unspecified
mozilla20
x86
Linux
csectype-uaf, sec-critical
Points:
---

Firefox Tracking Flags

(firefox15 wontfix, firefox16 wontfix, firefox17+ wontfix, firefox18+ fixed, firefox19+ fixed, firefox20 fixed, firefox-esr10 unaffected, firefox-esr1718+ fixed)

Details

(Whiteboard: [adv-main18+][adv-esr17+])

Attachments

(2 attachments, 3 obsolete attachments)

AutoWrapperChanger is a class used on stack.
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedNative.cpp?rev=ae68e6c539a2&mark=1406-1441#1406

I keeps the wrapper on stack to prevent it to be GCed, but
if aCOMObj has pointers to also other JSObjects, I don't see what is keeping them alive, 
since AutoWrapperChanger's ctor calls  nsContentUtils::ReleaseWrapper which
ends up calling nsContentUtils::DropJSObjects.
AutoWrapperChanger is on stack quite long time so I believe GC can happen between
ctor and dtor.

(I hope someone proves this isn't a problem.)
That does look bad. I can take a look at it.
Assignee: nobody → continuation
s/I keeps/It keeps/
I actually misread Olli's post at first.  It isn't as bad as I was thinking at first. The wrapper itself is kept alive in the |flat| local, thanks to stack scanning.  Silly me.

So yeah, as you said, if the class keeps alive additional pointers, and it can end up in ReparentWrapperIfFound, then we can end up losing things it keeps alive. I'm not sure exactly which classes can end up as orphans. I could add a CC traversal callback thing that checks that the only JS child an object has is its wrapper.
(In reply to Andrew McCreight [:mccr8] from comment #3)
> So yeah, as you said, if the class keeps alive additional pointers, and it
> can end up in ReparentWrapperIfFound, then we can end up losing things it
> keeps alive. I'm not sure exactly which classes can end up as orphans. I
> could add a CC traversal callback thing that checks that the only JS child
> an object has is its wrapper.

smaug gave the example of mResultArrayBuffer in nsXMLHttpRequest.cpp. Is this something we're willing to forbid? If so, fine by me.
Right after releasing the wrapper could we add the native object to object holders
(call NS_HOLD_JS_OBJECTS), and then right before calling preserveWrapper call NS_DROP_JS_OBJECTS
Created attachment 664679 [details] [diff] [review]
un-compiled

Something like this.
Comment on attachment 664679 [details] [diff] [review]
un-compiled

Andrew, what you thing of this approach.
The idea is to make sure xpconnect knows about stuff it needs to trace.
Attachment #664679 - Flags: feedback?(continuation)
It looks reasonable, but I need to look into why we're doing the drop in the first place.
Keywords: csec-uaf, sec-critical
Assignee: continuation → bugs
status-firefox-esr10: --- → unaffected
status-firefox15: --- → wontfix
status-firefox16: --- → wontfix
status-firefox17: --- → affected
status-firefox18: --- → affected
Attachment #664679 - Flags: feedback?(continuation) → feedback+
Comment on attachment 664679 [details] [diff] [review]
un-compiled

Maybe even review.


It is hard to find a test case for this.
Attachment #664679 - Flags: review?(continuation)
Why not call SetPreservingWrapper directly instead of adding a bool argument to give PreserveWrapper/ReleaseWrapper a different behaviour?
I guess that could be done too.
Comment on attachment 664679 [details] [diff] [review]
un-compiled

Hmm, I need to figure out what RemoveDOMExpandoObject does and whether it should
not be called either.
Attachment #664679 - Flags: review?(continuation)
status-firefox19: --- → affected
Created attachment 671255 [details] [diff] [review]
v2

But I'm not familiar with DOMExpandoObject handling.
I think we should call RemoveDOMExpandoObject, so the previous patch would be
correct, but I'm not really sure here.
Peter, you know DOMExpandoObject stuff...
Attachment #671255 - Flags: feedback?(peterv)
Peter, feedback ping
tracking-firefox17: --- → +
tracking-firefox18: --- → +
tracking-firefox19: --- → +
We looked at this again in CRITSMASH. Peter any comments?
Flags: needinfo?(peterv)
bholley> well, it seems like we have a more general issue that the DOM expando object gets lost during reparenting, no?
Sorry Peter, but I'm really not familiar with DOMExpandoObject handling.
Assignee: bugs → peterv
I'm a bit confused, this looks like it's all in code dealing with XPCWrappedNatives. The only thing that calls RegisterDOMExpandoObject is the proxy-based dom binding code, is this code also dealing with those?
Flags: needinfo?(peterv)
Assignee: peterv → bugs
(In reply to Peter Van der Beken [:peterv] from comment #18)
> I'm a bit confused, this looks like it's all in code dealing with
> XPCWrappedNatives. The only thing that calls RegisterDOMExpandoObject is the
> proxy-based dom binding code, is this code also dealing with those?

Is there ever a way that we can cause proxy-based DOM objects to get reparented? This could happen if:

1 - It were possible to adoptNode on them somehow.
2 - They could ever appear on the parent chain of something that might get fixed up as an orphan.

If not, we should just assert against them in ReparentWrapperIfFound.
(In reply to Bobby Holley (:bholley) from comment #19)
> 1 - It were possible to adoptNode on them somehow.
> 2 - They could ever appear on the parent chain of something that might get
> fixed up as an orphan.

Not right now (XPCWrappedNative::ReparentWrapperIfFound would already fail pretty badly in other ways). We might at some point, we don't have reparenting code for new DOM binding objects but I'm working on that.
Created attachment 678256 [details] [diff] [review]
patch

With assertion
Attachment #671255 - Attachment is obsolete: true
Attachment #671255 - Flags: feedback?(peterv)
Attachment #678256 - Flags: review?(peterv)
Comment on attachment 678256 [details] [diff] [review]
patch

Fun, this leaks.
Attachment #678256 - Attachment is obsolete: true
Attachment #678256 - Flags: review?(peterv)
Attachment #664679 - Attachment is obsolete: true
Wontfixing at this point since we're getting close to beta 6, but if a fix is ready before Monday please nominate since this is sec-critical.
status-firefox17: affected → wontfix
Another way to fix this would be to call the TRACE method of the native and shove all of the pointers you find into an array. That will at least root everything.
and manually root/unroot the values in the array?
That could work yes.
Comment on attachment 682230 [details] [diff] [review]
simpler approach

Doesn't leak, nor cause new assertions.
Attachment #682230 - Flags: review?(peterv)
Comment on attachment 682230 [details] [diff] [review]
simpler approach

Review of attachment 682230 [details] [diff] [review]:
-----------------------------------------------------------------

I like it.
Attachment #682230 - Flags: review?(peterv) → review+
Comment on attachment 682230 [details] [diff] [review]
simpler approach

[Security approval request comment]
How easily can the security issue be deduced from the patch?
Not easily

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The commit message will be about simplifying things - and that is what the patch does.

Which older supported branches are affected by this flaw?
The exact same issue is in release/beta/aurora/m-c,
In some form the problem is also in esr10 (Bug 731845 changed the code), but in esr10 the problem is
explicitly only about nodes, and I haven't found a node which keeps other JS stuff alive than its wrapper.
(And the wrapper isn't a problem here)

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I don't have a patch for esr10, and I think we don't need one.

How likely is this patch to cause regressions; how much testing does it need?
The patch shouldn't be regression risky.
Attachment #682230 - Flags: sec-approval?
status-firefox20: --- → affected
Comment on attachment 682230 [details] [diff] [review]
simpler approach

sec-approval+.
Attachment #682230 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/f42a83257653
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 813655
status-firefox20: affected → fixed
Target Milestone: --- → mozilla20
(In reply to Olli Pettay [:smaug] from comment #31)
> https://hg.mozilla.org/mozilla-central/rev/f42a83257653

Can we prepare uplifts for FF18 on beta and ESR17?
status-firefox-esr17: --- → affected
tracking-firefox-esr17: --- → 18+
Will prepare patches later today or tomorrow.
Comment on attachment 682230 [details] [diff] [review]
simpler approach

[Approval Request Comment]
Bug caused by (feature/regressing bug #): NA
User impact if declined: Odd GC crashes which can be security problems
Testing completed (on m-c, etc.): Landed m-c 2 weeks ago
Risk to taking this patch (and alternatives if risky): Shouldn't be risky
String or UUID changes made by this patch: NA
Attachment #682230 - Flags: approval-mozilla-aurora?
Created attachment 688422 [details] [diff] [review]
for branches
Attachment #688422 - Flags: approval-mozilla-esr17?
Attachment #688422 - Flags: approval-mozilla-beta?
Attachment #682230 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 688422 [details] [diff] [review]
for branches

Please go ahead with landing to Beta and ESR - see https://wiki.mozilla.org/Release_Management/ESR_Landing_Process if you need information on landing to ESR branch.
Attachment #688422 - Flags: approval-mozilla-esr17?
Attachment #688422 - Flags: approval-mozilla-esr17+
Attachment #688422 - Flags: approval-mozilla-beta?
Attachment #688422 - Flags: approval-mozilla-beta+
Whiteboard: [adv-main18+][adv-esr17+]
Alias: CVE-2013-0745
Group: core-security
You need to log in before you can comment on or make changes to this bug.