Hey Frank, dear list-members,

thanks for your proposed fix. The Pi-hole team adopted dnsmasq v2.81
early on and we're seeing reports for mysterious crashes scattered all
over the dnsmasq code since the very first days of releasing our latest
version. Crashes reports show several locations, e.g., in_zone(),
iface_check() and even main() in a few places. TCP forks were always
happening sometime (not necessarily immediately) before a crash.
Running dnsmasq in debug mode (prevents forking) stopped any crashes.

So far, we haven't had the time to dig into this ourselves properly,
however, your description perfectly matches with my preliminary
conclusion for one week ago:
> the dangling pointer is in **all** bug reports known so far in the
daemon pointer
(https://github.com/pi-hole/FTL/issues/805#issuecomment-643157367)

The addition of TCP forks being able to add cache replies in dnsmasq
v2.81 is what is causing this code (which has been like this for along
time) to become problematic. I recently looked at the same code but
missed the chance that the byte may be still in the pipe. This is a
very good catch!

I'm writing this mail to raise awareness that this appears to be a high
severity bug as it can easily lead to anything between indeterminable
behavior to fatal failures (typically the case). Even when the bug can
only be triggered under certain conditions, it recently caused over 200
posts on the Pi-hole issue ticket system (Github). Crashes due to this
have been reported independently by several (more than 20) individual
users.
Best
Dominik

On Wed, 2020-06-17 at 19:52 -0700, Frank wrote:
> Hello,
> 
> This patch fixes a buffer overflow in TCP requests. Since the read is
> not actually being retried, the byte written by the child can be left
> in the pipe. When that happens, cache_recv_insert() reads the length
> of the name, which is now multiplied by 256 due to the extra 0 byte
> (8 bit shift) and results in daemon->namebuff being overflowed.
> Namebuff is immediately before the daemon struct in memory so it ends
> up corrupting the beginning of the daemon struct.
> 
> diff --git a/src/dnsmasq.c b/src/dnsmasq.c
> index 6481de8..457dea3 100644
> --- a/src/dnsmasq.c
> +++ b/src/dnsmasq.c
> @@ -1887,7 +1887,7 @@ static void check_dns_listeners(time_t now)
>                            single byte comes back up the pipe, which
>                            is sent by the child after it has closed
> the
>                            netlink socket. */
> -                       retry_send(read(pipefd[0], &a, 1));
> +                       while (retry_send(read(pipefd[0], &a, 1)));
>  #endif
>                         break;
>                       }
> @@ -1928,7 +1928,7 @@ static void check_dns_listeners(time_t now)
>  #ifdef HAVE_LINUX_NETWORK
>                   /* See comment above re netlink socket. */
>                   close(daemon->netlinkfd);
> -                 retry_send(write(pipefd[1], &a, 1));
> +                 while (retry_send(write(pipefd[1], &a, 1)));
>  #endif
>                 }
> 
> Frank
> 
> 
> _______________________________________________
> Dnsmasq-discuss mailing list
> Dnsmasq-discuss@lists.thekelleys.org.uk
> http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss


_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

Reply via email to