No ICE candidates, No connectionStateChange events

[mehrvarz]

I have new findings regarding webRTC-policy patch.

There are two major issues. First, with your WebRTC patch, no ICE candidates are being generated. This prevents P2P connections from being established. The failure occurs without an error message or any explanation towards the user. This means that the WebRTC app developer is likely to receive complaints.

The second problem is even more severe. The connectionStateChange [1] event handler is never called. WebRTC apps often depend on this handler to be called at some point. If it is never called, it can cause flawless WebRTC apps to hang (!) without any user feedback. All web browsers call this handler on any OS - including Ungoogled-Chromium on Mac, Windows and Linux. But your implementation on Android does not, causing apps to hang.

Apps hanging for no apparent reason is a really bad experience. This is why I ask you to resolve these issues, for instance by returning error or "disconnect" messages where functionality is being disabled. If you break apps, at least let them fail graciously.

[1] https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/connectionstatechange_event

[wchen342]

I read the original discussion on this again and I think the Bromite patch is breaking on purpose for safe fallback. What if you use this patch instead: https://github.com/GrapheneOS/Vanadium/blob/12/patches/0051-most-private-WebRTC-IP-handling-policy-by-default.patch? Will it still break things?

[uazo]

surely you know more than me, but I'll try and then we'll talk about it

[mehrvarz]

despite the oversimplified description of the problem in this thread

*irrefutable

[csagan5]

The issue was summarised here: bromite/bromite#553 (comment)

