StructuredClone crash reading invalid data

RESOLVED FIXED in Firefox -esr52

Status

()

P1
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

(Blocks: 1 bug, 4 keywords)

unspecified
mozilla59
crash, csectype-wildptr, sec-high, testcase
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr5258+ fixed, firefox57 wontfix, firefox58+ fixed, firefox59+ fixed)

Details

(Whiteboard: [adv-main58+][adv-esr52.6+][post-critsmash-triage])

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
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));
(Assignee)

Comment 1

a year ago
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).
(Assignee)

Updated

a year ago
status-firefox59: --- → affected
(Assignee)

Comment 2

a year ago
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.
(Assignee)

Comment 4

a year ago
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".
Attachment #8937282 - Attachment description: No bug yet - Better error messages for invalid structured clone data → WIP - Better error messages for invalid structured clone data
(Assignee)

Comment 6

a year ago
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.
(Assignee)

Updated

a year ago
Attachment #8937282 - Attachment is obsolete: true
(Assignee)

Comment 8

a year ago
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 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.
Attachment #8938093 - Flags: review?(sphink) → review+
(Assignee)

Comment 11

a year ago
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.
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.
Keywords: crash, csectype-wildptr, sec-critical, testcase
Anything more found, Christian?
Flags: needinfo?(choller)
(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.
Flags: needinfo?(choller)
(Assignee)

Comment 15

a year ago
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.
Keywords: sec-critical → sec-high
(Assignee)

Comment 16

a year ago
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.
Attachment #8938093 - Flags: sec-approval?
sec-approval+ for trunk. We should take this on Beta. I'm unsure it is worth it on ESR52 this late in the cycle.
status-firefox57: --- → wontfix
status-firefox58: --- → affected
status-firefox-esr52: --- → affected
tracking-firefox58: --- → +
tracking-firefox59: --- → +
tracking-firefox-esr52: --- → ?
Attachment #8938093 - Flags: sec-approval? → sec-approval+
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.
Flags: needinfo?(gchang)
(Assignee)

Comment 19

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

Comment 23

a year ago
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.
(Assignee)

Comment 24

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

Comment 25

a year ago
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.
Flags: needinfo?(jorendorff)
Attachment #8941698 - Flags: review+
Attachment #8941698 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/a2cf3f843d0b
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment on attachment 8941698 [details] [diff] [review]
Better error messages for invalid structured clone data

Fix a sec-high. Beta58+.
Flags: needinfo?(gchang)
Attachment #8941698 - Flags: approval-mozilla-release+
Attachment #8941698 - Flags: approval-mozilla-beta?
Attachment #8941698 - Flags: approval-mozilla-beta+
(Assignee)

Comment 28

a year ago
The patch is now in beta due to today's central->beta merge. Will push to release.
(Assignee)

Comment 29

a year ago
uplift
https://hg.mozilla.org/releases/mozilla-release/rev/f4256fe890db
status-firefox58: affected → fixed
Whiteboard: [adv-main58+]
Flags: qe-verify-
Whiteboard: [adv-main58+] → [adv-main58+][post-critsmash-triage]
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.
Flags: needinfo?(jorendorff)
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
(Assignee)

Comment 33

a year ago
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
Attachment #8943705 - Attachment description: Better error messages for invalid structured clone data → Better error messages for invalid structured clone data (for esr52)
Flags: needinfo?(jorendorff)
Attachment #8943705 - Flags: review+
Attachment #8943705 - Flags: approval-mozilla-esr52?
tracking-firefox-esr52: ? → 58+
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+
Attachment #8943705 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
https://hg.mozilla.org/releases/mozilla-esr52/rev/410da936a1e8
status-firefox-esr52: affected → fixed
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
Group: javascript-core-security → core-security-release
Whiteboard: [adv-main58+][post-critsmash-triage] → [adv-main58+][adv-esr52.6+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.