ICP: Fix validation of packet sizes and URLs

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;

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,11 +457,43 @@ 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)) {
-        url = rfc1738_escape(url);
         icpCreateAndSend(ICP_ERR, 0, rfc1738_escape(url), reqnum, 0, fd, from, nullptr);
         return nullptr;
     }
@@ -476,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)
@@ -549,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);
@@ -661,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)

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)

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)