Re: [Dnsmasq-discuss] reproducible segmentation fault - bisected!

[Simon Kelley]

Thanks everyone who's been working on this, and apologies for going MIA
until now.

Looking through the code, I think I can seen what's happening:


  memset(((char *)header) + qlen, 0,
 (limit - ((char *)header)) - qlen);

Concentrate on the calculation of the length of the memset

(limit - ((char *)header)) - qlen)

limit is a pointer to (one more than) the last valid byte of the buffer,
it's calculated in the call to answer_request as header + buffer-length,
so the expression

limit - ((char *)header)

is actually equivalent to the length of the buffer.

qlen is the length of the received question, which resides at the start
of the buffer, and which we're going to append the answer too, so the
memset zeros the buffer from the end of the question, to the end of the
buffer. Simple. The question is smaller than the buffer (otherwise we
couldn't have received it in the fist place, so the size parameter to
memset must always be positive. There is no problem.

EXCEPT in forward.c where the limit is calculated (in  receive_query()),
it actually does something different to what's described above, it
doesn't calculate

header + buffer-length

it calculates header + 512, because 512 is the default maximum size of a
DNS reply, unless overridden by an EDNS0 field in the request. If the
EDNS0 is present in the request, it calculates

header + (EDNS0 maximum packet size)

So, if the request (qlen) is either larger than 512, _or_ it includes an
EDNS0 packet size field, and the request is larger than whatever that
specifies, then the size parameter to memset will go negative, and chaos
ensues.  The buffer we use to receive the query is certainly larger then
512 bytes, so there's nothing to stop this being the case, and it's
quite possible that  dnseval is sending a EDNS0 packet size of zero, as
Arne noted.

The solution is to calculate the memset size using the actual buffer
size, and not the limit on the size of the returned answer. Since the
question has been successfully received into this buffer, then is MUST
be larger than qlen, and the memset size can never go negative.

Doing that is not totally straightforward, since answer_request is
called from two places, which have very different buffer sizes. When
answering a TCP request, the buffer is 65536 bytes long. This answer is
to remove the memset from answer_request() and answer_auth() and do it
instead just after the reception of the packet, this can be done in the
UDP and TCP code paths and which know the true length of the buffer.



http://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=commit;h=63437ffbb58837b214b4b92cb1c54bc5f3279928

Is my attempt. Please check it out. I've not attempted to reproduce all
the triggers you've found, so it would be good if you can try them
against this code.



Cheers,

Simon.


On 29/08/17 14:15, Kevin Darbyshire-Bryant wrote:
> 
> 
> On 28/08/17 17:27, Christian Kujau wrote:
>> On Mon, 28 Aug 2017, Christian Kujau wrote:
>>> On Mon, 28 Aug 2017, Kevin Darbyshire-Bryant wrote:
 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.
>>>
>>> Thanks, with your patch dnsmasq doesn't crash anymore when receiving odd
>>> EDNS packets from dnseval.
> 
> Here is a fix rather than my sticking plaster workaround.  My workaround
> patch would actually allow dnsmasq to generate invalid replies, this
> actually *fixes* the problem!
> 
> Cheers,
> 
> Kevin
> 
> 
> ___
> Dnsmasq-discuss mailing list
> Dnsmasq-discuss@lists.thekelleys.org.uk
> http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
> 




signature.asc
Description: OpenPGP digital signature
___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

Re: [Dnsmasq-discuss] reproducible segmentation fault - bisected!

[Kevin Darbyshire-Bryant]

On 28/08/17 17:27, Christian Kujau wrote:

On Mon, 28 Aug 2017, Christian Kujau wrote:

On Mon, 28 Aug 2017, Kevin Darbyshire-Bryant wrote:

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.


Thanks, with your patch dnsmasq doesn't crash anymore when receiving odd
EDNS packets from dnseval.


Here is a fix rather than my sticking plaster workaround.  My workaround 
patch would actually allow dnsmasq to generate invalid replies, this 
actually *fixes* the problem!


Cheers,

Kevin
>From 38af9b1ac3242a4128e88069c495024caa565f0e Mon Sep 17 00:00:00 2001
From: Kevin Darbyshire-Bryant 
Date: Tue, 29 Aug 2017 12:35:40 +0100
Subject: [PATCH] forward.c: fix CVE-2017-13704

Fix SIGSEGV in rfc1035.c answer_request() line 1228 where memset()
is called with header & limit pointing at the same address and thus
tries to clear memory from before the buffer begins.

answer_request() is called with an invalid edns packet size provided by
the client.  Ensure the udp_size provided by the client is bounded by
512 and configured maximum as per RFC 6891 6.2.3 "Values lower than 512
MUST be treated as equal to 512"

The client that exposed the problem provided a payload udp size of 0.

Signed-off-by: Kevin Darbyshire-Bryant 
---
 src/forward.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/forward.c b/src/forward.c
index f22556a..62c5a5a 100644
--- a/src/forward.c
+++ b/src/forward.c
@@ -1408,6 +1408,8 @@ void receive_query(struct listener *listen, time_t now)
 	 defaults to 512 */
   if (udp_size > daemon->edns_pktsz)
 	udp_size = daemon->edns_pktsz;
+  if (udp_size < 512)
+	udp_size = 512; /* RFC 6891 6.2.3 */
 }
 
 #ifdef HAVE_AUTH