CCTs would be able to expose the local IP address (without VPN) with the default policy; thus the default policy is patches in Bromite to not allow multiple_routes and nonproxied_udp`.

[wchen342]

I think the concern here is not the IP, instead the concern is that because the switch is not standard implementation, JavaScript codes fail silently when running, thus make applications unusable without any indication.

[csagan5]

I think the concern here is not the IP, instead the concern is that because the switch is not standard implementation, JavaScript codes fail silently when running, thus make applications unusable without any indication.

If no WebRTC can be established the Javascript bindings should return an error; if they do not, that is a bug to fix.

[mehrvarz]

Trash the policy. Stop doctoring. It cannot work because no one wants a crippled WebRTC. Instead, allow users to turn it off completely.

[ x ] Disable WebRTC

Make it enabled by default, like a conformant browser. And tell VPN users they need to check this ON to prevent IP leakage.

In disabled state I expect RTCPeerConnection() to return null (Edit: or NotFoundError). This will ensure WebRTC apps don't "appear to work" while being hung with no user feedback.

[wchen342]

I went back to check how Firefox did the WebRTC leak prevention, and it turns out on Firefox you simply disable WebRTC, so yes, probably disabling is better. The whole problem actually started from there is no flag to disable WebRTC on Chromium-based browsers.
Brave has a more complicated approach but it doesn't seems to be easily achievable on Android because of the interface thing.
In this case I now agree with you that a switch to disable WebRTC is better.

[mehrvarz]

there is no flag to disable WebRTC on Chromium-based browsers.

There used to be one. Do you think it would make sense to check out an old version of Chromium to see how it was implemented?

Do you know what tools/metrics/histograms/enums.xml is good for? There is an enum 563980787 in there referring to "disable-webrtc".

[wchen342]

That is just usage statistics. Chromium has calls to gather usage data and save them in a lot of its internal functions. The file is just a list of entries they collect for usage statistics.

A flag to disable WebRTC completely shouldn't be hard to do. We just need to make sure (1) it is really disabled and (2) JS gives the correctly exception. That is doable I think.

[mehrvarz]

turns out on Firefox you simply disable WebRTC

You are referring to: media.peerconnection.enabled. After I set this to false, new RTCPeerConnection() in JS throws an exception: "ReferenceError: RTCPeerConnection is not defined". This is easy to catch.

A flag to disable WebRTC completely shouldn't be hard to do.

Appreciate your optimism. Please keep me updated on this journey. If I could make one wish: don't bury the switch deep in settings. Put it in the main menu, next to "Desktop site [ ]". So the current state can always be checked quickly.

[mehrvarz]

"ReferenceError: RTCPeerConnection is not defined". This is easy to catch.

What I meant to say: Everybody is likely to catch this.

Here is one of the sample apps referenced above running into WebRTC being disabled in FF:
https://timur.mobi/li/Screenshot-2022-02-21-00-28-56.png
The error message is a little cryptic (talking about "local media"). But the app has visibly stopped acting.

Here is my WebCall app running into WebRTC being disabled in FF:
https://timur.mobi/li/Screenshot-2022-02-21-00-29-44.png

[uazo]

after checking the code here is what i found.

there are three options:

• enable_multiple_routes
  true (default): if the page has access rights to the camera or microphone, it shows the local ip, otherwise only the public one. this logic is also applied to datachannels
  false: access to the ip is regardless of the permissions granted and controlled by enable_default_local_candidate

• enable_nonproxied_udp
  true (default): Enable both udp and tcp
  false: disable transmission via udp (so keep tcp only)

• enable_default_local_candidate
  true (default): enable transmission of local ip
  false: disable the transmission of local ip

the logic of enable_multiple_routes has priority over enable_default_local_candidate, the latter is not taken into account if enable_multiple_routes = true

@csagan5 I have not noticed any differences in the management of the cct, neither application nor functional. Would you give me a way to reproduce what you say?

about the error condition: the documentation explicitly says

RTCPeerConnection.onicecandidate:
If the event's candidate property is null, ICE gathering has finished.
https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/onicecandidate

This is not actually true in chromium, because due to the filtering done on candidates instead of deleting the value, the passing of null is one per interface.

But the documentation also says:

You don't need to watch for this explicitly; instead, if you need to sense the end of signaling, you should watch for a
icegatheringstatechange event indicating that the ICE negotiation has transitioned to the complete state.
https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/onicecandidate

so it seems to me that there is already a way to know if the webrtc negotiation cannot be successful by verifying the lack of ice candidates or the status of the event.

regarding the possibility of throwing an exception to the creation of RTCPeerConnection, although it does not seem foreseen in the
w3c documentation (https://www.w3.org/TR/webrtc/#constructor) but technically we would only break sites that do not comply with the specifications (i.e. without a try / catch) because in the idl it is contemplated that it might throw an exception (has RaisesException).

ultimately, we could go back to default and remove the patch, after all having webrtc only active on the public interface i think has
little sense in most cases while the IP leak seems mandatory given the p2p nature of the transmission and the user must grant the
permission for the camera or the microphone (which is perhaps worse than granting the ip)
there has also been an attempt to use mDNS (active by default) but it seems that the library used is not able to handle it (for now) because it requires a defined ip and not a name.

alternatively, as I said, it is possible to activate a content setting that allows a choice between (EDIT) true false / true / true (ice candidates provided by the browser) and false / false / false (ice candidates to pass manually, i.e. the site must provide them via javascript), but it would be a specific condition of bromite, perhaps only necessary to allow to activate a datachannel without necessarily granting access to the camera or microphone.

[mehrvarz]

So a browser provides a "[ _ ] Disable WebRTC" feature. Are you saying 192.168.1.50 should be prevented from leaking in cases where WebRTC is NOT disabled?

[mehrvarz]

When WebRTC is enabled, neither the public nor the local IP address should be blocked.

With respect to the local IP address, please read this post from end of January. In it I describe how the local IP address can provide very powerful functionality: the dynamic handover of a live P2P connection from wifi to mobile internet (in this particular example).

WebRTC is not a surveillance tool. Everything in it provides useful and honest functionality. It is not like a tracker inside a web page that you can just root out. If there is a WebRTC kill switch avail to the user, there is no need to modify the way WebRTC works. If you come to a different conclusion, how about disabling WebRTC completely?

Here is what I would find useful: a visual feedback for the user, indicating the mode the browser is in. Say, a red dot close to (or inside) the URL box, that lights up when WebRTC is enabled. This would prevent the user from making a false assumption.

[csagan5]

I will not continue discussing a Bromite patch here; feel free to change the patch as it best fits your use.

[wchen342]

Yes, I want to point out that although I borrow patches from Bromite, ungoogled-chromium and Bromite are different projects and have different goals. I will go ahead and implement the switch approach, while @csagan5 is free to do what he see fits for Bromite.

[csagan5]

I made a summary of webRTC in Bromite here: https://github.com/bromite/bromite/wiki/WebRTC

It also mentions what is the information leaked by default so that @wchen342 you can decide the strategy based on that.

[wchen342]

I have changed the way WebRTC is handled. A new flag is added and when it is enabled, new RTCPeerConnection() will now throw a DOMException with NotSupportedError.
A per-site UI is planned, but I need to get the update first so it will be added later.

[mehrvarz]

There is an app in F-Droid that makes verifying the System WebView super easy. It's called WebRTC-Check and it can be found here: https://f-droid.org/packages/timur.webrtc.check

[uazo]

A per-site UI is planned, but I need to get the update first so it will be added later.

I've already seen how to do it. if you need just tell me

[csagan5]

I have opened a Bromite feature request for this: bromite/bromite#1965