1288555 - wrong compartment while structured cloning a cross-compartment ArrayBuffer

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

When transferring the contents of an ArrayBuffer during a structured clone, a cross-compartment ArrayBuffer is unwrapped, but then the detach call happens in the original compartment. If there is more than one view on that ArrayBuffer, all views after the second will then fail to be detached (because we have an InnerViewsTable *on the compartment* that holds all views but the first.)

As a result, these views (eg typed arrays) will still have pointers into the ArrayBuffer's data, which is a huge correctness violation as well as a set up for lots of race conditions if the data has been transferred between threads. Furthermore, if the buffer gets GC'd, it's a very controllable UAF.

JS shell example:

b = new ArrayBuffer(100);
ta = new Int32Array(b);
ta[0] = 17;

g = newGlobal();
g.b = b;
ta2 = g.eval("new Int32Array(b)");

g.serialize(b, [b]);
// gc();

print(ta[0]);
print(ta2[0]); // This is 17, should be undefined. With the gc(), it is a UAF and will probably crash.

In the browser, you'd use postMessage to detach the buffer.

Comment 1

[Steve Fink [:sfink] [:s:]]

[Tracking Requested - why for this release]: This bug goes back to at least Fx45, and is trivially exploitable.

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Enter the transferred ArrayBuffer's compartment before stealing its data

Asking for review on this patch, but I'm not sure in what form we'd want to land this to be discreet. We can discuss that during the approval process. (Same thing for tests. I have a test, but it makes things horribly obvious.)

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 8773527 [details] [diff] [review]
Enter the transferred ArrayBuffer's compartment before stealing its data

Review of attachment 8773527 [details] [diff] [review]:
-----------------------------------------------------------------

*gulp*  I keep missing when our tracking of buffer views changes.  First it was a linked list, then it was something, then something else, now this.  CAN'T WE JUST GET IT RIGHT THE FIRST TIME.  :-)

::: js/src/vm/ArrayBufferObject.cpp
@@ +1062,1 @@
>                  MOZ_ASSERT_IF(buf.dataPointer() == nullptr, offset == 0);

The more these build up, the more I prefer #ifdef DEBUG around an if with MOZ_ASSERTs, but this is fine.

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Enter the transferred ArrayBuffer's compartment before stealing its data

MozReview-Commit-ID: 90tALXT2BVr

Comment 5

[Steve Fink [:sfink] [:s:]]

Attachment: Enter the transferred ArrayBuffer's compartment before stealing its data

MozReview-Commit-ID: 90tALXT2BVr

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: Version of the patch with a more mundane description

My original comment was a little too revealing.

Comment 7

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8775728 [details] [diff] [review]
Version of the patch with a more mundane description

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

If someone is familiar with the security properties of compartments, it's pretty easy. This gives you a read/write UAF with a completely controllable offset.

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

No, I changed to a less revealing comment.

Which older supported branches are affected by this flaw?

all of them

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

Easy to create. They should be nearly identical, though I suspect enough of the structured clone code will have changed slightly that they might not apply cleanly. I looked back to esr45 and the code is functionally identical.

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

Unlikely. Without this fix, some things would have worked that should not. (Specifically, if you transferred a buffer via postMessage, its views would continue to "work" when they shouldn't.)

[Approval Request Comment]
Fix Landed on Version: not yet landed at this time
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Approval Request Comment
[Feature/regressing bug #]: 1061404
[User impact if declined]: exploitable UAF
[Describe test coverage new/current, TreeHerder]: it fixes my test case, but I'm afraid to push it to a publicly visible place
[Risks and why]: theoretically somebody could be depending on views of detached buffers working, but if so we need to break them ASAP
[String/UUID change made/needed]: none

Comment 8

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

This is pretty bad and revealing from your comments. I'm going to sec-approve for trunk checkin but it needs to wait until August 16 so we're two weeks into the cycle so we don't overly expose this to anyone watching checkins.

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8775728 [details] [diff] [review]
Version of the patch with a more mundane description

Sec-high, approving for all affected branches. Based on Al's last comment this ought to be committed on Aug 16th and not before that. Aurora50+, Beta49+, ESR45+

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a9213b054219

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8ececbd9880d
https://hg.mozilla.org/releases/mozilla-beta/rev/6cfff98fe1a0

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/9d632865560a