Handle queries with EDNS client subnet fields better.

If dnsmasq is configured to add an EDNS client subnet to a query,
it is careful to suppress use of the cache, since a cached answer may
not be valid for a query with a different client subnet.
Extend this behaviour to queries which arrive a dnsmasq
already carrying an EDNS client subnet.

This change is rather more involved than may seem necessary at first sight,
since the existing code relies on all queries being decorated by dnsmasq
and therefore not cached, so there is no chance that an incoming query
might hit the cache and cache lookup don't need to be suppressed, just
cache insertion. When downstream queries may be a mix of client-subnet
bearing and plain vanilla, it can't be assumed that the answers are never
in the cache, and queries with subnets must not do lookups.

CHANGELOG

@@ -75,7 +75,14 @@ version 2.91
 	Fix erroneous "DNSSEC validated" state with non-DNSSEC
 	upstream servers.  Thanks to Dominik Derigs for the bug report.
 
+	Handle queries with EDNS client subnet fields better. If dnsmasq
+	is configured to add an EDNS client subnet to a query, it is careful
+	to suppress use of the cache, since a cached answer may not be valid
+	for a query with a different client subnet. Extend this behaviour
+	to queries which arrive a dnsmasq already carrying an EDNS client
+	subnet.
 
+	
 version 2.90
 	Fix reversion in --rev-server introduced in 2.88 which
 	caused breakage if the prefix length is not exactly divisible

src/dnsmasq.h

@@ -772,9 +772,8 @@ struct dyndir {
 #define FREC_DS_QUERY          16
 #define FREC_AD_QUESTION       32
 #define FREC_DO_QUESTION       64
-#define FREC_ADDED_PHEADER    128
+#define FREC_HAS_PHEADER      128
-#define FREC_HAS_PHEADER      256
+#define FREC_GONE_TO_TCP      256
-#define FREC_GONE_TO_TCP      512
 
 struct frec {
   struct frec_src {
@@ -1402,7 +1401,7 @@ void report_addresses(struct dns_header *header, size_t len, u32 mark);
 #endif
 size_t answer_request(struct dns_header *header, char *limit, size_t qlen,  
 		      struct in_addr local_addr, struct in_addr local_netmask, 
-		      time_t now, int ad_reqd, int do_bit, int *stale, int *filtered);
+		      time_t now, int ad_reqd, int do_bit, int no_cache, int *stale, int *filtered);
 int check_for_bogus_wildcard(struct dns_header *header, size_t qlen, char *name, 
 			     time_t now);
 int check_for_ignored_address(struct dns_header *header, size_t qlen);

src/edns0.c

@@ -417,21 +417,34 @@ static size_t add_source_addr(struct dns_header *header, size_t plen, unsigned c
   else if (option_bool(OPT_STRIP_ECS))
     replace = 2;
   else
-    return plen;
+    {
-
+      unsigned char *pheader;
+      /* If we still think the data is cacheable, and we're not
+	 messing with EDNS client subnet ourselves, see if the client
+	 sent a client subnet. If so, mark the data as uncacheable */
+      if (*cacheable &&
+	  (pheader = find_pseudoheader(header, plen, NULL, NULL, NULL, NULL)) &&
+	  !check_source(header, plen, pheader, NULL))
+	*cacheable = 0;
+      
+      return plen;
+    }
+  
   return add_pseudoheader(header, plen, (unsigned char *)limit, EDNS0_OPTION_CLIENT_SUBNET, (unsigned char *)&opt, len, 0, replace);
 }
 
 int check_source(struct dns_header *header, size_t plen, unsigned char *pseudoheader, union mysockaddr *peer)
 {
-  /* Section 9.2, Check that subnet option in reply matches. */
+  /* Section 9.2, Check that subnet option (if any) in reply matches.
+     if peer == NULL, this degrades to a check for the existence of and EDNS0 client-subnet option. */
   
   int len, calc_len;
   struct subnet_opt opt;
   unsigned char *p;
   int code, i, rdlen;
   
-  calc_len = calc_subnet_opt(&opt, peer, NULL);
+  if (peer)
+    calc_len = calc_subnet_opt(&opt, peer, NULL);
    
   if (!(p = skip_name(pseudoheader, header, plen, 10)))
     return 1;
@@ -443,21 +456,26 @@ int check_source(struct dns_header *header, size_t plen, unsigned char *pseudohe
     return 1; /* bad packet */
   
   /* check if option there */
-   for (i = 0; i + 4 < rdlen; i += len + 4)
+  for (i = 0; i + 4 < rdlen; i += len + 4)
      {
        GETSHORT(code, p);
        GETSHORT(len, p);
        if (code == EDNS0_OPTION_CLIENT_SUBNET)
 	 {
-	   /* make sure this doesn't mismatch. */
+	   if (peer)
-	   opt.scope_netmask = p[3];
+	     {
-	   if (len != calc_len || memcmp(p, &opt, len) != 0)
+	       /* make sure this doesn't mismatch. */
+	       opt.scope_netmask = p[3];
+	       if (len != calc_len || memcmp(p, &opt, len) != 0)
+		 return 0;
+	     }
+	   else if (((struct subnet_opt *)p)->source_netmask != 0)
 	     return 0;
 	 }
        p += len;
      }
-   
+  
-   return 1;
+  return 1;
 }
 
 /* See https://docs.umbrella.com/umbrella-api/docs/identifying-dns-traffic for
@@ -541,6 +559,6 @@ size_t add_edns0_config(struct dns_header *header, size_t plen, unsigned char *l
     plen = add_umbrella_opt(header, plen, limit, source, cacheable);
   
   plen = add_source_addr(header, plen, limit, source, cacheable);
-  	  
+
   return plen;
 }

src/forward.c

@@ -166,34 +166,19 @@ static int domain_no_rebind(char *domain)
 static int forward_query(int udpfd, union mysockaddr *udpaddr,
 			 union all_addr *dst_addr, unsigned int dst_iface,
 			 struct dns_header *header, size_t plen,  size_t replylimit, time_t now, 
-			 struct frec *forward, int ad_reqd, int do_bit, int fast_retry)
+			 struct frec *forward, unsigned int fwd_flags, int fast_retry)
 {
   unsigned int flags = 0;
-  unsigned int fwd_flags = 0;
   int is_dnssec = forward && (forward->flags & (FREC_DNSKEY_QUERY | FREC_DS_QUERY));
   struct server *master;
   unsigned int gotname;
   int old_src = 0, old_reply = 0;
   int first, last, start = 0;
-  int cacheable, forwarded = 0;
+  int forwarded = 0;
-  unsigned char *oph;
   int ede = EDE_UNSET;
-  (void)do_bit;
   unsigned short rrtype;
 
   gotname = extract_request(header, plen, daemon->namebuff, &rrtype);
-  oph = find_pseudoheader(header, plen, NULL, NULL, NULL, NULL);
-
-  if (header->hb4 & HB4_CD)
-    fwd_flags |= FREC_CHECKING_DISABLED;
-  if (ad_reqd)
-    fwd_flags |= FREC_AD_QUESTION;
-  if (oph)
-    fwd_flags |= FREC_HAS_PHEADER;
-#ifdef HAVE_DNSSEC
-  if (do_bit)
-    fwd_flags |= FREC_DO_QUESTION;
-#endif
   
   /* Check for retry on existing query.
      FREC_DNSKEY and FREC_DS_QUERY are never set in flags, so the test below 
@@ -321,11 +306,6 @@ static int forward_query(int udpfd, union mysockaddr *udpaddr,
 
       forward->flags = fwd_flags;
 
-      plen = add_edns0_config(header, plen, ((unsigned char *)header) + daemon->edns_pktsz, &forward->frec_src.source, now, &cacheable);
-      
-      if (!cacheable)
-	forward->flags |= FREC_NO_CACHE;
-      
 #ifdef HAVE_DNSSEC
       if (option_bool(OPT_DNSSEC_VALID) && (master->flags & SERV_DO_DNSSEC))
 	{
@@ -338,10 +318,6 @@ static int forward_query(int udpfd, union mysockaddr *udpaddr,
 	}
 #endif
       
-      /* If there wasn't a PH before, and there is now, we added it. */
-      if (!oph && find_pseudoheader(header, plen, NULL, NULL, NULL, NULL))
-	forward->flags |= FREC_ADDED_PHEADER;
-	      
       /* Do these before saving query. */
       forward->frec_src.orig_id = ntohs(header->id);
       forward->new_id = get_id();
@@ -365,15 +341,9 @@ static int forward_query(int udpfd, union mysockaddr *udpaddr,
       forward->forwardall = 0;
       if (domain_no_rebind(daemon->namebuff))
 	forward->flags |= FREC_NOREBIND;
-      if (header->hb4 & HB4_CD)
-	forward->flags |= FREC_CHECKING_DISABLED;
-      if (ad_reqd)
-	forward->flags |= FREC_AD_QUESTION;
 #ifdef HAVE_DNSSEC
       forward->work_counter = daemon->limit[LIMIT_WORK];
       forward->validate_counter = daemon->limit[LIMIT_CRYPTO]; 
-      if (do_bit)
-	forward->flags |= FREC_DO_QUESTION;
 #endif
       
       start = first;
@@ -546,14 +516,14 @@ static int forward_query(int udpfd, union mysockaddr *udpaddr,
       if (!(plen = make_local_answer(flags, gotname, plen, header, daemon->namebuff, (char *)(header + replylimit), first, last, ede)))
 	return 0;
       
-      if (oph)
+      if (forward->flags & FREC_HAS_PHEADER)
 	{
 	  u16 swap = htons((u16)ede);
 
 	  if (ede != EDE_UNSET)
-	    plen = add_pseudoheader(header, plen, (unsigned char *)(header + replylimit), EDNS0_OPTION_EDE, (unsigned char *)&swap, 2, do_bit, 0);
+	    plen = add_pseudoheader(header, plen, (unsigned char *)(header + replylimit), EDNS0_OPTION_EDE, (unsigned char *)&swap, 2, 0, 0);
 	  else
-	    plen = add_pseudoheader(header, plen, (unsigned char *)(header + replylimit), 0, NULL, 0, do_bit, 0);
+	    plen = add_pseudoheader(header, plen, (unsigned char *)(header + replylimit), 0, NULL, 0, 0, 0);
 	}
       
 #if defined(HAVE_CONNTRACK) && defined(HAVE_UBUS)
@@ -611,8 +581,7 @@ int fast_retry(time_t now)
 		daemon->log_display_id = f->frec_src.log_id;
 		daemon->log_source_addr = NULL;
 		
-		forward_query(-1, NULL, NULL, 0, header, f->stash_len, 0, now, f,
+		forward_query(-1, NULL, NULL, 0, header, f->stash_len, 0, now, f, 0, 1);
-			      f->flags & FREC_AD_QUESTION, f->flags & FREC_DO_QUESTION, 1);
 		
 		to_run = f->forward_delay = 2 * f->forward_delay;
 	      }
@@ -1192,8 +1161,7 @@ void reply_query(int fd, time_t now)
       /* Get the saved query back. */
       blockdata_retrieve(forward->stash, forward->stash_len, (void *)header);
       
-      forward_query(-1, NULL, NULL, 0, header, forward->stash_len, 0, now, forward,
+      forward_query(-1, NULL, NULL, 0, header, forward->stash_len, 0, now, forward, 0, 0);
-		    forward->flags & FREC_AD_QUESTION, forward->flags & FREC_DO_QUESTION, 0);
       return;
     }
 
@@ -1315,7 +1283,7 @@ void return_reply(time_t now, struct frec *forward, struct dns_header *header, s
     header->hb4 |= HB4_CD;
   else
     header->hb4 &= ~HB4_CD;
-  
+
   /* Never cache answers which are contingent on the source or MAC address EDSN0 option,
      since the cache is ignorant of such things. */
   if (forward->flags & FREC_NO_CACHE)
@@ -1323,7 +1291,7 @@ void return_reply(time_t now, struct frec *forward, struct dns_header *header, s
   
   if ((nn = process_reply(header, now, forward->sentto, (size_t)n, check_rebind, no_cache_dnssec, cache_secure, bogusanswer, 
 			  forward->flags & FREC_AD_QUESTION, forward->flags & FREC_DO_QUESTION, 
-			  forward->flags & FREC_ADDED_PHEADER, &forward->frec_src.source,
+			  !(forward->flags & FREC_HAS_PHEADER), &forward->frec_src.source,
 			  ((unsigned char *)header) + daemon->edns_pktsz, ede)))
     {
       struct frec_src *src;
@@ -1454,9 +1422,10 @@ void receive_query(struct listener *listen, time_t now)
   struct in_addr netmask, dst_addr_4;
   size_t m;
   ssize_t n;
-  int if_index = 0, auth_dns = 0, do_bit = 0, have_pseudoheader = 0;
+  int if_index = 0, auth_dns = 0, do_bit = 0;
+  unsigned int fwd_flags = 0;
   int stale = 0, filtered = 0, ede = EDE_UNSET, do_forward = 0;
-  int metric, ad_reqd, fd; 
+  int metric, fd; 
   struct blockdata *saved_question = NULL;
 #ifdef HAVE_CONNTRACK
   unsigned int mark = 0;
@@ -1734,7 +1703,8 @@ void receive_query(struct listener *listen, time_t now)
     { 
       unsigned short flags;
       
-      have_pseudoheader = 1;
+      fwd_flags |= FREC_HAS_PHEADER;
+      
       GETSHORT(udp_size, pheader);
       pheader += 2; /* ext_rcode */
       GETSHORT(flags, pheader);
@@ -1752,10 +1722,15 @@ void receive_query(struct listener *listen, time_t now)
 	udp_size = PACKETSZ; /* Sanity check - can't reduce below default. RFC 6891 6.2.3 */
     }
 
-  ad_reqd = do_bit;
   /* RFC 6840 5.7 */
-  if (header->hb4 & HB4_AD)
+  if (do_bit || (header->hb4 & HB4_AD))
-    ad_reqd = 1;
+    fwd_flags |= FREC_AD_QUESTION;
+
+  if (do_bit)
+    fwd_flags |= FREC_DO_QUESTION;
+
+  if (header->hb4 & HB4_CD)
+    fwd_flags |= FREC_CHECKING_DISABLED;
 
   fd = listen->fd;
   
@@ -1790,10 +1765,16 @@ void receive_query(struct listener *listen, time_t now)
 #endif
   else
     {
+      int cacheable;
+
+      n = add_edns0_config(header, n, ((unsigned char *)header) + daemon->edns_pktsz, &source_addr, now, &cacheable);
       saved_question = blockdata_alloc((char *) header, (size_t)n);
-      
+
+      if (!cacheable)
+	fwd_flags |= FREC_NO_CACHE;
+
       m = answer_request(header, ((char *) header) + udp_size, (size_t)n, 
-			 dst_addr_4, netmask, now, ad_reqd, do_bit, &stale, &filtered);
+			 dst_addr_4, netmask, now, fwd_flags & FREC_AD_QUESTION, do_bit, !cacheable, &stale, &filtered);
       
       metric = stale ? METRIC_DNS_STALE_ANSWERED : METRIC_DNS_LOCAL_ANSWERED;
       
@@ -1814,7 +1795,7 @@ void receive_query(struct listener *listen, time_t now)
   
   if (m != 0)
     {
-      if (have_pseudoheader)
+      if (fwd_flags & FREC_HAS_PHEADER)
 	{
 	  if (ede != EDE_UNSET)
 	    {
@@ -1863,9 +1844,9 @@ void receive_query(struct listener *listen, time_t now)
       blockdata_retrieve(saved_question, (size_t)n, (void *)header);
       blockdata_free(saved_question);
       saved_question = NULL;
-      
+
       if (forward_query(fd, &source_addr, &dst_addr, if_index, header, (size_t)n,
-			udp_size, now, NULL, ad_reqd, do_bit, 0))
+			udp_size, now, NULL, fwd_flags, 0))
 	daemon->metrics[METRIC_DNS_QUERIES_FORWARDED]++;
       else
 	daemon->metrics[METRIC_DNS_LOCAL_ANSWERED]++;
@@ -2212,7 +2193,7 @@ unsigned char *tcp_request(int confd, time_t now,
 #ifdef HAVE_AUTH
   int local_auth = 0;
 #endif
-  int checking_disabled, do_bit = 0, ad_reqd = 0, added_pheader = 0, have_pseudoheader = 0;
+  int checking_disabled, do_bit = 0, ad_reqd = 0, have_pseudoheader = 0;
   struct blockdata *saved_question = NULL;
   unsigned short qtype;
   unsigned int gotname = 0;
@@ -2284,7 +2265,7 @@ unsigned char *tcp_request(int confd, time_t now,
 
   while (1)
     {
-      int stale = 0, ede = EDE_UNSET;
+      int cacheable = 1, stale = 0, ede = EDE_UNSET;
       size_t m = 0;
       unsigned int flags = 0;
 #ifdef HAVE_AUTH
@@ -2329,6 +2310,7 @@ unsigned char *tcp_request(int confd, time_t now,
 	      if (saved_question)
 		blockdata_free(saved_question);
 	      
+	      size = add_edns0_config(header, size, ((unsigned char *) header) + 65536, &peer_addr, now, &cacheable);
 	      saved_question = blockdata_alloc((char *)header, (size_t)size);
 	      saved_size = size;
 	      
@@ -2395,7 +2377,7 @@ unsigned char *tcp_request(int confd, time_t now,
 #endif
 	      else
 		m = answer_request(header, ((char *) header) + 65536, (size_t)size, 
-				   dst_addr_4, netmask, now, ad_reqd, do_bit, &stale, &filtered);
+				   dst_addr_4, netmask, now, ad_reqd, do_bit, !cacheable, &stale, &filtered);
 	    }
 	}
       
@@ -2406,7 +2388,7 @@ unsigned char *tcp_request(int confd, time_t now,
 	{
 	  struct server *master;
 	  int start;
-	  int cacheable, no_cache_dnssec = 0, cache_secure = 0, bogusanswer = 0;
+	  int no_cache_dnssec = 0, cache_secure = 0, bogusanswer = 0;
 
 	  blockdata_retrieve(saved_question, (size_t)saved_size, header);
 	  size = saved_size;
@@ -2437,8 +2419,6 @@ unsigned char *tcp_request(int confd, time_t now,
 		  else
 		    start = master->last_server;
 		  
-		  size = add_edns0_config(header, size, ((unsigned char *) header) + 65536, &peer_addr, now, &cacheable);
-		  
 #ifdef HAVE_DNSSEC
 		  if (option_bool(OPT_DNSSEC_VALID) && (master->flags & SERV_DO_DNSSEC))
 		    {
@@ -2451,11 +2431,6 @@ unsigned char *tcp_request(int confd, time_t now,
 		    }
 #endif
 		  
-		  /* Check if we added a pheader on forwarding - may need to
-		     strip it from the reply. */
-		  if (!have_pseudoheader && find_pseudoheader(header, size, NULL, NULL, NULL, NULL))
-		    added_pheader = 1;
-		  
 		  /* Loop round available servers until we succeed in connecting to one. */
 		  if ((m = tcp_talk(first, last, start, packet, size, have_mark, mark, &serv)) == 0)
 		    ede = EDE_NETERR;
@@ -2530,7 +2505,7 @@ unsigned char *tcp_request(int confd, time_t now,
 		      
 		      m = process_reply(header, now, serv, (unsigned int)m, 
 					option_bool(OPT_NO_REBIND) && !norebind, no_cache_dnssec, cache_secure, bogusanswer,
-					ad_reqd, do_bit, added_pheader, &peer_addr, ((unsigned char *)header) + 65536, ede);
+					ad_reqd, do_bit, !have_pseudoheader, &peer_addr, ((unsigned char *)header) + 65536, ede);
 
 		      /* process_reply() adds pheader itself */
 		      have_pseudoheader = 0; 

src/rfc1035.c

@@ -1567,7 +1567,7 @@ static int cache_validated(const struct crec *crecp)
 /* return zero if we can't answer from cache, or packet size if we can */
 size_t answer_request(struct dns_header *header, char *limit, size_t qlen,  
 		      struct in_addr local_addr, struct in_addr local_netmask, 
-		      time_t now, int ad_reqd, int do_bit, int *stale, int *filtered) 
+		      time_t now, int ad_reqd, int do_bit, int no_cache, int *stale, int *filtered) 
 {
   char *name = daemon->namebuff;
   unsigned char *p, *ansp;
@@ -1582,6 +1582,10 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
   size_t len;
   int rd_bit = (header->hb3 & HB3_RD);
   int count = 255; /* catch loops */
+
+  /* Suppress cached answers of no_cache set. */
+  if (no_cache)
+    rd_bit = 0;
   
   if (stale)
     *stale = 0;