Security: fix high-severity vulnerabilities (v0.11.6) - #38
Merged
Conversation
Address the eight High findings from the security audit. - Auth: constant-time password/hash comparison for requirepass (AUTH, HELLO, HTTP bearer) and ACL hashes, via SHA-256-digest comparison that leaks neither length nor prefix. - Parser: cap aggregate pre-allocation (PREALLOC_CAP) so a tiny/nested multibulk header can't force a huge up-front Vec reservation. - Timeouts: 10s TLS handshake timeout, 15s HTTP header-read timeout, and a 30s RESP idle timeout that reaps partial/pre-auth connections while leaving idle authenticated clients alone. - HTTP: bound request bodies at 64MB (413 on excess) and add a 250ms delay on failed auth to slow brute force. - *RANDMEMBER/*RANDFIELD: cap negative-count magnitude and guard i64::MIN negation to prevent unbounded allocation. - Probabilistic: reject forged CMS/TopK values whose declared dimensions don't fit the buffer (OOB/OOM), and cap BF.RESERVE allocation. - BITCOUNT: guard the empty-string length underflow. Adds regression tests for constant-time comparison and forged-header rejection.
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
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.
Fixes the eight High findings from the security audit. Version bumped to 0.11.6.
cargo checkis clean (no warnings); 84 unit tests + all integration tests pass except the two pubsub integration tests in the untracked WIPtests/dir that fail identically onmain(pre-existing, unrelated).Findings addressed
7. Non-constant-time password comparison (timing attack).
requirepasschecks (AUTH, HELLO, HTTP bearer) now compare fixed-length SHA-256 digests in constant time (verify_secret), and ACL hash matching uses a non-short-circuitingconstant_time_eq. Neither length nor prefix leaks via timing.8. RESP parser pre-allocation amplification. Aggregate parsing (
array/map/set/push) previously reserved up toMAX_MULTIBULK_LEN(1M) slots from a tiny header, amplified ~32× by nesting. Pre-reservation is now capped atPREALLOC_CAP(1024) and the container grows lazily; the declared length is still validated againstMAX_MULTIBULK_LEN.9. Missing timeouts (slow-loris). Added a 10 s TLS-handshake timeout, a 15 s HTTP header-read timeout, and a 30 s RESP idle timeout that reaps connections which are mid-command (partial buffer) or not yet authenticated, while leaving idle authenticated pooled connections alone.
10. HTTP body-size limit + auth backoff. Request bodies are bounded at 64 MB via
Limited(413on excess), and a 250 ms delay is applied after a failed HTTP auth attempt.11. Negative-count unbounded Vec in
*RANDMEMBER/*RANDFIELD.SRANDMEMBER/ZRANDMEMBER/HRANDFIELDnow cap the negated count at 1,000,000 and guardi64::MINnegation withchecked_neg.12. Forged probabilistic headers (via plain
SET).is_cms/is_topknow validate that the declaredwidth × depthdimensions fit the actual buffer (checked arithmetic), so a forged blob is rejected asWRONGTYPEinstead of driving out-of-bounds reads;TopKState::from_bytesbounds itsnum_itemspre-allocation.13.
BF.RESERVE~512 MB per call. The computed byte count is validated against the 256 MB cap before allocation.14.
BITCOUNTempty-string underflow. Guarded so the ranged path can't computelen - 1on an empty value.Tests
Added unit tests for constant-time comparison (
test_constant_time_eq,test_verify_secret) and forged-header rejection (test_forged_cms_header_rejected,test_forged_topk_header_rejected).Notes / residuals
Medium/Low findings remain out of scope for this PR.