[Dnsmasq-discuss] [PATCH] --dont-mirror-queries option - updated for 2.77

Chris Novakovic chris at chrisn.me.uk
Wed Jun 7 23:46:33 BST 2017

Here's the "don't mirror queries" patch from last year updated for 2.77,
partially as a courtesy to anyone who was using it and partially to
stimulate discussion about including functionality like this in a future
version of dnsmasq.

To quickly recap:

- The patch prevents dnsmasq from forwarding an incoming DNS query to an
upstream server if its IP address matches the IP address from which the
query originated. This makes it possible for two dnsmasq DNS servers to
be mutually dependent on each other, without the risk of inducing
forwarding loops.

- There seemed to be agreement that this would also be a useful thing to
have outside of my mad network setup.

- Simon expressed the possibility of genericising the patch by including
a trace of dnsmasq instances that a query had been processed by in an
EDNS0 record, which could then be checked to prevent all forwarding
loops, not just those of length 2.

- Concerns raised about Simon's idea included the possibility of
firewalls interfering with the larger DNS packets this would create, and
the privacy implications of the details of internal DNS setups leaking
out over public networks.

My patch is certainly a hack, and I'd like to see something like this
genericised in the way Simon described. Here's a proposal that ought to
keep everyone happy:

- Every dnsmasq instance stamps the EDNS0 record with some identifier (I
don't know dnsmasq well enough to know what the source of this could be,
sorry) for each request it forwards to upstream servers.

- If an incoming request's EDNS0 record contains the dnsmasq instance's
identifier, dnsmasq replies with REFUSED.

- This behaviour becomes the default. A new option --no-dns-loop-detect
avoids stamping the identifier into any forwarded queries, for people
who don't want this behaviour (an optional list of upstream servers can
be given as an argument to --no-dns-loop-detect to specify that queries
should be stamped except for those forwarded to the listed servers).
Another option --only-dns-loop-detect only stamps the identifier into
queries forwarded to the specified servers. Both options are allowed to
be specified in --servers-file files, so the list of upstream servers
affected by this can be changed as the upstream servers change.

The extra options would be useful for people who don't want information
about their internal DNS infrastructure to leak publicly, or know that
upstream servers are protected by firewalls that may drop large DNS
query packets with EDNS0 records.

I'm afraid I don't know anywhere near enough about dnsmasq to implement
either Simon's original idea or my proposed refinement, but hopefully
someone does...

On 04/03/2016 21:35, Simon Kelley wrote:
> On 01/03/16 21:23, Kurt H Maier wrote:
>> On Tue, Mar 01, 2016 at 06:50:14PM +0000, Simon Kelley wrote:
>>> On 24/02/16 23:38, Kurt H Maier wrote:
>>> This approach assumes that all the servers are dnsmasq, and running the
>>> loop-detection code, which is a reasonable assumption. Once a query
>>> escapes from the "cloud" of interconnected dnsmasq servers towards an
>>> upstream server, the EDNS0 options are no longer required and can be
>>> stripped without problem. (They will be stripped from replies.)
>> Part of the concern here was that in some of these deployments we have  
>> 'enclaves' of devices with dnsmasq on the edge nodes.  I'm concerned    
>> about the interaction on those edges, because EDNS0 data suddenly
>> disappearing has caused problems for me in the past.  I'm also concerned
>> about whether we'll have to re-architect our DNS infrastructure to avoid
>> EDNS0 data growing too large. Do you have draft code for this solution 
>> anywhere?
>> Thanks,
>> khm
> No draft code yet. No version of dnsmasq has ever removed EDNS0 from
> queries, and note that queries are all we're concerned about here. The
> EDNS0 options should not be included in replies. Packet size of queries
> is not generally a problem.
> Cheers,
> Simon.

-------------- next part --------------
From f17894815ee3fe0828bec1184bbbb33c56e3014f Mon Sep 17 00:00:00 2001
From: Chris Novakovic <chris at chrisn.me.uk>
Date: Wed, 7 Jun 2017 23:39:06 +0100
Subject: [PATCH] Added --dont-mirror-queries option

This commit adds the --dont-mirror-queries option. When enabled, this
option prevents dnsmasq from forwarding an incoming DNS query to an
upstream server if its IP address matches the IP address from which the
query originated. This makes it possible for two dnsmasq DNS servers to
be mutually dependent on each other, without the risk of inducing
forwarding loops.
 man/dnsmasq.8 |  6 ++++++
 src/dnsmasq.h |  3 ++-
 src/forward.c | 27 ++++++++++++++++++++++++++-
 src/option.c  |  3 +++
 4 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/man/dnsmasq.8 b/man/dnsmasq.8
