ICP: Fix validation of packet sizes and URLs#2220
Open
MegaManSec wants to merge 18 commits intosquid-cache:masterfrom
Open
ICP: Fix validation of packet sizes and URLs#2220MegaManSec wants to merge 18 commits intosquid-cache:masterfrom
MegaManSec wants to merge 18 commits intosquid-cache:masterfrom
Conversation
Require len > sizeof(hdr)+4 and write NUL within packet bounds.
Among other things, this patch ensures that derefs of `icpOutgoingConn->local` do not result in null-pointer deref. For example, when Config.Addrs.udp_outgoing.isNoAddr() is true, icpOutgoingConn is not set in icpOpenPorts(). It is later assigned in icpIncomingConnectionOpened() after the read handler is installed. That short window allows icpHandleUdp() to run with icpOutgoingConn == nullptr.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
yadij
added
the
S-waiting-for-author
yadij
changed the title
rousskov
requested changes
Sep 8, 2025
rousskov
requested changes
Jan 26, 2026
This reverts branch commit 6ba936c but implements a different change that has the same effect: Protecting icpHandleUdp() from nil icpOutgoingConn. I reverted that branch commit because it was based on a false (AFAICT) assumption that Comm::SetSelect() handlers may be called immediately or "synchronously", before SetSelect() caller itself exits. That should not be happening in modern code, I see no signs of that happening, and if it does happen, then it is a separate (and significant!) bug affecting a lot of other code. AFAICT, as far as Squid startup is concerned, icpOutgoingConn should be set by the time icpHandleUdp() is called the first time. However, icpOutgoingConn management is messy! I am especially concerned about icpOutgoingConn becoming nil in icpClosePorts() during reconfiguration: Our Comm callback protections are based on a comm_close() sequence, but icpClosePorts() does not close icpOutgoingConn; it just resets its pointer! This messy code probably happens to work, but it does make sense to protect icpHandleUdp() from dereferencing nil icpOutgoingConn in these messy conditions. This change adds that protection. Long-term, icpIncomingConn and icpOutgoingConn handlers should be split.
This reverts commit 9af3d82. That change is correct, but it addresses a rather different bug, and it may be best to merge it separately (via squid-cache#2374).
Also do not accept ICPv2 query URLs with embedded NULs or trailing garbage. This change may backfire if popular ICP clients do send such malformed queries, and we will need to do more to handle them correctly, but it is _safe_ to reject them for now.
rousskov
reviewed
Feb 10, 2026
Contributor
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
I pushed changes addressing all change requests except the IPv3 one. I have not tested my changes yet.
yadij
previously approved these changes
Feb 11, 2026
Contributor
|
LGTM. @rousskov, I'm leaving the final approval to you to decide whether that ICPv3 change happens here or in a followup PR. |
yadij
added
the
S-waiting-for-reviewer
rousskov
requested changes
Feb 11, 2026
rousskov
removed
the
S-waiting-for-reviewer
rousskov
changed the title
Encapsulated branch-added URI extraction code in icpGetQueryUrl() and reused it in doV3Query(). To reduces chances of mishandling the raw I/O buffer or the URI pointing inside that buffer, I fixed const-correctness for buffer and URI pointers. Unfortunately, that triggered a few seemingly unrelated (but positive) const-correctness changes in several function signatures. Those functions and others need more const-correctness fixes, but an attempt to fix them now snowballed too much, and I undone it.
The key change is in icp_common_t::handleReply(). icpGetQueryUrl() was enhanced to also handle ICP replies.
When sending that error reply, send an empty URL instead of echoing raw invalid bytes. The query sender can usually identify the problematic transaction using the "Request Number" field. There is probably more work to be done here, including logging malformed ICP queries to access log. We still fail silently when receiving truncated ICP messages.
rousskov
changed the title
rousskov
changed the title
rousskov
approved these changes
Feb 11, 2026
Contributor
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
Current PR code passes my quick-and-dirty tests. I do not plan on running more tests at this time. PR title and description have been synced with recent code changes. AFAICT, this PR is ready for merging.
There are more bugs in this neglected ICP code, but I think they should be addressed in dedicated PRs.
|
|
||
| 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) |
Contributor
There was a problem hiding this comment.
I improved const-correctness of icpGetRequest() and icpDenyAccess() parameters to increase my (weak) confidence that the changes in this PR protect all relevant/in-scope code. Technically, these improvements can now be replaced with const_cast hacks and such, reducing diff noise and merge conflicts a bit, but I did not do that.
rousskov
added
S-waiting-for-more-reviewers
squid-anubis
pushed a commit
that referenced
this pull request
Feb 11, 2026
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.
squid-anubis
added
M-waiting-staging-checks
kinkie
approved these changes
Feb 12, 2026
squid-anubis
pushed a commit
that referenced
this pull request
Feb 12, 2026
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.
squid-anubis
added
M-waiting-staging-checks
Contributor
|
@yadij, you have approved an earlier version of this PR, but there were too many non-trivial changes since then to apply that approval to current PR state. I still think we should merge this PR now, but I want to give you another chance to stop this PR from going in. I plan to clear this PR for merging after 48 hours unless you stop me (or clear it yourself). P.S. I have another important ICP fix queued, but I plan that to post that after this PR is merged to avoid merge conflicts. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
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.