On 27/08/17 08:18, Christian Kujau wrote:
OK, so I should have done this in the first place and used git bisect to
find out which commit in Dnsmasq introduced this behaviour:

  fa78573778cb23337f67f5d0c9de723169919047 is the first bad commit
  commit fa78573778cb23337f67f5d0c9de723169919047
  Author: Simon Kelley <si...@thekelleys.org.uk>
  Date:   Fri Jul 22 20:56:01 2016 +0100

     Zero packet buffers before building output, to reduce risk
     of information leakage.
<snip>

Hi Christian,

Thanks for all your investigation and info so far. I too can now crash dnsmasq at will :-) So putting my novice C and even more novice gdb to work I've come up with what I feel is a slightly less invasive mitigation to the problem....which in essence is 'we've been sent a query but not yet allocated any buffer to it/updated the header limit offset but we pass a non zero query length. The result is we try to clear the memory before our buffer.

My workaround is to only call memset if the difference between buffer begin and buffer limit is bigger than the query length, thus it retains Simon's intent of clearing memory most of the time but avoids the SIGSEGV trampling.

This is to be regarded as a sticking plaster rather than real fix but that needs far greater minds than I to understand the code & intent :-)

Hope this helps someone.

Kevin


>From 340a26f915d8c3bb54c44f58d432cc7240631a74 Mon Sep 17 00:00:00 2001
From: Kevin Darbyshire-Bryant <ke...@darbyshire-bryant.me.uk>
Date: Mon, 28 Aug 2017 14:52:10 +0100
Subject: [PATCH] dnsmasq: rfc1035: mitigate CVE-2017-13704

Work around a problem where answer_request() attempts to clear from the
end of a request to end of request buffer but the end of the buffer is
at the same place as the start.

Originally this meant that memset() tried to clear data before the
buffer leading to segmentation violation.  Instead only clear to end of
buffer it is bigger than the request length.

Signed-off-by: Kevin Darbyshire-Bryant <ke...@darbyshire-bryant.me.uk>
---
 src/rfc1035.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/rfc1035.c b/src/rfc1035.c
index 26f5301..91a9641 100644
--- a/src/rfc1035.c
+++ b/src/rfc1035.c
@@ -1225,7 +1225,8 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 
   /* Clear buffer beyond request to avoid risk of
      information disclosure. */
-  memset(((char *)header) + qlen, 0, 
+  if ( (limit - ((char *)header)) > qlen )
+      memset(((char *)header) + qlen, 0,
 	 (limit - ((char *)header)) - qlen);
   
   if (ntohs(header->ancount) != 0 ||
-- 
2.7.4

_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

Reply via email to