1647118 - Assertion failure: false (Failed to read StructuredCloneData. Data incomplete), at vm/StructuredClone.cpp:203

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The attached testcase crashes on mozilla-central revision 20200620-81ed5bedd083 (debug-fuzzing build, run with FUZZER=StructuredCloneReader ./fuzz-tests test.bin).

Backtrace:

==8181==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x000000000000 (pc 0x5559cea67173 bp 0x7ffcf3698e70 sp 0x7ffcf3698e40 T8181)
==8181==The signal is caused by a WRITE memory access.
==8181==Hint: address points to the zero page.
    #0 0x5559cea67172 in js::BufferIterator<unsigned long, js::SystemAllocPolicy>::operator+=(unsigned long) (dist/bin/fuzz-tests+0x6c1172)
    #1 0x5559cea6f0bf in JSStructuredCloneReader::readTransferMap() (dist/bin/fuzz-tests+0x6c90bf)
    #2 0x5559cea586f6 in JSStructuredCloneReader::read(JS::MutableHandle<JS::Value>) (dist/bin/fuzz-tests+0x6b26f6)
    #3 0x5559cea5853c in ReadStructuredClone(JSContext*, JSStructuredCloneData const&, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, JSStructuredCloneCallbacks const*, void*) (dist/bin/fuzz-tests+0x6b253c)
    #4 0x5559cea7071d in JS_ReadStructuredClone(JSContext*, JSStructuredCloneData const&, unsigned int, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, JSStructuredCloneCallbacks const*, void*) (dist/bin/fuzz-tests+0x6ca71d)
    #5 0x5559ce9b298c in testStructuredCloneReaderFuzz(unsigned char const*, unsigned long) (dist/bin/fuzz-tests+0x60c98c)
    #6 0x5559cea43b80 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (dist/bin/fuzz-tests+0x69db80)
    #7 0x5559cea37266 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (dist/bin/fuzz-tests+0x691266)
    #8 0x5559cea3a874 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (dist/bin/fuzz-tests+0x694874)
    #9 0x5559ce9b7a36 in main (dist/bin/fuzz-tests+0x611a36)
    #10 0x7f3f9fa15b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
    #11 0x5559ce98fdb2 in _start (dist/bin/fuzz-tests+0x5e9db2)

UndefinedBehaviorSanitizer can not provide additional info.
SUMMARY: UndefinedBehaviorSanitizer: SEGV (dist/bin/fuzz-tests+0x6c1172) in js::BufferIterator<unsigned long, js::SystemAllocPolicy>::operator+=(unsigned long)
==8181==ABORTING

Marking s-s because it's unclear what incomplete data in the buffer could cause (uninitialized memory?).

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Christian Holler (:decoder)]

Steve, can you please check if the assert in question has any kind of security impact?

It's this one: https://searchfox.org/mozilla-central/rev/f66f5a235e7d74c29b951316f73001126a056734/js/src/vm/StructuredClone.cpp#203

Comment 3

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

I spent a little time looking at this so far. More tomorrow. I think this is probably harmless and without the assertion would either crash or quickly throw a JS exception saying that the input data is too short, but I haven't managed to figure out exactly how this is failing yet. I loaded that binary data and used it as a shell test case, but it didn't crash (or even error out) there so I must be doing it wrong. I can reproduce with the fuzz-tests binary, but when I run that under rr I don't seem to have the symbols for the spidermonkey code or something. I'll need to look at how that works.

Comment 4

[Christian Holler (:decoder)]

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

I spent a little time looking at this so far. More tomorrow. I think this is probably harmless and without the assertion would either crash or quickly throw a JS exception saying that the input data is too short, but I haven't managed to figure out exactly how this is failing yet. I loaded that binary data and used it as a shell test case, but it didn't crash (or even error out) there so I must be doing it wrong. I can reproduce with the fuzz-tests binary, but when I run that under rr I don't seem to have the symbols for the spidermonkey code or something. I'll need to look at how that works.

Running in the fuzz-tests binary is the right approach, however, I also noticed the symbols problem. I don't know exactly what is causing that at the moment, it could be a build system issue/misconfig of some sort. Did you try loading the testcase in the shell and use deserialize on it?

Comment 5

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

(In reply to Christian Holler (:decoder) from comment #4)

Did you try loading the testcase in the shell and use deserialize on it?

Yes, I did. It produced a floating point number, no error. It doesn't seem to be going through the correct paths. I'm looking into it now.

Comment 6

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

Er, wait. My local build does have good symbols. Either I misread before, or the last thing I did yesterday fixed it.

Anyway, that gives me the exact path to the problem. This would not actually read past the end of the data. The assert is only on advancing a pointer. The next access would check the size before reading anything. So this is not a security concern.

It is a bug; this should be throwing an exception, not asserting.

Comment 7

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

Attachment: Bug 1647118 - Throw error on short read in readTransferMap

Comment 8

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

Un-hiding; this would correctly throw a short read error in the next call.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb63fec29274
Throw error on short read in readTransferMap r=jonco

Comment 10

[Natalia Csoregi [:nataliaCs]]

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