RepoMirrors/haproxy
1
0
Fork 0
mirror of http://git.haproxy.org/git/haproxy.git/ synced 2025-04-18 13:05:38 +00:00
Code Issues Packages Projects Releases Wiki Activity

BUG/MEDIUM: resolvers: always check a valid item in query_list

Browse Source 
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.
This commit is contained in:
Willy Tarreau 2021-10-19 11:17:33 +02:00
parent 10c1a8c3bd
commit 25e010906a
1 changed files with 28 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

35
src/resolvers.c
View File

@ -926,7 +926,7 @@ static int resolv_validate_dns_response(unsigned char *resp, unsigned char *bufe
reader += 2;
/* Parsing dns queries */
LIST_INIT(&r_res->query_list);
BUG_ON(!LIST_ISEMPTY(&r_res->query_list));
for (query_record_id = 0; query_record_id < r_res->header.qdcount; query_record_id++) {
/* Use next pre-allocated resolv_query_item after ensuring there is
* still one available.
@ -1773,6 +1773,8 @@ static struct resolv_resolution *resolv_pick_resolution(struct resolvers *resolv
/* No resolution could be found, so let's allocate a new one */
res = pool_zalloc(resolv_resolution_pool);
if (res) {
int i;
res->resolvers = resolvers;
res->uuid = resolution_uuid;
res->status = RSLV_STATUS_NONE;
@ -1780,8 +1782,12 @@ static struct resolv_resolution *resolv_pick_resolution(struct resolvers *resolv
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;
@ -1807,6 +1813,15 @@ static void resolv_purge_resolution_answer_records(struct resolv_resolution *res
}
}
/* deletes all query_items from the resolution's query_list */
static void resolv_purge_resolution_query_items(struct resolv_resolution *resolution)
{
struct resolv_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 resolv_free_resolution(struct resolv_resolution *resolution)
{
@ -1822,6 +1837,8 @@ static void resolv_free_resolution(struct resolv_resolution *resolution)
req->resolution = NULL;
}
resolv_purge_resolution_answer_records(resolution);
resolv_purge_resolution_query_items(resolution);
LIST_DELETE(&resolution->list);
pool_free(resolv_resolution_pool, resolution);
}
@ -2160,12 +2177,16 @@ static int resolv_process_responses(struct dns_nameserver *ns)
/* 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 resolv_query_item *, list);
if (query && memcmp(query->name, res->hostname_dn, res->hostname_dn_len) != 0) {
dns_resp = RSLV_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 resolv_query_item *, list);
LIST_DEL_INIT(&query->list);
if (memcmp(query->name, res->hostname_dn, res->hostname_dn_len) != 0) {
dns_resp = RSLV_RESP_WRONG_NAME;
ns->counters->other++;
goto report_res_error;
}
}
/* So the resolution succeeded */