1425612 - StructuredClone crash reading invalid data

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jason Orendorff [:jorendorff]]

let bytes = new Uint8Array([
    2,0,0,0,0,0,241,255,0,0,0,0,8,0,255,255,
    8,0,0,128,4,0,255,255,105,110,116,101,103,101,114,115,
    0,0,0,0,7,0,255,255,0,0,0,0,3,0,255,255,
    0,0,0,0,3,0,255,255,1,0,0,0,3,0,255,255,
    1,0,0,0,3,0,255,255,2,0,0,0,3,0,255,255,
    255,255,255,255,3,0,255,255,3,0,0,0,3,0,255,255,
    0,0,224,255,255,255,239,65,4,0,0,0,3,0,255,255,
    0,0,0,128,3,0,255,255,0,0,0,0,19,0,255,255,
    6,0,0,128,4,0,255,255,102,108,111,97,116,115,0,0,
    0,0,0,0,7,0,255,255,0,0,0,0,3,0,255,255,
    24,45,68,84,251,33,9,64,1,0,0,0,3,0,255,255,
    105,87,20,139,10,191,5,64,0,0,0,0,19,0,255,255,
    10,0,0,128,4,0,255,255,117,114,69,108,101,109,101,110,
    116,115,0,0,0,0,0,0,0,0,0,0,7,0,255,255,
    0,0,0,0,3,0,255,255,1,0,0,0,2,0,255,255,
    1,0,0,0,3,0,255,255,0,0,0,0,2,0,255,255,
    2,0,0,0,3,0,255,255,0,0,0,0,16,0,255,255,
    3,0,0,0,3,0,255,255,0,0,0,0,1,0,255,255,
    0,0,0,0,19,0,255,255,12,0,0,128,4,0,255,255,
    119,101,105,114,100,78,117,109,98,101,114,115,0,0,0,0,
    0,0,0,0,7,0,255,255,0,0,0,0,3,0,255,255,
    0,0,0,0,0,0,0,128,1,0,0,0,3,0,255,255,
    0,0,0,0,0,0,240,127,2,0,0,0,3,0,255,255,
    0,0,0,0,0,0,240,255,3,0,0,0,3,0,255,255,
    0,0,0,0,0,0,248,127,0,0,0,0,19,0,255,255,
    7,0,0,128,4,0,255,255,115,116,114,105,110,103,115,0,
    0,0,0,0,7,0,255,255,0,0,0,0,3,0,255,255,
    0,0,0,128,4,0,255,255,1,0,0,0,3,0,255,255,
    1,0,0,128,4,0,255,255,120,0,0,0,0,0,0,0,
    2,0,0,0,3,0,255,255,11,0,0,128,4,0,255,255,
    104,101,108,108,111,32,119,111,114,108,100,0,0,0,0,0,
    0,0,0,0,19,0,255,255,7,0,0,128,4,0,255,255,
    111,98,106,101,99,116,115,0,0,0,0,0,7,0,255,255,
    0,0,0,0,3,0,255,255,0,0,0,0,5,0,255,255,
    0,144,120,15,190,5,118,66,0,0,0,0,19,0,255,255,
    0,0,0,0,19,0,255,255
]);
let cloneBuffer = serialize(null);
cloneBuffer.arraybuffer = bytes.buffer;
let obj = deserialize(cloneBuffer);
print(uneval(obj));

Comment 1

[Jason Orendorff [:jorendorff]]

Crashes here:
  https://searchfox.org/mozilla-central/rev/c8e791091973825680bbba807fc1c4f5bda0f5a1/js/src/vm/StructuredClone.cpp#1919

v is UndefinedValue. If that's all, this is a null-crash, not exploitable. Marking this s-s because I don't know if the input can make v be some other value (which would be Bad).

Comment 2

[Jason Orendorff [:jorendorff]]

v can be any value that serialization can produce, except a plain object or array. This is sec-crit, at least on 32-bit platforms, probably on all platforms.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: WIP - Better error messages for invalid structured clone data

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8937282 [details] [diff] [review]
WIP - Better error messages for invalid structured clone data

