replace legacy / hard-wired WebRTC IP handling policy changes with existing kWebRTCIPHandlingPolicy preference

[thestinger]

The patch in question:

https://github.com/bromite/bromite/blob/master/build/patches/Change-default-webRTC-policy-to-not-use-any-address.patch

The kWebRTCMultipleRoutesEnabled and kWebRTCNonProxiedUdpEnabled preferences are obsolete legacy options replaced by kWebRTCIPHandlingPolicy. Those shouldn't be used anymore and were supposed to be removed by now.

The changes to the DEFAULT case in the current patch are changing how the non-restricted IP handling policy works rather than enabling a restricted IP handling policy by default.

This is a better approach using the existing option:

From b8ef31cae41eaeb7407709b9c3f16cc3a1547bbc Mon Sep 17 00:00:00 2001
From: Daniel Micay <danielmicay@gmail.com>
Date: Thu, 21 May 2020 12:58:04 -0400
Subject: [PATCH 37/56] most private WebRTC IP handling policy by default

---
 chrome/browser/ui/browser_ui_prefs.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/chrome/browser/ui/browser_ui_prefs.cc b/chrome/browser/ui/browser_ui_prefs.cc
index c2581e827e46..d895fc115611 100644
--- a/chrome/browser/ui/browser_ui_prefs.cc
+++ b/chrome/browser/ui/browser_ui_prefs.cc
@@ -86,7 +86,7 @@ void RegisterBrowserUserPrefs(user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterBooleanPref(prefs::kWebRTCMultipleRoutesEnabled, true);
   registry->RegisterBooleanPref(prefs::kWebRTCNonProxiedUdpEnabled, true);
   registry->RegisterStringPref(prefs::kWebRTCIPHandlingPolicy,
-                               blink::kWebRTCIPHandlingDefault);
+                               blink::kWebRTCIPHandlingDisableNonProxiedUdp);
   registry->RegisterStringPref(prefs::kWebRTCUDPPortRange, std::string());
   registry->RegisterBooleanPref(prefs::kWebRtcEventLogCollectionAllowed, false);
   registry->RegisterListPref(prefs::kWebRtcLocalIpsAllowedUrls);
-- 
2.26.2

[csagan5]

Thanks for submitting an issue.

The kWebRTCMultipleRoutesEnabled and kWebRTCNonProxiedUdpEnabled preferences are obsolete legacy options replaced by kWebRTCIPHandlingPolicy. Those shouldn't be used anymore and were supposed to be removed by now.

Yes, I am aware of the deprecation in progress, I left them changed just in case as it was not clear to me if they had any effect. From reading the code a few weeks ago I remember that when the browser finds them at initialization time they are simply used to set the new preference value and supposedly never accessed again.

The changes to the DEFAULT case in the current patch are changing how the non-restricted IP handling policy works rather than enabling a restricted IP handling policy by default.

This is a better approach using the existing option:

This corresponds to the patch used before fixing #553; you can read there why this was not sufficient: Custom Content Tabs would ignore that policy and use the default instead.

[thestinger]

Yes, I am aware of the deprecation in progress, I left them changed just in case as it was not clear to me if they had any effect. From reading the code a few weeks ago I remember that when the browser finds them at initialization time they are simply used to set the new preference value and supposedly never accessed again.

Yeah, it just migrates from them in chrome/browser/renderer_preferences_util.cc if kWebRTCIPHandlingPolicy is not already set.

This corresponds to the patch used before fixing #553; you can read there why this was not sufficient: Custom Content Tabs would ignore that policy and use the default instead.

I think it does respect the policy. This code path just isn't used for the WebView or custom tabs since it sets up preferences for the browser specifically. You would need to register the preference elsewhere. The preference does work, just not registering it only for the browser. It applies to other preferences that are set there too.

[csagan5]

You would need to register the preference elsewhere.

That is correct, however I do not know how to set the default preference for the Custom Content Tabs or the SystemWebView, thus the old patch (same as yours) would leave those two use-cases uncovered. The only extra information that I have verified exposes is the local IP address (LAN), see this comment.

@thestinger would you know how to set the default preferences also for CCTs and the SystemWebView (or any other preferences set which may exist)?

[thestinger]

Maybe it can be set in third_party/blink/public/mojom/renderer_preferences.mojom.

[csagan5]

That looks like the set of blink-specific preferences, it is a very small set compared to the browser/user preferences/settings.

It might even be possible that the preferences we are looking for live outside Chromium, somehow, since apps might set/define them for Custom Content Tabs.

[thestinger]

The fallback causing kWebRTCIPHandlingDefault to actually be the default is implemented in third_party/blink/renderer/modules/peerconnection/peer_connection_dependency_factory.cc in the GetWebRTCIPHandlingPolicy function. Could instead add an explicit check for kWebRTCIPHandlingDefault and then have the fallthrough option be kWebRTCIPHandlingDisableNonProxiedUdp instead. It just seems better to set the preference everywhere since it's less invasive and less prone to error. I don't know where to set these for the WebView / custom tabs or how many of the existing preferences are being set there if any.

[thestinger]

Something like this would work and wouldn't require hard-wiring different values for the 'default' setting which is meant to have everything enabled:

