Closed Bug 1465860 Opened Last year Closed Last year

MOZ_CRASH in JS IPC under fuzzing

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: Alex_Gaynor, Assigned: Alex_Gaynor)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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)
Attachment #8982525 - Flags: review?(evilpies)
Assignee: nobody → agaynor
See Also: → 1465911
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 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 on attachment 8982525 [details]
Bug 1465860 - don't crash in JS IPC on invalid object id

https://reviewboard.mozilla.org/r/248514/#review255148
Attachment #8982525 - Flags: review?(evilpies) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6f29a4a9da70
Don't crash in JS IPC on invalid object id. r=evilpie
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6f29a4a9da70
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.