1736046 - Assertion failure: data.Size() % 8 == 0, at vm/StructuredClone.cpp:727

Status: RESOLVED FIXED

(Core :: DOM: postMessage, defect, P1)

Description

[Christian Holler (:decoder)]

Using the fuzzing target in bug 1590068 (a target that uses dom::ipc::StructuredCloneData as its entry point to cover the StructuredCloneReader code that is browser-only), I was able to find the following crash on mozilla-central revision 9b9f8bfe2625+.

Backtrace:

Assertion failure: data.Size() % 8 == 0, at vm/StructuredClone.cpp:727

==15383==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fb64f4eb133 bp 0x7ffdfbf3ae10 sp 0x7ffdfbf3abf0 T15383)
    #0 0x7fb64f4eb133 in SCInput js/src/vm/StructuredClone.cpp:727:3
    #1 0x7fb64f4eb133 in ReadStructuredClone(JSContext*, JSStructuredCloneData const&, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, JSStructuredCloneCallbacks const*, void*) js/src/vm/StructuredClone.cpp:703:11
    #2 0x7fb64f5001c0 in JS_ReadStructuredClone(JSContext*, JSStructuredCloneData const&, unsigned int, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, JSStructuredCloneCallbacks const*, void*) js/src/vm/StructuredClone.cpp:3382:10
    #3 0x7fb64f501358 in JSAutoStructuredCloneBuffer::read(JSContext*, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, JSStructuredCloneCallbacks const*, void*) js/src/vm/StructuredClone.cpp:3508:12
    #4 0x7fb64a7c20af in Read dom/base/StructuredCloneHolder.cpp:289:22
    #5 0x7fb64a7c20af in mozilla::dom::StructuredCloneHolder::Read(nsIGlobalObject*, JSContext*, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, mozilla::ErrorResult&) dom/base/StructuredCloneHolder.cpp:375:35
    #6 0x7fb64a7bff0e in mozilla::dom::StructuredCloneHolder::Read(nsIGlobalObject*, JSContext*, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) dom/base/StructuredCloneHolder.cpp:362:10
    #7 0x7fb64cf9e66a in mozilla::dom::ClonedErrorHolder::ToErrorValue(JSContext*, JS::MutableHandle<JS::Value>) dom/ipc/ClonedErrorHolder.cpp:263:10
    #8 0x7fb64cf9e464 in mozilla::dom::ClonedErrorHolder::ReadStructuredClone(JSContext*, JSStructuredCloneReader*, mozilla::dom::StructuredCloneHolder*) dom/ipc/ClonedErrorHolder.cpp:225:43
    #9 0x7fb64a7c4736 in mozilla::dom::StructuredCloneHolder::CustomReadHandler(JSContext*, JSStructuredCloneReader*, JS::CloneDataPolicy const&, unsigned int, unsigned int) dom/base/StructuredCloneHolder.cpp:1009:12
    #10 0x7fb64f4fb205 in JSStructuredCloneReader::startRead(JS::MutableHandle<JS::Value>, js::gc::InitialHeap) js/src/vm/StructuredClone.cpp:2814:11
    #11 0x7fb64f4eb2b2 in JSStructuredCloneReader::read(JS::MutableHandle<JS::Value>, unsigned long) js/src/vm/StructuredClone.cpp:3226:8
    #12 0x7fb64f4eb071 in ReadStructuredClone(JSContext*, JSStructuredCloneData const&, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, JSStructuredCloneCallbacks const*, void*) js/src/vm/StructuredClone.cpp:705:12
    #13 0x7fb64f5001c0 in JS_ReadStructuredClone(JSContext*, JSStructuredCloneData const&, unsigned int, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, JSStructuredCloneCallbacks const*, void*) js/src/vm/StructuredClone.cpp:3382:10
    #14 0x7fb64a7c29fd in mozilla::dom::StructuredCloneHolder::ReadFromBuffer(nsIGlobalObject*, JSContext*, JSStructuredCloneData&, unsigned int, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, mozilla::ErrorResult&) dom/base/StructuredCloneHolder.cpp:409:8
    #15 0x7fb64a7c2954 in mozilla::dom::StructuredCloneHolder::ReadFromBuffer(nsIGlobalObject*, JSContext*, JSStructuredCloneData&, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, mozilla::ErrorResult&) dom/base/StructuredCloneHolder.cpp:395:3
    #16 0x7fb64d0123a1 in mozilla::dom::ipc::StructuredCloneData::Read(JSContext*, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, mozilla::ErrorResult&) dom/ipc/StructuredCloneData.cpp:116:3
    #17 0x7fb64d00cade in mozilla::dom::ipc::StructuredCloneData::Read(JSContext*, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) dom/ipc/StructuredCloneData.cpp:104:3
    #18 0x7fb648137ddc in FuzzingRunDomSC(unsigned char const*, unsigned long) dom/base/fuzztest/FuzzStructuredClone.cpp:61:10
    [...]
    #29 0x55884436efe8 in _start (objdir-ff-fuzzing-debug/dist/bin/firefox+0x52fe8)

