From 8138e909d2058d4401e0ad49b583afaec912b165 Mon Sep 17 00:00:00 2001 From: Joshua Rogers Date: Thu, 12 Feb 2026 20:28:43 +0000 Subject: [PATCH] ICP: Fix validation of packet sizes and URLs (#2220) Fix handling of malformed ICP queries and replies instead of passing invalid URL pointer to consumers, leading to out-of-bounds memory reads and other problems. These fixes affect both ICP v2 and ICP v3 traffic. * Reject packets with URLs that are not NUL-terminated. * Reject packets with URLs containing embedded NULs or trailing garbage. The above two restrictions may backfire if popular ICP agents do send such malformed URLs, and we will need to do more to handle them correctly, but it is _safe_ to reject them for now. Also protect icpHandleUdp() from dereferencing a nil icpOutgoingConn pointer. It is not clear whether icpHandleUdp() can be exposed to nil icpOutgoingConn in current code. More work is needed to polish this. Upstream: https://github.com/squid-cache/squid/commit/8138e909d2058d4401e0ad49b583afaec912b165 CVE: CVE-2026-33515 Signed-off-by: Thomas Perale --- src/ICP.h | 8 ++++-- src/icp_v2.cc | 57 +++++++++++++++++++++++++++++++++++++------ src/icp_v3.cc | 10 +++++--- src/tests/stub_icp.cc | 5 ++-- 4 files changed, 66 insertions(+), 14 deletions(-) diff --git a/src/ICP.h b/src/ICP.h index 10a8e1c673b..e9ad8d0cef1 100644 --- a/src/ICP.h +++ b/src/ICP.h @@ -89,8 +89,12 @@ extern Comm::ConnectionPointer icpIncomingConn; extern Comm::ConnectionPointer icpOutgoingConn; extern Ip::Address theIcpPublicHostID; +/// A URI extracted from the given raw packet buffer. +/// On errors, details the problem and returns nil. +const char *icpGetUrl(const Ip::Address &from, const char *, const icp_common_t &); + /// \ingroup ServerProtocolICPAPI -HttpRequest* icpGetRequest(char *url, int reqnum, int fd, Ip::Address &from); +HttpRequest *icpGetRequest(const char *url, int reqnum, int fd, const Ip::Address &from); /// \ingroup ServerProtocolICPAPI bool icpAccessAllowed(Ip::Address &from, HttpRequest * icp_request); @@ -102,7 +106,7 @@ void icpCreateAndSend(icp_opcode, int flags, char const *url, int reqnum, int pa icp_opcode icpGetCommonOpcode(); /// \ingroup ServerProtocolICPAPI -void icpDenyAccess(Ip::Address &from, char *url, int reqnum, int fd); +void icpDenyAccess(const Ip::Address &from, const char *url, int reqnum, int fd); /// \ingroup ServerProtocolICPAPI PF icpHandleUdp; diff --git a/src/icp_v2.cc b/src/icp_v2.cc index 25f7b71d25e..312104024de 100644 --- a/src/icp_v2.cc +++ b/src/icp_v2.cc @@ -425,7 +425,7 @@ icpCreateAndSend(icp_opcode opcode, int flags, char const *url, int reqnum, int } void -icpDenyAccess(Ip::Address &from, char *url, int reqnum, int fd) +icpDenyAccess(const Ip::Address &from, const char * const url, const int reqnum, const int fd) { debugs(12, 2, "icpDenyAccess: Access Denied for " << from << " by " << AclMatchedName << "."); @@ -453,8 +453,41 @@ icpAccessAllowed(Ip::Address &from, HttpRequest * icp_request) return checklist.fastCheck().allowed(); } +const char * +icpGetUrl(const Ip::Address &from, const char * const buf, const icp_common_t &header) +{ + const auto receivedPacketSize = static_cast(header.length); + const auto payloadOffset = sizeof(header); + + // Query payload contains a "Requester Host Address" followed by a URL. + // Payload of other ICP packets (with opcode that we recognize) is a URL. + const auto urlOffset = payloadOffset + ((header.opcode == ICP_QUERY) ? sizeof(uint32_t) : 0); + + // A URL field cannot be empty because it includes a terminating NUL char. + // Ensure that the packet has at least one URL field byte. + if (urlOffset >= receivedPacketSize) { + debugs(12, 3, "too small packet from " << from << ": " << urlOffset << " >= " << receivedPacketSize); + return nullptr; + } + + // All ICP packets (with opcode that we recognize) _end_ with a URL field. + // RFC 2186 requires all URLs to be "Null-Terminated". + if (buf[receivedPacketSize - 1] != '\0') { + debugs(12, 3, "unterminated URL or trailing garbage from " << from); + return nullptr; + } + + const auto url = buf + urlOffset; // a possibly empty c-string + if (urlOffset + strlen(url) + 1 != receivedPacketSize) { + debugs(12, 3, "URL with an embedded NUL or trailing garbage from " << from); + return nullptr; + } + + return url; +} + HttpRequest * -icpGetRequest(char *url, int reqnum, int fd, Ip::Address &from) +icpGetRequest(const char * const url, const int reqnum, const int fd, const Ip::Address &from) { if (strpbrk(url, w_space)) { url = rfc1738_escape(url); @@ -472,13 +505,18 @@ icpGetRequest(char *url, int reqnum, int fd, Ip::Address &from) } static void -doV2Query(int fd, Ip::Address &from, char *buf, icp_common_t header) +doV2Query(const int fd, Ip::Address &from, const char * const 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); + + const auto url = icpGetUrl(from, buf, header); + if (!url) { + icpCreateAndSend(ICP_ERR, 0, "", header.reqnum, 0, fd, from, nullptr); + return; + } + HttpRequest *icp_request = icpGetRequest(url, header.reqnum, fd, from); if (!icp_request) @@ -545,7 +583,9 @@ icp_common_t::handleReply(char *buf, Ip::Address &from) neighbors_do_private_keys = 0; } - char *url = buf + sizeof(icp_common_t); + const auto url = icpGetUrl(from, buf, *this); + if (!url) + return; debugs(12, 3, "icpHandleIcpV2: " << icp_opcode_str[opcode] << " from " << from << " for '" << url << "'"); const cache_key *key = icpGetCacheKey(url, (int) reqnum); @@ -680,7 +720,10 @@ icpHandleUdp(int sock, void *) icp_version = (int) buf[1]; /* cheat! */ - if (icpOutgoingConn->local == from) + // XXX: The IP equality comparison below ignores port differences but + // should not. It also fails to detect loops when `local` is a wildcard + // address (e.g., [::]:3130) because `from` address is never a wildcard. + if (icpOutgoingConn && icpOutgoingConn->local == from) // ignore ICP packets which loop back (multicast usually) debugs(12, 4, "icpHandleUdp: Ignoring UDP packet sent by myself"); else if (icp_version == ICP_VERSION_2) diff --git a/src/icp_v3.cc b/src/icp_v3.cc index c912dd4c062..844768aa0bc 100644 --- a/src/icp_v3.cc +++ b/src/icp_v3.cc @@ -32,10 +32,14 @@ class ICP3State: public ICPState /// \ingroup ServerProtocolICPInternal3 static void -doV3Query(int fd, Ip::Address &from, char *buf, icp_common_t header) +doV3Query(int fd, Ip::Address &from, const char * const buf, icp_common_t header) { - /* We have a valid packet */ - char *url = buf + sizeof(icp_common_t) + sizeof(uint32_t); + const auto url = icpGetUrl(from, buf, header); + if (!url) { + icpCreateAndSend(ICP_ERR, 0, "", header.reqnum, 0, fd, from, nullptr); + return; + } + HttpRequest *icp_request = icpGetRequest(url, header.reqnum, fd, from); if (!icp_request) diff --git a/src/tests/stub_icp.cc b/src/tests/stub_icp.cc index 44091838b58..d148ab66d48 100644 --- a/src/tests/stub_icp.cc +++ b/src/tests/stub_icp.cc @@ -29,11 +29,12 @@ Comm::ConnectionPointer icpIncomingConn; Comm::ConnectionPointer icpOutgoingConn; Ip::Address theIcpPublicHostID; -HttpRequest* icpGetRequest(char *, int, int, Ip::Address &) STUB_RETVAL(nullptr) +const char *icpGetUrl(const Ip::Address &, const char *, const icp_common_t &) STUB_RETVAL(nullptr) +HttpRequest* icpGetRequest(const char *, int, int, const Ip::Address &) STUB_RETVAL(nullptr) bool icpAccessAllowed(Ip::Address &, HttpRequest *) STUB_RETVAL(false) void icpCreateAndSend(icp_opcode, int, char const *, int, int, int, const Ip::Address &, AccessLogEntryPointer) STUB icp_opcode icpGetCommonOpcode() STUB_RETVAL(ICP_INVALID) -void icpDenyAccess(Ip::Address &, char *, int, int) STUB +void icpDenyAccess(const Ip::Address &, const char *, int, int) STUB void icpHandleIcpV3(int, Ip::Address &, char *, int) STUB void icpConnectionShutdown(void) STUB int icpSetCacheKey(const cache_key *) STUB_RETVAL(0)