|
| 1 | +From 45a800839f475ecfff29b61516f7e112bd04aa28 Mon Sep 17 00:00:00 2001 |
| 2 | +From: Joshua Rogers <MegaManSec@users.noreply.github.com> |
| 3 | +Date: Thu, 12 Feb 2026 20:28:43 +0000 |
| 4 | +Subject: [PATCH] ICP: Fix validation of packet sizes and URLs (#2220) |
| 5 | + |
| 6 | +Fix handling of malformed ICP queries and replies instead of passing |
| 7 | +invalid URL pointer to consumers, leading to out-of-bounds memory reads |
| 8 | +and other problems. These fixes affect both ICP v2 and ICP v3 traffic. |
| 9 | + |
| 10 | +* Reject packets with URLs that are not NUL-terminated. |
| 11 | +* Reject packets with URLs containing embedded NULs or trailing garbage. |
| 12 | + |
| 13 | +The above two restrictions may backfire if popular ICP agents do send |
| 14 | +such malformed URLs, and we will need to do more to handle them |
| 15 | +correctly, but it is _safe_ to reject them for now. |
| 16 | + |
| 17 | +Also protect icpHandleUdp() from dereferencing a nil icpOutgoingConn |
| 18 | +pointer. It is not clear whether icpHandleUdp() can be exposed to nil |
| 19 | +icpOutgoingConn in current code. More work is needed to polish this. |
| 20 | + |
| 21 | +Signed-off-by: Azure Linux Security Servicing Account <azurelinux-security@microsoft.com> |
| 22 | +Upstream-reference: https://github.com/squid-cache/squid/commit/8138e909d2058d4401e0ad49b583afaec912b165.patch |
| 23 | + |
| 24 | +--- |
| 25 | + src/ICP.h | 6 ++++- |
| 26 | + src/icp_v2.cc | 55 ++++++++++++++++++++++++++++++++++++++----- |
| 27 | + src/icp_v3.cc | 10 +++++--- |
| 28 | + src/tests/stub_icp.cc | 3 ++- |
| 29 | + 4 files changed, 63 insertions(+), 11 deletions(-) |
| 30 | + |
| 31 | +diff --git a/src/ICP.h b/src/ICP.h |
| 32 | +index 6425270..88f5632 100644 |
| 33 | +--- a/src/ICP.h |
| 34 | ++++ b/src/ICP.h |
| 35 | +@@ -89,6 +89,10 @@ extern Comm::ConnectionPointer icpIncomingConn; |
| 36 | + extern Comm::ConnectionPointer icpOutgoingConn; |
| 37 | + extern Ip::Address theIcpPublicHostID; |
| 38 | + |
| 39 | ++/// A URI extracted from the given raw packet buffer. |
| 40 | ++/// On errors, details the problem and returns nil. |
| 41 | ++const char *icpGetUrl(const Ip::Address &from, const char *, const icp_common_t &); |
| 42 | ++ |
| 43 | + /// \ingroup ServerProtocolICPAPI |
| 44 | + HttpRequestPointer icpGetRequest(const char *url, int reqnum, int fd, const Ip::Address &from); |
| 45 | + |
| 46 | +@@ -99,7 +103,7 @@ void icpCreateAndSend(icp_opcode, int flags, char const *url, int reqnum, int pa |
| 47 | + icp_opcode icpGetCommonOpcode(); |
| 48 | + |
| 49 | + /// \ingroup ServerProtocolICPAPI |
| 50 | +-void icpDenyAccess(Ip::Address &from, char *url, int reqnum, int fd); |
| 51 | ++void icpDenyAccess(const Ip::Address &from, const char *url, int reqnum, int fd); |
| 52 | + |
| 53 | + /// \ingroup ServerProtocolICPAPI |
| 54 | + PF icpHandleUdp; |
| 55 | +diff --git a/src/icp_v2.cc b/src/icp_v2.cc |
| 56 | +index 6e88d3a..0c71096 100644 |
| 57 | +--- a/src/icp_v2.cc |
| 58 | ++++ b/src/icp_v2.cc |
| 59 | +@@ -425,7 +425,7 @@ icpCreateAndSend(icp_opcode opcode, int flags, char const *url, int reqnum, int |
| 60 | + } |
| 61 | + |
| 62 | + void |
| 63 | +-icpDenyAccess(Ip::Address &from, char *url, int reqnum, int fd) |
| 64 | ++icpDenyAccess(const Ip::Address &from, const char * const url, const int reqnum, const int fd) |
| 65 | + { |
| 66 | + debugs(12, 2, "icpDenyAccess: Access Denied for " << from << " by " << AclMatchedName << "."); |
| 67 | + |
| 68 | +@@ -454,6 +454,39 @@ icpAccessAllowed(const Ip::Address &from, HttpRequest * icp_request) |
| 69 | + return checklist.fastCheck().allowed(); |
| 70 | + } |
| 71 | + |
| 72 | ++const char * |
| 73 | ++icpGetUrl(const Ip::Address &from, const char * const buf, const icp_common_t &header) |
| 74 | ++{ |
| 75 | ++ const auto receivedPacketSize = static_cast<size_t>(header.length); |
| 76 | ++ const auto payloadOffset = sizeof(header); |
| 77 | ++ |
| 78 | ++ // Query payload contains a "Requester Host Address" followed by a URL. |
| 79 | ++ // Payload of other ICP packets (with opcode that we recognize) is a URL. |
| 80 | ++ const auto urlOffset = payloadOffset + ((header.opcode == ICP_QUERY) ? sizeof(uint32_t) : 0); |
| 81 | ++ |
| 82 | ++ // A URL field cannot be empty because it includes a terminating NUL char. |
| 83 | ++ // Ensure that the packet has at least one URL field byte. |
| 84 | ++ if (urlOffset >= receivedPacketSize) { |
| 85 | ++ debugs(12, 3, "too small packet from " << from << ": " << urlOffset << " >= " << receivedPacketSize); |
| 86 | ++ return nullptr; |
| 87 | ++ } |
| 88 | ++ |
| 89 | ++ // All ICP packets (with opcode that we recognize) _end_ with a URL field. |
| 90 | ++ // RFC 2186 requires all URLs to be "Null-Terminated". |
| 91 | ++ if (buf[receivedPacketSize - 1] != '\0') { |
| 92 | ++ debugs(12, 3, "unterminated URL or trailing garbage from " << from); |
| 93 | ++ return nullptr; |
| 94 | ++ } |
| 95 | ++ |
| 96 | ++ const auto url = buf + urlOffset; // a possibly empty c-string |
| 97 | ++ if (urlOffset + strlen(url) + 1 != receivedPacketSize) { |
| 98 | ++ debugs(12, 3, "URL with an embedded NUL or trailing garbage from " << from); |
| 99 | ++ return nullptr; |
| 100 | ++ } |
| 101 | ++ |
| 102 | ++ return url; |
| 103 | ++} |
| 104 | ++ |
| 105 | + HttpRequest::Pointer |
| 106 | + icpGetRequest(const char * const url, const int reqnum, const int fd, const Ip::Address &from) |
| 107 | + { |
| 108 | +@@ -478,13 +511,18 @@ icpGetRequest(const char * const url, const int reqnum, const int fd, const Ip:: |
| 109 | + } |
| 110 | + |
| 111 | + static void |
| 112 | +-doV2Query(int fd, Ip::Address &from, char *buf, icp_common_t header) |
| 113 | ++doV2Query(const int fd, Ip::Address &from, const char * const buf, icp_common_t header) |
| 114 | + { |
| 115 | + int rtt = 0; |
| 116 | + int src_rtt = 0; |
| 117 | + uint32_t flags = 0; |
| 118 | +- /* We have a valid packet */ |
| 119 | +- char *url = buf + sizeof(icp_common_t) + sizeof(uint32_t); |
| 120 | ++ |
| 121 | ++ const auto url = icpGetUrl(from, buf, header); |
| 122 | ++ if (!url) { |
| 123 | ++ icpCreateAndSend(ICP_ERR, 0, "", header.reqnum, 0, fd, from, nullptr); |
| 124 | ++ return; |
| 125 | ++ } |
| 126 | ++ |
| 127 | + const auto icp_request = icpGetRequest(url, header.reqnum, fd, from); |
| 128 | + |
| 129 | + if (!icp_request) |
| 130 | +@@ -543,7 +581,9 @@ icp_common_t::handleReply(char *buf, Ip::Address &from) |
| 131 | + neighbors_do_private_keys = 0; |
| 132 | + } |
| 133 | + |
| 134 | +- char *url = buf + sizeof(icp_common_t); |
| 135 | ++ const auto url = icpGetUrl(from, buf, *this); |
| 136 | ++ if (!url) |
| 137 | ++ return; |
| 138 | + debugs(12, 3, "icpHandleIcpV2: " << icp_opcode_str[opcode] << " from " << from << " for '" << url << "'"); |
| 139 | + |
| 140 | + const cache_key *key = icpGetCacheKey(url, (int) reqnum); |
| 141 | +@@ -678,7 +718,10 @@ icpHandleUdp(int sock, void *) |
| 142 | + |
| 143 | + icp_version = (int) buf[1]; /* cheat! */ |
| 144 | + |
| 145 | +- if (icpOutgoingConn->local == from) |
| 146 | ++ // XXX: The IP equality comparison below ignores port differences but |
| 147 | ++ // should not. It also fails to detect loops when `local` is a wildcard |
| 148 | ++ // address (e.g., [::]:3130) because `from` address is never a wildcard. |
| 149 | ++ if (icpOutgoingConn && icpOutgoingConn->local == from) |
| 150 | + // ignore ICP packets which loop back (multicast usually) |
| 151 | + debugs(12, 4, "icpHandleUdp: Ignoring UDP packet sent by myself"); |
| 152 | + else if (icp_version == ICP_VERSION_2) |
| 153 | +diff --git a/src/icp_v3.cc b/src/icp_v3.cc |
| 154 | +index 2054a41..e3cf041 100644 |
| 155 | +--- a/src/icp_v3.cc |
| 156 | ++++ b/src/icp_v3.cc |
| 157 | +@@ -32,10 +32,14 @@ public: |
| 158 | + |
| 159 | + /// \ingroup ServerProtocolICPInternal3 |
| 160 | + static void |
| 161 | +-doV3Query(int fd, Ip::Address &from, char *buf, icp_common_t header) |
| 162 | ++doV3Query(int fd, Ip::Address &from, const char * const buf, icp_common_t header) |
| 163 | + { |
| 164 | +- /* We have a valid packet */ |
| 165 | +- char *url = buf + sizeof(icp_common_t) + sizeof(uint32_t); |
| 166 | ++ const auto url = icpGetUrl(from, buf, header); |
| 167 | ++ if (!url) { |
| 168 | ++ icpCreateAndSend(ICP_ERR, 0, "", header.reqnum, 0, fd, from, nullptr); |
| 169 | ++ return; |
| 170 | ++ } |
| 171 | ++ |
| 172 | + const auto icp_request = icpGetRequest(url, header.reqnum, fd, from); |
| 173 | + |
| 174 | + if (!icp_request) |
| 175 | +diff --git a/src/tests/stub_icp.cc b/src/tests/stub_icp.cc |
| 176 | +index 6ffde0b..ccee1ac 100644 |
| 177 | +--- a/src/tests/stub_icp.cc |
| 178 | ++++ b/src/tests/stub_icp.cc |
| 179 | +@@ -30,10 +30,11 @@ Comm::ConnectionPointer icpIncomingConn; |
| 180 | + Comm::ConnectionPointer icpOutgoingConn; |
| 181 | + Ip::Address theIcpPublicHostID; |
| 182 | + |
| 183 | ++const char *icpGetUrl(const Ip::Address &, const char *, const icp_common_t &) STUB_RETVAL(nullptr) |
| 184 | + HttpRequestPointer icpGetRequest(const char *, int, int, const Ip::Address &) STUB_RETVAL(HttpRequestPointer()); |
| 185 | + void icpCreateAndSend(icp_opcode, int, char const *, int, int, int, const Ip::Address &, AccessLogEntryPointer) STUB |
| 186 | + icp_opcode icpGetCommonOpcode() STUB_RETVAL(ICP_INVALID) |
| 187 | +-void icpDenyAccess(Ip::Address &, char *, int, int) STUB |
| 188 | ++void icpDenyAccess(const Ip::Address &, const char *, int, int) STUB |
| 189 | + void icpHandleIcpV3(int, Ip::Address &, char *, int) STUB |
| 190 | + void icpConnectionShutdown(void) STUB |
| 191 | + int icpSetCacheKey(const cache_key *) STUB_RETVAL(0) |
| 192 | +-- |
| 193 | +2.43.0 |
| 194 | + |
0 commit comments