Last Comment Bug 794158 - (CVE-2013-0745) Investigate if AutoWrapperChanger causes security problems
(CVE-2013-0745)
: Investigate if AutoWrapperChanger causes security problems
Status: RESOLVED FIXED
[adv-main18+][adv-esr17+]
: csectype-uaf, sec-critical
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: mozilla20
Assigned To: Olli Pettay [:smaug]
:
Mentors:
: 813655 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-25 10:50 PDT by Olli Pettay [:smaug]
Modified: 2013-04-30 18:40 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
+
wontfix
+
fixed
+
fixed
fixed
unaffected
18+
fixed


Attachments
un-compiled (3.97 KB, patch)
2012-09-25 15:37 PDT, Olli Pettay [:smaug]
continuation: feedback+
Details | Diff | Review
v2 (915 bytes, patch)
2012-10-14 13:40 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Review
patch (1.04 KB, patch)
2012-11-05 02:48 PST, Olli Pettay [:smaug]
no flags Details | Diff | Review
simpler approach (3.80 KB, patch)
2012-11-15 15:25 PST, Olli Pettay [:smaug]
peterv: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
abillings: sec‑approval+
Details | Diff | Review
for branches (3.79 KB, patch)
2012-12-04 13:01 PST, Olli Pettay [:smaug]
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr17+
Details | Diff | Review

Description Olli Pettay [:smaug] 2012-09-25 10:50:31 PDT
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.)
Comment 1 Andrew McCreight [:mccr8] 2012-09-25 10:55:32 PDT
That does look bad. I can take a look at it.
Comment 2 Olli Pettay [:smaug] 2012-09-25 11:05:05 PDT
s/I keeps/It keeps/
Comment 3 Andrew McCreight [:mccr8] 2012-09-25 14:54:01 PDT
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.
Comment 4 Bobby Holley (PTO through June 13) 2012-09-25 15:09:05 PDT
(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.
Comment 5 Olli Pettay [:smaug] 2012-09-25 15:15:11 PDT
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
Comment 6 Olli Pettay [:smaug] 2012-09-25 15:37:26 PDT
Created attachment 664679 [details] [diff] [review]
un-compiled

Something like this.
Comment 7 Olli Pettay [:smaug] 2012-09-26 05:04:01 PDT
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.
Comment 8 Andrew McCreight [:mccr8] 2012-09-26 09:55:42 PDT
It looks reasonable, but I need to look into why we're doing the drop in the first place.
Comment 9 Olli Pettay [:smaug] 2012-10-08 13:58:42 PDT
Comment on attachment 664679 [details] [diff] [review]
un-compiled

Maybe even review.


It is hard to find a test case for this.
Comment 10 Peter Van der Beken [:peterv] 2012-10-08 23:59:39 PDT
Why not call SetPreservingWrapper directly instead of adding a bool argument to give PreserveWrapper/ReleaseWrapper a different behaviour?
Comment 11 Olli Pettay [:smaug] 2012-10-09 03:42:51 PDT
I guess that could be done too.
Comment 12 Olli Pettay [:smaug] 2012-10-10 17:38:29 PDT
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.
Comment 13 Olli Pettay [:smaug] 2012-10-14 13:40:41 PDT
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...
Comment 14 Olli Pettay [:smaug] 2012-10-21 16:15:16 PDT
Peter, feedback ping
Comment 15 David Bolter [:davidb] 2012-10-25 13:27:23 PDT
We looked at this again in CRITSMASH. Peter any comments?
Comment 16 Olli Pettay [:smaug] 2012-11-01 05:51:49 PDT
bholley> well, it seems like we have a more general issue that the DOM expando object gets lost during reparenting, no?
Comment 17 Olli Pettay [:smaug] 2012-11-01 05:53:32 PDT
Sorry Peter, but I'm really not familiar with DOMExpandoObject handling.
Comment 18 Peter Van der Beken [:peterv] 2012-11-01 07:36:37 PDT
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?
Comment 19 Bobby Holley (PTO through June 13) 2012-11-01 08:01:36 PDT
(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.
Comment 20 Peter Van der Beken [:peterv] 2012-11-01 08:30:16 PDT
(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.
Comment 21 Olli Pettay [:smaug] 2012-11-05 02:48:36 PST
Created attachment 678256 [details] [diff] [review]
patch

With assertion
Comment 22 Olli Pettay [:smaug] 2012-11-05 06:32:23 PST
Comment on attachment 678256 [details] [diff] [review]
patch

Fun, this leaks.
Comment 23 Lukas Blakk [:lsblakk] use ?needinfo 2012-11-07 18:02:57 PST
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.
Comment 24 Andrew McCreight [:mccr8] 2012-11-14 09:55:48 PST
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.
Comment 25 Olli Pettay [:smaug] 2012-11-14 10:03:21 PST
and manually root/unroot the values in the array?
That could work yes.
Comment 26 Olli Pettay [:smaug] 2012-11-15 15:25:13 PST
Created attachment 682230 [details] [diff] [review]
simpler approach
Comment 27 Olli Pettay [:smaug] 2012-11-15 17:24:28 PST
Comment on attachment 682230 [details] [diff] [review]
simpler approach

Doesn't leak, nor cause new assertions.
Comment 28 Peter Van der Beken [:peterv] 2012-11-20 04:55:54 PST
Comment on attachment 682230 [details] [diff] [review]
simpler approach

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

I like it.
Comment 29 Olli Pettay [:smaug] 2012-11-20 05:12:03 PST
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.
Comment 30 Al Billings [:abillings] 2012-11-20 11:51:26 PST
Comment on attachment 682230 [details] [diff] [review]
simpler approach

sec-approval+.
Comment 31 Olli Pettay [:smaug] 2012-11-20 13:07:47 PST
https://hg.mozilla.org/mozilla-central/rev/f42a83257653
Comment 32 Andrew McCreight [:mccr8] 2012-11-20 13:33:16 PST
*** Bug 813655 has been marked as a duplicate of this bug. ***
Comment 33 Alex Keybl [:akeybl] 2012-11-29 16:31:01 PST
(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?
Comment 34 Olli Pettay [:smaug] 2012-12-04 11:55:48 PST
Will prepare patches later today or tomorrow.
Comment 35 Olli Pettay [:smaug] 2012-12-04 12:40:51 PST
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
Comment 36 Olli Pettay [:smaug] 2012-12-04 13:01:02 PST
Created attachment 688422 [details] [diff] [review]
for branches
Comment 37 Lukas Blakk [:lsblakk] use ?needinfo 2012-12-05 12:29:42 PST
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.

Note You need to log in before you can comment on or make changes to this bug.