887420 - js::ClearStructuredClone can free any address

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

ClearStructuredClone looks like it just scans the entire buffer looking for 8-byte chunks that start with SCTAG_TRANSFER_MAP, and then reads the next 8 bytes as a pointer and passes that to js_free.

This is bad because strings are written directly to the buffer; that is, the user controls bytes in the middle of the buffer.

On little-endian platforms, something like this could trigger a bad free:

    var buf = new Int8Array(8).buffer;
    postMessage(
        {s: "\u0123\u4567\u000f\uffff\ucdef\u89ab\u4567\u0123", b: buf},
        "*",
        [buf]);

It doesn't seem to happen on my machine but probably for superficial reasons.

The shell is unfortunately immune, because it doesn't support transferables yet, and even with sfink's patch "Add an optional parameter to the shell serialize() function for specifying Transferables" in bug 861925, it never calls ClearStructuredClone, so the buffer is simply leaked and the bug can't be triggered. In a locally hacked build I added the call from Serialize() to JS_ClearStructuredClone() and ran this:

    var buf = new Int8Array(8).buffer;
    serialize(
        {s: "\u0123\u4567\u000f\uffff\ucdef\u89ab\u4567\u0123", b: buf},
        [buf]);

which caused this output:

    js(33558) malloc: *** error for object 0x123456789abcdef: pointer being freed
    was not allocated
    *** set a breakpoint in malloc_error_break to debug

Comment 1

[Jason Orendorff [:jorendorff]]

OK. The TRANSFER_MAP_HEADER is set to SCTAG_TM_MARKED at the very beginning of reading. That disables the broken scan.

So the broken scan doesn't happen in a success case, and if reading fails, we don't even call ClearStructuredClone; we just leak any buffers that we own.

Similarly, if something fails during writing, we don't bother to call ClearStructuredClone; we just leak any buffers that we own.

So the only way this could trigger is if we serialize some objects but then decide *not* to deserialize them. Then we could end up calling ClearStructuredClone and triggering the bad scan, and freeing an arbitrary address.

We could probably dig something up by inspecting the code, we have JSAutoStructuredCloneBuffers all over the place. Easier to just fix it, I hope. sfink, can you patch this?

Comment 2

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

I think I may have changed this from a leak to something more dangerous in bug 868700. When you have the main thread and a worker firing postMessages at each other and the worker can't keep up, many of these messages will end up getting dropped and therefore serialized but never deserialized. Before bug 868700, these would end up leaked. Bug 868700 converts things over to use JSAutoStructuredCloneBuffer so that they will now get cleaned up automatically, potentially triggering this bug.

Comment 3

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

Attachment: Do not read off the end of the transfer map

Unless I'm missing something, this seems like a straightforward fix.

Comment 4

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

Also, note that I intend to eliminate the serialized transfer map when I rewrite my patch in bug 861925, so the code this is fixing should go away then too. But this should be safe to backport.

Comment 5

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

and it looks like I managed to reset all of the status flags

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 777413 [details] [diff] [review]
Do not read off the end of the transfer map

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

That break is analogous to the one at jsclone.cpp:1230, in JSStructuredCloneReader::readTransferMap(), right?  r=me if so, as that seems to be fairly clear about the structure of what's being parsed here.  If not, clearly (pun intended) I need to do more reading to understand this.

Comment 7

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

Comment on attachment 777413 [details] [diff] [review]
Do not read off the end of the transfer map

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

In combination with bug 868700, very very easily. Without that, I'm not sure, but it would be at least moderately difficult.

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

The comment kind of does: "If..., we've walked off the end of the transfer map."

Which older supported branches are affected by this flaw?

It was introduced in bug 789593.

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

Bug 789593

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

They'll be the same. The patch is very simple, and works on code that hasn't changed since it landed.

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

Very unlikely. Regressions would most probably take the form of memory leaks.

Do you have anything else to add?

This patch should be considered in combination with the patch on bug 868700, which is a straightforward memory leak fix that greatly widens the exposure from this bug. Bug 868700 has already landed and made it to Aurora. We'd like to pull it into beta too. Reading that patch doesn't really point to this flaw (or the existence of this flaw), but we really wouldn't want to ship anything with bug 868700 landed and not this bug.

What did you have for lunch?

Slightly stale muesli with almond milk, but I don't see how that's relevant to sec-approval. You're overreaching yourself here.

Comment 8

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

Comment on attachment 777413 [details] [diff] [review]
Do not read off the end of the transfer map

[Triage Comment]

sec-approval+ and I marked this approved for Aurora since it doesn't effect Beta or before. Let's get this in trunk first and if it is all green, then Aurora.

Comment 9

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

(In reply to Al Billings [:abillings] from comment #8)
> Comment on attachment 777413 [details] [diff] [review]
> Do not read off the end of the transfer map
> 
> [Triage Comment]
> 
> sec-approval+ and I marked this approved for Aurora since it doesn't effect
> Beta or before. Let's get this in trunk first and if it is all green, then
> Aurora.

Just to be extra-paranoid -- the status-firefox23:unaffected isn't entirely accurate. Specifically, the bug is present starting in firefox23, it's just harder to trigger (as in, I don't know of any way, but there's a decent chance some way exists.) But also, the memory leak bug 868700 *does* affect firefox23, so we'd like to pull that fix in, in which case this bug becomes easily exploitable.

tl;dr: I think we want this on beta too.

So is it still ok for me to land on trunk right now?

Thanks!

Comment 10

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

We need to get input from Release Management about beta first. I've poked them in IRC and am going to Needinfo? them here.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

works for me - we can take it in beta now and it will go out in our 5th week of beta builds next week. if there's any fallout (which doesn't seem likely) we still have time to backout.

Comment 12

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

Comment on attachment 777413 [details] [diff] [review]
Do not read off the end of the transfer map

All right. Thanks. Good to go!

Comment 13

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/641e812ba1ad

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/641e812ba1ad

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3f338403652f
https://hg.mozilla.org/releases/mozilla-beta/rev/65afba378972

Comment 16

[Daniel Veditz [:dveditz]]

Not sure why firefox22 was marked as affected: comment 9 seems pretty clear that it's a regression in 23 (and only got bad as a security problem in 24).

That would mean ESR17 and b2g18 are clear, too.