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
