1465860 - MOZ_CRASH in JS IPC under fuzzing

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Alex Gaynor [:Alex_Gaynor]]

This is not a security issue, but it is a blocker to the efficiency of fuzzing:

https://searchfox.org/mozilla-central/source/js/ipc/JavaScriptShared.h#29-34

==30571==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f77437575df bp 0x7ffc850e35b0 sp 0x7ffc850e35a0 T0)
==30571==The signal is caused by a WRITE memory access.
==30571==Hint: address points to the zero page.
    #0 0x7f77437575de in mozilla::jsipc::ObjectId::ObjectId(unsigned long, bool) /home/osboxes/mozilla-central/js/ipc/JavaScriptShared.h:33:13
    #1 0x7f77437576c9 in mozilla::jsipc::ObjectId::deserialize(unsigned long) /home/osboxes/mozilla-central/js/ipc/JavaScriptShared.h:53:16
    #2 0x7f774375ec9c in mozilla::jsipc::JavaScriptBase<mozilla::jsipc::PJavaScriptParent>::RecvPreventExtensions(unsigned long const&, mozilla::jsipc::ReturnStatus*) /home/osboxes/mozilla-central/js/ipc/JavaScriptBase.h:34:44
    #3 0x7f77431f5f63 in mozilla::jsipc::PJavaScriptParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/ipc/ipdl/PJavaScriptParent.cpp:1547:20
    #4 0x7f774301c01e in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/ipc/ipdl/PContentParent.cpp:7850:28
    #5 0x7f774c891d2c in void mozilla::ipc::FuzzProtocol<mozilla::dom::ContentParent>(mozilla::dom::ContentParent*, unsigned char const*, unsigned long, nsTArray<nsTString<char> > const&) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/ProtocolFuzzer.h:47:18
    #6 0x7f774c8918ea in RunContentParentIPCFuzzing(unsigned char const*, unsigned long) /home/osboxes/mozilla-central/dom/ipc/fuzztest/content_parent_ipc_libfuzz.cpp:33:3
    #7 0x5897b0 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:517:13


The ideal solution would be to return an error from ObjectId::deserialize and having RecvPreventExtensions return |IPC_FAIL_NO_REASON| in that case. (This will probably apply to all the methods on PJavaScriptParent. You'll make the fuzzer look super good if I end up filing a seperate bug for each of them, but I think we'll all be happier if I don't :D)

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1465860 - don't crash in JS IPC on invalid object id

Instead, return an error up to the caller, who can return an IPC error, which
will kill the child. This is significantly friendlier to fuzzing.

Review commit: https://reviewboard.mozilla.org/r/248514/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248514/

Comment 2

[Tom S]

Comment on attachment 8982525 [details]
Bug 1465860 - don't crash in JS IPC on invalid object id

https://reviewboard.mozilla.org/r/248514/#review254984

::: js/ipc/JavaScriptBase.h:35
(Diff revision 1)
>  
>      /*** IPC handlers ***/
>  
>      mozilla::ipc::IPCResult RecvPreventExtensions(const uint64_t& objId, ReturnStatus* rs) override {
> -        if (!Answer::RecvPreventExtensions(ObjectId::deserialize(objId), rs)) {
> +        Maybe<ObjectId> obj(ObjectId::deserialize(objId));
> +        if (!obj.isSome() || !Answer::RecvPreventExtensions(obj.value(), rs)) {

I am not quite sure what the suggest patterns for Maybe are, but using isNothing instead of !isSome seems better. (Looking at the documentation for mozilla::Maybe there is also a bool operator?)

::: js/ipc/JavaScriptLogging.h:172
(Diff revision 1)
>                break;
>            }
>            case JSVariant::TObjectVariant: {
>                const ObjectVariant& ovar = value.get_ObjectVariant();
>                if (ovar.type() == ObjectVariant::TLocalObject)
> -                  formatObject(incoming, true, ObjectId::deserialize(ovar.get_LocalObject().serializedId()), out);
> +                  formatObject(incoming, true, ObjectId::deserialize(ovar.get_LocalObject().serializedId()).value(), out);

Can you clarify why this unchecked value() and few other below ar ok?

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8982525 [details]
Bug 1465860 - don't crash in JS IPC on invalid object id

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248514/diff/1-2/

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8982525 [details]
Bug 1465860 - don't crash in JS IPC on invalid object id

https://reviewboard.mozilla.org/r/248514/#review254984

> Can you clarify why this unchecked value() and few other below ar ok?

There were two mistakes I made; a) I assumed `Maybe<T>::value` used `MOZ_RELEASE_ASSERT` so it wasn't a big deal (it actually uses `MOZ_DIAGNOSTIC_ASSERT`), b) I misunderstood the context of this code and thought these IDs weren't via IPC, I'm not confident that's true.

I've changed the code to `MOZ_RELEASE_ASSERT(objId.isSome())`. This should be consistent with current behavior, and safe.

Comment 5

[Tom S]

Comment on attachment 8982525 [details]
Bug 1465860 - don't crash in JS IPC on invalid object id

https://reviewboard.mozilla.org/r/248514/#review255148

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f29a4a9da70
Don't crash in JS IPC on invalid object id. r=evilpie

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/6f29a4a9da70