1912471 - (CVE-2024-9396) Assertion failure: data == JS::SCTAG_TMO_ALLOC_DATA || data == JS::SCTAG_TMO_MAPPED_DATA, at js/src/vm/StructuredClone.cpp:3457

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Nils Bars]

Attachment: bug.js

Steps to reproduce:

Checkout commit ef4ef8198add3192d1e49157fb3f377ea7e60009 and invoke the js shell as follows:

js --fuzzing-safe <test-case>

Actual results:

Assertion failure: data == JS::SCTAG_TMO_ALLOC_DATA || data == JS::SCTAG_TMO_MAPPED_DATA, at js/src/vm/StructuredClone.cpp:3457

Comment 1

[Andrew McCreight [:mccr8]]

Please add a stack for the assertion, to make it easier to triage. Thanks.

Comment 2

[Nils Bars]

==2303777==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x000000000000 (pc 0x5653917a8ee7 bp 0x7fff4a338de0 sp 0x7fff4a338c80 T2303777)
==2303777==The signal is caused by a WRITE memory access.
==2303777==Hint: address points to the zero page.
    #0 0x5653917a8ee7 in JSStructuredCloneReader::readTransferMap() spidermonkey/src/js/src/vm/StructuredClone.cpp:3456:7
    #1 0x56539179295a in JSStructuredCloneReader::read(JS::MutableHandle<JS::Value>, unsigned long) spidermonkey/src/js/src/vm/StructuredClone.cpp:3893:8
    #2 0x565391792893 in ReadStructuredClone(JSContext*, JSStructuredCloneData const&, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, JSStructuredCloneCallbacks const*, void*) spidermonkey/src/js/src/vm/StructuredClone.cpp:778:12
    #3 0x5653917aab0d in JS_ReadStructuredClone(JSContext*, JSStructuredCloneData const&, unsigned int, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, JSStructuredCloneCallbacks const*, void*) spidermonkey/src/js/src/vm/StructuredClone.cpp:4055:10
    #4 0x565391965fb0 in Deserialize(JSContext*, unsigned int, JS::Value*) spidermonkey/src/js/src/builtin/TestingFunctions.cpp:5991:8
    #5 0x56539113667e in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) spidermonkey/src/js/src/vm/Interpreter.cpp:489:13
    #6 0x5653911358df in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) spidermonkey/src/js/src/vm/Interpreter.cpp:583:12
    #7 0x56539114be61 in js::CallFromStack(JSContext*, JS::CallArgs const&, js::CallReason) spidermonkey/src/js/src/vm/Interpreter.cpp:655:10
    #8 0x56539114be61 in js::Interpret(JSContext*, js::RunState&) spidermonkey/src/js/src/vm/Interpreter.cpp:3357:16
    #9 0x5653911348e1 in js::RunScript(JSContext*, js::RunState&) spidermonkey/src/js/src/vm/Interpreter.cpp:461:13
    #10 0x565391139ac1 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, js::AbstractFramePtr, JS::MutableHandle<JS::Value>) spidermonkey/src/js/src/vm/Interpreter.cpp:848:13
    #11 0x56539113a2cc in js::Execute(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) spidermonkey/src/js/src/vm/Interpreter.cpp:880:10
    #12 0x565391386969 in ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::MutableHandle<JS::Value>) spidermonkey/src/js/src/vm/CompilationAndEvaluation.cpp:495:10
    #13 0x565391386be7 in JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) spidermonkey/src/js/src/vm/CompilationAndEvaluation.cpp:519:10
    #14 0x56539107074e in RunFile(JSContext*, char const*, _IO_FILE*, CompileUtf8, bool, bool) spidermonkey/src/js/src/shell/js.cpp:1205:10
    #15 0x56539106fab5 in Process(JSContext*, char const*, bool, FileKind) spidermonkey/src/js/src/shell/js.cpp
    #16 0x56539102a7ae in ProcessArgs(JSContext*, js::cli::OptionParser*) spidermonkey/src/js/src/shell/js.cpp:11284:10
    #17 0x56539102a7ae in Shell(JSContext*, js::cli::OptionParser*) spidermonkey/src/js/src/shell/js.cpp:11536:12
    #18 0x5653910223c0 in main spidermonkey/src/js/src/shell/js.cpp:12068:12
    #19 0x7f9372dfed8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #20 0x7f9372dfee3f in __libc_start_main csu/../csu/libc-start.c:392:3
    #21 0x565390febd38 in _start (spidermonkey/src/reproducebuild/dist/bin/js+0x1c25d38) (BuildId: 35d8a8a3c389066dde8dc90be8c5997d)

Comment 3

[Iain Ireland [:iain]]

Here's my very slightly cleaned up version of the testcase.

let failOnReadTransfer = makeSerializable(0, 1);
let arr = new ArrayBuffer();
let transferables = [arr, failOnReadTransfer];
let serialized = serialize(arr, transferables);
try {
  deserialize(serialized)
} catch {}
deserialize(serialized)

I don't know the structured clone code well enough to really understand what's going on, but it looks like we're getting wedged in an unexpected state by failing part way through the first attempt at deserialization, and then the second attempt is going wrong.

It's not immediately clear to me whether this is shell-only or whether it could be triggered without access to testing functions.

Steve, any thoughts here?

Comment 4

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

I'm going to guess that this is unlikely to be a security problem, but it does require a code audit.

