fix(auth): bound the fixed-width reads of the client authentication response - #5998
Conversation
…esponse
The client's authentication response is heap-allocated from a length the CLIENT
declares:
unsigned char pass_len = pkt[cur]; // 0..255, attacker-chosen
if ((size_t)(packet_end - pass_ptr) < pass_len) return false;
pass = (unsigned char *)malloc(pass_len + 1);
The bounds check only confirms the packet carries that many bytes; nothing
enforces a MINIMUM. Six comparisons then read a fixed width from that buffer --
SHA_DIGEST_LENGTH (20), SCRAMBLE_LENGTH (20) or SHA256_DIGEST_LENGTH (32) -- so
a client sending a 1-byte response gets a 2-byte allocation that is read up to
30 bytes past its end, before authentication has succeeded.
Guarded sites (all reachable pre-auth):
verify_user_pass() memcmp native, cleartext-stored password 20
verify_user_pass() proxy_scramble_sha1, hashed-stored 20
PPHR_5passwordFalse_0() memcmp native, monitor credential 20
caching_sha2_fast_auth_verify() 32
PPHR_7auth1() proxy_scramble_sha1, hashed-stored 20
PPHR_verify_password() memcmp native 20
The proxy_scramble_sha1() sites matter most: that helper feeds the response to
proxy_my_crypt() for SCRAMBLE_LENGTH bytes, and both call sites are the
hashed-password path -- the common case, since stored passwords are normally
'*'-prefixed.
All six now route through one helper, auth_response_has(pass_len, need), which
tests 'pass_len + 1 >= need', i.e. the ALLOCATION size.
Gating on 'pass_len == need' instead would be wrong and is the trap here:
PPHR_2 strips a trailing NUL from the response ("remove the extra 0 if
present"), so a legitimate 20-byte native response ending in 0x00 -- about 1 in
256 -- arrives with pass_len == 19 while all 20 bytes are present. An equality
gate rejects real logins intermittently; when I tried it, test_auth_methods-t
showed 20 spurious denials across ~6520 connections. Comparing against the
allocation admits that case and still bounds the read. The rationale is recorded
on the helper so it is not "tightened" later.
process_pkt_auth_swich_response() needs no guard and did not get one: 'len' is
validated to be exactly sizeof(mysql_hdr)+20 and the buffer is a zeroed 128-byte
stack array. A comment now records why.
Verified: test_auth_methods-t passes 40194/40194 (all assertion numbers present,
zero 'not ok' anywhere in the stream, binary RC 0) on a PROXYSQL31 debug build.
That is a functional no-regression check; it does not exercise a short response.
A raw-socket short-response case under ASAN would be the direct regression test
and is not included here.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (6)
🧰 Additional context used📓 Path-based instructions (2)include/**/*.hpp📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{cpp,h,hpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🔇 Additional comments (2)
📝 WalkthroughWalkthroughAuthentication verification now validates client response allocations before fixed-length SHA-1 and SHA-256 comparisons. Caching-SHA2 verification receives the client response length. The ChangesAuthentication response validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SonarCloud flagged two C-style casts removing const (cpp:M23_090, CRITICAL) in verify_user_pass(). They are pre-existing lines that this branch does not modify -- they were reported as new only because the added length guards shifted the line numbers -- but they are in the function this PR touches and the fix is free. set_SHA1() takes 'char*' and does not modify the username, so const_cast states the intent explicitly instead of silently stripping const with a C-style cast. No behaviour change.
cpp:M23_090 fires on ANY cast that removes const, including const_cast -- the SonarCloud message after the change read 'const_cast removing const qualification', with the same two CRITICAL findings and the same B maintainability rating. The change achieved nothing, so it is reverted to keep this PR's diff to the length guards. The findings are pre-existing lines that this branch does not modify; they are attributed to the PR only because the added guards shifted the line numbers. The real fix is to make set_SHA1() take 'const char*' -- it only reads the username (strlen + SpookyHash::Update) -- but that signature change ripples through MySQL_Authentication, PgSQL_Authentication and ClickHouse_Authentication plus their headers, which does not belong in a pre-auth security fix.
Brings the branch up to date with v3.0 (37 commits, including #5991 and the #5999 salt-length/NUL fixes) and re-triggers a full CI run from a clean state. Merge verified: lib/MySQL_Protocol.cpp auto-merged keeping all six auth_response_has() bounds checks from this branch, with the reverted set_SHA1 const_cast staying reverted; the CACHING_SHA2_PASSWORD() salt-length and NUL rejections and the deterministic salt sweep come across from v3.0 intact. sqlite3 dep rebuilt against the merged patch; builds clean under PROXYSQL31.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.0 #5998 +/- ##
===========================================
+ Coverage 13.87% 52.97% +39.10%
===========================================
Files 154 473 +319
Lines 82411 143386 +60975
Branches 0 36265 +36265
===========================================
+ Hits 11431 75962 +64531
+ Misses 70980 50572 -20408
- Partials 0 16852 +16852
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
SonarCloud reported two CRITICAL cpp:M23_090 findings on this PR:
C-style cast removing const qualification from the type of a pointer
lib/MySQL_Protocol.cpp:1364, :1382
They are pre-existing lines this branch does not modify -- they surfaced only
because the added length guards shifted the line numbers -- but they sit in the
function this PR touches, so they are worth clearing properly.
An earlier attempt swapped the C-style casts for const_cast. That was useless:
cpp:M23_090 fires on ANY cast removing const, and the finding simply came back
reading "const_cast removing const qualification", with the same B maintainability
rating. It was reverted.
The actual fix is to stop removing const. set_SHA1() only reads the username --
strlen() plus SpookyHash::Update() -- so it can take 'const char*', and both call
sites then need no cast at all.
I had deferred this as a cross-class refactor. That was wrong: MySQL_Authentication
is standalone (PgSQL_Authentication declares its own set_SHA1 and ClickHouse's is
commented out), so the change is 4 lines across 3 files.
The remaining (char *) casts at :1289, :2298, :2410 and :2451 are untouched and
not flagged -- they convert unsigned char*/char*, they do not strip const.
|
1 participant
Problem
The client's authentication response is heap-allocated from a length the client declares:
The bounds check only confirms the packet carries that many bytes. Nothing enforces a minimum.
Six comparisons then read a fixed width from that buffer. A client sending a 1-byte response gets a 2-byte allocation and ProxySQL reads up to 30 bytes past its end — before authentication has succeeded.
Impact is a crash where the allocation abuts an unmapped page, or heap corruption detectable under ASAN.
memcmpleaks only equality, so this is a reliability/DoS issue rather than direct disclosure.Sites
verify_user_pass()memcmp, native, cleartext-storedverify_user_pass()proxy_scramble_sha1, hashed-storedPPHR_5passwordFalse_0()memcmp, native, monitor credentialcaching_sha2_fast_auth_verify()memcmpPPHR_7auth1()proxy_scramble_sha1, hashed-storedPPHR_verify_password()memcmp, nativeThe
proxy_scramble_sha1()sites matter most: that helper feeds the response toproxy_my_crypt()forSCRAMBLE_LENGTHbytes, and both call sites are the hashed-password path — the common case, since stored passwords are normally*-prefixed.Reach: the
verify_user_pass/PPHR_*sites apply to anymysql_usersaccount. The monitor sites additionally require the username to equalmysql-monitor_username(defaultmonitor).Fix
One helper, used at all six sites:
It tests against the allocation size (
pass_len + 1), not an exact length.Gating on
pass_len == needwould be wrong, and that is the trap here.PPHR_2strips a trailing NUL from the response (remove the extra 0 if present), so a legitimate 20-byte native response ending in0x00— about 1 in 256 — arrives withpass_len == 19while all 20 bytes are present. An equality gate rejects real logins intermittently; measured at 20 spurious denials across ~6520 connections intest_auth_methods-t. The rationale is recorded on the helper so it is not "tightened" later.process_pkt_auth_swich_response()deliberately has no guard:lenis validated to be exactlysizeof(mysql_hdr)+20and the buffer is a zeroed 128-byte stack array. A comment records why.Verification
test_auth_methods-t: 40194/40194 — all assertion numbers present, zeronot okanywhere in the stream, binary RC 0, on aPROXYSQL31debug build.That is a functional no-regression check. It does not exercise a short response — a raw-socket short-response case under ASAN would be the direct regression test and is not included here.
Summary by CodeRabbit