Add a CSP check to RTCPeerConnection.addIceCandidate(). #81

zenhack · 2021-05-30T18:45:21Z

Corresponding change to CSP spec:

I'm like 90% sure I've botched the formatting here in one way or another; I still haven't totally gotten my head around how links are generated in these docs, and it seems like there's a bit of inconsistency in how docs are built from spec to spec... so let me know if something's broken...

I'm also a little fuzzy on how the CSP doc should refer to this; right now it links to the constructor section in WebRTC 1.0, but of course this step isn't there, so maybe it should link to the extensions instead?

cc: @annevk, @jan-ivar

Preview | Diff

Corresponding change to CSP spec: w3c/webappsec-csp#457

annevk · 2021-05-31T07:20:58Z

I would prefer we add this to https://github.com/w3c/webrtc-pc directly. Monkey patching standards tends to lead to problems.

zenhack · 2021-05-31T16:58:56Z

I agree that would be cleaner. I'd gotten the impression somewhere that that document was more set in stone, but if you think it's ok I'll find time to put together a pr in the next couple days.

jan-ivar · 2021-06-01T18:56:58Z

I would prefer we add this to https://github.com/w3c/webrtc-pc directly. Monkey patching standards tends to lead to problems.

... I'd gotten the impression somewhere that that document was more set in stone, ...

@annevk @zenhack Yes, WebRTC is at Recommendation, and this WG is still finding its legs wrt a living spec within the W3C.

This PR is a trial-balloon for our new merge guide, and I would say it's not going well, as it looks trivially small, yet missed the cutoff we made for small bug-fixes vs features. cc @alvestrand

index.html

jan-ivar · 2021-06-01T19:26:56Z

... yet missed the cutoff we made for small bug-fixes vs features. ...

Unless we can get it in under "patch significant security holes in WebRTC"?

Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>

zenhack · 2021-06-01T20:07:19Z

I have no objection to using the security holes clause to justify merging this directly into webrtc-pc; if folks can come to a consensus I'll follow.

jan-ivar · 2021-06-01T20:08:24Z

@alvestrand @aboba WDYT?

index.html

alvestrand · 2021-06-02T11:03:15Z

I'm afraid I have an objection to using the "security hole" merge-fix loophole.
This is new functionality, it is not implemented in any browser, and has not been anyone's high priority since I last messed with it in 2018 (when it died for lack of compelling interest).

Our agreement when we did the merge-guide was that we'd keep the idea that REC documented stuff for which there was consensus agreement and had been implemented in at least 2 browsers. Anything else goes to -extensions.

alvestrand

Still not 100% convinced that this is a good idea. But let's get it properly documented.

index.html

alvestrand · 2021-06-02T11:07:39Z

index.html

