From 5964eb47969f4dc375adcdcd42305044e38e766d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 8 Feb 2024 12:31:09 +0100 Subject: [PATCH 1/4] Refactor the normal vs error path in control_senddone() The code flow in control_senddone() was modified to be simpler to follow and superfluous INSIST() was zapped from control_recvmessage(). --- bin/named/controlconf.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/bin/named/controlconf.c b/bin/named/controlconf.c index 46b7ab8928..e276497f9e 100644 --- a/bin/named/controlconf.c +++ b/bin/named/controlconf.c @@ -262,10 +262,11 @@ control_senddone(isc_nmhandle_t *handle, isc_result_t result, void *arg) { /* Everything is peachy, continue reading from the socket */ isccc_ccmsg_readmessage(&conn->ccmsg, control_recvmessage, conn); - goto done; + /* Detach the sending reference */ + controlconnection_detach(&conn); + return; } - /* This is the error path */ if (result != ISC_R_SHUTTINGDOWN) { char socktext[ISC_SOCKADDR_FORMATSIZE]; isc_sockaddr_t peeraddr = isc_nmhandle_peeraddr(handle); @@ -277,9 +278,9 @@ control_senddone(isc_nmhandle_t *handle, isc_result_t result, void *arg) { socktext, isc_result_totext(result)); } + /* Shutdown the reading */ conn_shutdown(conn); -done: /* Detach the sending reference */ controlconnection_detach(&conn); } @@ -559,9 +560,6 @@ cleanup: case ISC_R_EOF: break; default: - /* We can't get here on normal path */ - INSIST(result != ISC_R_SUCCESS); - log_invalid(&conn->ccmsg, result); } From 88a14985dbd7093df73c46e00a74539382e79e16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 8 Feb 2024 12:31:09 +0100 Subject: [PATCH 2/4] Add isc_nm_read_stop() and remove .reading member from ccmsg We need to stop reading when calling isc_ccmsg_disconnect() as the reading handle doesn't have to be last because sending might be in progress. After that, we can safely remove .reading member because the reading would not be called after the disconnect has been called. The ccmsg_senddone() should also not call the recv callback if the sending failed, that's the job of the caller's send callback - in fact it already does that, so the code in ccmsg_senddone() was superfluous. --- lib/isccc/ccmsg.c | 11 ++--------- lib/isccc/include/isccc/ccmsg.h | 1 - 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/lib/isccc/ccmsg.c b/lib/isccc/ccmsg.c index 80d622da79..4c033dd975 100644 --- a/lib/isccc/ccmsg.c +++ b/lib/isccc/ccmsg.c @@ -102,10 +102,7 @@ recv_data(isc_nmhandle_t *handle, isc_result_t eresult, isc_region_t *region, done: isc_nm_read_stop(handle); - if (ccmsg->reading) { - ccmsg->reading = false; - ccmsg->recv_cb(handle, eresult, ccmsg->recv_cbarg); - } + ccmsg->recv_cb(handle, eresult, ccmsg->recv_cbarg); return; } @@ -145,7 +142,6 @@ isccc_ccmsg_readmessage(isccc_ccmsg_t *ccmsg, isc_nm_cb_t cb, void *cbarg) { ccmsg->recv_cbarg = cbarg; ccmsg->length_received = false; - ccmsg->reading = true; isc_nm_read(ccmsg->handle, recv_data, ccmsg); } @@ -159,10 +155,6 @@ ccmsg_senddone(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) { ccmsg->send_cb(handle, eresult, ccmsg->send_cbarg); ccmsg->send_cb = NULL; - if (eresult != ISC_R_SUCCESS && ccmsg->reading) { - recv_data(handle, eresult, NULL, ccmsg); - } - isc_nmhandle_detach(&handle); } @@ -184,6 +176,7 @@ isccc_ccmsg_disconnect(isccc_ccmsg_t *ccmsg) { REQUIRE(VALID_CCMSG(ccmsg)); if (ccmsg->handle != NULL) { + isc_nm_read_stop(ccmsg->handle); isc_nmhandle_close(ccmsg->handle); isc_nmhandle_detach(&ccmsg->handle); } diff --git a/lib/isccc/include/isccc/ccmsg.h b/lib/isccc/include/isccc/ccmsg.h index 7a20a7c2e1..5120732901 100644 --- a/lib/isccc/include/isccc/ccmsg.h +++ b/lib/isccc/include/isccc/ccmsg.h @@ -54,7 +54,6 @@ typedef struct isccc_ccmsg { void *recv_cbarg; isc_nm_cb_t send_cb; void *send_cbarg; - bool reading; } isccc_ccmsg_t; ISC_LANG_BEGINDECLS From 315aa3135aabab6dbaac76e64b5196ced7bcb0f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 8 Feb 2024 12:31:09 +0100 Subject: [PATCH 3/4] Fix UAF in ccmsg.c when reading stopped before sending When shutting down the whole server, the reading could stop and detach from controlconnection before sending is done. If send callback then detaches from the last controlconnection handle, the ccmsg would be invalidated after the send callback and thus we must not access ccmsg after calling the send_cb(). --- lib/isccc/ccmsg.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/isccc/ccmsg.c b/lib/isccc/ccmsg.c index 4c033dd975..4c5ff61e5f 100644 --- a/lib/isccc/ccmsg.c +++ b/lib/isccc/ccmsg.c @@ -150,11 +150,13 @@ ccmsg_senddone(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) { isccc_ccmsg_t *ccmsg = arg; REQUIRE(VALID_CCMSG(ccmsg)); + REQUIRE(ccmsg->send_cb != NULL); - INSIST(ccmsg->send_cb != NULL); - ccmsg->send_cb(handle, eresult, ccmsg->send_cbarg); + isc_nm_cb_t send_cb = ccmsg->send_cb; ccmsg->send_cb = NULL; + send_cb(handle, eresult, ccmsg->send_cbarg); + isc_nmhandle_detach(&handle); } From 6c15e45328af67c23bd24f10b86fe5646c8a46fd Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Mon, 5 Feb 2024 18:15:59 +1100 Subject: [PATCH 4/4] Add CHANGES note for [GL #4549] --- CHANGES | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGES b/CHANGES index 5455544899..0bc037a467 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,5 @@ +6341. [bug] Address use after free in ccmsg_senddone. [GL #4549] + 6340. [test] Fix incorrectly reported errors when running tests with `make test` on platforms with older pytest. [GL #4560]