Fix crash when no upstream servers defined.

This is a regession introduced in 3b6df06fb8.

When dnsmasq is started without upstreams (yet), but a
DNS query comes in that needs forwarding dnsmasq now potentially crashes as
the value for "first" variable is undetermined.

A segmentation violation occurs when the index
is out of bounds of  serverarray.

Credits go to pedro0311 <pedro@freshtomato.org>

CHANGELOG

@@ -113,9 +113,11 @@ version 2.91
 	badly with rare broken DNS servers which don't preserve the case
 	of the query in their reply. The first time a reply is returned
 	which matches the query in all respects except case, a warning
-	will be logged. If this coincides with DNS not functioning, it
-	is necessary to disable bit 0x20 encoding with --no-0x20-encode.
-
+	will be logged. In this release, 0x020-encoding is default-off
+	and must be explicitly enabled with --do-0x20-encoding. In future
+	releases it may default on. You can avoid a future release
+	changing the behaviour of an installation with --no-x20-encode.
+	
 	Fix a long-standing problem when two queries which are identical
 	in every repect _except_ case, get combined by dnsmasq. If
 	dnsmasq gets eg, two queries for example.com and Example.com

man/dnsmasq.8

@@ -852,13 +852,17 @@ name on successive queries, for load-balancing. This turns off that
 behaviour, so that the records are always returned in the order
 that they are received from upstream.
 .TP
-.B --no-0x20-encode
-By default, dnsmasq scrambles the case of letters in DNS queries it sends upstream as a security feature.
+.B --do-0x20-encode, --no-0x20-encode
+Dnsmasq can scramble the case of letters in DNS queries it sends upstream as a security feature.
 This technique can interact badly with rare broken DNS servers which don't preserve the case
 of the query in their reply. The first time a reply is returned
 which matches the query in all respects except case, a warning
 will be logged. If this coincides with DNS not functioning, it
