Re-enable mDNS #2116
Re-enable mDNS #2116
Conversation
|
What is the purpose of mDNS? Could it be abused somehow in the future if we enable it? |
There was a problem hiding this comment.
I had forgotten about the service discovery flag. I made a new build with your changes and everything looks good to me!
What is the purpose of mDNS? Could it be abused somehow in the future if we enable it?
It's usually used for discovering other devices on the local network. It's how Avahi or Bonjour work if you're familiar with those. It should be used when finding a device for casting as an example. I'm not sure how it could be abused but there's always potential. It may be better to implement a flag to disable it rather building without it entirely if there are future concerns.
|
I'd also be in favor of a flag but thats not blocking |
Those are the things that are banned/blocked/removed from my systems that's why I'm concerned ;) |
|
I use Chromecast often so this is my main motivation for enabling this flag. Chromecast (starting from protocol v2 I believe) uses mDNS to discover casting targets. I'm also not sure if there are other uses of mDNS/Service Discovery 🤷 I can put this behind a flag (that allows mDNS by default?) probably this weekend or so. |
|
Should we merge before or after 107? 🤔 |
|
Either way works. There were a couple small changes that needed to be made in order to build. After making those changes it ran fine and allowed the local IP to be anonymized for WebRTC. There weren't any mDNS connections by default when I had tested previously and there still weren't any with this patch. |
patches/extra/ungoogled-chromium/add-flag-to-disable-mdns.patch
Outdated
Show resolved
Hide resolved
patches/extra/ungoogled-chromium/add-flag-to-disable-mdns.patch
Outdated
Show resolved
Hide resolved
|
@Ahrotahn Wow you're fast, I haven't even tested it myself yet. Good to know it works well, thanks 👍 |
|
Hmm how did you test mDNS connections @Ahrotahn? I tested Casting behavior and it works the same as the build with mDNS/Service Discovery enabled (i.e. before I added the flag) regardless of the flag value. Also I'm getting UDP listeners on |
|
Ran the build and I can confirm local ICE candidates are generated with a .local address, a WebRTC connection can successfully be made using it, and the Here's a screenshot from the demo app I made to test this.
On a different note, just saw this on hacker news - the procedure chromium uses to generate anonymized addresses is not actually random and can be reversed. niespodd/webrtc-local-ip-leak |
|
@Eloston I somehow overlooked that the port was still open, I was only checking the connections made in wireshark. I was more concerned that it would be like dial and send repeated queries for connectable devices on the network. That means there is at least one other place the flag will need to be implemented. I'll dig into that further as I get the time. @lilnasy That's some impeccable timing on that leak post. I was hoping that this change would negate the need to set the WebRTCIPHandlingPolicy... |
|
I was able to prevent the listener with: --- a/net/dns/mdns_client_impl.cc
+++ b/net/dns/mdns_client_impl.cc
@@ -452,6 +452,8 @@
}
int MDnsClientImpl::StartListening(MDnsSocketFactory* socket_factory) {
+ if (base::FeatureList::IsEnabled(network::features::kDisableMdns))
+ return ERR_ABORTED;
DCHECK(!core_.get());
core_ = std::make_unique<Core>(clock_, cleanup_timer_.get());
int rv = core_->Init(socket_factory);I wonder if the patch could be simplified to that and it's flag since it shouldn't be able to make connections in the first place. |
Won't it crash somewhere down the road expecting the listener to succeed beforehand and not checking its actual state? |
|
It didn't when I tested that change. The listener is created separately so it shouldn't be a problem. |
|
Then I suppose this could be a better solution to make everyone happy :) |
|
Unless it has changed recently, enable_service_discovery is false by default (ie deleting it from flags.gn would be a no-op). |
|
It varies depending on your platform and whether mdns is enabled: https://source.chromium.org/chromium/chromium/src/+/refs/tags/107.0.5304.62:chrome/common/features.gni;l=58 |
|
@Ahrotahn Regarding #2116 (comment), I'm a bit unclear on how mDNS is setup; do you know if your patch would also prevent mDNS queries? If so, I can simplify the patch further by removing |
|
It'll prevent the browser's local addresses from being broadcast and resolved but it wouldn't prevent the browser from being able to resolve other devices' addresses. I haven't had a chance to make a build with the new changes to test with yet, but the changes you made in |
|
There were a couple small changes needed to build, but after that everything seems to be working as intended: diff--- a/net/dns/mdns_client_impl.cc
+++ b/net/dns/mdns_client_impl.cc
@@ -25,6 +25,7 @@
#include "net/dns/public/util.h"
#include "net/dns/record_rdata.h"
#include "net/socket/datagram_socket.h"
+#include "services/network/public/cpp/features.h"
// TODO(gene): Remove this temporary method of disabling NSEC support once it
// becomes clear whether this feature should be
--- a/content/browser/direct_sockets/resolve_host_and_open_socket.cc
+++ b/content/browser/direct_sockets/resolve_host_and_open_socket.cc
@@ -33,7 +33,7 @@
#if BUILDFLAG(ENABLE_MDNS)
bool ResemblesMulticastDNSName(const std::string& hostname) {
- if (!base::FeatureList::IsEnabled(network::features::kDisableMdns) {
+ if (!base::FeatureList::IsEnabled(network::features::kDisableMdns)) {
return false;
}
return base::EndsWith(hostname, ".local") || |
|
@Ahrotahn Please check whether this feature works as intended after my intervention and whether it is ready for merge. |
|
Sure thing, I'll have to take some extra time in wireshark to be sure everything is working as intended. |
|
With the recent changes everything works as expected as far as I can tell. One thing I've been thinking about is whether we should invert the flag so that mDNS would be off by default and the flag would enable the functionality. There's pros and cons to this and I would like to hear your thoughts on the idea. A downside would be that it's a deviation from chromium's default, but that's been the existing state of things for a long time in ungoogled-chromium. At least it would now be able to be enabled with these changes. What prompted me to consider this is that currently the browser makes no network requests until the user initiates a request (in a clean profile). With mDNS enabled by default a burst of network traffic happens a bit after startup. It's all contained to the local network, but it could be a privacy concern since that would act as an announcement to other devices on the network of your presence. It's also awkward to have features using |
|
@Ahrotahn I had a hard time figuring the logic behind this inverted flag and I would be in favour of making it "less inverted" for the sake of avoiding the vagueness. Also I think it would be appropriate to have it disabled by default. Thanks for looking into this! |
|
Minor nitpick but inverting the flag made it such that MDNS is disabled by default which I think is a bad default. As far as I know, this feature doesn't have any real privacy implications and just causes Chromecast to be confusingly broken for users. |
|
|
@PF4Public I was referring to this line: c6fa0d4#diff-0d417477f79d089dbf61e8105859741606baaf2d56f6382c31397795eabb8910L85 |
This line changes nothing since we disable mdns via flags.gn anyway. |
Chromecast is crippled in upstream UC, so media remoting can be safely disabled. We can remove enable_media_remoting_rpc=false from debian/rules, as this will automatically become false when enable_media_remoting is set to false. There is an unmerged pull request which enables chromecast [1], which I will implement in a subsequent commit. However I will keep the media remoting configuration option so that it can be disabled independently of chromecast. [1] ungoogled-software/ungoogled-chromium#2116
Did not build or test yet, just submitting a PR based on #1845 (comment)