775777 - IPDL actor deserialization allows type punning from maliciously-constructed packets

Status: RESOLVED FIXED

(Core :: IPC, defect)

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Whups, fat-fingered enter.

Currently all managed actors are stored in a single hash, and on deserialization we statically cast the lookup-up generic pointer to the more specific type specified by the message decl.  This is fine for trusted content, but with compromised processes can send specifically-crafted messages that allow type-punning a concrete PFooParent* as PBarParent*, with predicable dangerous results.

To fix this, we need either managee-specific storage, or need to store type information along with the actor map and then do a dynamic type check on lookup.  The latter is probably easier.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

A good target for IPC fuzzing is finding this bug independently.  However, I need to fix it sooner rather than later.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Test

This is a fairly nontrivial test, so important to get on the same page before the fix.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Urge ... to ... refactor ... building ...

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Check dynamic actor type when deserializing

$ MOZ_IPC_MESSAGE_LOG=1 ~/mozilla/iff-dbg/dist/bin/run-mozilla.sh ~/mozilla/iff-dbg/dist/bin/ipdlunittest TestActorPunning
[snip]
[time:1348105394788337][27305][PTestActorPunningParent] Received Msg_Pun([TODO])
Protocol error: actor that should be of type PTestActorPunning has different type

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 662768 [details] [diff] [review]
Check dynamic actor type when deserializing

Oops, there's a bug here that the rest of the tests caught.  Sorry for the spam.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Check dynamic actor type when deserializing, v1.1

Comment 9

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

Comment on attachment 662776 [details] [diff] [review]
Check dynamic actor type when deserializing, v1.1

Review of attachment 662776 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

::: ipc/glue/AsyncChannel.h
@@ +72,5 @@
>          virtual Result OnMessageReceived(const Message& aMessage) = 0;
>          virtual void OnProcessingError(Result aError) = 0;
> +        // FIXME/bug 792652: this doesn't really belong here, but a
> +        // large refactoring is needed to put it where it belongs.
> +        virtual int32_t GetTypeTag() = 0;

Nit: Can we make this more explicit (GetProtocolId? GetProtocolTypeId?)

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

OK.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c9af3e022da
https://hg.mozilla.org/integration/mozilla-inbound/rev/b68455ccdd9a

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7c9af3e022da
https://hg.mozilla.org/mozilla-central/rev/b68455ccdd9a