Closed Bug 794158 (CVE-2013-0745) Opened 12 years ago Closed 12 years ago

Investigate if AutoWrapperChanger causes security problems

Categories

(Core :: XPConnect, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox15 --- wontfix
firefox16 --- wontfix
firefox17 + wontfix
firefox18 + fixed
firefox19 + fixed
firefox20 --- fixed
firefox-esr10 --- unaffected
firefox-esr17 18+ fixed

People

(Reporter: smaug, Assigned: smaug)

References

Details

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

Attachments

(2 files, 3 obsolete files)

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
Attached patch un-compiled (obsolete) — Splinter Review
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.
Assignee: continuation → bugs
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)
Attached patch v2 (obsolete) — Splinter Review
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
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.
Attached patch patch (obsolete) — Splinter Review
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.
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?
Comment on attachment 682230 [details] [diff] [review] simpler approach sec-approval+.
Attachment #682230 - Flags: sec-approval? → sec-approval+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → 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?
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?
Attached patch for branchesSplinter Review
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.

Attachment

General

Created:
Updated:
Size: