Harden against invalid cache peer digests - #2485
Conversation
An assert() call was improperly used to validate a received cache digest from a cache_peer. Switch to using an invalid value, so that the improper digest is rejected instead.
squid-anubis
added
the
M-failed-description
This comment was marked as resolved.
This comment was marked as resolved.
|
The relevant call is in peerDigestSetCBlock():783, which calls CalcMaskSize() with a peer-supplied The new code will cause mismatch between cblock.mask_size and the calculated capacity (we may also add an explicit non-zero check as an additional precaution), causing the rejection of the digest. No other call to CalcMaskSize( ) uses user input |
kinkie
added
the
backport-to-v7
squid-anubis
removed
the
M-failed-description
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
The proposed solution has several conceptual and implementation problems. I will find the time to disclose and fix them. The ball is in my court.
rousskov
added
the
S-waiting-for-reviewer
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
I will fix the problems identified in this review. The ball is in my court.
| uint64_t bitCount = (cap * bpe) + 7; | ||
| assert(bitCount < INT_MAX); // do not 31-bit overflow later | ||
| const uint64_t bitCount = (cap * bpe) + 7; | ||
| if (bitCount >= std::numeric_limits<int>::max()) |
There was a problem hiding this comment.
The proposed code is buggy. For example, CalcMaskSize(2^57 + 1, 128) overflows cap * bpe, resulting in incorrect 135 return value:
cap * bpe = (2^57 + 1) * 128 = (2^57 + 1) * 2^7 = 2^64 + 2^7 = 2^64 + 128;
The above multiplication wraps modulo 2^64, producing just 128, and leading to an incorrect "positive" 135 result rather than overflow detection:
(cap * bpe) + 7 = 128 + 7 = 135;
N.B. Unlike this PR code, the corresponding official code did not even try to overcome 64-bit overflows, but it is also buggy, for the same reason.
| assert(bitCount < INT_MAX); // do not 31-bit overflow later | ||
| const uint64_t bitCount = (cap * bpe) + 7; | ||
| if (bitCount >= std::numeric_limits<int>::max()) | ||
| return 0; // overflow; caller must treat 0 as invalid |
There was a problem hiding this comment.
Callers may forget to obey this "must". Depending on one's definition of "treat as invalid", it can be argued that at least one and possibly even two existing callers do not really "treat 0 as invalid" already.
In general, please avoid using special valid (from the compiler point of view) values like zeros or empty strings to flag invalid input.
|
|
||
| /// calculate the size of mask required to digest up to | ||
| /// a specified capacity and bitsize. | ||
| /// \returns 0 when inputs would overflow (invalid). |
There was a problem hiding this comment.
It also returns 0 in other cases. For example, CalcMaskSize(0, 8) AFAICT. This is one of the dangers with using valid (from the compiler point of view) values like zeros or empty strings to flag invalid input.
P.S. Current code may not have any CalcMaskSize(0, 8) callers, but that assertion does not address this concern because code will change (and because changing such code increases associated caller risks).
Also marked a few unaddressed problems. Addressing them may change this solution!
... with digest mask size calculations and with sender/recipient mask size calculations getting out of sync, causing legacy senders to reject digests generated by this modern/patched code. The only (pre-existing) problem this code does not solve is support for senders and receivers that have different `int` sizes. Fixed code does not assert in such environments. It rejects the digests with a level-0 cache.log message. That is good enough for now. The key in this solution is to limit calculated digest capacity (and derive mask size from that) rather than just focusing on safe mask size calculations derived from raw capacity estimates. This solution works because the recipient uses sender's digest capacity to re-calculate the mask size. If we provide the recipient with safe capacity values and a matching mask size, the legacy recipient should be happy. TODO: * Remove a temporary assertion that duplicates unsafe code. * Polish touched error messages. * Consider reducing diff (and hiding unchanged callers) by avoiding MaskSize() renaming.
It would be nice to keep that assertion, but it requires adding a public CacheDigest::UnsafeMaskSize() method, which is probably too much.
The script mentioned in 2022 commit d816f28 could not handle this case but applied a similar change to a nearby similar debugs().
This addition has no effect on typical 32-bit and 64-bit POSIX systems: SSIZE_MAX is far larger than other limits.
... because the corresponding CacheDigest::init() assertions are (and should always be) satisfied by positive capacity already. We do not have to force mask sizes to be always positive from that point of view.
|
|
||
| uint32_t | ||
| CacheDigest::CalcMaskSize(uint64_t cap, uint8_t bpe) | ||
| CacheDigest::MaskSize(const uint64_t cap, const uint8_t bpe) |
There was a problem hiding this comment.
@kinkie, I pushed my current changes. I still need to annotate a few code lines and adjust PR description, but I do have one question that I need your help with: Should we rename CalcMaskSize() in this PR to expose all the current callers, like the current PR code is doing?
If you are not sure that we should rename, then I will undo that change: While that caller information can be useful to understand how this code works, it is not really sufficient because some of the callers are buried three levels deep in the code that may need to be checked/analyzed to reach that understanding...
3 participants
An assert() call was improperly used to validate
a received cache digest from a cache_peer.
Switch to returning an invalid value, so that
the improper digest is rejected instead.