[Dnsmasq-discuss] [PATCH 1/1] forward.c: fix handling of truncated response

[Rahul Thakur]

Hi Simon,

So what do you think of my reasoning for this patch? Do you agree?

Best regards,
Rahul Thakur
________________________________
From: Rahul Thakur <rahul.thakur at genexis.eu>
Sent: 25 September 2024 15:29
To: Simon Kelley <simon at thekelleys.org.uk>; dnsmasq-discuss at lists.thekelleys.org.uk <dnsmasq-discuss at lists.thekelleys.org.uk>
Subject: Re: [Dnsmasq-discuss] [PATCH 1/1] forward.c: fix handling of truncated response

Hi Simon,

Thanks for responding to this patch, please find my justification for this patch as follows:

I think rfc 2181 is defining the behaviour for DNS server and not DNS proxy.

I am relying on and referring to rfc 5625 while making this change.

In section 4.4 (https://datatracker.ietf.org/doc/html/rfc5625#section-4.4), the rfc 5625 states,

   If a proxy must unilaterally truncate a response, then the proxy MUST
   set the TC bit.  Similarly, proxies MUST NOT remove the TC bit from
   responses.

Dnsmasq is ofcourse complying to this behaviour and not meddling with the TC bit while setting the answers to 0. But, if I read further section 4.4.1 (https://datatracker.ietf.org/doc/html/rfc5625#section-4.4.1),

 Whilst TCP transport is not strictly mandatory, it is supported by
   the vast majority of stub resolvers and recursive servers.

So, this indicates that it is not mandatory that the client ignores this truncated response. This is further supported by section 6.1.3.2 of rfc 1123 (https://datatracker.ietf.org/doc/html/rfc1123#section-6.1.3.2). In paragraph 3 of the DISCUSSION in section 6.1.3.2, it states,

                 Whether it is possible to use a truncated answer
                 depends on the application.

Hence, when dnsmasq explicitly deletes the answers, then it deprives clients that do not fallback to TCP and are happy with the truncated response to be able to resolve their queries.

To me, it sounds like a better strategy to forward the truncated response as is to the client and let the client decide what it wants to do rather than forcefully dropping the answers.

Best regards,
Rahul Thakur






________________________________
From: Dnsmasq-discuss <dnsmasq-discuss-bounces at lists.thekelleys.org.uk> on behalf of Simon Kelley <simon at thekelleys.org.uk>
Sent: 25 September 2024 13:39
To: dnsmasq-discuss at lists.thekelleys.org.uk <dnsmasq-discuss at lists.thekelleys.org.uk>
Subject: Re: [Dnsmasq-discuss] [PATCH 1/1] forward.c: fix handling of truncated response

I think that this is legitimate behaviour. RFC 2181 para 9 says

    Where TC is set, the partial RRSet that would not completely fit may
    be left in the response.  When a DNS client receives a reply with TC
    set, it should ignore that response, and query again, using a
    mechanism, such as a TCP connection, that will permit larger replies.

Which means the contents (or lack of them) of the answer, auth and
additional sections has to be ignored by the client anyway.

Do you have a standards reference which says otherwise? Test suites can
tell you either that behaviour has changed over releases or that
behaviour differs from other implementations. They cant tell you that
behaviour is correct.

There is a subtle reason for the code being as it is. Dnsmasq
has various functions which change the contents of a packet being
returned, and these can't reliably be applied to a truncated packet, so
data in a truncated packet may (for instance) disclose DNS data which
should be blocked.

The patch is, in any case, broken because it gratuitously removes the
call to the logging code.


Cheers,

Simon.

On 24/09/2024 11:01, Rahul Thakur via Dnsmasq-discuss wrote:
> From: Rahul Thakur <rahul.thakur at iopsys.eu>
>
> the handling of truncated reponse is broken in 2.90. The answers
> are removed before forwarding in case TC bit is set, which
> seems incorrect.
>
> test details-
> the regression was caught by a CDrouter run and this change fixes
> the regression.
> ---
>   src/forward.c | 7 -------
>   1 file changed, 7 deletions(-)
>
> diff --git a/src/forward.c b/src/forward.c
> index 10e7496..c893d84 100644
> --- a/src/forward.c
> +++ b/src/forward.c
> @@ -782,13 +782,6 @@ static size_t process_reply(struct dns_header *header, time_t now, struct server
>        server->flags |= SERV_WARNED_RECURSIVE;
>       }
>
> -  if (header->hb3 & HB3_TC)
> -    {
> -      log_query(F_UPSTREAM, NULL, NULL, "truncated", 0);
> -      header->ancount = htons(0);
> -      header->nscount = htons(0);
> -      header->arcount = htons(0);
> -    }
>
>     if  (!(header->hb3 & HB3_TC) && (!bogusanswer || (header->hb4 & HB4_CD)))
>       {


_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss at lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/attachments/20240930/e4b72f12/attachment-0001.htm>