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).
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.