BUG/MEDIUM: resolvers: properly skip root-named authority and additional records

resolv_read_name() returns zero both on error and when the name is the
root (a single zero byte), since it returns the length of the decoded
name. When parsing the authority and additional records of an SRV
response, resolv_validate_dns_response() handles a zero length by moving
to the next record without advancing the read pointer. For a root name,
all remaining records are then parsed again from the same place and
ignored, and for an error, the following records are parsed from an
unknown position.

The most common root-named record is the EDNS OPT pseudo-record, which is
placed in the additional section, and which RFC 6891 allows anywhere in
it. When a server places it before the glue records, all of them are
ignored, and the servers' addresses must be resolved with extra A/AAAA
queries. An attempt to make this an error in commit fefce297a
("BUG/MEDIUM: dns: fix long loops in additional records parse on name
failure") had to be reverted by commit 091768ab3 because it rejected all
responses containing an OPT record.

This can be reproduced with an SRV response whose additional section
starts with an OPT record followed by the A glue records: A queries are
sent for all targets instead of using the glue records. The srv_glue
reg-test was extended to cover this case.

Let's explicitly detect the root name, which is then processed as any
other name (the OPT record being ignored and skipped as an unsupported
type). On real errors, since we can't know where the next record starts,
let's stop parsing the remaining authority and additional records instead
of parsing garbage, but still keep the answers.

This patch should be backported to all stable branches, after the previous
fix ("BUG/MEDIUM: resolvers: skip the data of ignored additional records")
which is needed for the OPT record to be skipped.

diff --git a/src/resolvers.c b/src/resolvers.c
index 27b383f0f1ae6bd09d845e51bca0b0f5abe89d6c..db8714ac8da65e04a25da02e2637a56568d51a2b 100644 (file)
--- a/src/resolvers.c
+++ b/src/resolvers.c
@@ -1478,8 +1478,12 @@ static int resolv_validate_dns_response(unsigned char *resp, unsigned char *bufe
                offset = 0;
                len = resolv_read_name(resp, bufend, reader, tmpname, DNS_MAX_NAME_SIZE,
                                    &offset, 0);
-               if (len == 0)
-                       continue;
+
+               /* a zero length is either an error or the root name. We can't
+                * know where the next record starts in case of error.
+                */
+               if (len == 0 && (reader >= bufend || *reader != 0))
+                       goto skip_parsing_additional_records;
 
                if (reader + offset + 10 > bufend)
                        goto invalid_resp;
@@ -1515,10 +1519,14 @@ static int resolv_validate_dns_response(unsigned char *resp, unsigned char *bufe
                offset = 0;
                len = resolv_read_name(resp, bufend, reader, tmpname, DNS_MAX_NAME_SIZE, &offset, 0);
 
-               if (len == 0) {
+               /* a zero length is either an error or the root name (e.g. the
+                * OPT pseudo-record). We can't know where the next record
+                * starts in case of error.
+                */
+               if (len == 0 && (reader >= bufend || *reader != 0)) {
                        pool_free(resolv_answer_item_pool, answer_record);
                        answer_record = NULL;
-                       continue;
+                       goto skip_parsing_additional_records;
                }
 
                memcpy(answer_record->name, tmpname, len);