Merge ~lvoytek/ubuntu/+source/dnsmasq:fix-duplicate-query-failure-kinetic into ubuntu/+source/dnsmasq:ubuntu/devel

[Lena Voytek]

PPA: https://launchpad.net/~lvoytek/+archive/ubuntu/dnsmasq-fix-denied-dns-retries

Testing:

# lxc launch images:ubuntu/jammy test-dnsmasq
# lxc exec test-dnsmasq bash

# apt update && apt dist-upgrade -y
# systemctl disable systemd-resolved
# systemctl stop systemd-resolved
# unlink /etc/resolv.conf
# echo nameserver 8.8.8.8 | tee /etc/resolv.conf
# apt install dnsutils dnsmasq -y
# systemctl enable dnsmasq

Access internet through dnsmasq, flaky internet can be modeled by disconnecting from router, etc. This will lead to denial of DNS query retries.

# lxc launch images:ubuntu/jammy test-dnsmasq-fixed
# lxc exec test-dnsmasq-fixed bash

# apt update && apt dist-upgrade -y
# apt install software-properties-common -y
# add-apt-repository ppa:lvoytek/dnsmasq-fix-denied-dns-retries
# systemctl disable systemd-resolved
# systemctl stop systemd-resolved
# unlink /etc/resolv.conf
# echo nameserver 8.8.8.8 | tee /etc/resolv.conf
# apt install dnsutils dnsmasq -y
# systemctl enable dnsmasq

DNS query retries can now succeed

[Utkarsh Gupta (utkarsh)]

Hello,

Thanks for working on this. Since this is d/source/format=1.0 (which means no use of d/patches/), can you please mention in d/ch or/and in the commit message that it's a cherry pick of https://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=commit;h=2561f9fe0eb9c0be1df48da1e2bd3d3feaa138c2 and the upstream version release it's a part of? This would help in case something goes wrong and we need to debug and in general, when the next person who works on this?

TIA.

review: Needs Fixing

[Utkarsh Gupta (utkarsh)]

Just btw, I realllyyyyy like how to document the testing plan and the tests performed. This makes it sooooo easy. I just love it. Thank you!

8c5fe05... by Lena Voytek on 2022-10-07

changelog

[Lena Voytek (lvoytek)]

Thanks for the review and compliment! :) Added more detail to the changelog and commit message

[Utkarsh Gupta (utkarsh)]

Super! Thank you! Sponsored the upload:

$ dput ubuntu ../dnsmasq_2.86-1.1ubuntu2_source.changes
Checking signature on .changes
gpg: ../dnsmasq_2.86-1.1ubuntu2_source.changes: Valid signature from 823E967606C34B96
Checking signature on .dsc
gpg: ../dnsmasq_2.86-1.1ubuntu2.dsc: Valid signature from 823E967606C34B96
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading dnsmasq_2.86-1.1ubuntu2.dsc: done.
  Uploading dnsmasq_2.86-1.1ubuntu2.tar.gz: done.
  Uploading dnsmasq_2.86-1.1ubuntu2_source.buildinfo: done.
  Uploading dnsmasq_2.86-1.1ubuntu2_source.changes: done.
Successfully uploaded packages.

review: Approve

[git-ubuntu bot (git-ubuntu-bot)]

Approvers: utkarsh, lvoytek
Uploaders: utkarsh
MP auto-approved

review: Approve

diff --git a/debian/changelog b/debian/changelog
index 209e4e4..1475a56 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,15 @@
+dnsmasq (2.86-1.1ubuntu2) kinetic; urgency=medium
+
+  * src/forward.c: Do not refuse retries from client DNS queries. Behaviour to
+    stop infinite loops when all servers return REFUSED was wrongly activated
+    on client retries, resulting in incorrect REFUSED replies to client
+    retries. The code added here is a cherry pick released in upstream version
+    2.87, originating at
+    https://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=commit;h=2561f9fe0eb9c0be
+    (LP: #1981794)
+
+ -- Lena Voytek <[e-mail]>  Fri, 30 Sep 2022 08:42:39 -0700
+
 dnsmasq (2.86-1.1ubuntu1) kinetic; urgency=medium
 
   * SECURITY UPDATE: Heap use after free
diff --git a/src/forward.c b/src/forward.c
index 3d638e4..dc9f245 100644
--- a/src/forward.c
+++ b/src/forward.c
@@ -172,7 +172,7 @@ static int forward_query(int udpfd, union mysockaddr *udpaddr,
   unsigned int gotname = extract_request(header, plen, daemon->namebuff, NULL);
   void *hash = hash_questions(header, plen, daemon->namebuff);
   unsigned char *oph = find_pseudoheader(header, plen, NULL, NULL, NULL, NULL);
-  int old_src = 0;
+  int old_src = 0, old_reply = 0;
   int first, last, start = 0;
   int subnet, cacheable, forwarded = 0;
   size_t edns0_len;
@@ -198,7 +198,10 @@ static int forward_query(int udpfd, union mysockaddr *udpaddr,
      Similarly FREC_NO_CACHE is never set in flags, so a query which is
      contigent on a particular source address EDNS0 option will never be matched. */
   if (forward)
-    old_src = 1;
+    {
+      old_src = 1;
+      old_reply = 1;
+    }
   else if ((forward = lookup_frec_by_query(hash, fwd_flags,
 					   FREC_CHECKING_DISABLED | FREC_AD_QUESTION | FREC_DO_QUESTION |
 					   FREC_HAS_PHEADER | FREC_DNSKEY_QUERY | FREC_DS_QUERY | FREC_NO_CACHE)))
@@ -375,9 +378,18 @@ static int forward_query(int udpfd, union mysockaddr *udpaddr,
 		  /* In strict order mode, there must be a server later in the list
 		     left to send to, otherwise without the forwardall mechanism,
 		     code further on will cycle around the list forwever if they
-		     all return REFUSED. If at the last, give up. */
+		     all return REFUSED. If at the last, give up.
+		     Note that we can get here EITHER because a client retried,
+		     or an upstream server returned REFUSED. The above only
+		     applied in the later case. For client retries,
+		     keep tyring the last server.. */
 		  if (++start == last)
-		    goto reply;
+            {
+              if (old_reply)
+            goto reply;
+              else
+            start--;
+            }
 		}
 	    }	  
 	}