794158 - (CVE-2013-0745) Investigate if AutoWrapperChanger causes security problems

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

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]]

That does look bad. I can take a look at it.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

s/I keeps/It keeps/

Comment 3

[Andrew McCreight [:mccr8]]

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 (:bholley)]

(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][[e-mail]]]

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][[e-mail]]]

Attachment: un-compiled

Something like this.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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]]

It looks reasonable, but I need to look into why we're doing the drop in the first place.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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]]

Why not call SetPreservingWrapper directly instead of adding a bool argument to give PreserveWrapper/ReleaseWrapper a different behaviour?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

I guess that could be done too.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

Attachment: 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][[e-mail]]]

Peter, feedback ping

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

We looked at this again in CRITSMASH. Peter any comments?

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

Sorry Peter, but I'm really not familiar with DOMExpandoObject handling.

Comment 18

[Peter Van der Beken [: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?

Comment 19

[Bobby Holley (:bholley)]

(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]]

(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][[e-mail]]]

Attachment: patch

With assertion

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 678256 [details] [diff] [review]
patch

Fun, this leaks.

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

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]]

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][[e-mail]]]

and manually root/unroot the values in the array?
That could work yes.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Attachment: simpler approach

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 682230 [details] [diff] [review]
simpler approach

Doesn't leak, nor cause new assertions.

Comment 28

[Peter Van der Beken [:peterv]]

Comment on attachment 682230 [details] [diff] [review]
simpler approach

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

I like it.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

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 - ex-MoCo]]

Comment on attachment 682230 [details] [diff] [review]
simpler approach

sec-approval+.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/f42a83257653

Comment 33

[Alex Keybl [:akeybl]]

(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][[e-mail]]]

Will prepare patches later today or tomorrow.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

Attachment: for branches

Comment 37

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

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