1072174 - XrayToString assumes that there are only two XrayTrait types

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

I noticed this while reviewing peterv's patches in bug 787070. The XrayToString method currently checks that we have an XrayWrapper, handles DOMXrays, and then assumes that we have an XPCWrappedNativeXray. If this were done with a JSXray, we'd end up casting arbitrary bits of the JSObject layout to an XPCWrappedNative* and dereferencing it. :-(

This is mitigated somewhat by the fact that Xrays generally aren't exposed to the web. But they are exposed to sandboxes, and we run enough untrusted code in sandboxes that I think this is sec-high.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Handle all the cases XrayWrapper.cpp. v1

Peter, can you make sure I didn't miss any other cases?

Comment 2

[Bobby Holley (:bholley)]

Attachment: Tests. v1

Comment 3

[Bobby Holley (:bholley)]

This is a regression from bug 975042, which first introduced the third Xray trait.

Comment 4

[Bobby Holley (:bholley)]

(we now have 4)

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8494376 [details] [diff] [review]
Handle all the cases XrayWrapper.cpp. v1

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

Medium. Triggering the memory hazard is straightforward, though I used a fair amount of specialized knowledge to construct the testcase. It also requires access to a sandbox.

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.

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

Bug 975042.

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

Straightforward.

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

Low risk.

Comment 6

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

Comment on attachment 8494376 [details] [diff] [review]
Handle all the cases XrayWrapper.cpp. v1

sec-approval+ for trunk. Can we get Aurora, Beta, and ESR31 patches made and nominated too?

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8494376 [details] [diff] [review]
Handle all the cases XrayWrapper.cpp. v1

Approval Request Comment
[Feature/regressing bug #]: bug 975042
[User impact if declined]: sec-high
[Describe test coverage new/current, TBPL]: Test in bug, but not checking in.
[Risks and why]: Low risk.
[String/UUID change made/needed]: None

Comment 8

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3dfe5612e17b

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3dfe5612e17b

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ea83cf4e9233
https://hg.mozilla.org/releases/mozilla-beta/rev/bb4423c0da47
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/97d964f765af
https://hg.mozilla.org/releases/mozilla-esr31/rev/face4eab3098

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/97d964f765af

Comment 12

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed assert with attached test in Fx32. Verified fixed in Fx33, built 2014-10-06.

Will verify on other branches shortly.

Comment 13

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified fixed on Fx34/Fx35/Fx31.2.0esr, 2014-10-07.