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 > Dnsmasqemail@example.com > http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss >
Description: OpenPGP digital signature
_______________________________________________ Dnsmasq-discuss mailing list Dnsmasqfirstname.lastname@example.org http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss