Hi, I found a couple of bugs in that part of the code. Can you please try the attached patch? (0001 is useless but I share it too in case of) It will allow parsing of additional records for SRV queries only and when done, will silently ignore any record which are not A or AAAA.
@maint team, please don't apply the patch yet, I want to test it much more before. Baptiste On Tue, Feb 18, 2020 at 2:03 PM Baptiste <[email protected]> wrote: > Hi guys, > > Thx Tim for investigating. > I'll check the PCAP and see why such behavior happens. > > Baptiste > > > On Tue, Feb 18, 2020 at 12:09 AM Tim Düsterhus <[email protected]> wrote: > >> Pieter, >> >> Am 09.02.20 um 15:35 schrieb PiBa-NL: >> > Before commit '2.2-dev0-13a9232, released 2020/01/22 (use additional >> > records from SRV responses)' i get seemingly proper working resolving of >> > server a name. >> > After this commit all responses are counted as 'invalid' in the socket >> > stats. >> >> I can confirm the issue with the provided configuration. The 'if (len == >> 0) {' check in line 1045 of the commit causes HAProxy to consider the >> responses 'invalid': >> >> >> https://github.com/haproxy/haproxy/commit/13a9232ebc63fdf357ffcf4fa7a1a5e77a1eac2b#diff-b2ddf457bc423779995466f7d8b9d147R1045-R1048 >> >> Best regards >> Tim Düsterhus >> >
From fa0b9563c40006be83c3fa1b52eeb3dbbb1b028b Mon Sep 17 00:00:00 2001 From: Baptiste Assmann <[email protected]> Date: Wed, 19 Feb 2020 00:53:26 +0100 Subject: [PATCH 1/2] CLEANUP: remove obsolete comments --- src/dns.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/dns.c b/src/dns.c index 86147a417..9e49babf1 100644 --- a/src/dns.c +++ b/src/dns.c @@ -1030,7 +1030,6 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend, /* now parsing additional records */ nb_saved_records = 0; - //TODO: check with Dinko for DNS poisoning for (i = 0; i < dns_p->header.arcount; i++) { if (reader >= bufend) return DNS_RESP_INVALID; @@ -1202,7 +1201,6 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend, continue; tmp_record->ar_item = dns_answer_record; } - //TODO: there is a leak for now, since we don't clean up AR records LIST_ADDQ(&dns_p->ar_list, &dns_answer_record->list); } -- 2.17.1
From 96a09ab7538af2644c7247be2313fc0cc294949b Mon Sep 17 00:00:00 2001 From: Baptiste Assmann <[email protected]> Date: Wed, 19 Feb 2020 01:08:51 +0100 Subject: [PATCH 2/2] BUG/MEDIUM: dns: improper parsing of aditional records --- src/dns.c | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/src/dns.c b/src/dns.c index 9e49babf1..5550ab976 100644 --- a/src/dns.c +++ b/src/dns.c @@ -1028,7 +1028,9 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend, /* Save the number of records we really own */ dns_p->header.ancount = nb_saved_records; - /* now parsing additional records */ + /* now parsing additional records for SRV queries only */ + if (dns_query->type != DNS_RTYPE_SRV) + goto skip_parsing_additional_records; nb_saved_records = 0; for (i = 0; i < dns_p->header.arcount; i++) { if (reader >= bufend) @@ -1043,25 +1045,7 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend, if (len == 0) { pool_free(dns_answer_item_pool, dns_answer_record); - return DNS_RESP_INVALID; - } - - /* Check if the current record dname is valid. previous_dname - * points either to queried dname or last CNAME target */ - if (dns_query->type != DNS_RTYPE_SRV && memcmp(previous_dname, tmpname, len) != 0) { - pool_free(dns_answer_item_pool, dns_answer_record); - if (i == 0) { - /* First record, means a mismatch issue between - * queried dname and dname found in the first - * record */ - return DNS_RESP_INVALID; - } - else { - /* If not the first record, this means we have a - * CNAME resolution error */ - return DNS_RESP_CNAME_ERROR; - } - + continue; } memcpy(dns_answer_record->name, tmpname, len); @@ -1206,6 +1190,8 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend, } } /* for i 0 to arcount */ + skip_parsing_additional_records: + /* Save the number of records we really own */ dns_p->header.arcount = nb_saved_records; -- 2.17.1
