Closed Bug 897676 Opened 11 years ago Closed 11 years ago

GC hazard in XPCWrappedNative::WrapNewGlobal with AutoMarkingNativeScriptableInfoPtr

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 + fixed
firefox25 + fixed
firefox-esr17 24+ fixed
b2g18 24+ fixed
b2g-v1.1hd --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

(Keywords: csectype-uaf, regression, sec-critical, Whiteboard: [adv-main24+][adv-esr1709+])

Attachments

(1 file)

In WrapNewGlobal, we created a a new XPCNativeScriptableInfo, and stash it in a AutoMarkingNativeScriptableInfoPtr to keep it (or rather its associated XPCNativeScriptableShared) alive. But in certain cases, we end up using the proto's |si|, and delete the one we created. However, we fail to null out the AutoMarkingPtr. So if we trigger a GC before the pointer goes out of scope (which is exactly what my patch in bug 897322 does), we'll end up marking garbage data, which is a use-after-free.

This is probably a regression from bug 720580, which I landed about a year and a half ago.

I'm going to hazard a guess that this is sec-high. It's a use-after-free, but mitigated in several ways. In particular, it's not clear that there's a way on trunk to trigger a GC at the right place (though I'm far from convinced that it's impossible), and it the Mark() operation is limited enough that exploiting its use-after-free behavior would be very tricky.
Comment on attachment 780589 [details] [diff] [review]
Null out |si| if we end up using that of the proto in WrapNewGlobal. v1

I suspect this is sg-crit. We can definitely trigger GC under XPCWrappedNative::FinishInit and the Mark function in question sets a bit. In general, we've treated writes to invalid locations as sg-crit...
Attachment #780589 - Flags: review?(mrbkap) → review+
okie dokie.
Keywords: sec-critical
Comment on attachment 780589 [details] [diff] [review]
Null out |si| if we end up using that of the proto in WrapNewGlobal. v1

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

Pretty difficult. It requires triggering a GC at a special moment and then exploiting a one-bit use-after-free. But it's use-after-free nonetheless.

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

The comments do, yes.

Which older supported branches are affected by this flaw?

All of them.

If not all supported branches, which bug introduced the flaw?

Introduced in bug 720580, mozilla13.

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

Backport should be trivial and identical. That code hasn't changed recently.

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

Extremely safe. But I'd like to land it soon, because the bug it blocks is needed to fix some Firebug crashes that are blocking Honza.
Attachment #780589 - Flags: sec-approval?
Adding Release Management for input. I think we take this on Aurora and Trunk but not Beta right now since it is so late in cycle. I'd say next cycle if it wasn't blocking work.
Flags: needinfo?(release-mgmt)
With how long this has been present and the lateness in the 23 cycle, I concur with comment 5.
Flags: needinfo?(release-mgmt)
Comment on attachment 780589 [details] [diff] [review]
Null out |si| if we end up using that of the proto in WrapNewGlobal. v1

all right. Sec-approval+ to go into trunk and then check it into Aurora.
Attachment #780589 - Flags: sec-approval?
Attachment #780589 - Flags: sec-approval+
Attachment #780589 - Flags: approval-mozilla-aurora+
Bug 899455 indicates that this wasn't the backout that fixed the problem, which is a relief, given that this patch only drops a reference to deleted memory. Repushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d86b4d537e1
https://hg.mozilla.org/mozilla-central/rev/1d86b4d537e1

Possible to write a test for this?
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
The current patches for bug 897322 cause us to crash here in automation, because we GC at the end of this function. But I'm trashing that patch and writing a different one, so that won't help.

The only way to test this would be to add a debug-only GC to the end of this function. But I think the debug-opt behavioral divergence that would introduce is more problematic than the test coverage we'd get.

in-testsuite-.
Flags: in-testsuite? → in-testsuite-
Whiteboard: [adv-main24+]
Bobby does the patch apply to ESR17 as-is or do you need to provide a branch-specific patch?
Flags: needinfo?(bobbyholley+bmo)
(In reply to lsblakk@mozilla.com [:lsblakk] from comment #14)
> Bobby does the patch apply to ESR17 as-is or do you need to provide a
> branch-specific patch?

I would guess that it applies as-is.
Flags: needinfo?(bobbyholley+bmo)
Comment on attachment 780589 [details] [diff] [review]
Null out |si| if we end up using that of the proto in WrapNewGlobal. v1

In that case please uplift.
Attachment #780589 - Flags: approval-mozilla-esr17+
Whiteboard: [adv-main24+] → [adv-main24+][adv-esr1709+]
No longer blocks: 720580
Depends on: 720580
Keywords: regression
:bholley looks like this will need a backport to mozilla-b2g18 given its still affected. Can you please help with a follow-up patch?
Flags: needinfo?(bobbyholley+bmo)
Comment on attachment 780589 [details] [diff] [review]
Null out |si| if we end up using that of the proto in WrapNewGlobal. v1

(In reply to bhavana bajaj [:bajaj] from comment #18)
> :bholley looks like this will need a backport to mozilla-b2g18 given its
> still affected. Can you please help with a follow-up patch?

Given that this patch landed as-is on esr17, I think we can just push the patch.
Attachment #780589 - Flags: approval-mozilla-b2g18?
Comment on attachment 780589 [details] [diff] [review]
Null out |si| if we end up using that of the proto in WrapNewGlobal. v1

https://hg.mozilla.org/releases/mozilla-b2g18/rev/1f633b123e9c

a=bajaj over email
Attachment #780589 - Flags: approval-mozilla-b2g18?
Flags: needinfo?(bobbyholley+bmo)
Group: core-security
You need to log in before you can comment on or make changes to this bug.