ICP: bound-check ICP_QUERY; register read after init

[MegaManSec]

Previously, 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. This is fixed by requiring length >
sizeof(icp_common_t)+4, guaranteeing at least one URL byte, and writing
a NUL within the received packet bounds; short packets are logged and
dropped.

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.

Previously, icpGetRequest() reassigned url = rfc1738_escape(url) on the
error path, causing double encoding of the URL used in the error
response.

[yadij]

Yay one less rfc1738_escape.
Some code polish needed.

[yadij]

Suggested change

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

[rousskov]

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.

[yadij]

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

[rousskov]

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.

[rousskov]

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.

[rousskov]

Suggested change

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

[rousskov]

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.