diff --git a/src/ICP.h b/src/ICP.h index 26bdfaecf32..5efbe482961 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 cc9c76306f9..ef1595b8fcc 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) { 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(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) diff --git a/src/icp_v3.cc b/src/icp_v3.cc index f93cdcbdbf4..d05e5475121 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 0eb9872fc80..66f396cba07 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)