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.

diff --git a/src/ICP.h b/src/ICP.h
index 10a8e1c673bb88316a36694be05d236cdd037da1..e9ad8d0cef1bbfe3a3dd8792d1b0bf77ea27329e 100644 (file)
--- 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 25f7b71d25ef8566d71a41782c8ddfafdcf7c00d..312104024de4a7382400353c0f53dba0566143cf 100644 (file)
--- 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)
 {
     if (clientdbCutoffDenied(from)) {
         /*
@@ -457,8 +457,41 @@ icpAccessAllowed(Ip::Address &from, HttpRequest * icp_request)
     return false;
 }
 
+const char *
+icpGetUrl(const Ip::Address &from, const char * const buf, const icp_common_t &header)
+{
+    const auto receivedPacketSize = static_cast<size_t>(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)) {
         icpCreateAndSend(ICP_ERR, 0, rfc1738_escape(url), reqnum, 0, fd, from, nullptr);
@@ -475,13 +508,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)
@@ -548,7 +586,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);
@@ -660,7 +700,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 c912dd4c06274ae2b579faea747ef41f46215974..844768aa0bc8934d93fb3299c3cb86de022a7697 100644 (file)
--- a/src/icp_v3.cc
+++ b/src/icp_v3.cc
@@ -32,10 +32,14 @@ public:
 
 /// \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 44091838b58f24a12af0d65d625a47ba82f745f1..d148ab66d48b134d69ced4dfdff4be82ed4aaaf0 100644 (file)
--- 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)