From 175c4d905567a841c0f76704ebc126ee9268ecd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Witold=20Kr=C4=99cicki?= Date: Thu, 21 May 2020 14:31:09 +0200 Subject: [PATCH 1/3] Fix a data access race in resolver We were passing client address to dns_resolver_createfetch as a pointer and it was saved as a pointer. The client (with its address) could be gone before the fetch is finished, and in a very odd scenario log_formerr would call isc_sockaddr_format() which first checks if the address family is valid (and at this point it still is), then the sockaddr is cleared, and then isc_netaddr_fromsockaddr is called which fails an assertion as the address family is now invalid. --- lib/dns/resolver.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index e97bc41b5d..a6d0a65eab 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -395,9 +395,9 @@ struct fetchctx { unsigned int valfail; bool timeout; dns_adbaddrinfo_t *addrinfo; - const isc_sockaddr_t *client; dns_messageid_t id; unsigned int depth; + char clientstr[ISC_SOCKADDR_FORMATSIZE]; }; #define FCTX_MAGIC ISC_MAGIC('F', '!', '!', '!') @@ -2099,7 +2099,8 @@ fctx_query(fetchctx_t *fctx, dns_adbaddrinfo_t *addrinfo, if (result != ISC_R_SUCCESS) { goto cleanup_socket; } -#endif /* ifndef BROKEN_TCP_BIND_BEFORE_CONNECT */ +#endif /* ifndef BROKEN_TCP_BIND_BEFORE_CONNECT */ + /* * A dispatch will be created once the connect succeeds. */ @@ -3494,11 +3495,13 @@ findname(fetchctx_t *fctx, const dns_name_t *name, in_port_t port, fctx->adb, res->buckets[fctx->bucketnum].task, fctx_finddone, fctx, name, &fctx->name, fctx->type, options, now, NULL, res->view->dstport, fctx->depth + 1, fctx->qc, &find); + isc_log_write(dns_lctx, DNS_LOGCATEGORY_RESOLVER, DNS_LOGMODULE_RESOLVER, ISC_LOG_DEBUG(3), - "fctx %p(%s): createfind for %p/%d - %s", fctx, - fctx->info, fctx->client, fctx->id, + "fctx %p(%s): createfind for %s/%d - %s", fctx, + fctx->info, fctx->clientstr, fctx->id, isc_result_totext(result)); + if (result != ISC_R_SUCCESS) { if (result == DNS_R_ALIAS) { char namebuf[DNS_NAME_FORMATSIZE]; @@ -4989,7 +4992,12 @@ fctx_create(dns_resolver_t *res, const dns_name_t *name, dns_rdatatype_t type, fctx->rand_bits = 0; fctx->timeout = false; fctx->addrinfo = NULL; - fctx->client = client; + if (client != NULL) { + isc_sockaddr_format(client, fctx->clientstr, + sizeof(fctx->clientstr)); + } else { + memmove(fctx->clientstr, "", sizeof("ns_ttl = 0; fctx->ns_ttl_ok = false; @@ -5286,8 +5294,6 @@ log_lame(fetchctx_t *fctx, dns_adbaddrinfo_t *addrinfo) { static inline void log_formerr(fetchctx_t *fctx, const char *format, ...) { char nsbuf[ISC_SOCKADDR_FORMATSIZE]; - char clbuf[ISC_SOCKADDR_FORMATSIZE]; - const char *clmsg = ""; char msgbuf[2048]; va_list args; @@ -5297,17 +5303,10 @@ log_formerr(fetchctx_t *fctx, const char *format, ...) { isc_sockaddr_format(&fctx->addrinfo->sockaddr, nsbuf, sizeof(nsbuf)); - if (fctx->client != NULL) { - clmsg = " for client "; - isc_sockaddr_format(fctx->client, clbuf, sizeof(clbuf)); - } else { - clbuf[0] = '\0'; - } - isc_log_write(dns_lctx, DNS_LOGCATEGORY_RESOLVER, DNS_LOGMODULE_RESOLVER, ISC_LOG_NOTICE, - "DNS format error from %s resolving %s%s%s: %s", nsbuf, - fctx->info, clmsg, clbuf, msgbuf); + "DNS format error from %s resolving %s for %s: %s", nsbuf, + fctx->info, fctx->clientstr, msgbuf); } static isc_result_t From f0f859411f95bc7957a4e1d01acca3e3cc491263 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Witold=20Kr=C4=99cicki?= Date: Thu, 21 May 2020 14:31:09 +0200 Subject: [PATCH 2/3] Add CHANGES entry for #1808 --- CHANGES | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGES b/CHANGES index ca91a901ec..b56d003225 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,5 @@ +5437. [bug] Fix a data race in resolver log_formerr. [GL #1808] + 5436. [placeholder] 5435. [placeholder] From 5a9f594629557eef74f89bfbaaaffe5b22a20c8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 5 Jun 2020 12:08:13 +0200 Subject: [PATCH 3/3] Add release note for #1808 --- doc/notes/notes-current.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index e0c7067d22..ed9bb769d9 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -171,3 +171,6 @@ Bug Fixes - With dnssec-policy, when creating a successor key, the goal state of the current active key (the predecessor) was not changed and thus was never is removed from the zone. [GL #1846] + +- Fix a data race in resolver.c:formerr() that could lead to assertion + failure. [GL #1808]