967176 - Faulty: Assertion failure: (nbytes & 7) == 0, at js/src/vm/StructuredClone.cpp:414

Status: RESOLVED DUPLICATE of bug 1743515

(Core :: JavaScript Engine, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Faulty session

Found by Christoph Diehl's "Faulty" fuzzer, see bug 777067

Reproduces on desktop linux debug build with tabs.remote pref, with the Faulty IPC fuzzer, using this environment:

FAULTY_SEED=3 FAULTY_PICKLE=1 FAULTY_PARENT=1 FAULTY_ENABLE_LOGGING=1 FAULTY_PROBABILITY=10

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Classification: non-gfx, needs JS people to look at it

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Ben, whom should we ask to look into this?

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Hi Jason, we're starting to fuzz messages that come from child processes (for B2G especially, but this is vital for electrolysis too). If a hacked child process sends bad structured clone data we're at least seeing this one assertion. Who would be able to look into the structured clone code to make sure that the parent process can't be compromised in this way?

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

CC'ing sfink and bhackett too since they've at least looked at the structured clone code before.

Comment 5

[(no longer active)]

The JS engine is not doing anything wrong here, we should just not make it deal with something which violates its assumptions.

Comment 6

[(no longer active)]

Attachment: Protect against some of the values that invalidate the JS engine assumptions when reading structured clones; r=bzbarsky

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8371158 [details] [diff] [review]
Protect against some of the values that invalidate the JS engine assumptions when reading structured clones; r=bzbarsky

There are a lot more places than just this that read in structured clone data from a potentially untrusted source (the IPC wire, the disk, etc).

I really think the JS engine should just deal here.

Comment 8

[(no longer active)]

Fair enough!  (and thanks for the review)

Comment 9

[Jason Orendorff [:jorendorff]]

Agreed. Taking.

The serialization format here was *originally* designed to be very nicely behaved in the face of malicious data: report an error and return false; don't assert, don't abort, and by no means trigger undefined behavior.

But we have drifted away from this. For example, this line in js/src/vm/StructuredClone.cpp
        MOZ_ASSUME_UNREACHABLE("unknown TypedArrayObject type");
should be replaced with a JS_ReportErrorNumber call.

I'll document this requirement and review the code to make sure we implement it, as before.

Comment 10

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

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Comment 11

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

This specific problem ended up being fixed in bug 1743515, though there have been some other fixes for the more general problem of non trusting DifferentProcess/ForStorage clone data.