Closed Bug 1425612 Opened 3 years ago Closed 3 years ago

StructuredClone crash reading invalid data


(Core :: JavaScript Engine, defect, P1)




Tracking Status
firefox-esr52 58+ fixed
firefox57 --- wontfix
firefox58 + fixed
firefox59 + fixed


(Reporter: jorendorff, Assigned: jorendorff)


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


(4 files, 1 obsolete file)

let bytes = new Uint8Array([
let cloneBuffer = serialize(null);
cloneBuffer.arraybuffer = bytes.buffer;
let obj = deserialize(cloneBuffer);
Crashes here:

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).
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 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
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.
Attachment #8937282 - Attachment is obsolete: true
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+
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.
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)
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-criticalsec-high
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?


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.
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)
Bug 1425612 - Better error messages for invalid structured clone data. r=sfink, a=abillings.
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.
Bug 1425612 - Better error messages for invalid structured clone data. r=sfink, a=abillings.
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?
Closed: 3 years ago
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+
The patch is now in beta due to today's central->beta merge. Will push to release.
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)
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?
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+
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.