922009 - MOZ_CRASH when trying to transplant objects with same-compartment security wrappers

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

See bug 921454 comment 5.

I'm going to use this bug to track the security issue here (since this is what we'll end up landing on all branches), and mark bug 921454 sec-other.

Comment 1

[Al Billings [:abillings - ex-MoCo]]

How far back does this go?

Comment 2

[Andrew McCreight [:mccr8]]

This affects everything.

Assigning to bholley, as it looks like he's going to work on this.

Comment 3

[Bobby Holley (:bholley)]

Attachment: MOZ_CRASH when trying to transplant objects with SCSWs. v1

Comment 4

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=92110dc3d9a2

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Wow, this is great. How does the location object work nowadays?

Comment 6

[Bobby Holley (:bholley)]

(In reply to Bill McCloskey (:billm) from comment #5)
> Wow, this is great. How does the location object work nowadays?

Security checks in the C++ implementations. No more same-compartment security wrapper.

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 814417 [details] [diff] [review]
MOZ_CRASH when trying to transplant objects with SCSWs. v1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not easily. This just tells people that we're worried about our code that transplants SOWs. And we've already told them that by landing bug 921454.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

All.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Trivial. We just remove the code and MOZ_CRASH.

How likely is this patch to cause regressions; how much testing does it need?

Testing is crucial, because this might cause us to crash with addons.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

I'd like to get more input from release management about whether we can take this on beta now or not. The comment about testing will probably need more detail. What kind of testing do you need? Potential crashing will be a concern.

Comment 9

[Bobby Holley (:bholley)]

I think we should land now on m-c and aurora and let it ride the train to beta.

Comment 10

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 814417 [details] [diff] [review]
MOZ_CRASH when trying to transplant objects with SCSWs. v1

sec-approval+ for trunk. Please make an Aurora patch and nominate it.

Comment 11

[Al Billings [:abillings - ex-MoCo]]

If things work out fine. We'll need to track this for Firefox 26 era ESR24.

Comment 12

[bhavana bajaj [:bajaj]]

Looks like we do not need this on current beta(Fx25) anymore, so clearing the needinfo.

Comment 13

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fd2088036f2

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 814417 [details] [diff] [review]
MOZ_CRASH when trying to transplant objects with SCSWs. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): longstanding
User impact if declined: security bugs
Testing completed (on m-c, etc.): just pushed to m-c
Risk to taking this patch (and alternatives if risky): The patch might cause addons to crash if they happen to be doing the thing we're disallowing here, but it's not super likely.
String or IDL/UUID changes made by this patch: None

Comment 15

[Carsten Book [:Tomcat]]

fixed on central https://hg.mozilla.org/mozilla-central/rev/8fd2088036f2

Comment 16

[Andrew McCreight [:mccr8]]

No test or direct QA is possible for this.  We just have to watch crash-stats to see if we hit this crash.  Hopefully we don't.

Comment 17

[Andrew McCreight [:mccr8]]

This will show up as a MOZ_CRASH in XPCWrappedNative::ReparentWrapperIfFound or ReparentWrapper.

Comment 18

[Tracy Walker [:tracy]]

no crash reports on Nightly with signature containing ReparentWrapper in the past 7 days.

Comment 19

[Bobby Holley (:bholley)]

Is there a reason this hasn't been uplifted to aurora yet?

Comment 20

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #19)
> Is there a reason this hasn't been uplifted to aurora yet?

(I don't mean to sound ungrateful for this incredible sanity-saving productivity-boosting service. It's only that it's been so reliable that I've got in the habit of crossing bugs off my list after requesting branch approval). :-)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

PTO all week

Comment 22

[Al Billings [:abillings - ex-MoCo]]

Who is supposed to be landing this on Aurora for you, Bobby?

Comment 23

[Bobby Holley (:bholley)]

(In reply to Al Billings [:abillings] from comment #22)
> Who is supposed to be landing this on Aurora for you, Bobby?

Generally, Ryan does. I'd assumed this was a widespread practice, because I haven't had to push a branch patch myself in over 6 months. Maybe it's been more narrow than that?

Anyway, it's sleepytime for me. I'll uplift this when I get the chance.

Comment 24

[Andrew McCreight [:mccr8]]

Yeah, Ryan has been landing everything, but he's on vacation this week. :)

I'll land this.

Comment 25

[Al Billings [:abillings - ex-MoCo]]

I had no idea that Ryan was doing it across the board but I approve!

Comment 26

[Andrew McCreight [:mccr8]]

(In reply to Al Billings [:abillings] from comment #25)
> I had no idea that Ryan was doing it across the board but I approve!

Yeah, it has made fixing security bugs much more pleasant.

https://hg.mozilla.org/releases/mozilla-aurora/rev/328cd24aaed6

Comment 27

[Bobby Holley (:bholley)]

Thanks Andrew!

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Is this WONTFIX for esr24/b2g18?

Comment 29

[Bobby Holley (:bholley)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4][PTO 10/19 - 10/28] from comment #28)
> Is this WONTFIX for esr24/b2g18?

No, we should land this there, but for the next cycle, so that this hits release in all channels simultaneously.

Comment 30

[Bobby Holley (:bholley)]

Comment on attachment 814417 [details] [diff] [review]
MOZ_CRASH when trying to transplant objects with SCSWs. v1

[Approval Request Comment]
User impact if declined: sec
Fix Landed on Version: 27 and 26
Risk to taking this patch (and alternatives if risky): This can cause addons to crash if they're transplanting NAC, which is something we're hoping that nobody does, because we don't handle it very well.
String or UUID changes made by this patch: None

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/a06affe796da

On b2g18, this has conflicts in XPCWrappedNative.cpp that I'm not entirely sure how to resolve.

Comment 32

[Bobby Holley (:bholley)]

Oh, crap. I just realized that this is not backportable to b2g18, because it doesn't have bug 808608, which landed in mozilla20. I recommend WONTFIX/embargo.

Comment 33

[Al Billings [:abillings - ex-MoCo]]

Marking won't fix for b2g18 and embargo'ing until we EOL 1.1 in March.