+      <p>Whenever the <a data-cite="WEBRTC#dom-peerconnection">RTCPeerConnection constructor</a>
+      algorithm is invoked, run the following steps instead:
+        <ol>
+          <li>If [[WEBAPPSEC-CSP#should-block-rtc-connection]] on the current global object returns "Blocked",


Query: the constructor steps have a [[DocumentOrigin]] initialized to the relevant settings object's origin. Is "the relevant settings object" the same as "the current global object", or are they different? And if they're different, is this intentional?

In the context of a constructor this's relevant ... and current ... are identical, though a settings object is different from a global object. https://html.spec.whatwg.org/multipage/webappapis.html#realms-settings-objects-global-objects has a longer explanation on this. Sticking with one pattern is better though, I'd recommend sticking with this's relevant ....

should-block-rtc-connection says it takes a global object, and it ends up passing that through to create-violation-for-global, so it seems like passing in a settings object is conceptually a type error here, and changing it on the other end would be fairly invasive. But I'm still a little fuzzy on how all these relate, so it's possible I'm missing something.

index.html

aboba · 2021-06-02T14:29:41Z

@alvestrand I agree that we cannot add this directly to webrtc-pc due to lack of implementation. So it should live here (with a "feature at risk" note) until it is implemented.

zenhack · 2021-06-02T16:42:22Z

Addressed/replied to all outstanding comments.

I believe I still need to sign the ipr commitment for this repo -- I think for webappsec-csp @samuelweiler sent me the relevant form, not sure if someone needs to intervene to send me one for this repo or if it's up somewhere public that I haven't found.

annevk · 2021-06-02T17:11:52Z

So this makes the constructor throw, which is a bit different from how CSP works with other network APIs. There CSP violations effectively ends up causing a network error. Was that considered here? That might also make this more compatible in the couple cases where it might cause issues in the sense that applications would not end up getting an exception they might not have accounted for, but would rather get network errors they likely deal with already anyway.

zenhack · 2021-06-02T17:50:57Z

My rationale was that putting it here makes everything simpler -- there's one entry point, so this just cuts the whole thing off at the source, which is easier to reason about than if we had to track down all of the places where network activity might happen lazily. Though this opens up the question of what should we do if we find ourselves wanting to add finer-grained permissions?

This is perhaps another argument in favor of using Permissions-Policy or such instead of CSP, though that doesn't solve the practical problem of errors where apps don't expect them.

zenhack · 2021-06-09T19:18:29Z

Outstanding issues as I see them:

@alvestrand says he's not sold on the idea. Could you elaborate on your objection?
@annevk raised concerns regarding new places exceptions may be raised. Relevant things to decide there:
- Are we ok with a new exception being thrown if an operator sets this?
- Do we think pushing the error reporting later is worth the extra complexity, and if so where would this go instead?
- If we keep the current design, how might this evolve if we make the csp directive finer-grained?

I'm of the opinion that the additional complexity is not worth it for the error handling consideration alone, but am interested in hearing other perspectives. I don't have a great answer to the evolvability question though. I also am not sure where the check should go if we push it later -- if others have insights please share. My biggest concern with pushing the check later is I want to have a clear story as to how we will have confidence that we haven't missed a spot.

zenhack · 2021-06-16T18:28:09Z

Ping?

alvestrand · 2021-06-16T19:01:13Z

Not sold on the idea:

I'm not sure CSP is the right interface to modify in order to restrict WebRTC usage; my conversations with @mkwst back in 2018 indicated that there was some thought that CSP was already too complex and that better solutions were coming.
I saw this idea brought up in 2018 and die for lack of a customer that really wanted it; I'm not sure we have a strong enough customer to get it implemented across browsers this round.

That said, I'm fine with the proposal technically - there are a number of usages of WebRTC that don't involve communication (attempts at inappropriate user tracking, mostly), and I'm not at all unhappy about blocking them rather than waiting until we can signal a "network failure" as a response to an attempt at communication.

So I won't block this at all, but I'm somewhat skeptical of it getting through the process and getting deployed.

zenhack · 2021-06-16T19:39:03Z

I saw this idea brought up in 2018 and die for lack of a customer that really wanted it; I'm not sure we have a strong enough customer to get it implemented across browsers this round.

This is pretty much mandatory for where Sandstorm wants to go; if I have to I'll send a patch to all three major FOSS browser engines myself, though it would obviously more efficient if someone already familiar with the relevant codebases had the bandwidth.

alvestrand · 2021-06-16T20:42:25Z

What's Sandstorm, and where does it want to go?

zenhack · 2021-06-16T21:52:56Z

See #64 (comment) (which was in response to a separate but related issue for which there seems to be a broader interest)

annevk · 2021-06-17T06:54:16Z

With regards to pushing the check later: #73 is about sharing more infrastructure with Fetch. once we share connection setup infrastructure across all (web platform) specifications, we also have a single point to block them. It would just act as if establishing the connection resulted in failure.

zenhack · 2022-01-05T06:18:05Z

Ack, now that I've actually looked at your pr I understand that your intention was to just fix the issue there; I guess I should back out the change I just made?

jan-ivar

I hadn't really processed that the definition of administratively prohibited was local to addIceCandidate

That's a mistake in the spec actually, and is issue w3c/webrtc-pc#2675 for which I've provided the PR. With the PR the algorithm is invoked from addIceCandidate, setRemoteDescription and setLocalDescription. The first two matches the reality that web developers who know SDP may provide candidates to the browser through both methods, and SLD is what triggers local gathering.

index.html

This had not been rendering properly. No content change.

...since this may be moved to its own section in the future.

jan-ivar · 2022-01-20T15:40:38Z

OK to merge once w3c/webappsec-csp#457 is merged.

...as specified in: - w3c/webappsec-csp#457 - w3c/webrtc-extensions#81

* Test webrtc/content-security-policy integration ...as specified in: - w3c/webappsec-csp#457 - w3c/webrtc-extensions#81 * Fix typos in comment. Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com> * Fix ice candidate exchange in CSP webrtc tests. we should be passing each candidate to the *other* pc. Tests now behave as expected. * CSP webrtc tests: listen for connection state change, not gathering. See #32914 (comment) * CSP webrtc tests: drop unnecessary stun server. * webrtc csp: simplify state checking "new" is the initial state. Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com> Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>

zenhack · 2022-04-23T04:57:56Z

Fixed the merge conflicts, and w3c/webappsec-csp#457 has been merged.

I'd added this back in when fixing merge conflicts, but it's been moved to a different section. This commit fixes it so only the CSP stuff is in this section.

…2914) * Test webrtc/content-security-policy integration ...as specified in: - w3c/webappsec-csp#457 - w3c/webrtc-extensions#81 * Fix typos in comment. Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com> * Fix ice candidate exchange in CSP webrtc tests. we should be passing each candidate to the *other* pc. Tests now behave as expected. * CSP webrtc tests: listen for connection state change, not gathering. See web-platform-tests#32914 (comment) * CSP webrtc tests: drop unnecessary stun server. * webrtc csp: simplify state checking "new" is the initial state. Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com> Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>

zenhack · 2022-05-02T06:10:41Z

Anything still need to happen here?

…gration, a=testonly Automatic update from web-platform-tests Test webrtc/content-security-policy integration (#32914) * Test webrtc/content-security-policy integration ...as specified in: - w3c/webappsec-csp#457 - w3c/webrtc-extensions#81 * Fix typos in comment. Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com> * Fix ice candidate exchange in CSP webrtc tests. we should be passing each candidate to the *other* pc. Tests now behave as expected. * CSP webrtc tests: listen for connection state change, not gathering. See web-platform-tests/wpt#32914 (comment) * CSP webrtc tests: drop unnecessary stun server. * webrtc csp: simplify state checking "new" is the initial state. Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com> Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com> -- wpt-commits: 0abba58602758eb8be11c38788c6f51fed2529e4 wpt-pr: 32914

zenhack · 2022-05-17T03:48:37Z

@jan-ivar, ping?

…gration, a=testonly Automatic update from web-platform-tests Test webrtc/content-security-policy integration (#32914) * Test webrtc/content-security-policy integration ...as specified in: - w3c/webappsec-csp#457 - w3c/webrtc-extensions#81 * Fix typos in comment. Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com> * Fix ice candidate exchange in CSP webrtc tests. we should be passing each candidate to the *other* pc. Tests now behave as expected. * CSP webrtc tests: listen for connection state change, not gathering. See web-platform-tests/wpt#32914 (comment) * CSP webrtc tests: drop unnecessary stun server. * webrtc csp: simplify state checking "new" is the initial state. Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com> Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com> -- wpt-commits: 0abba58602758eb8be11c38788c6f51fed2529e4 wpt-pr: 32914

zenhack · 2022-06-03T23:00:59Z

I think somebody just needs to push the button...

index.html

Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>

zenhack · 2022-06-14T22:29:57Z

Merged the change. The link looks right now, though the doc it's pointing to seems to be a version of the doc that doesn't have that section updated. LGTM though.

jan-ivar · 2022-06-15T17:30:12Z

Checks seem to be failing over error: Duplicate ID “rtcpeerconnection-interface”. which seems unrelated to this PR (reuse is in "Removed features" section added in #105). I'll see if I can patch it here.

Add a CSP check to RTCPeerConnection.constructor().

9375e0f

Corresponding change to CSP spec: w3c/webappsec-csp#457

zenhack mentioned this pull request May 30, 2021

Introduce 'webrtc' as a simple on/off switch w3c/webappsec-csp#457

Merged

alvestrand requested review from alvestrand and jan-ivar May 31, 2021 05:49

jan-ivar requested changes Jun 1, 2021

View reviewed changes

index.html Outdated Show resolved Hide resolved

Avoid referencing step numbers.

256fd09

Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>

jan-ivar reviewed Jun 1, 2021

View reviewed changes

index.html Outdated Show resolved Hide resolved

alvestrand reviewed Jun 2, 2021

View reviewed changes

zenhack added 2 commits June 2, 2021 12:14

Link to #constructor rather than #dom-peerconnection

99fcce0

Throw SecurityError instead of NotAllowedError

c63021b

dontcallmedom added the security-tracker Group bringing to attention of security, or tracked by the security Group but not needing response. label Jun 18, 2021

w3cbot mentioned this pull request Jun 18, 2021

Add a CSP check to RTCPeerConnection.constructor(). w3c/security-review#110

Open

jan-ivar approved these changes Jan 5, 2022

View reviewed changes

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

Fix citation markup

6511a9d

This had not been rendering properly. No content change.

zenhack force-pushed the csp-integration branch from 5cbba4a to 6511a9d Compare January 6, 2022 15:28

Refer to adminsistratively prohibited, not addIceCandidate

5696895

...since this may be moved to its own section in the future.

zenhack requested a review from jan-ivar January 6, 2022 15:38

jan-ivar approved these changes Jan 8, 2022

View reviewed changes

jan-ivar added the editors can integrate label Jan 20, 2022

zenhack added a commit to zenhack/wpt that referenced this pull request Feb 20, 2022

Test webrtc/content-security-policy integration

7040121

...as specified in: - w3c/webappsec-csp#457 - w3c/webrtc-extensions#81

zenhack mentioned this pull request Feb 20, 2022

Test webrtc/content-security-policy integration web-platform-tests/wpt#32914

Merged

zenhack mentioned this pull request Mar 3, 2022

Ian: Push web standards to allow blocking WebRTC sandstormports/community-project#12

Open

Merge remote-tracking branch 'origin/main' into csp-integration

bbab234

Remove duplicate section getDefaultIceServers().

c434511

I'd added this back in when fixing merge conflicts, but it's been moved to a different section. This commit fixes it so only the CSP stuff is in this section.

zenhack requested review from alvestrand and jan-ivar June 3, 2022 23:00

jan-ivar reviewed Jun 14, 2022

View reviewed changes

index.html Outdated Show resolved Hide resolved

Update index.html

170f6c1

Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>

Fix Duplicate “rtcpeerconnection-interface” ID to unblock merge

acab467

jan-ivar merged commit cfd1425 into w3c:main Jun 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a CSP check to RTCPeerConnection.addIceCandidate(). #81

Add a CSP check to RTCPeerConnection.addIceCandidate(). #81

zenhack commented May 30, 2021 •

edited by pr-preview bot

Loading

annevk commented May 31, 2021

zenhack commented May 31, 2021

jan-ivar commented Jun 1, 2021 •

edited

Loading

jan-ivar commented Jun 1, 2021

zenhack commented Jun 1, 2021

jan-ivar commented Jun 1, 2021

alvestrand commented Jun 2, 2021

alvestrand left a comment

alvestrand Jun 2, 2021

annevk Jun 2, 2021

zenhack Jun 2, 2021

aboba commented Jun 2, 2021

zenhack commented Jun 2, 2021

annevk commented Jun 2, 2021

zenhack commented Jun 2, 2021

zenhack commented Jun 9, 2021

zenhack commented Jun 16, 2021

alvestrand commented Jun 16, 2021

zenhack commented Jun 16, 2021

alvestrand commented Jun 16, 2021

zenhack commented Jun 16, 2021

annevk commented Jun 17, 2021

zenhack commented Jan 5, 2022

jan-ivar left a comment

jan-ivar commented Jan 20, 2022

zenhack commented Apr 23, 2022

zenhack commented May 2, 2022

zenhack commented May 17, 2022

zenhack commented Jun 3, 2022

zenhack commented Jun 14, 2022

jan-ivar commented Jun 15, 2022

Add a CSP check to RTCPeerConnection.addIceCandidate(). #81

Add a CSP check to RTCPeerConnection.addIceCandidate(). #81

Conversation

zenhack commented May 30, 2021 • edited by pr-preview bot Loading

annevk commented May 31, 2021

zenhack commented May 31, 2021

jan-ivar commented Jun 1, 2021 • edited Loading

jan-ivar commented Jun 1, 2021

zenhack commented Jun 1, 2021

jan-ivar commented Jun 1, 2021

alvestrand commented Jun 2, 2021

alvestrand left a comment

Choose a reason for hiding this comment

alvestrand Jun 2, 2021

Choose a reason for hiding this comment

annevk Jun 2, 2021

Choose a reason for hiding this comment

zenhack Jun 2, 2021

Choose a reason for hiding this comment

aboba commented Jun 2, 2021

zenhack commented Jun 2, 2021

annevk commented Jun 2, 2021

zenhack commented Jun 2, 2021

zenhack commented Jun 9, 2021

zenhack commented Jun 16, 2021

alvestrand commented Jun 16, 2021

zenhack commented Jun 16, 2021

alvestrand commented Jun 16, 2021

zenhack commented Jun 16, 2021

annevk commented Jun 17, 2021

zenhack commented Jan 5, 2022

jan-ivar left a comment

Choose a reason for hiding this comment

jan-ivar commented Jan 20, 2022

zenhack commented Apr 23, 2022

zenhack commented May 2, 2022

zenhack commented May 17, 2022

zenhack commented Jun 3, 2022

zenhack commented Jun 14, 2022

jan-ivar commented Jun 15, 2022

zenhack commented May 30, 2021 •

edited by pr-preview bot

Loading

jan-ivar commented Jun 1, 2021 •

edited

Loading