-- 
2.7.4

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

Re: [Dnsmasq-discuss] reproducible segmentation fault - bisected!

[Kevin Darbyshire-Bryant]

I've a *much* better fix for this.  Will submit once I've collected 
someone from the station!


Mad busy life,

Kevin

On 28/08/17 17:27, Christian Kujau wrote:

On Mon, 28 Aug 2017, Christian Kujau wrote:

On Mon, 28 Aug 2017, Kevin Darbyshire-Bryant wrote:

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.


Thanks, with your patch dnsmasq doesn't crash anymore when receiving odd
EDNS packets from dnseval.

And thanks for requesting the CVE - I thought about this too, as the bug
constitutes some kind of DoS issue, but since nobody else complained, I
suspected it to be some variation of PEBKAC on my part :)


Oh, I believe it was Juan Manuel requesting the CVE - thanks!

C.



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

Re: [Dnsmasq-discuss] reproducible segmentation fault - bisected!

[Christian Kujau]

On Mon, 28 Aug 2017, Kevin Darbyshire-Bryant wrote:
> 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.

Thanks, with your patch dnsmasq doesn't crash anymore when receiving odd 
EDNS packets from dnseval.

And thanks for requesting the CVE - I thought about this too, as the bug 
constitutes some kind of DoS issue, but since nobody else complained, I 
suspected it to be some variation of PEBKAC on my part :)

Christian.
-- 
BOFH excuse #247:

Due to Federal Budget problems we have been forced to cut back on the number of 
users able to access the system at one time. (namely none allowed)

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

Re: [Dnsmasq-discuss] reproducible segmentation fault - bisected!

[Christian Kujau]

On Mon, 28 Aug 2017, Christian Kujau wrote:
> On Mon, 28 Aug 2017, Kevin Darbyshire-Bryant wrote:
> > 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.
> 
> Thanks, with your patch dnsmasq doesn't crash anymore when receiving odd 
> EDNS packets from dnseval.
> 
> And thanks for requesting the CVE - I thought about this too, as the bug 
> constitutes some kind of DoS issue, but since nobody else complained, I 
> suspected it to be some variation of PEBKAC on my part :)

Oh, I believe it was Juan Manuel requesting the CVE - thanks!

C.
-- 
BOFH excuse #247:

Due to Federal Budget problems we have been forced to cut back on the number of 
users able to access the system at one time. (namely none allowed)

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

Re: [Dnsmasq-discuss] reproducible segmentation fault - bisected!

[Kevin Darbyshire-Bryant]

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 
  Date:   Fri Jul 22 20:56:01 2016 +0100

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



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 problemwhich 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 
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 
---
 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

Re: [Dnsmasq-discuss] reproducible segmentation fault - bisected!

[Christian Kujau]

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 
 Date:   Fri Jul 22 20:56:01 2016 +0100

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

The whole commit cannot be reverted cleanly now, but in my case reverting 
only the change to src/rfc1035.c did the trick (as it appears to have have 
a problem there, see the GDB dump[0]). I've attached a patch as a 
temporary (!) workaround to this email.

However, commenting out this section is clearly not the correct solution,
maybe somebody can have another look on what this routine was supposed to
do here and try again.

For completeness' sake, I was curious to see what exactly dnseval[1] was
sending to Dnsmasq and why it would crash the dnsmasq process in the 
first place. So, this dnseval thingy is a Python script and in commit 
efeccef[2] ("Fix text alignment") they not only changed the "text anlignment" 
but switched to sending EDNS queries too. Their ENDS routine was later 
modified again and its current version (v1.6.3) doesn't make dnsmasq crash 
- but their v1.4.0 does and that's the version that made it to the Debian 
distribution :-\


Thanks for listening,
Christian.

[0] https://paste.fedoraproject.org/paste/awbvnGEvj57ru1TtAuA3ag
[1] https://github.com/farrokhi/dnsdiag/blob/master/dnseval.py
[2] https://github.com/farrokhi/dnsdiag/commit/efeccef
-- 
BOFH excuse #72:

Satan did itdiff --git a/Makefile b/Makefile
index 73ea23e..be7ec72 100644
--- a/Makefile
+++ b/Makefile
@@ -24,8 +24,8 @@ MANDIR= $(PREFIX)/share/man
 LOCALEDIR = $(PREFIX)/share/locale
 BUILDDIR  = $(SRC)
 DESTDIR   = 
-CFLAGS= -Wall -W -O2
-LDFLAGS   = 
+CFLAGS= -Wall -W -Og -g -fstack-protector-strong -Wformat 
-Werror=format-security
+LDFLAGS   = -Wl,-z,relro
 COPTS = 
 RPM_OPT_FLAGS = 
 LIBS  = 
diff --git a/src/rfc1035.c b/src/rfc1035.c
index 26f5301..fb5e0fb 100644
--- a/src/rfc1035.c
+++ b/src/rfc1035.c
@@ -1225,8 +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, 
-(limit - ((char *)header)) - qlen);
+//  memset(((char *)header) + qlen, 0, 
+//  (limit - ((char *)header)) - qlen);
   
   if (ntohs(header->ancount) != 0 ||
   ntohs(header->nscount) != 0 ||
___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss