Fix issue with stale caching.

After replying with stale data, dnsmasq sends the query upstream to
refresh the cache asynchronously and sometimes sends the wrong packet:
packet length can be wrong, and if an EDE marking stale data is added
to the answer that can end up in the query also. This bug only seems
to cause problems when the usptream server is a DOH/DOT proxy. Thanks
to Justin He for the bug report.

2 files changed, 59 insertions, 51 deletions

diff --git a/CHANGELOG b/CHANGELOG
index c0694eb..2ce53a8 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -17,6 +17,14 @@ version 2.90
 
 	Add --no-dhcpv4-interface and --no-dhcpv6-interface for
 	better control over which inetrfaces are providing DHCP service.
+
+	Fix issue with stale caching: After replying with stale data,
+	dnsmasq sends the query upstream to refresh the cache asynchronously
+	and sometimes sends the wrong packet: packet length can be wrong,
+	and if an EDE marking stale data is added to the answer that can
+	end up in the query also. This bug only seems to cause problems
+	when the usptream server is a DOH/DOT proxy. Thanks to Justin He
+	for the bug report.
 	
 	
 version 2.89
diff --git a/src/forward.c b/src/forward.c
index 10b68f8..ecfeebd 100644
--- a/src/forward.c
+++ b/src/forward.c
@@ -1803,13 +1803,17 @@ void receive_query(struct listener *listen, time_t now)
     {
       int stale, filtered;
       int ad_reqd = do_bit;
-      u16 hb3 = header->hb3, hb4 = header->hb4;
       int fd = listen->fd;
+      struct blockdata *saved_question = NULL;
+      
+      /* In case answer is stale */
+      if (daemon->cache_max_expiry != 0)
+	saved_question = blockdata_alloc((char *) header, (size_t)n);
       
       /* RFC 6840 5.7 */
       if (header->hb4 & HB4_AD)
 	ad_reqd = 1;
-      
+
       m = answer_request(header, ((char *) header) + udp_size, (size_t)n, 
 			 dst_addr_4, netmask, now, ad_reqd, do_bit, have_pseudoheader, &stale, &filtered);
       
@@ -1818,7 +1822,7 @@ void receive_query(struct listener *listen, time_t now)
 	  if (have_pseudoheader)
 	    {
 	      int ede = EDE_UNSET;
-	      
+
 	      if (filtered)
 		ede = EDE_FILTERED;
 	      else if (stale)
@@ -1847,29 +1851,22 @@ void receive_query(struct listener *listen, time_t now)
 	    daemon->metrics[METRIC_DNS_STALE_ANSWERED]++;
 	}
       
-      if (m == 0 || stale)
+      if (stale && saved_question)
 	{
-	  if (m != 0)
-	    {
-	      size_t plen;
-	      
-	      /* We answered with stale cache data, so forward the query anyway to
-		 refresh that. Restore the query from the answer packet. */
-	      pheader = find_pseudoheader(header, (size_t)m, &plen, NULL, NULL, NULL);
-	      
-	      header->hb3 = hb3;
-	      header->hb4 = hb4;
-	      header->ancount = htons(0);
-	      header->nscount = htons(0);
-	      header->arcount = htons(0);
-
-	      m = resize_packet(header, m, pheader, plen);
-
-	      /* We've already answered the client, so don't send it the answer 
-		 when it comes back. */
-	      fd = -1;
-	    }
+	  /* We answered with stale cache data, so forward the query anyway to
+	     refresh that. Restore saved query. */
+	  blockdata_retrieve(saved_question, (size_t)n, header);
+	  m = 0;
 	  
+	  /* We've already answered the client, so don't send it the answer 
+	     when it comes back. */
+	  fd = -1;
+	}
+      
+      blockdata_free(saved_question);
+
+      if (m == 0)
+	{
 	  if (forward_query(fd, &source_addr, &dst_addr, if_index,
 			    header, (size_t)n,  ((char *) header) + udp_size, now, NULL, ad_reqd, do_bit, 0))
 	    daemon->metrics[METRIC_DNS_QUERIES_FORWARDED]++;
@@ -2074,7 +2071,7 @@ static int tcp_key_recurse(time_t now, int status, struct dns_header *header, si
 unsigned char *tcp_request(int confd, time_t now,
 			   union mysockaddr *local_addr, struct in_addr netmask, int auth_dns)
 {
-  size_t size = 0;
+  size_t size = 0, saved_size = 0;
   int norebind;
 #ifdef HAVE_CONNTRACK
   int is_single_query = 0, allowed = 1;
@@ -2085,6 +2082,7 @@ unsigned char *tcp_request(int confd, time_t now,
   int checking_disabled, do_bit, added_pheader = 0, have_pseudoheader = 0;
   int cacheable, no_cache_dnssec = 0, cache_secure = 0, bogusanswer = 0;
   size_t m;
+  struct blockdata *saved_question = NULL;
   unsigned short qtype;
   unsigned int gotname;
   /* Max TCP packet + slop + size */
@@ -2104,7 +2102,6 @@ unsigned char *tcp_request(int confd, time_t now,
   int have_mark = 0;
   int first, last, filtered, stale, do_stale = 0;
   unsigned int flags = 0;
-  u16 hb3, hb4;
     
   if (!packet || getpeername(confd, (struct sockaddr *)&peer_addr, &peer_len) == -1)
     return packet;
@@ -2159,35 +2156,25 @@ unsigned char *tcp_request(int confd, time_t now,
     {
       int ede = EDE_UNSET;
 
-      if (query_count == TCP_MAX_QUERIES)
-	return packet;
-
       if (do_stale)
 	{
-	  size_t plen;
-
 	  /* We answered the last query with stale data. Now try and get fresh data.
-	     Restore query from answer. */
-	  pheader = find_pseudoheader(header, m, &plen, NULL, NULL, NULL);
-	  
-	  header->hb3 = hb3;
-	  header->hb4 = hb4;
-	  header->ancount = htons(0);
-	  header->nscount = htons(0);
-	  header->arcount = htons(0);
-	  
-	  size = resize_packet(header, m, pheader, plen);
+	     Restore saved query */
+	  if (!saved_question)
+	    break;
+
+	  blockdata_retrieve(saved_question, (size_t)saved_size, header);
+	  size = saved_size;
 	}
       else
 	{
+	  if (query_count == TCP_MAX_QUERIES)
+	    return packet;
+
 	  if (!read_write(confd, &c1, 1, 1) || !read_write(confd, &c2, 1, 1) ||
 	      !(size = c1 << 8 | c2) ||
 	      !read_write(confd, payload, size, 1))
 	    return packet;
-	  
-	  /* for stale-answer processing. */
-	  hb3 = header->hb3;
-	  hb4 = header->hb4;
 	}
       
       if (size < (int)sizeof(struct dns_header))
@@ -2294,10 +2281,20 @@ unsigned char *tcp_request(int confd, time_t now,
 	   if (do_stale)
 	     m = 0;
 	   else
-	     /* m > 0 if answered from cache */
-	     m = answer_request(header, ((char *) header) + 65536, (size_t)size, 
-				dst_addr_4, netmask, now, ad_reqd, do_bit, have_pseudoheader, &stale, &filtered);
-	   
+	     {
+	       if (daemon->cache_max_expiry != 0)
+		 {
+		   if (saved_question)
+		     blockdata_free(saved_question);
+		   
+		   saved_question = blockdata_alloc((char *) header, (size_t)size);
+		   saved_size = size;
+		 }
+	       
+	       /* m > 0 if answered from cache */
+	       m = answer_request(header, ((char *) header) + 65536, (size_t)size, 
+				  dst_addr_4, netmask, now, ad_reqd, do_bit, have_pseudoheader, &stale, &filtered);
+	     }
 	  /* Do this by steam now we're not in the select() loop */
 	  check_log_writer(1); 
 	  
@@ -2435,10 +2432,10 @@ unsigned char *tcp_request(int confd, time_t now,
 		m = add_pseudoheader(header, m, ((unsigned char *) header) + 65536, daemon->edns_pktsz, 0, NULL, 0, do_bit, 0);
 	    }
 	}
-      else
+      else if (have_pseudoheader)
 	{
 	  ede = EDE_UNSET;
-	      
+	  
 	  if (filtered)
 	    ede = EDE_FILTERED;
 	  else if (stale)
@@ -2485,6 +2482,9 @@ unsigned char *tcp_request(int confd, time_t now,
       close(confd);
     }
 
+  if (saved_question)
+    blockdata_free(saved_question);
+  
   return packet;
 }