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

[Erik Karlsson]

Hi Dominik,

Nice to hear that it works and solves the issues. To me it seems this is a
simple oversight which has gone undetected for years because like I
described previously the triggering conditions are rather convoluted. It is
really a corner case issue.

Best regards,
Erik

Den mån 7 okt. 2024 21:25Dominik Derigs via Dnsmasq-discuss <
dnsmasq-discuss at lists.thekelleys.org.uk> skrev:

> 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 listDnsmasq-discuss at lists.thekelleys.org.ukhttps://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
>
> _______________________________________________
> 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/20241009/17f191a1/attachment.htm>