From decc17d3b0e0a30d9fdae6ed4b7efec46e30dd9b Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Tue, 21 Nov 2023 14:33:07 +1100 Subject: [PATCH] Ineffective DbC protections Dereference before NULL checks. Thanks to Eric Sesterhenn from X41 D-Sec GmbH for reporting this. --- lib/dns/stats.c | 10 ++++++---- lib/isc/netmgr/http.c | 8 ++++++-- lib/isc/netmgr/streamdns.c | 8 ++++++-- lib/isc/netmgr/tcp.c | 7 +++++-- lib/isc/netmgr/tlsstream.c | 8 ++++++-- lib/isc/netmgr/udp.c | 15 +++++++++++---- 6 files changed, 40 insertions(+), 16 deletions(-) diff --git a/lib/dns/stats.c b/lib/dns/stats.c index 1833c79f70..4c430467ec 100644 --- a/lib/dns/stats.c +++ b/lib/dns/stats.c @@ -351,11 +351,12 @@ void dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, uint8_t alg, dnssecsignstats_type_t operation) { uint32_t kval; - int num_keys = isc_stats_ncounters(stats->counters) / - dnssecsign_block_size; REQUIRE(DNS_STATS_VALID(stats) && stats->type == dns_statstype_dnssec); + int num_keys = isc_stats_ncounters(stats->counters) / + dnssecsign_block_size; + /* Shift algorithm in front of key tag, which is 16 bits */ kval = (uint32_t)(alg << 16 | id); @@ -398,11 +399,12 @@ dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, uint8_t alg, void dns_dnssecsignstats_clear(dns_stats_t *stats, dns_keytag_t id, uint8_t alg) { uint32_t kval; - int num_keys = isc_stats_ncounters(stats->counters) / - dnssecsign_block_size; REQUIRE(DNS_STATS_VALID(stats) && stats->type == dns_statstype_dnssec); + int num_keys = isc_stats_ncounters(stats->counters) / + dnssecsign_block_size; + /* Shift algorithm in front of key tag, which is 16 bits */ kval = (uint32_t)(alg << 16 | id); diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index a3b5bd6cb5..d74d02d3e2 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -1449,7 +1449,7 @@ isc_nm_httpconnect(isc_nm_t *mgr, isc_sockaddr_t *local, isc_sockaddr_t *peer, unsigned int timeout) { isc_sockaddr_t local_interface; isc_nmsocket_t *sock = NULL; - isc__networker_t *worker = &mgr->workers[isc_tid()]; + isc__networker_t *worker = NULL; REQUIRE(VALID_NM(mgr)); REQUIRE(cb != NULL); @@ -1457,6 +1457,8 @@ isc_nm_httpconnect(isc_nm_t *mgr, isc_sockaddr_t *local, isc_sockaddr_t *peer, REQUIRE(uri != NULL); REQUIRE(*uri != '\0'); + worker = &mgr->workers[isc_tid()]; + if (isc__nm_closing(worker)) { cb(NULL, ISC_R_SHUTTINGDOWN, cbarg); return; @@ -2461,13 +2463,15 @@ isc_nm_listenhttp(isc_nm_t *mgr, uint32_t workers, isc_sockaddr_t *iface, isc_nmsocket_t **sockp) { isc_nmsocket_t *sock = NULL; isc_result_t result; - isc__networker_t *worker = &mgr->workers[isc_tid()]; + isc__networker_t *worker = NULL; + REQUIRE(VALID_NM(mgr)); REQUIRE(!ISC_LIST_EMPTY(eps->handlers)); REQUIRE(!ISC_LIST_EMPTY(eps->handler_cbargs)); REQUIRE(atomic_load(&eps->in_use) == false); REQUIRE(isc_tid() == 0); + worker = &mgr->workers[isc_tid()]; sock = isc_mem_get(worker->mctx, sizeof(*sock)); isc__nmsocket_init(sock, worker, isc_nm_httplistener, iface, NULL); atomic_init(&sock->h2.max_concurrent_streams, diff --git a/lib/isc/netmgr/streamdns.c b/lib/isc/netmgr/streamdns.c index af312506c7..a0926f1a3d 100644 --- a/lib/isc/netmgr/streamdns.c +++ b/lib/isc/netmgr/streamdns.c @@ -372,10 +372,12 @@ isc_nm_streamdnsconnect(isc_nm_t *mgr, isc_sockaddr_t *local, unsigned int timeout, isc_tlsctx_t *ctx, isc_tlsctx_client_session_cache_t *client_sess_cache) { isc_nmsocket_t *nsock = NULL; - isc__networker_t *worker = &mgr->workers[isc_tid()]; + isc__networker_t *worker = NULL; REQUIRE(VALID_NM(mgr)); + worker = &mgr->workers[isc_tid()]; + if (isc__nm_closing(worker)) { cb(NULL, ISC_R_SHUTTINGDOWN, cbarg); return; @@ -716,11 +718,13 @@ isc_nm_listenstreamdns(isc_nm_t *mgr, uint32_t workers, isc_sockaddr_t *iface, isc_nmsocket_t **sockp) { isc_result_t result; isc_nmsocket_t *listener = NULL; - isc__networker_t *worker = &mgr->workers[isc_tid()]; + isc__networker_t *worker = NULL; REQUIRE(VALID_NM(mgr)); REQUIRE(isc_tid() == 0); + worker = &mgr->workers[isc_tid()]; + if (isc__nm_closing(worker)) { return (ISC_R_SHUTTINGDOWN); } diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index db07202b80..548f9bf650 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -223,13 +223,15 @@ isc_nm_tcpconnect(isc_nm_t *mgr, isc_sockaddr_t *local, isc_sockaddr_t *peer, isc_nmsocket_t *sock = NULL; isc__nm_uvreq_t *req = NULL; sa_family_t sa_family; - isc__networker_t *worker = &mgr->workers[isc_tid()]; + isc__networker_t *worker = NULL; uv_os_sock_t fd = -1; REQUIRE(VALID_NM(mgr)); REQUIRE(local != NULL); REQUIRE(peer != NULL); + worker = &mgr->workers[isc_tid()]; + if (isc__nm_closing(worker)) { connect_cb(NULL, ISC_R_SHUTTINGDOWN, connect_cbarg); return; @@ -446,7 +448,7 @@ isc_nm_listentcp(isc_nm_t *mgr, uint32_t workers, isc_sockaddr_t *iface, isc_nmsocket_t *sock = NULL; uv_os_sock_t fd = -1; isc_result_t result = ISC_R_UNSET; - isc__networker_t *worker = &mgr->workers[0]; + isc__networker_t *worker = NULL; REQUIRE(VALID_NM(mgr)); REQUIRE(isc_tid() == 0); @@ -456,6 +458,7 @@ isc_nm_listentcp(isc_nm_t *mgr, uint32_t workers, isc_sockaddr_t *iface, } REQUIRE(workers <= mgr->nloops); + worker = &mgr->workers[0]; sock = isc_mem_get(worker->mctx, sizeof(*sock)); isc__nmsocket_init(sock, worker, isc_nm_tcplistener, iface, NULL); diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c index b66afde405..d81f250032 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -947,11 +947,13 @@ isc_nm_listentls(isc_nm_t *mgr, uint32_t workers, isc_sockaddr_t *iface, isc_result_t result; isc_nmsocket_t *tlssock = NULL; isc_nmsocket_t *tsock = NULL; - isc__networker_t *worker = &mgr->workers[isc_tid()]; + isc__networker_t *worker = NULL; REQUIRE(VALID_NM(mgr)); REQUIRE(isc_tid() == 0); + worker = &mgr->workers[isc_tid()]; + if (isc__nm_closing(worker)) { return (ISC_R_SHUTTINGDOWN); } @@ -1171,10 +1173,12 @@ isc_nm_tlsconnect(isc_nm_t *mgr, isc_sockaddr_t *local, isc_sockaddr_t *peer, isc_tlsctx_client_session_cache_t *client_sess_cache, unsigned int timeout) { isc_nmsocket_t *sock = NULL; - isc__networker_t *worker = &mgr->workers[isc_tid()]; + isc__networker_t *worker = NULL; REQUIRE(VALID_NM(mgr)); + worker = &mgr->workers[isc_tid()]; + if (isc__nm_closing(worker)) { connect_cb(NULL, ISC_R_SHUTTINGDOWN, connect_cbarg); return; diff --git a/lib/isc/netmgr/udp.c b/lib/isc/netmgr/udp.c index 4671b9f9f0..d9aa3f7716 100644 --- a/lib/isc/netmgr/udp.c +++ b/lib/isc/netmgr/udp.c @@ -211,11 +211,13 @@ isc_nm_listenudp(isc_nm_t *mgr, uint32_t workers, isc_sockaddr_t *iface, isc_result_t result = ISC_R_UNSET; isc_nmsocket_t *sock = NULL; uv_os_sock_t fd = -1; - isc__networker_t *worker = &mgr->workers[0]; + isc__networker_t *worker = NULL; REQUIRE(VALID_NM(mgr)); REQUIRE(isc_tid() == 0); + worker = &mgr->workers[0]; + if (isc__nm_closing(worker)) { return (ISC_R_SHUTTINGDOWN); } @@ -357,12 +359,14 @@ isc_nm_routeconnect(isc_nm_t *mgr, isc_nm_cb_t cb, void *cbarg) { isc_result_t result = ISC_R_SUCCESS; isc_nmsocket_t *sock = NULL; isc__nm_uvreq_t *req = NULL; - isc__networker_t *worker = &mgr->workers[isc_tid()]; + isc__networker_t *worker = NULL; uv_os_sock_t fd = -1; REQUIRE(VALID_NM(mgr)); REQUIRE(isc_tid() == 0); + worker = &mgr->workers[isc_tid()]; + if (isc__nm_closing(worker)) { return (ISC_R_SHUTTINGDOWN); } @@ -647,7 +651,7 @@ isc__nm_udp_send(isc_nmhandle_t *handle, const isc_region_t *region, isc_nm_cb_t cb, void *cbarg) { isc_nmsocket_t *sock = handle->sock; const isc_sockaddr_t *peer = &handle->peer; - const struct sockaddr *sa = sock->connected ? NULL : &peer->type.sa; + const struct sockaddr *sa = NULL; isc__nm_uvreq_t *uvreq = NULL; isc__networker_t *worker = NULL; uint32_t maxudp; @@ -660,6 +664,7 @@ isc__nm_udp_send(isc_nmhandle_t *handle, const isc_region_t *region, worker = sock->worker; maxudp = atomic_load(&worker->netmgr->maxudp); + sa = sock->connected ? NULL : &peer->type.sa; /* * We're simulating a firewall blocking UDP packets bigger than @@ -768,13 +773,15 @@ isc_nm_udpconnect(isc_nm_t *mgr, isc_sockaddr_t *local, isc_sockaddr_t *peer, isc_nmsocket_t *sock = NULL; isc__nm_uvreq_t *req = NULL; sa_family_t sa_family; - isc__networker_t *worker = &mgr->workers[isc_tid()]; + isc__networker_t *worker = NULL; uv_os_sock_t fd = -1; REQUIRE(VALID_NM(mgr)); REQUIRE(local != NULL); REQUIRE(peer != NULL); + worker = &mgr->workers[isc_tid()]; + if (isc__nm_closing(worker)) { cb(NULL, ISC_R_SHUTTINGDOWN, cbarg); return;