Re: [Dnsmasq-discuss] reproducible segmentation fault - bisected!
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!
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!
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!
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!
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!
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!
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