1644816 - Assertion failure: getSlot(slot).isUndefined(), at vm/NativeObject.h:1018 with StructuredClone deserialization

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20200610-b2df79a80c03 (fuzzing-debug build, run with FUZZER=StructuredCloneReader dist/bin/fuzz-tests test.bin):

See attachment.

Backtrace:

==23281==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x000000000000 (pc 0x55cdde1514aa bp 0x7ffc974f9120 sp 0x7ffc974f9100 T23281)
==23281==The signal is caused by a WRITE memory access.
==23281==Hint: address points to the zero page.
    #0 0x55cdde1514a9 in js::NativeObject::initSlot(unsigned int, JS::Value const&) (dist/bin/fuzz-tests+0x7c64a9)
    #1 0x55cdde48645d in js::SavedFrame::initParent(js::SavedFrame*) (dist/bin/fuzz-tests+0xafb45d)
    #2 0x55cdde08c9ae in JSStructuredCloneReader::read(JS::MutableHandle<JS::Value>) (dist/bin/fuzz-tests+0x7019ae)
    #3 0x55cdde08c24c in ReadStructuredClone(JSContext*, JSStructuredCloneData const&, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, JSStructuredCloneCallbacks const*, void*) (dist/bin/fuzz-tests+0x70124c)
    #4 0x55cdde0a37ad in JS_ReadStructuredClone(JSContext*, JSStructuredCloneData const&, unsigned int, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, JSStructuredCloneCallbacks const*, void*) (dist/bin/fuzz-tests+0x7187ad)
    #5 0x55cdddf9a2dc in testStructuredCloneReaderFuzz(unsigned char const*, unsigned long) (dist/bin/fuzz-tests+0x60f2dc)
    [...]
    #11 0x55cdddf76db2 in _start (dist/bin/fuzz-tests+0x5ebdb2)

Marking s-s until triaged as StructuredClone deserialization ocurrs in IPC and bugs in it can potentially be used for sandbox escapes.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Unable to reproduce bug using the following builds:
> mozilla-central 20200611093454-10ad7868f3ca
> mozilla-central 20200610043607-b2df79a80c03
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.

Comment 3

[Nicolas B. Pierron [:nbp]]

Setting severity to S3, despite not knowing the impact on the user base.

Comment 4

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

Attachment: Bug 1644816 - Track number of deserialized SavedFrame children to fix error handling

Comment 5

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

I believe this would be harmless in practice. When deserializing a SavedFrame, it must have a single parent stored (which can be a null value). If no parent were given, then the parent field would never be initialized, and would be left at its initial UndefinedValue. This will result in a weird state, since SavedFrame assumes that its parent field holds either an ObjectValue or a NullValue, but the getParent() accessor will treat an UndefinedValue as if it were a NullValue and so everything will work.

If multiple parents are stored in the stream, then the parent field will be initialized multiple times. That's not supposed to happen, and is what triggers the assertion in this case. But in practice, each parent will clobber its predecessors, and it will behave as if only the final parent were in the stream. The earlier ones will not have their pre-barriers called, since we expect these to be initializations, but they are no longer reachable and so violating snapshot-at-the-beginning will be harmless.

That said, both of these cases should really be an error. Unfortunately, the current code doesn't have a place to store the number of parents seen while reading. It could be done by checking for UndefinedValue in the parent slot, but that seems a little gross because this state would only be exposed for the purpose of structured cloning; nothing else could ever observe the partially initialized state.

Still, it's by far the most straightforward solution to implement.

Comment 6

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

I'm trying a fix that maintains a stack of "property" counters (in this case, just the SavedFrame parent count) alongside the object stack. It parallels the counts field used while writing. On the plus side, it provides a lot of flexibility for adding other serializations that need something similar. On the minus side, it's extra memory usage during deserialization that is only used for error checking.

An intermediate solution would be to maintain the stack only for SavedFrame objects (and anything else in the future that opts in). Since SavedFrames are rare, this would basically eliminate the memory issue. But it's a little more complex, and no longer precisely parallels the serialization code.

Comment 7

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

Attachment: Bug 1644816 - Validate that a SavedFrame has a single parent when deserializing

Comment 8

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

This isn't a security bug after all. It is invalidating one of our invariants: you should only be able to initialize a slot once (that's why it's called "initialization", duh). The reason for the invariant is that initialization is allowed to skip the pre-barrier. But in this case, the previous value being overwritten cannot escape to anywhere else. (From this location, at least -- it can be referred to from somewhere else, but this particular edge can't be the only record of it in a way that would mess up incremental GC.) So the only effect of the missing pre-barrier is that the old value gets cleaned up a little earlier, and then only if it is safe to do so.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca7a13a93a5d
Track number of deserialized SavedFrame children to fix error handling r=jonco

Comment 10

[Razvan Maries]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Is this something that should ride the trains or is there value in uplifting?

Comment 12

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

In terms of the product, I don't think there's a need to uplift. The only reason to uplift would be if it were in the way of fuzzing.

Comment 13

[Christian Holler (:decoder)]

From a fuzzing perspective, no uplift is required here.