The internal state of what is owned vs unowned is being properly tracked, but then the unexpected combination (where we're reading an ArrayBuffer from the Transferable list a second time) is not anticipated. In a DEBUG build, it asserts. In an opt build, it does the right thing and ignores it.

In order for this to be an actual problem, it'll require two things from an in-browser readTransfer hook: (1) it can fail, and (2) it does something problematic when reading unowned data. (Custom readTransfer hooks do not ignore unowned data, unlike ArrayBuffers, so the potential is there.) (2) will probably require not failing again before doing anything sketchy, which is theoretically possible because intervening code could change something to fix the failure, but it's still unlikely.

I will audit all readTransfer hooks in the browser to see if we have any like that.

Comment 5

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

Btw, this is an excellent test case that I should have written myself in the first place! I just barely missed this in js/src/jit-test/tests/structured-clone/transferable-cleanup.js. Fuzzers are awesome.

Comment 6

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

Ugh, that's a lot of hairy Gecko code that I don't really understand.

SCTAG_DOM_MAP_MESSAGEPORT: can fail if the port ID is larger than the number of ports. So if you Transferred a MessagePort from a hacked content process that sends an invalid ID, then more MessagePorts get created and the clone is deserialized again, you could end up with the wrong port. But the reader isn't going to read again after a failed deserialization, so this won't happen. Still, it depends on a lot of details.

Then again, you could do just as much damage by corrupting a port ID to a valid range and reading once. So this must be protected in other ways?

I do see that all StructuredCloneHolders that support transferring are restricted to SameProcess, which defeats any attack like the above. Except for one: mozilla::dom::ipc::StructuredCloneData can sometimes be used across processes.

SCTAG_DOM_CANVAS and _IMAGEBITMAP and _VIDEOFRAME and _AUDIODATA: SameProcess only. It won't deserialize a buffer that already failed deserialization unless there's a pretty weird bug.

SCTAG_DOM_READABLESTREAM and _WRITABLESTREAM and _TRANSFORMSTREAM: Ugh, lots of ways to fail. Lots of different code paths.

I think this is complicated enough that I'm going to assume it's security-sensitive, though it still seems very difficult to trigger in practice. sec-medium?

Comment 7

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

I originally wanted to detect this on a per-Transferable basis, but it got messy because stored ArrayBuffers initialize their ownership to SCO_TMO_UNOWNED. So the simplest thing seems to do a whole-buffer check, where you're just not allowed to deserialize twice if there are Transferables in the buffer.

This does add a new restriction, since deserializing multiple times isn't fundamentally a problem. You could even imagine it being useful to instantiate a template multiple times. The Transferable stuff is a little weird, but it's not like you're going to detach the source data multiple times (that happens during serialization, not during deserialization). But it's a bit of a weird constraint for the custom readTransfer hooks—you'd need to allow them to be invoked multiple times. And they would have to know how to skip over their data in the case where the ownership has already been transferred.

Anyway, at least for now this seems like a reasonable restriction. You can still deserialize non-Transferable buffers as many times as you want. (And that is commonly done, given that they can be written to disk or whatever.) The semantics of transferring ownership from a single source to multiple destinations are unclear and it seems reasonable to disallow it until it's needed for something. At that point, then each individual Transferable will need to start paying attention to the ownership value; for now, we can continue to assert.

Comment 8

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

Attachment: Bug 1912471 - Disallow deserializing structured clone buffers with transferables more than once

Comment 9

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

Comment on attachment 9421711 [details]
Bug 1912471 - Disallow deserializing structured clone buffers with transferables more than once

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Difficult if possible.
• 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 branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: all
• If not all supported branches, which bug introduced the flaw?: Bug 861925
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: I'd guess it will apply cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: This will break things that expect to be able to deserialize multiple times a clone buffer that contains transferable things. As far as I know, nothing does. But if something does, the error will probably make itself known quickly. So I would expect that existing testing should exercise this adequately.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 10

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 9421711 [details]
Bug 1912471 - Disallow deserializing structured clone buffers with transferables more than once

Approved to land and uplift

Comment 11

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

Attachment: Bug 1912471 - Disallow deserializing structured clone buffers with transferables more than once

Original Revision: https://phabricator.services.mozilla.com/D220644

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/477a37c49b20
Disallow deserializing structured clone buffers with transferables more than once r=iain

Comment 13

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: I could not find one, but it is possible that this exposes a vulnerability. Most likely it'd give a UAF.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: I know of no way of triggering it through the browser, though I can't be certain.
• Risk associated with taking this patch: low
• Explanation of risk level: It makes a previously possible API usage into an error, though it is unlikely that anything is using it that way and even less likely that it is correct if they are.
• String changes made/needed: none
• Is Android affected?: yes

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/477a37c49b20

Comment 15

[Andrew McCreight [:mccr8]]

This kind of sounds like a sec-want hardening measure? We're protecting against a misuse of an API by the browser, but we don't know that any exists.

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/5720aeaffba7

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Given comment 15, I set ESR128 to wontfix, but feel free to nominate for uplift there also if you think we're better off taking it.

Comment 18

[Daniel Veditz [:dveditz]]

Given comment 6 the bounty committee is not 100% convinced this is unexploitable and we're happy this got reported. We're going to go with sec-moderate for an unknown, perhaps unlikely, but possibly bad amount of risk.

Comment 19

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

Comment on attachment 9421711 [details]
Bug 1912471 - Disallow deserializing structured clone buffers with transferables more than once

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: It seems like we decided it's questionable enough to be concerning. Also, it sounds like it may be coming up in fuzzing fairly often.
• User impact if declined: I can't prove it won't give a UAF
• Fix Landed on Version: 132
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Adds some additional error checking. Valid cases won't change.

Comment 20

[Dianna Smith [:diannaS]]

Comment on attachment 9421711 [details]
Bug 1912471 - Disallow deserializing structured clone buffers with transferables more than once

Approved for 128.3esr

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/a0a3a240084e

Comment 22

[Jason Kratzer [:jkratzer]]

Attachment: advisory.txt