I think we can land this under an open bug with a summary like "improve error messages in structured cloning".

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: Scripts for fuzzing structured clone deserialization

Comment 6

[Jason Orendorff [:jorendorff]]

The "scripts for fuzzing" patch requires the patches in bug 1426455 and bug 1426457.

Also it requires a Mac. But the changes to run it on Linux should be straightforward: change clang/clang++ to gcc/g++; comment out the line that disables the fork server.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: No bug yet - Better error messages for invalid structured clone data

Comment 8

[Jason Orendorff [:jorendorff]]

The "Better error messages" patch here fixes all crashes the fuzzer found so far. It's enough of an improvement I think we should land it.

Comment 9

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

Comment on attachment 8938093 [details] [diff] [review]
No bug yet - Better error messages for invalid structured clone data

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

Wow. That's an impressive collection of serious fixes.

::: js/src/vm/StructuredClone.cpp
@@ +2040,5 @@
>      // We must not transfer buffer pointers cross-process.  The cloneDataPolicy
>      // in the sender should guard against this; check that it does.
> +    if (storedScope > JS::StructuredCloneScope::SameProcessDifferentThread) {
> +        JS_ReportErrorNumberASCII(context(), GetErrorMessage, nullptr, JSMSG_SC_BAD_SERIALIZED_DATA,
> +                                  "can't transfer SharedArrayBuffer cross-process");

I wonder if the verb "transfer" is problematic, because of Transferrables. "send"?

@@ +2065,5 @@
>  bool
>  JSStructuredCloneReader::readSharedWasmMemory(uint32_t nbytes, MutableHandleValue vp)
>  {
> +    if (nbytes != 0) {
> +        JS_ReportErrorNumberASCII(context(), GetErrorMessage, nullptr, JSMSG_SC_BAD_SERIALIZED_DATA,

Hoist cx and use it here.

Comment 11

[Jason Orendorff [:jorendorff]]

I don't know for sure.

I'm on vacation until January 3 (two weeks from today), so I will not file an open "cover" bug or seeking sec-approval until then. In the mean time I'll try to find time to do some more fuzzing. Christian pointed me at aflloop() which should help a lot.

Comment 12

[Christian Holler (:decoder)]

I was able to find an exploitable bug pretty much immediately with a libfuzzer-based API fuzzer for the StructuredCloneReader. Jason's patch fixes this crash, I will check if there is more to find.

Comment 13

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

Anything more found, Christian?

Comment 14

[Christian Holler (:decoder)]

(In reply to Al Billings [:abillings] from comment #13)
> Anything more found, Christian?

Yes, bug 1426783 also reproduces with the fixes from this bug applied. I am also seeing a GC crash but wasn't able to get a testcase for it so far. Any other issues found will block the tracking bug 1426778.

Comment 15

[Jason Orendorff [:jorendorff]]

OK, bug 1426783 is clearly quite severe, so I have not filed an open bug yet. Right now it looks like the patch in that bug should land at the same time as this one; I'll request sec-review in a second.

-> sec-high because attacker must take over a content process in order to exploit this.

Comment 16

[Jason Orendorff [:jorendorff]]

Comment on attachment 8938093 [details] [diff] [review]
No bug yet - Better error messages for invalid structured clone data

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

    Hard work, but it's likely one of these bugs is exploitable: if an attacker can take over a content process first, they might be able to use one of the bugs fixed in this patch to take over the chrome process or other content processes.

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

    Yes.

Which older supported branches are affected by this flaw?

    All supported branches.

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

    I have backports. Conflicts were few and easy to resolve. The only wrinkle is that backporting to esr52 required also backporting some changes from bug 1352681 and bug 1359461, earlier partial fixes for similar issues.

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

    Quite safe. These fixes touch error cases that Shouldn't Happen, so (fingers crossed) they should inconvenience only the bad guys.

Comment 17

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

sec-approval+ for trunk. We should take this on Beta. I'm unsure it is worth it on ESR52 this late in the cycle.

Comment 18

[Liz Henry (:lizzard)]

We can take this for Thursday's beta 16 build.
n-i to Gerry as 58 release owner just so you know we should be on the lookout.

Comment 19

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e2b4cf8e26b900983e33cc2fb6b48f1e5747b2
Bug 1425612 - Better error messages for invalid structured clone data. r=sfink, a=abillings.

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Bug 1426783 and bug 1425612 got backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/600bee353e155608a5832f7eb9d5e42987d66591 for failing non262/extensions/sharedtypedarray.js:

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a8e2b4cf8e26b900983e33cc2fb6b48f1e5747b2&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
TEST-UNEXPECTED-FAIL | non262/extensions/sharedtypedarray.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so")

Comment 21

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=acf6298fe0c361ce555ad8668d08899f7901c567

Comment 22

[Jason Orendorff [:jorendorff]]

Attachment: Better error messages for invalid structured clone data

Comment 23

[Jason Orendorff [:jorendorff]]

Comment on attachment 8941698 [details] [diff] [review]
Better error messages for invalid structured clone data

Original patch bounced.

The difference between this patch and the previous is using ArrayBufferObjectMaybeShared instead of ArrayBufferObject in two places.

Will re-land tonight.

Comment 24

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a2cf3f843d0b5d99c9603d6d4c83146719634a4a
Bug 1425612 - Better error messages for invalid structured clone data. r=sfink, a=abillings.

Comment 25

[Jason Orendorff [:jorendorff]]

Comment on attachment 8941698 [details] [diff] [review]
Better error messages for invalid structured clone data

Approval Request Comment
[Feature/Bug causing the regression]: Structured cloning being used as a serialization format across trust boundaries.
[User impact if declined]: Sec-high risk as in comment 16.
[Is this code covered by automated tests?]: No, it needs to be covered by fuzzing.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Bug 1426783.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Changes are in error handling for invalid structured-clone data, which we do not generate. This code triggers only if we're attacked.
[String changes made/needed]: None.

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 27

[Gerry Chang [:gchang]]

Comment on attachment 8941698 [details] [diff] [review]
Better error messages for invalid structured clone data

Fix a sec-high. Beta58+.

Comment 28

[Jason Orendorff [:jorendorff]]

The patch is now in beta due to today's central->beta merge. Will push to release.

Comment 29

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/releases/mozilla-release/rev/f4256fe890db

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/db3476b3775994ca22aebbd17854b9a53d2a50b5 (FIREFOX_58b_RELBRANCH)

Comment 31

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

Hi Jason, since this was fixed in 58, I am wondering whether we ought to also uplift this fix to ESR52.6. Wdyt?

Al, Dan, I will gtb ESR52.6 build2 in ~2 hrs. Is this fix worth including in build2? I will if you agree it's a good idea.

Comment 32

[Jason Orendorff [:jorendorff]]

Attachment: Better error messages for invalid structured clone data (for esr52)

Comment 33

[Jason Orendorff [:jorendorff]]

Comment on attachment 8943705 [details] [diff] [review]
Better error messages for invalid structured clone data (for esr52)

This patch drops a change in JSStructuredCloneReader::readSharedArrayBuffer(), because there were conflicts (we would have to uplift bug 1352681 first) and because SAB is disabled in ESR52 anyway.

[Approval Request Comment]
User impact if declined: Sec-high risk as described in comment 16.
Fix Landed on Version: 58
Risk to taking this patch (and alternatives if risky): low, it affects error paths that neither content nor chrome should be able to trigger
String or UUID changes made by this patch: none

Comment 34

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fd5b0df5f6701dd510ccd7f4408e2f08081acdf

Comment 35

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

Comment on attachment 8943705 [details] [diff] [review]
Better error messages for invalid structured clone data (for esr52)

Sec-high, in 58, Al said he's fine taking this one, ESR52+

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/410da936a1e8