From 75da7f1673d22e846b0f9bab63018b51132af599 Mon Sep 17 00:00:00 2001 From: Joshua Rogers Date: Fri, 5 Sep 2025 13:40:59 +0200 Subject: [PATCH 1/4] icp_v2: bound-check ICP_QUERY in doV2Query (fix OOB read) Require len > sizeof(hdr)+4 and write NUL within packet bounds. --- src/icp_v2.cc | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/icp_v2.cc b/src/icp_v2.cc index cc9c76306f9..833db7b6522 100644 --- a/src/icp_v2.cc +++ b/src/icp_v2.cc @@ -481,8 +481,18 @@ doV2Query(int fd, Ip::Address &from, char *buf, icp_common_t header) int rtt = 0; int src_rtt = 0; uint32_t flags = 0; - /* We have a valid packet */ - char *url = buf + sizeof(icp_common_t) + sizeof(uint32_t); + /* Ensure QUERY has space for the 32-bit field and at least one URL byte. */ + const auto hdr = sizeof(icp_common_t); + const auto qhdr = hdr + sizeof(uint32_t); + const auto pkt_len = static_cast(header.length); + if (pkt_len <= qhdr) { + debugs(12, 3, "icpHandleIcpV2: short ICP_QUERY from " << from); + return; + } + /* Guarantee an in-bounds NUL terminator for any downstream C-string use. */ + reinterpret_cast(buf)[pkt_len - 1] = '\0'; + auto url = buf + qhdr; + HttpRequest *icp_request = icpGetRequest(url, header.reqnum, fd, from); if (!icp_request) From 9af3d82523b05d005b5cc2245f2cb4de56fc1d81 Mon Sep 17 00:00:00 2001 From: Joshua Rogers Date: Sat, 6 Sep 2025 21:07:10 +0200 Subject: [PATCH 2/4] Remove extra rfc1738_escape call --- src/icp_v2.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/icp_v2.cc b/src/icp_v2.cc index 833db7b6522..f3e27caec76 100644 --- a/src/icp_v2.cc +++ b/src/icp_v2.cc @@ -461,7 +461,6 @@ HttpRequest * icpGetRequest(char *url, int reqnum, int fd, Ip::Address &from) { if (strpbrk(url, w_space)) { - url = rfc1738_escape(url); icpCreateAndSend(ICP_ERR, 0, rfc1738_escape(url), reqnum, 0, fd, from, nullptr); return nullptr; } From 6ba936c9f3179268ff4c84f59e8bf81857d5766a Mon Sep 17 00:00:00 2001 From: Joshua Rogers Date: Sat, 6 Sep 2025 21:12:45 +0200 Subject: [PATCH 3/4] Register icp listener after connection setup. Among other things, this patch ensures that derefs of `icpOutgoingConn->local` do not result in null-pointer deref. For example, when Config.Addrs.udp_outgoing.isNoAddr() is true, icpOutgoingConn is not set in icpOpenPorts(). It is later assigned in icpIncomingConnectionOpened() after the read handler is installed. That short window allows icpHandleUdp() to run with icpOutgoingConn == nullptr. --- src/icp_v2.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/icp_v2.cc b/src/icp_v2.cc index f3e27caec76..8e7442700a9 100644 --- a/src/icp_v2.cc +++ b/src/icp_v2.cc @@ -746,8 +746,6 @@ icpIncomingConnectionOpened(Ipc::StartListeningAnswer &answer) if (!Comm::IsConnOpen(conn)) fatal("Cannot open ICP Port"); - Comm::SetSelect(conn->fd, COMM_SELECT_READ, icpHandleUdp, nullptr, 0); - for (const wordlist *s = Config.mcast_group_list; s; s = s->next) ipcache_nbgethostbyname(s->key, mcastJoinGroups, nullptr); // XXX: pass the conn for mcastJoinGroups usage. @@ -759,6 +757,8 @@ icpIncomingConnectionOpened(Ipc::StartListeningAnswer &answer) icpOutgoingConn = conn; debugs(12, DBG_IMPORTANT, "Sending ICP messages from " << icpOutgoingConn->local); } + + Comm::SetSelect(conn->fd, COMM_SELECT_READ, icpHandleUdp, nullptr, 0); } /** From d7ad9f61e2fbe4dd686f64d0f6ca3e4c6829200c Mon Sep 17 00:00:00 2001 From: Joshua Rogers Date: Sat, 6 Sep 2025 21:16:18 +0200 Subject: [PATCH 4/4] remove reinterpret_cast --- src/icp_v2.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/icp_v2.cc b/src/icp_v2.cc index 8e7442700a9..3e0a832043c 100644 --- a/src/icp_v2.cc +++ b/src/icp_v2.cc @@ -489,7 +489,7 @@ doV2Query(int fd, Ip::Address &from, char *buf, icp_common_t header) return; } /* Guarantee an in-bounds NUL terminator for any downstream C-string use. */ - reinterpret_cast(buf)[pkt_len - 1] = '\0'; + buf[pkt_len - 1] = '\0'; auto url = buf + qhdr; HttpRequest *icp_request = icpGetRequest(url, header.reqnum, fd, from);