[Dnsmasq-discuss] dnsmasq 2.25 crashing in some DHCP NAK cases (and possible fix)

[Lutz Pressler]

Hello,

at least 2.25 (and probably since 2.23 as then sending a server identifier
with DHCPNAK had been introduced) crashes "in case of a machine moving whilst
it has a lease". Reason (rfc2131.c line 631ff):

   if (!(context = narrow_context(context, mess->yiaddr)))
      {
        /* If a machine moves networks whilst it has a lease, we catch that 
here. */
        message = _("wrong network");
        /* ensure we broadcast NAK */
        unicast_dest = 0;
      }

... so in this case "context" is NULL.
But in the section starting line 664 "context" is dereferenced:

   if (message)
      {
          log_packet("NAK", &mess->yiaddr, chaddr, iface_name, message);

        mess->siaddr.s_addr = mess->yiaddr.s_addr = 0;
        bootp_option_put(mess, NULL, NULL);
        p = option_put(p, end, OPTION_MESSAGE_TYPE, 1, DHCPNAK);
-->     p = option_put(p, end, OPTION_SERVER_IDENTIFIER, INADDRSZ, 
ntohl(context->local.s_addr));
        p = option_put_string(p, end, OPTION_MESSAGE, message);
        /* This fixes a problem with the DHCP spec, broadcasting a NAK to a 
host on
           a distant subnet which unicast a REQ to us won't work. */
        if (!unicast_dest || mess->giaddr.s_addr != 0 ||
-->         mess->ciaddr.s_addr == 0 || is_same_net(context->local, 
mess->ciaddr, context->netmask))
            {
              mess->flags |= htons(0x8000); /* broadcast */
              mess->ciaddr.s_addr = 0;
            }
        }

The second case is no problem because of lazy evaluation as
"unicast_dest" is zero and tested earlier - but the first case is:
The attached patch changes "ntohl(context->local.s_addr)" into
"context ? ntohl(context->local.s_addr) : NULL" and fixes the
problem for me - but probably sending a NULL server identifier
is not correct. As I understand RFC 2131, the option has to be
included, but maybe one should use a local address of the
interface the request came in?

Have a nice day,
  Lutz

-- 
Lutz Preßler, Göttingen, Germany 

--- dnsmasq-2.25/src/rfc2131.c  2005-12-13 17:23:13.000000000 +0100
+++ dnsmasq-2.25.fix/src/rfc2131.c      2006-01-22 03:32:14.000000000 +0100
@@ -668,7 +668,7 @@
          mess->siaddr.s_addr = mess->yiaddr.s_addr = 0;
          bootp_option_put(mess, NULL, NULL);
          p = option_put(p, end, OPTION_MESSAGE_TYPE, 1, DHCPNAK);
-         p = option_put(p, end, OPTION_SERVER_IDENTIFIER, INADDRSZ, 
ntohl(context->local.s_addr));
+         p = option_put(p, end, OPTION_SERVER_IDENTIFIER, INADDRSZ, context ? 
ntohl(context->local.s_addr) : NULL);
          p = option_put_string(p, end, OPTION_MESSAGE, message);
          /* This fixes a problem with the DHCP spec, broadcasting a NAK to a 
host on 
             a distant subnet which unicast a REQ to us won't work. */

pgpaWetKt9HmB.pgp
Description: PGP signature