-is necessary to disable this scrambling with --no-0x20-encode.
+is necessary to disable the feature. As at version 2.91, 0x20 encoding
+is disabled by default, and must be enabled with --do-0x20-encode. The default
+may change in the future, so to be sure of its status after an upgrade, set --do-0x20-encode
+or --no-0x20-encode in your config. --no-0x20-encode overrides --do-x20-encode or a future default
+0x20-encode enable.
 .TP
 .B --use-stale-cache[=<max TTL excess in s>]
 When set, if a DNS name exists in the cache, but its time-to-live has expired, dnsmasq will return the data anyway. (It attempts to refresh the

src/bpf.c

@@ -126,112 +126,110 @@ int iface_enumerate(int family, void *parm, callback_t callback)
   
   for (addrs = head; addrs; addrs = addrs->ifa_next)
     {
-      if (addrs->ifa_addr->sa_family == family)
+      int iface_index = if_nametoindex(addrs->ifa_name);
+      
+      if (iface_index == 0 || !addrs->ifa_addr || 
+	  addrs->ifa_addr->sa_family != family ||
+	  (!addrs->ifa_netmask && family != AF_LINK))
+	continue;
+      
+      if (family == AF_INET)
 	{
-	  int iface_index = if_nametoindex(addrs->ifa_name);
-
-	  if (iface_index == 0 || !addrs->ifa_addr || 
-	      (!addrs->ifa_netmask && family != AF_LINK))
+	  struct in_addr addr, netmask, broadcast;
+	  addr = ((struct sockaddr_in *) addrs->ifa_addr)->sin_addr;
+#ifdef HAVE_BSD_NETWORK
+	  if (del_family == AF_INET && del_addr.addr4.s_addr == addr.s_addr)
 	    continue;
-
-	  if (family == AF_INET)
-	    {
-	      struct in_addr addr, netmask, broadcast;
-	      addr = ((struct sockaddr_in *) addrs->ifa_addr)->sin_addr;
-#ifdef HAVE_BSD_NETWORK
-	      if (del_family == AF_INET && del_addr.addr4.s_addr == addr.s_addr)
-		continue;
 #endif
-	      netmask = ((struct sockaddr_in *) addrs->ifa_netmask)->sin_addr;
-	      if (addrs->ifa_broadaddr)
-		broadcast = ((struct sockaddr_in *) addrs->ifa_broadaddr)->sin_addr; 
-	      else 
-		broadcast.s_addr = 0;	      
-	      if (!callback.af_inet(addr, iface_index, NULL, netmask, broadcast, parm))
-		goto err;
-	    }
-	  else if (family == AF_INET6)
-	    {
-	      struct in6_addr *addr = &((struct sockaddr_in6 *) addrs->ifa_addr)->sin6_addr;
-	      unsigned char *netmask = (unsigned char *) &((struct sockaddr_in6 *) addrs->ifa_netmask)->sin6_addr;
-	      int scope_id = ((struct sockaddr_in6 *) addrs->ifa_addr)->sin6_scope_id;
-	      int i, j, prefix = 0;
-	      u32 valid = 0xffffffff, preferred = 0xffffffff;
-	      int flags = 0;
+	  netmask = ((struct sockaddr_in *) addrs->ifa_netmask)->sin_addr;
+	  if (addrs->ifa_broadaddr)
+	    broadcast = ((struct sockaddr_in *) addrs->ifa_broadaddr)->sin_addr; 
+	  else 
+	    broadcast.s_addr = 0;	      
+	  if (!callback.af_inet(addr, iface_index, NULL, netmask, broadcast, parm))
+	    goto err;
+	}
+      else if (family == AF_INET6)
+	{
+	  struct in6_addr *addr = &((struct sockaddr_in6 *) addrs->ifa_addr)->sin6_addr;
+	  unsigned char *netmask = (unsigned char *) &((struct sockaddr_in6 *) addrs->ifa_netmask)->sin6_addr;
+	  int scope_id = ((struct sockaddr_in6 *) addrs->ifa_addr)->sin6_scope_id;
+	  int i, j, prefix = 0;
+	  u32 valid = 0xffffffff, preferred = 0xffffffff;
+	  int flags = 0;
 #ifdef HAVE_BSD_NETWORK
-	      if (del_family == AF_INET6 && IN6_ARE_ADDR_EQUAL(&del_addr.addr6, addr))
-		continue;
+	  if (del_family == AF_INET6 && IN6_ARE_ADDR_EQUAL(&del_addr.addr6, addr))
+	    continue;
 #endif
 #if defined(HAVE_BSD_NETWORK) && !defined(__APPLE__)
-	      struct in6_ifreq ifr6;
-
-	      memset(&ifr6, 0, sizeof(ifr6));
-	      safe_strncpy(ifr6.ifr_name, addrs->ifa_name, sizeof(ifr6.ifr_name));
+	  struct in6_ifreq ifr6;
+	  
+	  memset(&ifr6, 0, sizeof(ifr6));
+	  safe_strncpy(ifr6.ifr_name, addrs->ifa_name, sizeof(ifr6.ifr_name));
+	  
+	  ifr6.ifr_addr = *((struct sockaddr_in6 *) addrs->ifa_addr);
+	  if (fd != -1 && ioctl(fd, SIOCGIFAFLAG_IN6, &ifr6) != -1)
+	    {
+	      if (ifr6.ifr_ifru.ifru_flags6 & IN6_IFF_TENTATIVE)
+		flags |= IFACE_TENTATIVE;
+	      
+	      if (ifr6.ifr_ifru.ifru_flags6 & IN6_IFF_DEPRECATED)
+		flags |= IFACE_DEPRECATED;
 	      
-	      ifr6.ifr_addr = *((struct sockaddr_in6 *) addrs->ifa_addr);
-	      if (fd != -1 && ioctl(fd, SIOCGIFAFLAG_IN6, &ifr6) != -1)
-		{
-		  if (ifr6.ifr_ifru.ifru_flags6 & IN6_IFF_TENTATIVE)
-		    flags |= IFACE_TENTATIVE;
-		  
-		  if (ifr6.ifr_ifru.ifru_flags6 & IN6_IFF_DEPRECATED)
-		    flags |= IFACE_DEPRECATED;
-
 #ifdef IN6_IFF_TEMPORARY
-		  if (!(ifr6.ifr_ifru.ifru_flags6 & (IN6_IFF_AUTOCONF | IN6_IFF_TEMPORARY)))
-		    flags |= IFACE_PERMANENT;
+	      if (!(ifr6.ifr_ifru.ifru_flags6 & (IN6_IFF_AUTOCONF | IN6_IFF_TEMPORARY)))
+		flags |= IFACE_PERMANENT;
 #endif
-
+	      
 #ifdef IN6_IFF_PRIVACY
-		  if (!(ifr6.ifr_ifru.ifru_flags6 & (IN6_IFF_AUTOCONF | IN6_IFF_PRIVACY)))
-		    flags |= IFACE_PERMANENT;
+	      if (!(ifr6.ifr_ifru.ifru_flags6 & (IN6_IFF_AUTOCONF | IN6_IFF_PRIVACY)))
+		flags |= IFACE_PERMANENT;
 #endif
-		}
-	      
-	      ifr6.ifr_addr = *((struct sockaddr_in6 *) addrs->ifa_addr);
-	      if (fd != -1 && ioctl(fd, SIOCGIFALIFETIME_IN6, &ifr6) != -1)
-		{
-		  valid = ifr6.ifr_ifru.ifru_lifetime.ia6t_vltime;
-		  preferred = ifr6.ifr_ifru.ifru_lifetime.ia6t_pltime;
-		}
+	    }
+	  
+	  ifr6.ifr_addr = *((struct sockaddr_in6 *) addrs->ifa_addr);
+	  if (fd != -1 && ioctl(fd, SIOCGIFALIFETIME_IN6, &ifr6) != -1)
+	    {
+	      valid = ifr6.ifr_ifru.ifru_lifetime.ia6t_vltime;
+	      preferred = ifr6.ifr_ifru.ifru_lifetime.ia6t_pltime;
+	    }
 #endif
-	      	      
-	      for (i = 0; i < IN6ADDRSZ; i++, prefix += 8) 
-                if (netmask[i] != 0xff)
-		  break;
-	      
-	      if (i != IN6ADDRSZ && netmask[i]) 
-                for (j = 7; j > 0; j--, prefix++) 
-		  if ((netmask[i] & (1 << j)) == 0)
-		    break;
-	      
-	      /* voodoo to clear interface field in address */
-	      if (!option_bool(OPT_NOWILD) && IN6_IS_ADDR_LINKLOCAL(addr))
-		{
-		  addr->s6_addr[2] = 0;
-		  addr->s6_addr[3] = 0;
-		} 
-	     
-	      if (!callback.af_inet6(addr, prefix, scope_id, iface_index, flags,
-				(unsigned int) preferred, (unsigned int)valid, parm))
-		goto err;	      
-	    }
-
-#ifdef HAVE_DHCP6      
-	  else if (family == AF_LINK)
-	    { 
-	      /* Assume ethernet again here */
-	      struct sockaddr_dl *sdl = (struct sockaddr_dl *) addrs->ifa_addr;
-	      if (sdl->sdl_alen != 0 && 
-		  !callback.af_local(iface_index, ARPHRD_ETHER, LLADDR(sdl), sdl->sdl_alen, parm))
-		goto err;
-	    }
-#endif 
+	  
+	  for (i = 0; i < IN6ADDRSZ; i++, prefix += 8) 
+	    if (netmask[i] != 0xff)
+	      break;
+	  
+	  if (i != IN6ADDRSZ && netmask[i]) 
+	    for (j = 7; j > 0; j--, prefix++) 
+	      if ((netmask[i] & (1 << j)) == 0)
+		break;
+	  
+	  /* voodoo to clear interface field in address */
+	  if (!option_bool(OPT_NOWILD) && IN6_IS_ADDR_LINKLOCAL(addr))
+	    {
+	      addr->s6_addr[2] = 0;
+	      addr->s6_addr[3] = 0;
+	    } 
+	  
+	  if (!callback.af_inet6(addr, prefix, scope_id, iface_index, flags,
+				 (unsigned int) preferred, (unsigned int)valid, parm))
+	    goto err;	      
 	}
+      
+#ifdef HAVE_DHCP6      
+      else if (family == AF_LINK)
+	{ 
+	  /* Assume ethernet again here */
+	  struct sockaddr_dl *sdl = (struct sockaddr_dl *) addrs->ifa_addr;
+	  if (sdl->sdl_alen != 0 && 
+	      !callback.af_local(iface_index, ARPHRD_ETHER, LLADDR(sdl), sdl->sdl_alen, parm))
+	    goto err;
+	}
+#endif 
     }
   
   ret = 1;
-
+  
  err:
   errsave = errno;
   freeifaddrs(head); 

src/dnsmasq.h

@@ -280,7 +280,8 @@ struct event_desc {
 #define OPT_LOCALHOST_SERVICE  72
 #define OPT_LOG_PROTO      73
 #define OPT_NO_0x20        74
-#define OPT_LAST           75
+#define OPT_DO_0x20        75
+#define OPT_LAST           76
 
 #define OPTION_BITS (sizeof(unsigned int)*8)
 #define OPTION_SIZE ( (OPT_LAST/OPTION_BITS)+((OPT_LAST%OPTION_BITS)!=0) )

src/forward.c

@@ -197,12 +197,18 @@ static void forward_query(int udpfd, union mysockaddr *udpaddr,
 					     FREC_HAS_PHEADER | FREC_DNSKEY_QUERY | FREC_DS_QUERY | FREC_NO_CACHE)))
     {
       struct frec_src *src;
-      struct dns_header *saved_header;
       unsigned int casediff = 0;
       unsigned int *bitvector = NULL;
+      unsigned short id = ntohs(header->id); /* Retrieve the id from the new query before we overwrite it. */
       
+      /* Get the case-scambled version of the query to resend. This is important because we
+	 may fall through below and forward the query in the packet buffer again and we
+	 want to use the same case scrambling as the first time. */
+      blockdata_retrieve(forward->stash, forward->stash_len, (void *)header); 
+      plen = forward->stash_len;
+
       for (src = &forward->frec_src; src; src = src->next)
-	if (src->orig_id == ntohs(header->id) && 
+	if (src->orig_id == id && 
 	    sockaddr_isequal(&src->source, udpaddr))
 	  break;
       
@@ -242,7 +248,7 @@ static void forward_query(int udpfd, union mysockaddr *udpaddr,
 	      goto reply;
 	    }
 
-	  /* Find a bitmap of case differences between the query send upstream and this one,
+	  /* Find a bitmap of case differences between the query sent upstream and this one,
 	     so we can reply to each query with the correct case pattern.
 	     Since we need this to get back the exact case pattern of each query when doing
 	     query combining, we have to handle the (rare) case that there are case differences
@@ -253,9 +259,12 @@ static void forward_query(int udpfd, union mysockaddr *udpaddr,
 	     we only scramble the first 32 letters for security reasons.
 
 	     Note the two names are guaranteed to be the same length and differ only in the case
-	     of letters. */
-	  if ((saved_header = blockdata_retrieve(forward->stash, forward->stash_len, NULL)) &&
-	      extract_request(saved_header, forward->stash_len, daemon->workspacename, NULL))
+	     of letters at this point.
+
+	     The original query we sent is now in packet buffer and the query name in the
+	     new instance is on daemon->namebuff. */
+	    	  
+	  if (extract_request(header, forward->stash_len, daemon->workspacename, NULL))
 	    {
 	      unsigned int i, gobig = 0;
 	      char *s1, *s2;
@@ -284,8 +293,9 @@ static void forward_query(int udpfd, union mysockaddr *udpaddr,
 
 	      if (gobig && !bitvector)
 		{
-		  casediff = (i/BITS_IN_INT) + 1; /* length of array */
-		  if ((bitvector = whine_malloc(casediff)))
+		  casediff = ((i - 1)/BITS_IN_INT) + 1; /* length of array */
+		  /* whine_malloc() zeros memory */
+		  if ((bitvector = whine_malloc(casediff * sizeof(unsigned int))))
 		    goto big_redo;
 		}
 	    }
@@ -294,7 +304,7 @@ static void forward_query(int udpfd, union mysockaddr *udpaddr,
 	  daemon->free_frec_src = src->next;
 	  src->next = forward->frec_src.next;
 	  forward->frec_src.next = src;
-	  src->orig_id = ntohs(header->id);
+	  src->orig_id = id;
 	  src->source = *udpaddr;
 	  src->dest = *dst_addr;
 	  src->log_id = daemon->log_id;
@@ -344,8 +354,6 @@ static void forward_query(int udpfd, union mysockaddr *udpaddr,
 	  flags = 0;
 	}
 
-      master = daemon->serverarray[first];
-
       /* don't forward A or AAAA queries for simple names, except the empty name */
       if (!flags &&
 	  option_bool(OPT_NODOTS_LOCAL) &&
@@ -357,7 +365,9 @@ static void forward_query(int udpfd, union mysockaddr *udpaddr,
       /* Configured answer. */
       if (flags || ede == EDE_NOT_READY)
 	goto reply;
-      
+
+      master = daemon->serverarray[first];
+
       if (!(forward = get_new_frec(now, master, 0)))
 	goto reply;
       /* table full - flags == 0, return REFUSED */
@@ -381,7 +391,8 @@ static void forward_query(int udpfd, union mysockaddr *udpaddr,
       forward->new_id = get_id();
       header->id = ntohs(forward->new_id);
       
-      forward->frec_src.encode_bitmap = option_bool(OPT_NO_0x20) ? 0 : rand32();
+      forward->frec_src.encode_bitmap = (!option_bool(OPT_NO_0x20) && option_bool(OPT_DO_0x20)) ? rand32() : 0;
+      forward->frec_src.encode_bigmap = NULL;
       p = (unsigned char *)(header+1);
       if (!extract_name(header, plen, &p, (char *)&forward->frec_src.encode_bitmap, EXTR_NAME_FLIP, 1))
 	goto reply;
@@ -1031,6 +1042,7 @@ static void dnssec_validate(struct frec *forward, struct dns_header *header,
 		  new->flags |= flags;
 		  new->forwardall = 0;
 		  new->frec_src.encode_bitmap = 0;
+		  new->frec_src.encode_bigmap = NULL;
 
 		  forward->next_dependent = NULL;
 		  new->dependent = forward; /* to find query awaiting new one. */
@@ -1481,13 +1493,13 @@ void return_reply(time_t now, struct frec *forward, struct dns_header *header, s
 	  log_query(F_UPSTREAM, NULL, NULL, "truncated", 0);
 
 	  /* This gets the name back to the state it was in when we started. */
-	  flip_queryname(header, nn, prev, &forward->frec_src);
+	  flip_queryname(header, new, prev, &forward->frec_src);
 	  
 	  for (src = &forward->frec_src, prev = NULL; src; prev = src, src = src->next)
 	    {
 	      /* If you didn't undertand this above, you won't understand it here either. */
 	      if (prev)
-		flip_queryname(header, nn, prev, src);
+		flip_queryname(header, new, prev, src);
 	      
 	      if (src->fd != -1 && nn > src->udp_pkt_size)
 		{
@@ -3033,7 +3045,7 @@ static void free_frec(struct frec *f)
   struct frec_src *last;
   
   /* add back to freelist if not the record builtin to every frec,
-     also free any bigmaps they's been decorated with. */
+     also free any bigmaps they've been decorated with. */
   for (last = f->frec_src.next; last && last->next; last = last->next)
     if (last->encode_bigmap)
       {
@@ -3043,6 +3055,12 @@ static void free_frec(struct frec *f)
   
   if (last)
     {
+      /* final link in the chain loses bigmap too. */
+      if (last->encode_bigmap)
+	{
+	  free(last->encode_bigmap);
+	  last->encode_bigmap = NULL;
+	}
       last->next = daemon->free_frec_src;
       daemon->free_frec_src = f->frec_src.next;
     }
@@ -3182,12 +3200,12 @@ static struct frec *lookup_frec(char *target, int class, int rrtype, int id, int
   struct dns_header *header;
   int compare_mode = EXTR_NAME_COMPARE;
 
-  /* Only compare case-sensitive when matching frec to a recieved answer,
+  /* Only compare case-sensitive when matching frec to a received answer,
      NOT when looking for a duplicated question. */
   if (flags & FREC_ANSWER)
     {
       flags &= ~FREC_ANSWER;
-      if (!option_bool(OPT_NO_0x20))
+      if (!option_bool(OPT_NO_0x20) && option_bool(OPT_DO_0x20))
 	compare_mode = EXTR_NAME_NOCASE;
     }
   

src/option.c

@@ -194,6 +194,7 @@ struct myoption {
 #define LOPT_DNSSEC_LIMITS 385
 #define LOPT_PXE_OPT       386
 #define LOPT_NO_ENCODE     387
+#define LOPT_DO_ENCODE     388
 
 #ifdef HAVE_GETOPT_LONG
 static const struct option opts[] =  
@@ -249,6 +250,7 @@ static const struct myoption opts[] =
     { "no-negcache", 0, 0, 'N' },
     { "no-round-robin", 0, 0, LOPT_NORR },
     { "no-0x20-encode", 0, 0, LOPT_NO_ENCODE },
+    { "do-0x20-encode", 0, 0, LOPT_DO_ENCODE },
     { "cache-rr", 1, 0, LOPT_CACHE_RR },
     { "addn-hosts", 1, 0, 'H' },
     { "hostsdir", 1, 0, LOPT_HOST_INOTIFY },
@@ -594,6 +596,7 @@ static struct {
   { LOPT_QUIET_TFTP, OPT_QUIET_TFTP, NULL, gettext_noop("Do not log routine TFTP."), NULL },
   { LOPT_NORR, OPT_NORR, NULL, gettext_noop("Suppress round-robin ordering of DNS records."), NULL },
   { LOPT_NO_ENCODE, OPT_NO_0x20, NULL, gettext_noop("Suppress DNS bit 0x20 encoding."), NULL },
+  { LOPT_DO_ENCODE, OPT_DO_0x20, NULL, gettext_noop("Enable DNS bit 0x20 encoding."), NULL },
   { LOPT_NO_IDENT, OPT_NO_IDENT, NULL, gettext_noop("Do not add CHAOS TXT records."), NULL },
   { LOPT_CACHE_RR, ARG_DUP, "<RR-type>", gettext_noop("Cache this DNS resource record type."), NULL },
   { LOPT_MAX_PROCS, ARG_ONE, "<integer>", gettext_noop("Maximum number of concurrent tcp connections."), NULL },

src/rfc1035.c

@@ -35,9 +35,9 @@ int extract_name(struct dns_header *header, size_t plen, unsigned char **pp,
 {
   unsigned char *cp = (unsigned char *)name, *p = *pp, *p1 = NULL;
   unsigned int j, l, namelen = 0, hops = 0;
-  unsigned int bigmap_counter = 0, bigmap_posn = 0, bigmap_size, bitmap;
+  unsigned int bigmap_counter = 0, bigmap_posn = 0, bigmap_size = parm, bitmap = 0;
   int retvalue = 1, case_insens = 1, isExtract = 0, flip = 0, extrabytes = (int)parm;
-  unsigned int *bigmap;
+  unsigned int *bigmap = (unsigned int *)name;
 
   if (func == EXTR_NAME_EXTRACT)
     isExtract = 1, *cp = 0;
@@ -46,9 +46,7 @@ int extract_name(struct dns_header *header, size_t plen, unsigned char **pp,
   else if (func == EXTR_NAME_FLIP)
     {
       flip = 1, extrabytes = 0;
-      bigmap = (unsigned int *)name;
       name = NULL;
-      bigmap_size = parm;
     }
   
   while (1)

src/tftp.c

@@ -360,7 +360,7 @@ void tftp_request(struct listener *listen, time_t now)
     }
   
   p = packet + 2;
-  end = packet + 2 + len;
+  end = packet + len;
   
   if (ntohs(*((unsigned short *)packet)) != OP_RRQ ||
       !(filename = next(&p, end)) ||
@@ -742,15 +742,16 @@ static void free_transfer(struct tftp_transfer *transfer)
 
 static char *next(char **p, char *end)
 {
-  char *ret = *p;
-  size_t len;
+  char *n, *ret = *p;
+  
+  /* Look for end of string, without running off the end of the packet. */
+  for (n = *p; n < end && *n != 0; n++);
 
-  if (*(end-1) != 0 || 
-      *p == end ||
-      (len = strlen(ret)) == 0)
+  /* ran off the end or zero length string - failed */
+  if (n == end || n == ret)
     return NULL;
-
-  *p += len + 1;
+  
+  *p = n + 1;
   return ret;
 }