The target code has not landed on mozilla-central yet, if you need it to reproduce (in case stack + test attached do not suffice), you will need to rebuild with the patch in bug 1590068 (feel free to ping me for instructions).

Also marking s-s because the parent fuzzing bug is still locked and it's unclear what caused the misalignment here.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Testcase

Comment 3

[Christian Holler (:decoder)]

Attachment: Testcase for comment 2

Comment 4

[Ted Campbell [:tcampbell]]

An debug-assert is being used instead of a dynamic check. Probably just checking in ReadStructuredClone is sufficient. I believe sfink is already investigating this and we should try to close this soon to help fuzzing out.

Comment 5

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

What's happening is that there are a couple of DOM types that store a data member as a nested serialization, and that inner data can have an invalid length that should be checked for somewhere. I found calls to nested deserialization for Blobs and Errors.

Comment 6

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

Attachment: Bug 1736046 - Validate length of nested structured serialization data

Comment 7

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

Switching component to something more appropriate, since this is code outside of the JS engine.

It looks like the effect of getting a corrupted length would be reading out of bounds, then looping while trying to decrement the length to zero, then hitting OOM and returning failure. So this looks to me like it may not need to be sec-high.

Comment 8

[Jens Stutte [:jstutte]]

(In reply to Steve Fink [:sfink] [:s:] from comment #5)

What's happening is that there are a couple of DOM types that store a data member as a nested serialization, and that inner data can have an invalid length that should be checked for somewhere. I found calls to nested deserialization for Blobs and Errors.

I might not have enough context and I am happy with the check to solve the sec implications, but it reads as if we should also find the culprit who stores that invalid length? Is it happening here?

Comment 10

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

Comment on attachment 9248778 [details]
Bug 1736046 - Validate length of nested structured serialization data

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I couldn't come up with a way. The only risk I see is it giving someone the idea to fuzz this interface and find other ways to exploit it.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: It's a trivial patch, I'm guessing it will either apply or require some minor code motion.
• How likely is this patch to cause regressions; how much testing does it need?: It should be safe, this will have no effect in normal situations.

Comment 11

[Tom Ritter [:tjr]]

Comment on attachment 9248778 [details]
Bug 1736046 - Validate length of nested structured serialization data

Approved to land and request uplift

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Landed on autoland:
https://hg.mozilla.org/integration/autoland/rev/707e4d920d25fe5dd249c9cdca400c241a8fa0c8

This grafts cleanly to Beta & ESR91, so go ahead and request uplift at your earliest convenience.

Comment 13

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

Comment on attachment 9248778 [details]
Bug 1736046 - Validate length of nested structured serialization data

Beta/Release Uplift Approval Request

• User impact if declined: none
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce: (The fuzzer will test this patch. We don't have a mechanism for committing specific tests for it, though once the fuzzing stuff lands it ought to be easy to do.)
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This changes behavior in a condition that never happens unless you're going through a fuzzing interface or something else is compromised.
• String changes made/needed: none

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This would be for avoiding rebasing issues with potential future sec-high/sec-crit bugs.
• User impact if declined: none
• Fix Landed on Version: 96
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): see above
• String or UUID changes made by this patch: none

Comment 14

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

Validate length of nested structured serialization data r=baku
https://hg.mozilla.org/integration/autoland/rev/707e4d920d25fe5dd249c9cdca400c241a8fa0c8
https://hg.mozilla.org/mozilla-central/rev/707e4d920d25

Comment 15

[Pascal Chevrel:pascalc]

Comment on attachment 9248778 [details]
Bug 1736046 - Validate length of nested structured serialization data

Approved for 95 beta 11, thanks.

Comment 16

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/3a4623786fbc

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9248778 [details]
Bug 1736046 - Validate length of nested structured serialization data

Approved for 91.4esr.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/45d3fd63bd24

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 20

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

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #19)

Please visit this google form to reply.

done