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

[Dominik Derigs]

Hi Erik,

the decision of the Pi-hole team to apply your patch without the usual 
waiting for patches to go into dnsmasq main trunk first is one way to 
show that we were (and still are) absolutely convinced about the patch 
and consider the likeliness of having to revert it at any point very 
low. On top, we actually had at least one but I think even a small 
couple of bug reports which were resolved with this patch right away.

There is nothing wrong with your patch (as far as my point of view 
weights in here) but Simon Kelley is the sole maintainer of dnsmasq and 
he decides which patch gets merged, when and, if.

Best,

Dominik

On 07.10.24 02:13, Erik Karlsson wrote:
> 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
>
>
> _______________________________________________
> Dnsmasq-discuss mailing list
> Dnsmasq-discuss at lists.thekelleys.org.uk
> https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/attachments/20241007/4b005463/attachment.htm>