Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

restricting available media types in the constructor #48

Open
fippo opened this issue Aug 6, 2020 · 7 comments
Open

restricting available media types in the constructor #48

fippo opened this issue Aug 6, 2020 · 7 comments

Comments

@fippo
Copy link
Contributor

fippo commented Aug 6, 2020

WebRTC attempts to treat SDP as an opaque blob and generally assumes that developers do not need to inspect the SDP before passing it to setRemoteDescription. Which is quite questionable from a security point of view.

This can have unexpected consequences. Suppose you have an audio-only application. Now you get an offer that includes video because there was a H264 vulnerability. If that gets negotiated and decodes H264 bad things may happen in quite unexpected ways which have blown up in the past:
https://bugs.chromium.org/p/webrtc/issues/detail?id=11013
https://bugs.chromium.org/p/project-zero/issues/detail?id=1936
and just now
https://googleprojectzero.blogspot.com/2020/08/exploiting-android-messengers-part-3.html
which explicitly mentions the scenario:

To do this, I wrote a Frida script that hooks nativeCreateOffer in Java, and makes a call to createDataChannel
before the offer is created. This was sufficient to enable SCTP on both devices, as the target device determines
whether to enable SCTP based on the SDP provided by the attacker.

Similarly, datachannel-only applications may not expect SDP with audio or video m-lines.

To mitigate this, add a new property to the RTCConfiguration:

const pc = new RTCPeerConnection({supportedMediaTypes: {
    audio: true,
    video: true,
    data: true,
  }
});

Behaviour:
When processing a remote description reject any m-line where supportedMediaType[mline.type] is false.
If supportedMediaType[audio or video] is false and addTrack is called with a track of that kind throw an error. Similar for addTransceiver.
if supportedMediaType[data] is false and createDatachannel is called, throw an error.

This can be polyfilled.

@Sean-Der
Copy link

Sean-Der commented Aug 6, 2020

@fippo Would it be possible to also use this to force a certain codec? We have setCodecPreference but somewhat awkward to use if you are just doing an answer and not creating any transceivers yourself.

I see a lot of people who only want H264 (or vpx). Even if people don't agree with their rationale would be possible to support it with this.

@fippo
Copy link
Contributor Author

fippo commented Aug 6, 2020

@Sean-Der no, i think setCodecPreferences is the right api for that use-case. This proposal is aiming for defense-in-depth.

@Sean-Der
Copy link

Sean-Der commented Aug 6, 2020

Makes sense!

One worry I have is that this won’t get used that often. Since it is opt-in and developers will not notice behavior changes if it gets removed by accident.

@nils-ohlmeier
Copy link

I thin @Sean-Der raises a good point here. While I understand the good intention here, this will add more complexity to the browser implementations while I'm not fully convinced yet how many developers would actually use this.

As the effort to implement this is probably not very high I think it could be added, but I'm not fully convinced yet that it is worth it.

BTW another way to accomplish the same would be for the application itself to scan the remote SDP for unwanted media types and bail.

@fippo
Copy link
Contributor Author

fippo commented Aug 7, 2020

BTW another way to accomplish the same would be for the application itself to scan the remote SDP for unwanted media types and bail.

Yes, the polyfill I had simply overwrote SRD and then did those manipulations.

What we've been telling people for years is this:

signaling.onmessage = async ({data: {description, candidate}}) => {
  try {
    if (description) {
      await pc.setRemoteDescription(description);

and JSEP is saying

most applications should be able to treat the SessionDescriptions produced and consumed by these various API 
calls as opaque blobs; that is, the application will not need to read or change them

If we say this, we should also note the risks of doing so.

@lgrahl
Copy link

lgrahl commented Aug 10, 2020

I think this is a good idea and the location (an object on construction of RTCPeerConnection) makes sense there, too. Another option would be to add it to setLocalDescription and setRemoteDescription which would reduce the amount of checks needed but could also be awkward (create an offer, then attempt to apply it and it suddenly fails).

I'm not sure we should complain about the added complexity by it considering what a bag of surprises SDP can be for security if left unfiltered. This API addition is really only the equivalent of a bouncer and I think we should be able to manage that. Heck, perhaps it's even a good idea to be forced to at least virtually untangle RTCPeerConnection into the three sections audio, video and data (while obviously audio and video are technically more close together than data).

More delicate constraints already exist in other places such as the transceiver API which I think is a good model to follow when it comes to exposing more options in other places (say SCTP settings).

@alvestrand
Copy link
Collaborator

For process reasons, I think this should be a PR on webrtc-extensions.
But the basic idea seems reasonable.

@jan-ivar jan-ivar transferred this issue from w3c/webrtc-pc Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants