897676 - GC hazard in XPCWrappedNative::WrapNewGlobal with AutoMarkingNativeScriptableInfoPtr

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

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 1

[Bobby Holley (:bholley)]

Attachment: Null out |si| if we end up using that of the proto in WrapNewGlobal. v1

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 3

[Bobby Holley (:bholley)]

okie dokie.

Comment 4

[Bobby Holley (:bholley)]

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.

Comment 5

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

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.

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

With how long this has been present and the lateness in the 23 cycle, I concur with comment 5.

Comment 7

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

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.

Comment 8

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0bc16b5a428

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for OSX mochitest asserts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcb2b4276185

https://tbpl.mozilla.org/php/getParsedLog.php?id=25888743&tree=Mozilla-Inbound

Comment 10

[Bobby Holley (:bholley)]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Possible to write a test for this?

Comment 12

[Bobby Holley (:bholley)]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1195a625a88e

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Bobby does the patch apply to ESR17 as-is or do you need to provide a branch-specific patch?

Comment 15

[Bobby Holley (:bholley)]

(In reply to [e-mail] [: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.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/dcf164aca02e

Comment 18

[bhavana bajaj [:bajaj]]

:bholley looks like this will need a backport to mozilla-b2g18 given its still affected. Can you please help with a follow-up patch?

Comment 19

[Bobby Holley (:bholley)]

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.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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