index 7d1dea1..d18f3a0 100644
--- a/man/dnsmasq.8
+++ b/man/dnsmasq.8
@@ -388,6 +388,12 @@ the upstream server through which it was sent is disabled and this event is logg
 set of upstream servers changes, the test is re-run on all of them, including ones which
 were previously disabled.
+.B --dont-mirror-queries
+Don't forward DNS queries to upstream DNS servers whose IP address
+matches that of the original sender of the query. This option makes
+it possible for two dnsmasq DNS servers to be mutually dependent on
+each other, without the risk of inducing forwarding loops.
 .B --stop-dns-rebind
 Reject (and log) addresses from upstream nameservers which are in the
 private IP ranges. This blocks an attack where a browser behind a
diff --git a/src/dnsmasq.h b/src/dnsmasq.h
index 06fae35..8df3a1c 100644
--- a/src/dnsmasq.h
+++ b/src/dnsmasq.h
@@ -239,7 +239,8 @@ struct event_desc {
 #define OPT_MAC_B64        54
 #define OPT_MAC_HEX        55
 #define OPT_TFTP_APREF_MAC 56
-#define OPT_LAST           57
+#define OPT_DONT_MIRROR    57
+#define OPT_LAST           58
 /* extra flags for my_syslog, we use a couple of facilities since they are known 
    not to occupy the same bits as priorities, no matter how syslog.h is set up. */
diff --git a/src/forward.c b/src/forward.c
index 83f392d..1306bde 100644
--- a/src/forward.c
+++ b/src/forward.c
@@ -443,7 +443,32 @@ static int forward_query(int udpfd, union mysockaddr *udpaddr,
 	     domain may be NULL, in which case server->domain 
 	     must be NULL also. */
-	  if (type == (start->flags & SERV_TYPE) &&
+      /* If --dont-mirror-queries is enabled, don't forward the query to this upstream
+       * server if it has the same IP address as the sender of the original query */
+      int is_mirrored_query = 0;
+      if (option_bool(OPT_DONT_MIRROR))
+        {
+          char sender_ip[ADDRSTRLEN], upstream_ip[ADDRSTRLEN];
+          if (udpaddr->sa.sa_family == AF_INET)
+            inet_ntop(AF_INET, &udpaddr->in.sin_addr, sender_ip, ADDRSTRLEN);
+#ifdef HAVE_IPV6
+          else
+            inet_ntop(AF_INET6, &udpaddr->in6.sin6_addr, sender_ip, ADDRSTRLEN);
+          if (start->addr.sa.sa_family == AF_INET)
+            inet_ntop(AF_INET, &start->addr.in.sin_addr, upstream_ip, ADDRSTRLEN);
+#ifdef HAVE_IPV6
+          else
+            inet_ntop(AF_INET6, &start->addr.in6.sin6_addr, upstream_ip, ADDRSTRLEN);
+          is_mirrored_query = (strcmp(sender_ip, upstream_ip) == 0);
+        }
+      if (is_mirrored_query == 0 &&
+          type == (start->flags & SERV_TYPE) &&
 	      (type != SERV_HAS_DOMAIN || hostname_isequal(domain, start->domain)) &&
 	      !(start->flags & (SERV_LITERAL_ADDRESS | SERV_LOOP)))
diff --git a/src/option.c b/src/option.c
index 064ef62..a6c9d0c 100644
--- a/src/option.c
+++ b/src/option.c
@@ -160,6 +160,7 @@ struct myoption {
 #define LOPT_DHCPTTL       348
 #define LOPT_TFTP_MTU      349
 #define LOPT_REPLY_DELAY   350
+#define LOPT_DONT_MIRROR   351
 static const struct option opts[] =  
@@ -325,6 +326,7 @@ static const struct myoption opts[] =
     { "script-arp", 0, 0, LOPT_SCRIPT_ARP },
     { "dhcp-ttl", 1, 0 , LOPT_DHCPTTL },
     { "dhcp-reply-delay", 1, 0, LOPT_REPLY_DELAY },
+    { "dont-mirror-queries", 0, 0, LOPT_DONT_MIRROR },
     { NULL, 0, 0, 0 }
@@ -497,6 +499,7 @@ static struct {
   { LOPT_IGNORE_ADDR, ARG_DUP, "<ipaddr>", gettext_noop("Ignore DNS responses containing ipaddr."), NULL }, 
   { LOPT_DHCPTTL, ARG_ONE, "<ttl>", gettext_noop("Set TTL in DNS responses with DHCP-derived addresses."), NULL }, 
   { LOPT_REPLY_DELAY, ARG_ONE, "<integer>", gettext_noop("Delay DHCP replies for at least number of seconds."), NULL },
+  { LOPT_DONT_MIRROR, OPT_DONT_MIRROR, NULL, gettext_noop("Don't forward DNS queries to requestor."), NULL },
   { 0, 0, NULL, NULL, NULL }

More information about the Dnsmasq-discuss mailing list