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.karls...@gmail.com> skrev: > From: Erik Karlsson <erik.karls...@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.karls...@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@lists.thekelleys.org.uk https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
