[Dnsmasq-discuss] [PATCH] Fix segfault and regressions in option --rebind-domain-ok

[Simon Kelley]

On 18/11/2021 18:21, Sung Pae wrote:
> Hello list,
> 
> The --rebind-domain-ok option is broken in v2.86 and on master in the
> following ways:
> 
> * In v2.85, --stop-dns-rebind --rebind-domain-ok=test.me would only allow
>   "test.me" and subdomains of "test.me" to return private addresses to the
>   user. A query for localtest.me, which is known to return 127.0.0.1, is
>   blocked as expected.
> 
>   In v2.86, the --rebind-domain-ok feature is implemented with a simple suffix
>   comparison, which means that --stop-dns-rebind --rebind-domain-ok=test.me
>   fails to block the response of "127.0.0.1" for "localtest.me" because
>   "test.me" is a suffix of "localtest.me".
> 
>   Here is a reproducible example:
> 
>     v2.85$ src/dnsmasq -C /dev/null -a 127.0.0.1 -p 5353 -S 1.1.1.1 -qd --no-resolv --stop-dns-rebind --rebind-domain-ok=test.me
>     dnsmasq: started, version 2.85 cachesize 150
>     dnsmasq: compile time options: IPv6 GNU-getopt no-DBus no-UBus no-i18n no-IDN DHCP DHCPv6 no-Lua TFTP no-conntrack ipset auth no-cryptohash no-DNSSEC loop-detect inotify dumpfile
>     dnsmasq: using nameserver 1.1.1.1#53
>     dnsmasq: read /etc/hosts - 1 addresses
>     dnsmasq: query[A] localtest.me from 127.0.0.1
>     dnsmasq: forwarded localtest.me to 1.1.1.1
>     dnsmasq: possible DNS-rebind attack detected: localtest.me
> 
>     master$ src/dnsmasq -C /dev/null -a 127.0.0.1 -p 5353 -S 1.1.1.1 -qd --no-resolv --stop-dns-rebind --rebind-domain-ok=test.me
>     dnsmasq: started, version 2.87test4-4-gc0409fa cachesize 150
>     dnsmasq: compile time options: IPv6 GNU-getopt no-DBus no-UBus no-i18n no-IDN DHCP DHCPv6 no-Lua TFTP no-conntrack ipset no-nftset auth no-cryptohash no-DNSSEC loop-detect inotify dumpfile
>     dnsmasq: using nameserver 1.1.1.1#53
>     dnsmasq: read /etc/hosts - 1 addresses
>     dnsmasq: query[A] localtest.me from 127.0.0.1
>     dnsmasq: forwarded localtest.me to 1.1.1.1
>     dnsmasq: reply localtest.me is 127.0.0.1
> 
> * In v2.85, --stop-dns-rebind --rebind-domain-ok=// means "stop potential DNS
>   rebinding attacks, but allow private responses for dotless domains", which
>   mirrors the special meaning of // in the --server option.
> 
>   In v2.86, --stop-dns-rebind --rebind-domain-ok=// crashes dnsmasq during
>   resolution.
> 
>     v2.85$ src/dnsmasq -C /dev/null -a 127.0.0.1 -p 5353 -S 192.168.0.1 -qd --no-resolv --stop-dns-rebind --rebind-domain-ok=//
>     dnsmasq: started, version 2.85 cachesize 150
>     dnsmasq: compile time options: IPv6 GNU-getopt no-DBus no-UBus no-i18n no-IDN DHCP DHCPv6 no-Lua TFTP no-conntrack ipset auth no-cryptohash no-DNSSEC loop-detect inotify dumpfile
>     dnsmasq: using nameserver 1.1.1.1#53
>     dnsmasq: read /etc/hosts - 1 addresses
>     dnsmasq: query[A] brother-laser-printer from 127.0.0.1
>     dnsmasq: forwarded brother-laser-printer to 192.168.0.1
>     dnsmasq: reply brother-laser-printer is 192.168.0.50
> 
>     master$ src/dnsmasq -C /dev/null -a 127.0.0.1 -p 5353 -S 192.168.0.1 -qd --no-resolv --stop-dns-rebind --rebind-domain-ok=//
>     dnsmasq: started, version 2.87test4-2-g9560658 cachesize 150
>     dnsmasq: compile time options: IPv6 GNU-getopt no-DBus no-UBus no-i18n no-IDN DHCP DHCPv6 no-Lua TFTP no-conntrack ipset no-nftset auth no-cryptohash no-DNSSEC loop-detect inotify dumpfile
>     dnsmasq: using nameserver 1.1.1.1#53
>     dnsmasq: read /etc/hosts - 1 addresses
>     dnsmasq: query[A] brother-laser-printer from 127.0.0.1
>     Segmentation fault (core dumped)
> 
>   Note that the new suffix-matching algorithm of --rebind-domain-ok means that
>   even if the crash above is fixed, an empty option value effectively negates
>   --stop-dns-rebind because the empty string is a suffix of all possible
>   strings.
> 
> The attached patches address the issues above and restore the behavior of
> --rebind-domain-ok back to the semantics of v2.85. The patches are also
> available on Github:
> 
> https://github.com/guns/dnsmasq/compare/master...fix-option-rebind-domain-ok
> https://github.com/guns/dnsmasq/commit/3abd86eb9e53efeea270767fd251284851d706d4
> https://github.com/guns/dnsmasq/commit/4afb5b4ce50a4d3b7f917d2ce83ea1b27dd55db5
> 

Thanks for the clear and complete bug report. I accept that these are
bugs and regressions. I applied your first patch as is, but the second
one seems to be a way over-complicated fix. I've therefore taken the
liberty of fixing the issue a different way. Please see
https://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=commit;h=1176cd58c90fc37bf98a6f774b26fc1adc8fd8e9

If you could test that, and make sure I didn't break things differently,
I'd be very grateful.

Thanks again.


Simon.