1348116 - Add a whitelist for sync IPC messages triggered by JS

Status: NEW

(Core :: IPC, enhancement, P3)

Description

[(no longer active)]

We currently have no protection against new sync IPC messages being added by JS.  We obviously can't protect against this at compile time.  I suggest having a list of whitelisted messages that we check the argument to sendRpcMessage() against and if it doesn't match one of the known ones, we abort.

We should be careful to not do this for add-ons.  The situation for add-ons is actually quite sad.  A *lot* of add-ons seem to be using sync IPC <https://dxr.mozilla.org/addons/search?q=sendRpcMessage&redirect=false>.  Kris, is there anything that we can do about this, besides waiting for the 57 release?

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

sendSyncMessage is actually the main way of doing sync RPC. sendRpcMessage is what you do if you're using CPOWs.

One problem we have is that add-on frame scripts (which is where I would expect most of these calls happen) run in the same TabChildGlobal as our own frame scripts. We're somewhat constrained here because we don't want to create an extra global for every tab--that would be pretty expensive. We might be able to tell if an add-on is running through stack inspection, but that's always a little iffy. I think that checking the whitelist only on Tinderbox is probably the best we can realistically do.

Comment 2

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #0)
> We should be careful to not do this for add-ons.  The situation for add-ons
> is actually quite sad.  A *lot* of add-ons seem to be using sync IPC
> <https://dxr.mozilla.org/addons/search?q=sendRpcMessage&redirect=false>. 
> Kris, is there anything that we can do about this, besides waiting for the
> 57 release?

A bit, but probably not much. We can definitely add warnings to the AMO validator (and probably console warnings), and make changes to the AMO review policy to forbid new sync RPC. We can probably also do some outreach to authors of the most popular add-ons to try to get them fixed.

On the upside, It looks like a lot of the search results for sendSyncMessage are from an ancient version of the add-on SDK that doesn't work in recent Firefox versions. And a lot of others (I'm not sure this should really be considered a positive thing) don't even use the response, so don't really have a reason to use sync messaging at all.

In the mean time, I suppose we can just exempt add-on compartments from the whitelist...

Comment 3

[Kris Maglione [:kmag]]

(In reply to Bill McCloskey (:billm) from comment #1)
> One problem we have is that add-on frame scripts (which is where I would
> expect most of these calls happen) run in the same TabChildGlobal as our own
> frame scripts. We're somewhat constrained here because we don't want to
> create an extra global for every tab--that would be pretty expensive. We
> might be able to tell if an add-on is running through stack inspection, but
> that's always a little iffy. I think that checking the whitelist only on
> Tinderbox is probably the best we can realistically do.

Hm... Well, for the frame message manager, we should at least be able to give
add-on scripts their own sendSyncMessage and sendRpcMessage wrapper functions.
If they're sending messages on the cpmm, hopefully they're doing it from a
JSM, rather than a frame script, so we should probably be fine there.

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information