Skip to content

ICP: bound-check ICP_QUERY; register read after init #2220

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MegaManSec wants to merge 4 commits into
base: master
Choose a base branch
from
+14 −5
Open

ICP: bound-check ICP_QUERY; register read after init #2220

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
75da7f1
icp_v2: bound-check ICP_QUERY in doV2Query (fix OOB read)
MegaManSec Sep 5, 2025
9af3d82
Remove extra rfc1738_escape call
MegaManSec Sep 6, 2025
6ba936c
Register icp listener after connection setup.
MegaManSec Sep 6, 2025
d7ad9f6
remove reinterpret_cast
MegaManSec Sep 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions src/icp_v2.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,7 +461,6 @@ HttpRequest *
icpGetRequest(char *url, int reqnum, int fd, 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;
}
Expand All @@ -481,8 +480,18 @@ doV2Query(int fd, Ip::Address &from, char *buf, icp_common_t header)
int rtt = 0;
int src_rtt = 0;
uint32_t flags = 0;
/* We have a valid packet */
Copy link
Contributor

@rousskov rousskov Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR description: doV2Query() trusted header.length and passed a potentially
unterminated URL to consumers, which could lead to out-of-bounds reads
on truncated ICPv2 QUERY packets

doV2Query() caller validates that the query was not truncated:

    /*
     * Length field should match the number of bytes read
     */

    if (len != header.length) {
        debugs(12, 3, "icpHandleIcpV2: ICP message is too small");
        return;
    }

Moreover, doV2Query grand-caller -- icpHandleUdp() -- unconditionally terminates the buffer:

        buf[len] = '\0';

If the above analysis is correct, then neither alleged truncation nor lack of URL termination are real. If you agree, please adjust this PR metadata and code accordingly.

char *url = buf + sizeof(icp_common_t) + sizeof(uint32_t);
/* Ensure QUERY has space for the 32-bit field and at least one URL byte. */
const auto hdr = sizeof(icp_common_t);
const auto qhdr = hdr + sizeof(uint32_t);
Comment on lines +484 to +485
Copy link
Contributor

@yadij yadij Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a size. Please indicate that in the variable name.

Suggested change
const auto hdr = sizeof(icp_common_t);
const auto qhdr = hdr + sizeof(uint32_t);
const auto queryHeaderSize = sizeof(icp_common_t) + sizeof(uint32_t);

related lines will need adjusting to match

const auto pkt_len = static_cast<size_t>(header.length);
if (pkt_len <= qhdr) {
debugs(12, 3, "icpHandleIcpV2: short ICP_QUERY from " << from);
Copy link
Contributor

@yadij yadij Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
debugs(12, 3, "icpHandleIcpV2: short ICP_QUERY from " << from);
debugs(12, 3, "short ICP_QUERY from " << from);

Copy link
Contributor

@rousskov rousskov Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
debugs(12, 3, "icpHandleIcpV2: short ICP_QUERY from " << from);
debugs(12, 3, "too small query from " << from);

or (if we have already checked header size) perhaps

Suggested change
debugs(12, 3, "icpHandleIcpV2: short ICP_QUERY from " << from);
debugs(12, 3, "query without URL from " << from);

"ICP" context will be added to output by debugs() because this file name is "icp_v2.cc".

Other related messages already use "too small" terminology. I do not insist on it, but the message should indicate that there is a problem here.

return;
}
/* Guarantee an in-bounds NUL terminator for any downstream C-string use. */
buf[pkt_len - 1] = '\0';
auto url = buf + qhdr;
Copy link
Contributor

@rousskov rousskov Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
auto url = buf + qhdr;
const auto url = buf + queryHeaderSize;


HttpRequest *icp_request = icpGetRequest(url, header.reqnum, fd, from);

if (!icp_request)
Expand Down Expand Up @@ -737,8 +746,6 @@ icpIncomingConnectionOpened(Ipc::StartListeningAnswer &answer)
if (!Comm::IsConnOpen(conn))
fatal("Cannot open ICP Port");

Comm::SetSelect(conn->fd, COMM_SELECT_READ, icpHandleUdp, nullptr, 0);

for (const wordlist *s = Config.mcast_group_list; s; s = s->next)
ipcache_nbgethostbyname(s->key, mcastJoinGroups, nullptr); // XXX: pass the conn for mcastJoinGroups usage.

Expand All @@ -750,6 +757,8 @@ icpIncomingConnectionOpened(Ipc::StartListeningAnswer &answer)
icpOutgoingConn = conn;
debugs(12, DBG_IMPORTANT, "Sending ICP messages from " << icpOutgoingConn->local);
}

Comm::SetSelect(conn->fd, COMM_SELECT_READ, icpHandleUdp, nullptr, 0);
Copy link
Contributor

@rousskov rousskov Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, the incoming ICP socket installed the read handler before multicast/outgoing setup, leaving a window where icpHandleUdp() could run with icpOutgoingConn == nullptr (when udp_outgoing.isNoAddr()) and attempt to dereference icpOutgoingConn->local. This is fixed by registering the read handler after initialization completes.

AFAICT, the above allegation is misleading or incorrect: Calling Comm::SetSelect(handler) must never call the handler callback synchronously/immediately as it may result in "reentrant" code; the "window" described above does not exist. I/O handlers are called much later, by Comm::DoSelect() and such.

Official code is structured/ordered slightly better IMO. Please undo this change unless my analysis above is flawed.

It is probably a good idea to add a "icpOutgoingConn is set" assertion to icpHandleUdp(). AFAICT, we have similar assertions elsewhere, and existing module initialization code is trying to uphold that invariant (albeit clumsily; if you want to improve that, I would consider replacing the above if...isNoAddr statement condition to !icpOutgoingConn).

Please do not forget to update PR title/description after addressing this change request.

}

/**
Expand Down