diff --git a/third_party/blink/renderer/modules/peerconnection/peer_connection_dependency_factory.cc b/third_party/blink/renderer/modules/peerconnection/peer_connection_dependency_factory.cc
index 15929f4ae020..4ad46c6bf38c 100644
--- a/third_party/blink/renderer/modules/peerconnection/peer_connection_dependency_factory.cc
+++ b/third_party/blink/renderer/modules/peerconnection/peer_connection_dependency_factory.cc
@@ -82,7 +82,9 @@ WebRTCIPHandlingPolicy GetWebRTCIPHandlingPolicy(const String& preference) {
     return DEFAULT_PUBLIC_INTERFACE_ONLY;
   if (preference == kWebRTCIPHandlingDisableNonProxiedUdp)
     return DISABLE_NON_PROXIED_UDP;
-  return DEFAULT;
+  if (preference == kWebRTCIPHandlingDefault)
+    return DEFAULT;
+  return DISABLE_NON_PROXIED_UDP;
 }
 
 bool IsValidPortRange(uint16_t min_port, uint16_t max_port) {

That way, only having it explicitly set to that option would trigger that case. This will work unless something is actually explicitly setting it to that, and that doesn't seem to be what's happening. I think what's happening is the preference is just not being set so it falls through.

[csagan5]

That way, only having it explicitly set to that option would trigger that case. This will work unless something is actually explicitly setting it to that, and that doesn't seem to be what's happening. I think what's happening is the preference is just not being set so it falls through.

I think so too, it is a default. I had seen that switch-like block in GetWebRTCIPHandlingPolicy but did not think of explicitly comparing towards kWebRTCIPHandlingDefault as you did. It would work only if kWebRTCIPHandlingDefault is not kWebRTCIPHandlingDisableNonProxiedUdp.

The change you proposed can be tested relatively easily thanks to the VLOG function there (I changed it to LOG in the patch already).

I am however hesitant to change the patch in any direction until I understand better how preferences work for Custom Content Tabs and the SystemWebView; on a side topic: other preferences whose default we are changing are also reverting to default for these 2 use cases, which is a bit concerning.

[csagan5]

From #553:

disable_non_proxied_udp has been used since version 69.0.3497.73 (v68 was the last version with the possibility of disabling the build of webRTC) however I can observe:

• it does not leak local/public IP addresses in the webRTC offer when opening a web page in a regular tab
• it shows local and public IP when connecting from a custom tab. If you are on a VPN it will however show only the VPN public IP, thus no leak is happening.

When navigating Bromite regular tabs we have in logs:

WebRTC routing preferences: policy: 3, multiple_routes: 0, nonproxied_udp: 0, min_udp_port: 0, max_udp_port: 0, allow_mdns_obfuscation: 1

When using the custom tab intent instead:

WebRTC routing preferences: policy: 0, multiple_routes: 1, nonproxied_udp: 1, min_udp_port: 0, max_udp_port: 0, allow_mdns_obfuscation: 1

This is because the custom tabs appear to use a different set of preferences; @uazo are you aware of how custom tabs work in terms of preferences?

I will check other preferences we set for other potential issues related to CCTs.

I will patch Bromite for next release to change the default policy enforced flags. @Eloston you might find this interesting as well, the patch is available here: #555

An update related to most recent v100.

Chromium

Vanilla Chromium behaviour currently is:

Regular tabs

WebRTC routing preferences: policy: 0, multiple_routes: 1, nonproxied_udp: 1, min_udp_port: 0, max_udp_port: 0, allow_mdns_obfuscation: 1

CCTs

WebRTC routing preferences: policy: 0, multiple_routes: 1, nonproxied_udp: 1, min_udp_port: 0, max_udp_port: 0, allow_mdns_obfuscation: 1

Support/Leaks

For both regular tabs and CCTs:

• RTCPeerConnection
• RTCDataChannel
• Local IP Address ⚠️
• Public IP Address ⚠️ (it's a problem when using VPN if it shows the non-VPN public address)
• IPv6 Address

This has not changed recently in upstream Chromium, I am summarising it here for reference.

Bromite

Regular tabs

WebRTC routing preferences: policy: 2, multiple_routes: 0, nonproxied_udp: 0, min_udp_port: 0, max_udp_port: 0, allow_mdns_obfuscation: 1

CCTs

WebRTC routing preferences: policy: 2, multiple_routes: 0, nonproxied_udp: 0, min_udp_port: 0, max_udp_port: 0, allow_mdns_obfuscation: 1

Support/Leaks

For both regular tabs and CCTs:

• RTCPeerConnection ❓
• RTCDataChannel
• Local IP Address 🆗
• Public IP Address 🆗 (not leaking when using VPN)
• IPv6 Address

Conclusion

I made a summary here of how it works in Chromium and what Bromite changes here: https://github.com/bromite/bromite/wiki/WebRTC

@thestinger after some tests and changes I believe that now the patch is behaving as you suggested and it is less invasive; functionality is still not working as expected because of the disabled non-proxied UDP, but that is on purpose in Bromite.

With the introduction of an user setting it will be possible for users to control this as they please while the defaults in Bromite stays non-leaking: #1965

[qua3k]

I don’t understand why you’re explicitly adding a case for kDefault when it’s the only remaining case. All the other cases are listed directly above…

[qua3k]

Ah, I hadn't read the thread fully. it's weird that the preference isn't being passed to GetWebRTCIPHandlingPolicy but it's possible for you to just eliminate the duplicate check for kWebRTCIPHandlingDisableNonProxiedUdp.

[csagan5]

it's possible for you to just eliminate the duplicate check for kWebRTCIPHandlingDisableNonProxiedUdp.

Which duplicate check?