From 6ba9eaecb166db42efc2b555fdc46c35b39ec14d Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Tue, 19 Oct 2021 11:17:33 +0200 Subject: [PATCH] BUG/MEDIUM: resolvers: always check a valid item in query_list The query_list is physically stored in the struct resolution itself, so we have a list that contains a list to items stored in itself (and there is a single item). But the list is first initialized in resolv_validate_dns_response(), while it's scanned in resolv_process_responses() later after calling the former. First, this results in crashes as soon as the code is instrumented a little bit for debugging, as elements from a previous incarnation can appear. But in addition to this, the presence of an element is checked by verifying that the return of LIST_NEXT() is not NULL, while it may never be NULL even for an empty list, resulting in bugs or crashes if the number of responses does not match the list's contents. This is easily triggered by testing for the list non-emptiness outside of the function. Let's make sure the list is always correct, i.e. it's initialized to an empty list when the structure is allocated, elements are checked by first verifying the list is not empty, they are deleted once checked, and in any case at end so that there are no dangling pointers. This should be backported, but only as long as the patch fits without modifications, as adaptations can be risky there given that bugs tend to hide each other. (cherry picked from commit 25e010906a016c9d6e8677726e6e55de80010616) [wt: use resolv_hostname_cmp() in 2.4] Signed-off-by: Willy Tarreau (cherry picked from commit 51128f86129a1257b59005356253f78411d77e95) Signed-off-by: Christopher Faulet --- src/dns.c | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/src/dns.c b/src/dns.c index 9d84bae..3d89868 100644 --- a/src/dns.c +++ b/src/dns.c @@ -1002,7 +1002,7 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend, reader += 2; /* Parsing dns queries */ - LIST_INIT(&dns_p->query_list); + BUG_ON(!LIST_ISEMPTY(&dns_p->query_list)); for (dns_query_record_id = 0; dns_query_record_id < dns_p->header.qdcount; dns_query_record_id++) { /* Use next pre-allocated dns_query_item after ensuring there is * still one available. @@ -1848,6 +1848,8 @@ static struct dns_resolution *dns_pick_resolution(struct dns_resolvers *resolver /* No resolution could be found, so let's allocate a new one */ res = pool_alloc(dns_resolution_pool); if (res) { + int i; + memset(res, 0, sizeof(*res)); res->resolvers = resolvers; res->uuid = resolution_uuid; @@ -1856,8 +1858,12 @@ static struct dns_resolution *dns_pick_resolution(struct dns_resolvers *resolver res->last_valid = now_ms; LIST_INIT(&res->requesters); + LIST_INIT(&res->response.query_list); LIST_INIT(&res->response.answer_list); + for (i = 0; i < DNS_MAX_QUERY_RECORDS; i++) + LIST_INIT(&res->response_query_records[i].list); + res->prefered_query_type = query_type; res->query_type = query_type; res->hostname_dn = *hostname_dn; @@ -1882,6 +1888,15 @@ void dns_purge_resolution_answer_records(struct dns_resolution *resolution) } } +/* deletes all query_items from the resolution's query_list */ +static void dns_purge_resolution_query_items(struct dns_resolution *resolution) +{ + struct dns_query_item *item, *itemback; + + list_for_each_entry_safe(item, itemback, &resolution->response.query_list, list) + LIST_DEL_INIT(&item->list); +} + /* Releases a resolution from its requester(s) and move it back to the pool */ static void dns_free_resolution(struct dns_resolution *resolution) { @@ -1889,6 +1904,8 @@ static void dns_free_resolution(struct dns_resolution *resolution) /* clean up configuration */ dns_reset_resolution(resolution); + dns_purge_resolution_query_items(resolution); + resolution->hostname_dn = NULL; resolution->hostname_dn_len = 0; @@ -2255,12 +2272,16 @@ static void dns_resolve_recv(struct dgram_conn *dgram) /* Now let's check the query's dname corresponds to the one we * sent. We can check only the first query of the list. We send - * one query at a time so we get one query in the response */ - query = LIST_NEXT(&res->response.query_list, struct dns_query_item *, list); - if (query && dns_hostname_cmp(query->name, res->hostname_dn, res->hostname_dn_len) != 0) { - dns_resp = DNS_RESP_WRONG_NAME; - ns->counters->other++; - goto report_res_error; + * one query at a time so we get one query in the response. + */ + if (!LIST_ISEMPTY(&res->response.query_list)) { + query = LIST_NEXT(&res->response.query_list, struct dns_query_item *, list); + LIST_DEL_INIT(&query->list); + if (dns_hostname_cmp(query->name, res->hostname_dn, res->hostname_dn_len) != 0) { + dns_resp = DNS_RESP_WRONG_NAME; + ns->counters->other++; + goto report_res_error; + } } /* So the resolution succeeded */ -- 1.7.10.4