967104 - [Inter App Communication API] child process defined fields used in security check in parent

Status: RESOLVED INVALID

(Core Graveyard :: DOM: Apps, defect)

Description

[Rob Fletcher [:omerta]]

dom/apps/src/InterAppCommService.js#814
receiveMessage() in the parent process gets a message and attempts to prevent a
compromised child process from sending commands to the parent in the following
code:

814   receiveMessage: function(aMessage) {
...
816     let message = aMessage.json;
817     let target = aMessage.target;
818 
819     // To prevent the hacked child process from sending commands to parent
820     // to do illegal connections, we need to check its manifest URL.
821     if (aMessage.name !== "child-process-shutdown" &&
822         kMessages.indexOf(aMessage.name) != -1) {
823       if (!target.assertContainApp(message.manifestURL)) {
824         if (DEBUG) {
825           debug("Got message from a process carrying illegal manifest URL.");
826         }
827         return null;
828       }
829     }

'message' is defined in aMessage.json, which is sent from the child. The parent
process then uses message.manifestURL to perform the check. Since the child
process sends/creates the message, it might be possible or a compromised child
process to send a crafted manifestURL string to bypass the check. Something
like:
var aMessage = {
  ...
    json: {
        manifestURL: 'child-process-defined-manifest-file'
    }
    ...
}

If the child process can control manifestURL via its message, the value should
not be used for security checking.

Comment 1

[Rob Fletcher [:omerta]]

CC'ing bent on pauljt's recommendation that he would know if this is a real issue or not.

Comment 2

[Rob Fletcher [:omerta]]

After doing some more inspection, it appears that this code is checking to see if aMessage.json.manifestURL is in fact the manifest URL for the application sending that message. Therefore, if I send mismatched manifest in an effort to bypass the security check it will fail as it should.

Will follow up and close once I've verified.

Comment 3

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

(In reply to Rob Fletcher [:omerta] from comment #2)
> After doing some more inspection, it appears that this code is checking to
> see if aMessage.json.manifestURL is in fact the manifest URL for the
> application sending that message. Therefore, if I send mismatched manifest
> in an effort to bypass the security check it will fail as it should.

Correct, if the child sends anything other than the url we expect then we kill it. At least, that is definitely the intention.

Of course there could always be bugs so I'm glad you're digging in here. :)

Comment 4

[Rob Fletcher [:omerta]]

Verified this check actually does exactly what we were worried about. :)

Did however lead to bug 967653

Comment 5

[Benjamin Smedberg]

Does this need to remain private? Is it waiting on bug 967653?

Comment 6

[Gene Lian (I already quit Mozilla)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> Does this need to remain private? Is it waiting on bug 967653?

It's fine to make this bug open but I think keeping it private makes no harm.

Bug 967653 solves another similar issue but for AppStatus.