1770783 - BrowserElementParent indexes into an Object with data from the child process

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect, P3)

Description

[Andrew McCreight [:mccr8]]

I came across this code while looking for things similar to bug 1770137:

    let mmCalls = {
      hello: this._recvHello,
    };
...
    if (aMsg.data.msg_name in mmCalls) {
      return mmCalls[aMsg.data.msg_name].apply(this, arguments);

aMsg.data.msg_name was sent to us by the other process, so in the case of a compromised child process, it could be anything. While the precise scenario of bug 1770137 doesn't do anything much, Nika pointed out that if you pass in, say "toString", it will actually run toString().

This is likely difficult to exploit, because arguments will always have a single value, the message object which is constructed by nsFrameMessageManager. Only the data and json fields can really be controlled by the attacker to be an arbitrary structured cloned object. I haven't looked at all of the methods on the Object prototype, but it feels unlikely that any of them are going to look at that object.

The same issue also needs to be fixed in mmSecuritySensitiveCalls.

Probably the most straightforward fix would be to change these two Objects into Maps.

I don't know if we actually use the browser-element BrowserElementParent.jsm in Firefox any more. Bug 1630691 removed much of the functionality.

Comment 1

[Tom Schuster]

Probably the most straightforward fix would be to change these two Objects into Maps.

Both objects just have one element. We should probably just use a switch.

Comment 2

[Andrew McCreight [:mccr8]]

Fabrice, do you have any opinions on how this should be fixed? I don't know if you all have some local modifications to this file.

Comment 3

[Andrew McCreight [:mccr8]]

I'll mark this sec-other, as it doesn't look like it is actually exploitable, just a bit spooky.

Comment 4

[[:fabrice] Fabrice Desré]

(In reply to Andrew McCreight [:mccr8] from comment #2)

Fabrice, do you have any opinions on how this should be fixed? I don't know if you all have some local modifications to this file.

We don't use the browser element code anymore, instead we wrote a <web-view> custom element that wraps a <xul:browser> and exposes apis we need. So from our side, you can even remove it :)

Comment 5

[:Gijs (he/him)]

(In reply to [:fabrice] Fabrice Desré from comment #4)

(In reply to Andrew McCreight [:mccr8] from comment #2)

Fabrice, do you have any opinions on how this should be fixed? I don't know if you all have some local modifications to this file.

We don't use the browser element code anymore, instead we wrote a <web-view> custom element that wraps a <xul:browser> and exposes apis we need. So from our side, you can even remove it :)

Do we have any consumers in Firefox/Mozilla left? From a frontend perspective, this code is always confusing because it pretends to be like a browser but isn't normally what you're looking for... So if we can remove it, that'd be very nice...

Comment 6

[Kris Maglione [:kmag]]

(In reply to :Gijs (he/him) from comment #5)

Do we have any consumers in Firefox/Mozilla left? From a frontend perspective, this code is always confusing because it pretends to be like a browser but isn't normally what you're looking for... So if we can remove it, that'd be very nice...

No, we removed them all and most of the guts of mozbrowser a long time ago.

Comment 7

[Jens Stutte [:jstutte]]

Hi :jkrause ! Could you rip this out of the tree and make some try push, please? IIUC the entire content of dom/browser-element is obsolete.

Comment 8

[Andrew McCreight [:mccr8]]

That sounds like a good idea. Please file a new bug blocking this one for the actual removal, so we can leave this one closed.

Comment 9

[Andrew McCreight [:mccr8]]

I went ahead and filed bug 1770944 for the removal.

Comment 10

[Jens Stutte [:jstutte]]

(In reply to Andrew McCreight [:mccr8] from comment #9)

I went ahead and filed bug 1770944 for the removal.

OK, :jkrause, please take that one then. Thanks!

Comment 11

[Jan Rio Krause]

Sure, I'm on it. Thanks for filing and assigning the bug.

Comment 12

[Kris Maglione [:kmag]]

Note that the various references to the "mozbrowser" attribute should also be removed.

Comment 13

[Frederik Braun [:freddy]]

jkrause, it looks like this was never properly set to assigned. Are you still going to work on this potential sandbox escape?

Comment 14

[Andrew McCreight [:mccr8]]

They have patches up in bug 1770944, so this is waiting on whatever is going on there.

Comment 15

[:Gijs (he/him)]

Bug 1770944 fixed this. \o/