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

RESOLVED FIXED in Firefox 26

Status

()

defect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

({sec-high})

unspecified
mozilla27
x86
macOS
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox24 wontfix, firefox25 wontfix, firefox26 fixed, firefox27 verified, firefox-esr17 wontfix, firefox-esr2426+ fixed, b2g1826+ wontfix, b2g-v1.1hd wontfix, b2g-v1.2 fixed)

Details

(Whiteboard: [embargo until B2G 1.1 EOL][adv-main26+][adv-esr24.2+])

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

6 years ago
Keywords: sec-high
How far back does this go?
This affects everything.

Assigning to bholley, as it looks like he's going to work on this.
Wow, this is great. How does the location object work nowadays?
(Assignee)

Comment 6

6 years ago
(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.
Attachment #814417 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 7

6 years ago
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.
Attachment #814417 - Flags: sec-approval?
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.
Flags: needinfo?(release-mgmt)
(Assignee)

Comment 9

6 years ago
I think we should land now on m-c and aurora and let it ride the train to beta.
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.
Attachment #814417 - Flags: sec-approval? → sec-approval+
If things work out fine. We'll need to track this for Firefox 26 era ESR24.
Looks like we do not need this on current beta(Fx25) anymore, so clearing the needinfo.
Flags: needinfo?(release-mgmt)
(Assignee)

Comment 14

6 years ago
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
Attachment #814417 - Flags: approval-mozilla-aurora?
Attachment #814417 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
fixed on central https://hg.mozilla.org/mozilla-central/rev/8fd2088036f2
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Flags: in-testsuite? → in-testsuite-
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.
This will show up as a MOZ_CRASH in XPCWrappedNative::ReparentWrapperIfFound or ReparentWrapper.
no crash reports on Nightly with signature containing ReparentWrapper in the past 7 days.
(Assignee)

Comment 19

6 years ago
Is there a reason this hasn't been uplifted to aurora yet?
Flags: needinfo?(ryanvm)
(Assignee)

Comment 20

6 years ago
(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). :-)
PTO all week
Flags: needinfo?(ryanvm)
Who is supposed to be landing this on Aurora for you, Bobby?
(Assignee)

Comment 23

6 years ago
(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.
Flags: needinfo?(bobbyholley+bmo)
Yeah, Ryan has been landing everything, but he's on vacation this week. :)

I'll land this.
Flags: needinfo?(bobbyholley+bmo)
I had no idea that Ryan was doing it across the board but I approve!
(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
(Assignee)

Comment 27

6 years ago
Thanks Andrew!
Is this WONTFIX for esr24/b2g18?
(Assignee)

Comment 29

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

Comment 30

6 years ago
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
Attachment #814417 - Flags: approval-mozilla-esr24?
Attachment #814417 - Flags: approval-mozilla-b2g18?
Attachment #814417 - Flags: approval-mozilla-esr24?
Attachment #814417 - Flags: approval-mozilla-esr24+
Attachment #814417 - Flags: approval-mozilla-b2g18?
Attachment #814417 - Flags: approval-mozilla-b2g18+
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.
Flags: needinfo?(bobbyholley+bmo)
(Assignee)

Comment 32

6 years ago
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.
Flags: needinfo?(bobbyholley+bmo) → needinfo?(abillings)
Marking won't fix for b2g18 and embargo'ing until we EOL 1.1 in March.
Flags: needinfo?(abillings)
Whiteboard: [embargo until B2G 1.1 EOL]
Whiteboard: [embargo until B2G 1.1 EOL] → [embargo until B2G 1.1 EOL][adv-main26+][adv-esr26.2+]
Whiteboard: [embargo until B2G 1.1 EOL][adv-main26+][adv-esr26.2+] → [embargo until B2G 1.1 EOL][adv-main26+][adv-esr24.2+]

Updated

4 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.