[Dnsmasq-discuss] [PATCH] Update DNS records after pruning DHCP leases

[Erik Karlsson]

Hi everyone,

Does anyone have something to say about this patch? I sent it for the first
time about 6 months ago but the discussion quickly died down and it seems
to have been forgotten about since then.

To elaborate a bit about the rationale behind it the issue was discovered
by analyzing core dumps from live customer installations which I for
obvious confidentiality/privacy reasons cannot share. The core dumps showed
a use after free of crecp->name.namep in cache_scan_free for records
derived from DHCP. The core dumps also showed that the value of dns_dirty
was 1 which is indicative of the fact that lease_update_dns needs to be run
but has not been run.

For the issue to reproduce with default configuration I think you need at
least two DHCP derived names which share the same domain (otherwise the
issue is masked by the is_expired logic). One of them has to expire
followed by a request for either the domain itself or something with a
coinciding hash before any DHCP requests are handled. Additionally for an
actual crash to happen the memory has to be unmapped as opposed to just
returned to the free list. In most cases rather than crashing garbage data
will be returned, something which may or may not go unnoticed. So, it is a
bit of a corner case issue but a very real one as evidenced by the core
dumps.

>From analysis of the code and especially other places where lease_prune is
used it also seems to be evident that lease_update_dns is needed
afterwards. The only place where this is not done is async_event and this
is what the patch is addressing.

In addition when use-stale-cache is enabled the issue seems to reproduce
more easily, something which has been reported by the Pi-hole community:

https://discourse.pi-hole.net/t/host-name-of-client-xxx-contains-at-least-one-invalid-character-at-position-0/69132

The patch was subsequently applied to Pi-hole with seemingly favourable
outcome:

https://github.com/pi-hole/FTL/pull/1965

Best regards,
Erik

Den tis 1 okt. 2024 11:01Erik Karlsson <erik.r.karlsson at gmail.com> skrev:

> From: Erik Karlsson <erik.karlsson at iopsys.eu>
>
> Not doing so can result in a use after free since the name for DHCP
> derived DNS records is represented as a pointer into the DHCP lease
> table. Update will only happen when necessary since lease_update_dns
> tests internally on dns_dirty and the force argument is zero.
>
> Signed-off-by: Erik Karlsson <erik.karlsson at iopsys.eu>
> ---
>  src/dnsmasq.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/dnsmasq.c b/src/dnsmasq.c
> index a9f26ae..1be3b82 100644
> --- a/src/dnsmasq.c
> +++ b/src/dnsmasq.c
> @@ -1518,6 +1518,7 @@ static void async_event(int pipe, time_t now)
>           {
>             lease_prune(NULL, now);
>             lease_update_file(now);
> +           lease_update_dns(0);
>           }
>  #ifdef HAVE_DHCP6
>         else if (daemon->doing_ra)
> --
> 2.46.2
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/attachments/20241007/dab03944/attachment.htm>