bdraco / aiodiscover

Discover Hosts via ARP and PTR lookup
Other
4 stars 5 forks source link

Not all routers are DNS resolvers #46

Open brianjmurrell opened 2 months ago

brianjmurrell commented 2 months ago

There seems to be a strange assumption built in here that a subnet's router is also it's resolver:

https://github.com/bdraco/aiodiscover/blame/2e79d022a2f6cc854cd3adbee02ccc75a90336eb/aiodiscover/discovery.py#L135-L140

While this might be true of simple/home/consumer networks, this is most likely not going to be the case in any even moderately managed/secure networks.

In such a managed/secure network, hammering the router/firewall with unwanted/unexpected traffic like this is going to set off alarm bells as it looks quite like a internally initiated DoS attack.

bdraco commented 2 months ago

Do you have a suggestion on how to improve this?

brianjmurrell commented 2 months ago

IMO, just remove the adding of the subnet's router to the nameserver list. I have seen no other device, IoT or otherwise on my network that does this (and I would, just as I have been alerted to this library doing it) so it doesn't seem like it's typically necessary. All of the rest of the devices on my network, just use the subnet configured (i.e. DHCP) nameservers.

I did try to dig into the history of that piece of code to see if there were any issues it was resolving (i.e. to try to understand why it was added) but it seems it's history goes back to an initial import that also includes adding the subnet's gateway address to the nameserver list.

Do you have a recollection of why that functionality was necessary?

bdraco commented 2 months ago

There are quite a few consumer routers that use dnsmasq to provide names for their DHCP assignments. Without this discovery doesn't work for these users because we end up querying their ISP's nameservers instead

brianjmurrell commented 2 months ago

Such consumer routers, running dnsmasq on them, should be giving the DHCP clients it's own address as the resolver they should be using in the DHCP responses. That has been my experience with such consumer routers.

Frankly I cannot even see the point to a router running dnsmasq if it's not going to provide it's own IP address as the nameserver that clients should use. What purpose would such a dnsmasq be providing? Since the answer is none I would say that if there actually were any routers doing this, it would be an outlier/rare/buggy firmware.

If you really do want to placate such routers (although I am sceptical they exist outside of one-off firmware bugs, and as such I would recommend simply removing that functionality) you could gate your code adding the router IP address to the name server list with a test that none of the configured nameservers are in the same subnet as the client and router (i.e. to deal with your use-case above of clients using their ISP resolvers) and moreover, append the router IP to the end of the list of nameservers such that it doesn't even get used if one of the nameservers in the list before it respond to the queries. And to be perfectly clear, an answer of "no such name" is a valid answer and does not result in using other nameservers in the list. Other nameservers should only be used as long as previous nameservers provided no response at all (or I suppose SERVFAIL, etc. -- but not "no such name").

With such a configuration you will only be hitting routers for resolver service in the most rarest of circumstances (and I would argue brokeness of networks).

brianjmurrell commented 1 month ago

I've created 2 PRs for this issue that solve the problem in different ways as I have describe previously. My recommendation is #48 for the most comprehensive solution.