[Connection-Allowlist] Enforce "webrtc" header param. [chromium/src : main]
Andrew Verge (Gerrit)
Andrew Verge voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andrew Verge
| Commit-Queue | +1 |
1 comment
LocalDOMWindow* window = To<LocalDOMWindow>(context);Is RTCPeerConnection only allowed on windows? I ask because we need to pull the relevant WebRTC behavior from the PolicyContainer, and that's not always set for workers on the renderer-side.
From the [spec](https://www.w3.org/TR/webrtc/#interface-definition) that appears to be the case, but I saw other interfaces like RTCDataChannel defined on workers so want to be doubly sure.
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Verge (Gerrit)
Andrew Verge added 1 comment![]( "Open in Gerrit")
exception_state.ThrowDOMException(Spec note for @mk...@chromium.org:
Currently the spec reads: "To constrain WebRTC connections, [[webrtc]] can call into the [$should WebRTC be blocked by Connection Allowlists$]
algorithm while determining whether candidates are <a spec="webrtc">administratively prohibited</a>."
This CL throws in the RTCPeerConnection constructor, which is before we consider any potential candidates for communication. We should change that line in the spec to reflect as such.
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Verge (Gerrit)
Andrew Verge added 1 comment![]( "Open in Gerrit")
LocalDOMWindow* window = To<LocalDOMWindow>(context);Is RTCPeerConnection only allowed on windows? I ask because we need to pull the relevant WebRTC behavior from the PolicyContainer, and that's not always set for workers on the renderer-side.
From the [spec](https://www.w3.org/TR/webrtc/#interface-definition) that appears to be the case, but I saw other interfaces like RTCDataChannel defined on workers so want to be doubly sure.
Related details
- Harald Alvestrand
- Mike West
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mike West
| Code-Review | +1 |
2 comments
I'll defer to Harald on the RTC bits, as he's much more familiar with that code than I. That said, some test suggestions: otherwise LGTM.
await fetch('resources/foo.html');Two suggestions for this bit across all the tests:
1. Let's not add another file: just load `/common/blank.html` if the content doesn't matter.
2. I'd move this to a separate test, e.g. `promise_test(t => { return fetch('/common/blank.html`); }, "Fetches are unaffected by the `webrtc` property's value.")`. That makes things clearer, IMO.
Related details
- Andrew Verge
- Harald Alvestrand
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Verge (Gerrit)
Andrew Verge added 1 comment![]( "Open in Gerrit")
Two suggestions for this bit across all the tests:
1. Let's not add another file: just load `/common/blank.html` if the content doesn't matter.
2. I'd move this to a separate test, e.g. `promise_test(t => { return fetch('/common/blank.html`); }, "Fetches are unaffected by the `webrtc` property's value.")`. That makes things clearer, IMO.
- Harald Alvestrand
- Mike West
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Verge (Gerrit)
Andrew Verge added 1 comment![]( "Open in Gerrit")
Spec note for @mk...@chromium.org:
Currently the spec reads: "To constrain WebRTC connections, [[webrtc]] can call into the [$should WebRTC be blocked by Connection Allowlists$]
algorithm while determining whether candidates are <a spec="webrtc">administratively prohibited</a>."This CL throws in the RTCPeerConnection constructor, which is before we consider any potential candidates for communication. We should change that line in the spec to reflect as such.
I put up a spec PR to rectify this: https://github.com/WICG/connection-allowlists/pull/13
Mike West (Gerrit)
Mike West added 1 comment![]( "Open in Gerrit")
policy_container_policies.connection_allowlists.enforcedLooking at this again, I think it's fine for us to enforce here to ensure that we have the correct web-facing behavior, but it would be ideal to have something outside the renderer that's backing this up with a check in a high-trust process.
I'd imagine that `RTCPeerConnection` will eventually poke something across a mojo pipe to actually initiate network connections. Is there some point at which we could respond with `mojo::ReportBadMessage` if the relevant policy container says "Nope."?
Related details
- Andrew Verge
- Harald Alvestrand
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Verge (Gerrit)
Andrew Verge added 1 comment![]( "Open in Gerrit")
policy_container_policies.connection_allowlists.enforcedLooking at this again, I think it's fine for us to enforce here to ensure that we have the correct web-facing behavior, but it would be ideal to have something outside the renderer that's backing this up with a check in a high-trust process.
I'd imagine that `RTCPeerConnection` will eventually poke something across a mojo pipe to actually initiate network connections. Is there some point at which we could respond with `mojo::ReportBadMessage` if the relevant policy container says "Nope."?
Looking at this again, I think it's fine for us to enforce here to ensure that we have the correct web-facing behavior, but it would be ideal to have something outside the renderer that's backing this up with a check in a high-trust process.
That's what the change in `browser_interface_binders.cc` is for. The `P2PSocketManager` gets [bound in the network service](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/p2p/socket_dispatcher_host.cc;drc=2fff3fdd8ba33169c02b49c8ceb92b424f8e9a56;l=86) via `browser_interface_binders` in the trusted browser process. If our WebRTC policy is `kBlock`, then we won't bind, and can't even send RPCs in the first place.
Related details
- Harald Alvestrand
- Mike West
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Verge (Gerrit)
Andrew Verge added 1 comment![]( "Open in Gerrit")
policy_container_policies.connection_allowlists.enforcedAndrew VergeLooking at this again, I think it's fine for us to enforce here to ensure that we have the correct web-facing behavior, but it would be ideal to have something outside the renderer that's backing this up with a check in a high-trust process.
I'd imagine that `RTCPeerConnection` will eventually poke something across a mojo pipe to actually initiate network connections. Is there some point at which we could respond with `mojo::ReportBadMessage` if the relevant policy container says "Nope."?
Looking at this again, I think it's fine for us to enforce here to ensure that we have the correct web-facing behavior, but it would be ideal to have something outside the renderer that's backing this up with a check in a high-trust process.
That's what the change in `browser_interface_binders.cc` is for. The `P2PSocketManager` gets [bound in the network service](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/p2p/socket_dispatcher_host.cc;drc=2fff3fdd8ba33169c02b49c8ceb92b424f8e9a56;l=86) via `browser_interface_binders` in the trusted browser process. If our WebRTC policy is `kBlock`, then we won't bind, and can't even send RPCs in the first place.
(There's even more context [in this CL description](https://crrev.com/c/5672111) where we did the same for fenced frames).
Mike West (Gerrit)
Mike West voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mike West
| Code-Review | +1 |
1 comment
policy_container_policies.connection_allowlists.enforcedAndrew VergeLooking at this again, I think it's fine for us to enforce here to ensure that we have the correct web-facing behavior, but it would be ideal to have something outside the renderer that's backing this up with a check in a high-trust process.
I'd imagine that `RTCPeerConnection` will eventually poke something across a mojo pipe to actually initiate network connections. Is there some point at which we could respond with `mojo::ReportBadMessage` if the relevant policy container says "Nope."?
Andrew VergeLooking at this again, I think it's fine for us to enforce here to ensure that we have the correct web-facing behavior, but it would be ideal to have something outside the renderer that's backing this up with a check in a high-trust process.
That's what the change in `browser_interface_binders.cc` is for. The `P2PSocketManager` gets [bound in the network service](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/p2p/socket_dispatcher_host.cc;drc=2fff3fdd8ba33169c02b49c8ceb92b424f8e9a56;l=86) via `browser_interface_binders` in the trusted browser process. If our WebRTC policy is `kBlock`, then we won't bind, and can't even send RPCs in the first place.
(There's even more context [in this CL description](https://crrev.com/c/5672111) where we did the same for fenced frames).
Got it, thank you. Still LGTM, still interested in someone with more WebRTC experience weighing in.
Related details
- Andrew Verge
- Harald Alvestrand
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Harald Alvestrand (Gerrit)
Harald Alvestrand added 4 comments![]( "Open in Gerrit")
LocalDOMWindow* window = To<LocalDOMWindow>(context);Andrew VergeIs RTCPeerConnection only allowed on windows? I ask because we need to pull the relevant WebRTC behavior from the PolicyContainer, and that's not always set for workers on the renderer-side.
From the [spec](https://www.w3.org/TR/webrtc/#interface-definition) that appears to be the case, but I saw other interfaces like RTCDataChannel defined on workers so want to be doubly sure.
At the moment, RTCPeerConnection is only exposed on Window. There have been discussions on exposing it on Worker, but so far, nobody's been willing to invest in specifying how it would behave.
RTCDataChannel is (newly) defined as transferable, so that's how it can be exposed on worker.
"\"Connection-Allowlist\" header.");Do you intend to add an UMA counter to figure out how often this happens in practice?
error_callback)) {Should these reformatting things be done in a separate CL?
Related details
- Andrew Verge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Verge (Gerrit)
Andrew Verge added 3 comments![]( "Open in Gerrit")
LocalDOMWindow* window = To<LocalDOMWindow>(context);Andrew VergeIs RTCPeerConnection only allowed on windows? I ask because we need to pull the relevant WebRTC behavior from the PolicyContainer, and that's not always set for workers on the renderer-side.
From the [spec](https://www.w3.org/TR/webrtc/#interface-definition) that appears to be the case, but I saw other interfaces like RTCDataChannel defined on workers so want to be doubly sure.
Harald Alvestrand
At the moment, RTCPeerConnection is only exposed on Window. There have been discussions on exposing it on Worker, but so far, nobody's been willing to invest in specifying how it would behave.
RTCDataChannel is (newly) defined as transferable, so that's how it can be exposed on worker.
Given that creating a data channel still requires an RTCPeerConnection, and we're blocking creation at the Window level, I think we can resolve this.
Do you intend to add an UMA counter to figure out how often this happens in practice?
Done
Should these reformatting things be done in a separate CL?
Done, reverted the formatting changes for now (this was just the result of running my usual `git cl format` over the file before uploading).
Related details
- Harald Alvestrand
- Mike West
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Harald Alvestrand (Gerrit)
Harald Alvestrand voted and added 4 comments![]( "Open in Gerrit")
Votes added by Harald Alvestrand
| Code-Review | +1 |
4 comments
true);Should there be an unit test that verifies that the histogram is updated?
'iceServers': [{'urls': 'stun:stun.example.com:19302'}]The way the forbidding is implemented, you shouldn't need any configuration - it gets rejected anyway. (Having no ICE servers is a legal use case, just not for 100% reachability of VC servics)
'iceServers': [{'urls': 'stun:stun.example.com:19302'}]Same comment here.
Related details
- Andrew Verge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Kron (Gerrit)
Johannes Kron added 1 comment![]( "Open in Gerrit")
UMA_HISTOGRAM_BOOLEAN("WebRTC.PeerConnection.BlockedByConnectionAllowlist",It's preferred to use the corresponding histogram function (base::UmaHistogramBoolean) unless this is in a critical path.
See this link for more information, https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#coding-emitting-to-histograms
Related details
- Andrew Verge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |