The default bug view has changed. See this FAQ.
Bug 794158 (CVE-2013-0745)

Investigate if AutoWrapperChanger causes security problems

RESOLVED FIXED in Firefox 18

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
4 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)

(Assignee)

Description

5 years ago
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
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 5

5 years ago
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
(Assignee)

Comment 6

5 years ago
Created attachment 664679 [details] [diff] [review]
un-compiled

Something like this.
(Assignee)

Comment 7

5 years ago
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+
(Assignee)

Comment 9

5 years ago
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?
(Assignee)

Comment 11

5 years ago
I guess that could be done too.
(Assignee)

Comment 12

5 years ago
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)

Updated

5 years ago
status-firefox19: --- → affected
(Assignee)

Comment 13

5 years ago
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)
(Assignee)

Comment 14

5 years ago
Peter, feedback ping
tracking-firefox17: --- → +
tracking-firefox18: --- → +
tracking-firefox19: --- → +
We looked at this again in CRITSMASH. Peter any comments?
Flags: needinfo?(peterv)
(Assignee)

Comment 16

5 years ago
bholley> well, it seems like we have a more general issue that the DOM expando object gets lost during reparenting, no?
(Assignee)

Comment 17

5 years ago
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.
(Assignee)

Comment 21

4 years ago
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)
(Assignee)

Comment 22

4 years ago
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.
(Assignee)

Comment 25

4 years ago
and manually root/unroot the values in the array?
That could work yes.
(Assignee)

Comment 26

4 years ago
Created attachment 682230 [details] [diff] [review]
simpler approach
(Assignee)

Comment 27

4 years ago
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+
(Assignee)

Comment 29

4 years ago
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+
(Assignee)

Comment 31

4 years ago
https://hg.mozilla.org/mozilla-central/rev/f42a83257653
Status: NEW → RESOLVED
Last Resolved: 4 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+
(Assignee)

Comment 34

4 years ago
Will prepare patches later today or tomorrow.
(Assignee)

Comment 35

4 years ago
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?
(Assignee)

Comment 36

4 years ago
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/16b6f2a69aa5
https://hg.mozilla.org/releases/mozilla-beta/rev/c0833cabec9a
https://hg.mozilla.org/releases/mozilla-esr17/rev/ef204577c87c
status-firefox18: affected → fixed
status-firefox19: affected → fixed
status-firefox-esr17: affected → fixed
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.