1562600 - (CVE-2020-12424) webrtcUI.jsm trusts the URI from the child process

Status: RESOLVED FIXED

(Firefox :: Site Permissions, defect, P2)

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

webrtcUI.jsm trusts the URI from the child process when constructing a permission prompt for WebRTC. The principal used for security decisions is supplied as a string from the child, rather than obtaining a principal from message.target. I'm not sure if there is a reason for this (does message.target always equal aRequest.origin?) but currently there is not really any security that the parent enforces here: a content process can just pick an aRequest.Origin that already has WebRTC permissions (if one exists?)

Note: In single content process world this probably made sense, but as we more towards Fission this likely needs to be improved.

Taking a guess at sec rating - its probably sec-low here as exploiting it would require there to be an origin that already has permission for the attacker to take advatnage of. And obviously the attacker needs to compromise the child process first.

Comment 1

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Meh, I think I filed this bug based on an old assumption that I had that we could trust message.target. But actually I don't think thats the case, and we likely can't do much here without Fission. Gijs mentioned that we might " stop passing the message and instead trust the fission interfaces that tell us what's loaded in what docshell from the parent side" and while it might not give a strong guarentees until fission is complete at least we'd get it for free once its done. Gijs, if you are watching this bug, can you elaborate?

Comment 2

[:Gijs (he/him)]

As noted on slack, although we currently don't enforce anything about what principals/origins are loaded in docshells from the parent to the child (ie we trust the child's idea of what is in a docshell and send that up to the parent and believe it), I think we can use fission APIs today to make this better, and once we ship fission, it will then actually be providing us with some guarantees here. Plus it'll make it at least a little bit harder for a broken child process to be lying to us even today.

Mike, is there some guidance somewhere about how to pass browsing context data from child to parent through the message manager? The request could come from a subframe, and although XUL browser objects have a frameLoader property that I can use to get a browsingContext, I don't see a good way of finding the right context in that tree by id or something like that (ie all I see is doing a depth-first search using getChildren() which seems wrong...), so I assume I'm meant to use some kind of IPC-friendly reference object instead... just not sure what.

Comment 3

[Frederik Braun [:freddy]]

This seems to align with the fission work required for LoadInfo, that ckerschb is planning. The result should be that a LoadInfo object for any kind of load would be "always correct" about who is loading what. CCing him for visibility.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

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

Mike, is there some guidance somewhere about how to pass browsing context data from child to parent through the message manager? The request could come from a subframe, and although XUL browser objects have a frameLoader property that I can use to get a browsingContext, I don't see a good way of finding the right context in that tree by id or something like that (ie all I see is doing a depth-first search using getChildren() which seems wrong...), so I assume I'm meant to use some kind of IPC-friendly reference object instead... just not sure what.

Hey - yeah, currently, you can get at the BrowsingContext by ID via the BrowsingContext.get global static method.

At that point, what I'd recommend doing is getting at the principal from the WindowGlobalParent, instead of trusting one sent up from content.

So something like:

let bc = BrowsingContext.get(<some id>);
let principal = bc.currentWindowGlobal.documentPrincipal;

Does that help?

Comment 5

[:Gijs (he/him)]

Johann, I believe this is still the case after the work in bug 1601301, because we use state.uri from webrtcUI.jsm's list of _streams which gets items added fromstreamAddedOrRemoved, which still passes in the data from the child.

Is there any reason not to just use the browsing context's principal / origin URI here? Is it different from the thing we send up as data?

Comment 6

[Johann Hofmann [:johannh]]

Thank for pulling this up again, I think you're right, we didn't fix that. I think ideally we would always amend the parameters with, say, the document principal (like this) processing the request in receiveMessage and then the individual functions can pick whatever they want from the principal.

I don't really see why it would be different from what is sent in data.

Comment 7

[Emma Zühlcke [:emz]]

Hey Johann, do you think this is still an issue? While constructing the principal again from the origin isn't ideal, the origin that is used comes from the WindowGlobalParent associated with the JSWindowActor:
https://searchfox.org/mozilla-central/rev/9193635dca8cfdcb68f114306194ffc860456044/browser/actors/WebRTCParent.jsm#123-126
That should be safe to do? Am I missing something here?

Comment 8

[:Gijs (he/him)]

(In reply to Paul Zühlcke [:pbz] from comment #7)

Hey Johann, do you think this is still an issue? While constructing the principal again from the origin isn't ideal, the origin that is used comes from the WindowGlobalParent associated with the JSWindowActor:
https://searchfox.org/mozilla-central/rev/9193635dca8cfdcb68f114306194ffc860456044/browser/actors/WebRTCParent.jsm#123-126
That should be safe to do? Am I missing something here?

The parent actor calls streamAddedOrRemoved at https://searchfox.org/mozilla-central/rev/9193635dca8cfdcb68f114306194ffc860456044/browser/actors/WebRTCParent.jsm#152 with aData, which is unfiltered data from the client. That ends up in _streams on webRTCUI at https://searchfox.org/mozilla-central/rev/9193635dca8cfdcb68f114306194ffc860456044/browser/modules/webrtcUI.jsm#274-278 , from where it is returned when we ask for streams for a given browser at https://searchfox.org/mozilla-central/rev/9193635dca8cfdcb68f114306194ffc860456044/browser/modules/webrtcUI.jsm#138 , which gets used in a number of places, including https://searchfox.org/mozilla-central/rev/9193635dca8cfdcb68f114306194ffc860456044/browser/modules/webrtcUI.jsm#687,704 .

We should make the uri member not child-controlled.

Comment 9

[Emma Zühlcke [:emz]]

Oh, I see. Thanks for the explanation! I was looking at the wrong method.
I'll look into it.

Comment 10

[Emma Zühlcke [:emz]]

Attachment: Bug 1562600 - WebRTCParent: Get document URI from WindowGlobalParent instead of child message. r=gijs!

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/354ce68d14f8f9499d05c5912b4b7e886e87ff4e
https://hg.mozilla.org/mozilla-central/rev/354ce68d14f8

Comment 12

[Tom Ritter [:tjr]]

Attachment: advisory.txt