From 625189538235595d097cf4263c045608472e87eb Mon Sep 17 00:00:00 2001 From: Rene Cannao Date: Sun, 9 Aug 2026 05:17:15 +0000 Subject: [PATCH 1/4] fix(auth): bound the fixed-width reads of the client authentication response 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. --- lib/MySQL_Protocol.cpp | 78 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 64 insertions(+), 14 deletions(-) diff --git a/lib/MySQL_Protocol.cpp b/lib/MySQL_Protocol.cpp index ed1321e5f1..892d8ca17f 100644 --- a/lib/MySQL_Protocol.cpp +++ b/lib/MySQL_Protocol.cpp @@ -1201,6 +1201,29 @@ void ch_account_to_my(account_details_t& account, ch_account_details_t& ch_accou } #endif /* PROXYSQLCLICKHOUSE */ +/** + * @brief Can 'need' bytes be read from the client's authentication response? + * @details The response buffer is heap-allocated from a CLIENT-CONTROLLED length + * as 'malloc(pass_len + 1)', with a terminating NUL written at [pass_len], so + * exactly 'pass_len + 1' bytes are readable. Nothing in the packet parsing + * enforces a minimum: a client may send a 1-byte response and get a 2-byte + * allocation. The fixed-width comparisons below read SHA_DIGEST_LENGTH (20) or + * SHA256_DIGEST_LENGTH (32) bytes, so without this check they read past the end + * of the allocation, before authentication has succeeded. + * + * Do NOT tighten this to 'pass_len == need'. 'pass_len' is not the amount of + * valid data in the buffer: the packet parser strips a trailing NUL from the + * client's response ("remove the extra 0 if present"), so a legitimate 20-byte + * native response whose last byte is 0x00 -- about 1 in 256 -- arrives with + * pass_len == 19 while all 20 bytes are present. An equality gate therefore + * rejects real logins intermittently; measured at 20 spurious denials across + * ~6520 connections in test_auth_methods-t. Comparing against the allocation + * size ('pass_len + 1') admits that case and still bounds the read. + */ +static inline bool auth_response_has(int64_t pass_len, size_t need) { + return pass_len >= 0 && static_cast(pass_len) + 1 >= need; +} + bool MySQL_Protocol::process_pkt_auth_swich_response(unsigned char *pkt, unsigned int len) { bool ret=false; char *password=NULL; @@ -1251,6 +1274,10 @@ bool MySQL_Protocol::process_pkt_auth_swich_response(unsigned char *pkt, unsigne if (password[0]!='*') { // clear text password proxy_scramble(reply, (*myds)->myconn->scramble_buff, password); + // No bounds check needed here: 'len' is validated to be exactly + // sizeof(mysql_hdr)+20 above, and 'pass' is a zeroed 128-byte stack + // buffer holding those 20 bytes. Unlike the PPHR_* paths, nothing + // here is sized from a client-declared length. if (memcmp(reply, pass, SHA_DIGEST_LENGTH)==0) { ret=true; } @@ -1298,7 +1325,8 @@ bool MySQL_Protocol::verify_user_pass( if (password[0]!='*') { // clear text password if (auth_plugin_id == 0) { // mysql_native_password proxy_scramble(reply, (*myds)->myconn->scramble_buff, password); - if (memcmp(reply, pass, SHA_DIGEST_LENGTH)==0) { + if (auth_response_has(pass_len, SHA_DIGEST_LENGTH) && + memcmp(reply, pass, SHA_DIGEST_LENGTH)==0) { ret=true; } } else if (auth_plugin_id == 1) { // mysql_clear_password @@ -1324,7 +1352,12 @@ bool MySQL_Protocol::verify_user_pass( } } else { if (auth_plugin_id == 0) { - if (session_type == PROXYSQL_SESSION_MYSQL || session_type == PROXYSQL_SESSION_SQLITE) { + // proxy_scramble_sha1() feeds 'pass' to proxy_my_crypt() for + // SCRAMBLE_LENGTH (20) bytes, so it needs the same bound as the + // cleartext branch above. This is the COMMON path -- stored passwords are + // normally hashed ('*'-prefixed). + if ((session_type == PROXYSQL_SESSION_MYSQL || session_type == PROXYSQL_SESSION_SQLITE) && + auth_response_has(pass_len, SCRAMBLE_LENGTH)) { ret=proxy_scramble_sha1((char *)pass,(*myds)->myconn->scramble_buff,password+1, reply); if (ret) { if (sha1_pass==NULL) { @@ -2076,7 +2109,8 @@ void MySQL_Protocol::PPHR_5passwordTrue( static bool caching_sha2_fast_auth_verify( const char* cleartext_password, const char* scramble, - const unsigned char* client_response + const unsigned char* client_response, + int64_t client_response_len ); /** @@ -2121,7 +2155,9 @@ void MySQL_Protocol::PPHR_5passwordFalse_0( // ("remove the extra 0 if present"), so a legitimate 20-byte native // response whose last byte is 0x00 -- about 1 in 256 -- arrives with // pass_len == 19 while all 20 bytes are present in the buffer. - verified = (memcmp(reply, vars1.pass, SHA_DIGEST_LENGTH) == 0); + verified = + auth_response_has(vars1.pass_len, SHA_DIGEST_LENGTH) && + (memcmp(reply, vars1.pass, SHA_DIGEST_LENGTH) == 0); break; case AUTH_MYSQL_CACHING_SHA2_PASSWORD: @@ -2134,7 +2170,7 @@ void MySQL_Protocol::PPHR_5passwordFalse_0( } else { verified = caching_sha2_fast_auth_verify( mysql_thread___monitor_password, (*myds)->myconn->scramble_buff, - vars1.pass + vars1.pass, vars1.pass_len ); } break; @@ -2296,20 +2332,29 @@ void MySQL_Protocol::PPHR_5passwordFalse_auth2( * @param cleartext_password The password ProxySQL holds, NUL-terminated. * @param scramble The 20-byte connection scramble. * @param client_response The response bytes sent by the client. + * @param client_response_len The client-declared response length, i.e. the + * 'pass_len' the buffer was allocated from. See @ref auth_response_has: the + * allocation is 'client_response_len + 1' bytes, and this function reads a + * fixed SHA256_DIGEST_LENGTH (32), so a short response would otherwise be read + * past its end before authentication. * @return true when the response matches. */ static bool caching_sha2_fast_auth_verify( const char* cleartext_password, const char* scramble, - const unsigned char* client_response + const unsigned char* client_response, + int64_t client_response_len ) { - // Deliberately NOT length-checked against 'vars1.pass_len'. That field is not - // the amount of valid data in the response buffer: PPHR_2 strips a trailing - // NUL byte ("remove the extra 0 if present"), so a legitimate 32-byte - // caching_sha2 response ending in 0x00 -- about 1 in 256 -- reports - // pass_len == 31 while all 32 bytes are present. Gating on it rejects real + // Bounds the 32-byte compare below against the allocation, NOT against an + // exact length. 'client_response_len' is not the amount of valid data: PPHR_2 + // strips a trailing NUL byte ("remove the extra 0 if present"), so a + // legitimate 32-byte caching_sha2 response ending in 0x00 -- about 1 in 256 -- + // reports 31 while all 32 bytes are present. An equality gate rejects real // logins intermittently; measured at 20 spurious denials across ~6520 // connections in test_auth_methods-t. + if (auth_response_has(client_response_len, SHA256_DIGEST_LENGTH) == false) { + return false; + } if (cleartext_password == NULL || client_response == NULL) { return false; } @@ -2339,7 +2384,7 @@ void MySQL_Protocol::PPHR_6auth2( if (session_type == PROXYSQL_SESSION_MYSQL || session_type == PROXYSQL_SESSION_SQLITE || session_type == PROXYSQL_SESSION_ADMIN || session_type == PROXYSQL_SESSION_STATS) { if ( caching_sha2_fast_auth_verify( - vars1.password, (*myds)->myconn->scramble_buff, vars1.pass + vars1.password, (*myds)->myconn->scramble_buff, vars1.pass, vars1.pass_len ) ) { ret = true; @@ -2354,7 +2399,10 @@ void MySQL_Protocol::PPHR_7auth1( account_details_t& attr1 ) { enum proxysql_session_type session_type = (*myds)->sess->session_type; - if (session_type == PROXYSQL_SESSION_MYSQL || session_type == PROXYSQL_SESSION_SQLITE || session_type == PROXYSQL_SESSION_ADMIN || session_type == PROXYSQL_SESSION_STATS) { + // As in verify_user_pass(): proxy_scramble_sha1() reads SCRAMBLE_LENGTH (20) + // bytes from 'vars1.pass', which is sized from the client-declared length. + if ((session_type == PROXYSQL_SESSION_MYSQL || session_type == PROXYSQL_SESSION_SQLITE || session_type == PROXYSQL_SESSION_ADMIN || session_type == PROXYSQL_SESSION_STATS) && + auth_response_has(vars1.pass_len, SCRAMBLE_LENGTH)) { ret=proxy_scramble_sha1((char *)vars1.pass,(*myds)->myconn->scramble_buff,vars1.password+1, reply); if (ret) { if (attr1.sha1_pass==NULL) { @@ -3031,7 +3079,9 @@ bool MySQL_Protocol::PPHR_verify_password(MyProt_tmp_auth_vars& vars1, account_d } else if (vars1.password[0]!='*') { // clear text password if (auth_plugin_id == AUTH_MYSQL_NATIVE_PASSWORD) { // mysql_native_password proxy_scramble(reply, (*myds)->myconn->scramble_buff, vars1.password); - if (vars1.pass_len != 0 && memcmp(reply, vars1.pass, SHA_DIGEST_LENGTH)==0) { + if (vars1.pass_len != 0 && + auth_response_has(vars1.pass_len, SHA_DIGEST_LENGTH) && + memcmp(reply, vars1.pass, SHA_DIGEST_LENGTH)==0) { ret=true; } } else if (auth_plugin_id == AUTH_MYSQL_CLEAR_PASSWORD) { // mysql_clear_password From 2f9bcb0c73338c9a954f22e427982253e7ff6316 Mon Sep 17 00:00:00 2001 From: Rene Cannao Date: Sun, 9 Aug 2026 05:34:12 +0000 Subject: [PATCH 2/4] fix(auth): use const_cast for the set_SHA1 username argument 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. --- lib/MySQL_Protocol.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/MySQL_Protocol.cpp b/lib/MySQL_Protocol.cpp index 892d8ca17f..ae765533ae 100644 --- a/lib/MySQL_Protocol.cpp +++ b/lib/MySQL_Protocol.cpp @@ -1361,7 +1361,9 @@ bool MySQL_Protocol::verify_user_pass( ret=proxy_scramble_sha1((char *)pass,(*myds)->myconn->scramble_buff,password+1, reply); if (ret) { if (sha1_pass==NULL) { - GloMyAuth->set_SHA1((char *)user, cred_scope_for_session(session_type),reply); + // const_cast rather than a C-style cast: set_SHA1() takes char* + // but does not modify the username (cpp:M23_090). + GloMyAuth->set_SHA1(const_cast(user), cred_scope_for_session(session_type),reply); } if (userinfo->sha1_pass) free(userinfo->sha1_pass); userinfo->sha1_pass=sha1_pass_hex(reply); @@ -1379,7 +1381,7 @@ bool MySQL_Protocol::verify_user_pass( if (strcasecmp(double_hashed_password,password)==0) { ret = true; if (sha1_pass==NULL) { - GloMyAuth->set_SHA1((char *)user, cred_scope_for_session(session_type),md1_buf); + GloMyAuth->set_SHA1(const_cast(user), cred_scope_for_session(session_type),md1_buf); } if (userinfo->sha1_pass) free(userinfo->sha1_pass); From 7eff91d5658cfe2e7853f41a954b2a7cf28fc2a0 Mon Sep 17 00:00:00 2001 From: Rene Cannao Date: Sun, 9 Aug 2026 06:00:30 +0000 Subject: [PATCH 3/4] Revert the set_SHA1 const_cast 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. --- lib/MySQL_Protocol.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/MySQL_Protocol.cpp b/lib/MySQL_Protocol.cpp index ae765533ae..892d8ca17f 100644 --- a/lib/MySQL_Protocol.cpp +++ b/lib/MySQL_Protocol.cpp @@ -1361,9 +1361,7 @@ bool MySQL_Protocol::verify_user_pass( ret=proxy_scramble_sha1((char *)pass,(*myds)->myconn->scramble_buff,password+1, reply); if (ret) { if (sha1_pass==NULL) { - // const_cast rather than a C-style cast: set_SHA1() takes char* - // but does not modify the username (cpp:M23_090). - GloMyAuth->set_SHA1(const_cast(user), cred_scope_for_session(session_type),reply); + GloMyAuth->set_SHA1((char *)user, cred_scope_for_session(session_type),reply); } if (userinfo->sha1_pass) free(userinfo->sha1_pass); userinfo->sha1_pass=sha1_pass_hex(reply); @@ -1381,7 +1379,7 @@ bool MySQL_Protocol::verify_user_pass( if (strcasecmp(double_hashed_password,password)==0) { ret = true; if (sha1_pass==NULL) { - GloMyAuth->set_SHA1(const_cast(user), cred_scope_for_session(session_type),md1_buf); + GloMyAuth->set_SHA1((char *)user, cred_scope_for_session(session_type),md1_buf); } if (userinfo->sha1_pass) free(userinfo->sha1_pass); From 7268e5100d5f02ad9b649e65d4ba244e7e6e21b4 Mon Sep 17 00:00:00 2001 From: Rene Cannao Date: Sun, 9 Aug 2026 08:47:53 +0000 Subject: [PATCH 4/4] fix(auth): make MySQL_Authentication::set_SHA1() take a const username 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. --- include/MySQL_Authentication.hpp | 2 +- lib/MySQL_Authentication.cpp | 2 +- lib/MySQL_Protocol.cpp | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/include/MySQL_Authentication.hpp b/include/MySQL_Authentication.hpp index 1a1d628abb..f064b4ac03 100644 --- a/include/MySQL_Authentication.hpp +++ b/include/MySQL_Authentication.hpp @@ -101,7 +101,7 @@ class MySQL_Authentication { void decrease_frontend_user_connections(char *username, PASSWORD_TYPE::E passtype); void set_all_inactive(enum cred_username_type usertype); void remove_inactives(enum cred_username_type usertype); - bool set_SHA1(char *username, enum cred_username_type usertype, void *sha_pass); + bool set_SHA1(const char *username, enum cred_username_type usertype, void *sha_pass); bool set_clear_text_password(char* username, enum cred_username_type usertype, const char* clear_text_password, PASSWORD_TYPE::E passtype); unsigned int memory_usage(); uint64_t get_runtime_checksum(); diff --git a/lib/MySQL_Authentication.cpp b/lib/MySQL_Authentication.cpp index 39e71b51f6..104e7af945 100644 --- a/lib/MySQL_Authentication.cpp +++ b/lib/MySQL_Authentication.cpp @@ -538,7 +538,7 @@ bool MySQL_Authentication::del(char * username, enum cred_username_type usertype return ret; }; -bool MySQL_Authentication::set_SHA1(char * username, enum cred_username_type usertype, void *sha_pass) { +bool MySQL_Authentication::set_SHA1(const char * username, enum cred_username_type usertype, void *sha_pass) { bool ret=false; uint64_t hash1, hash2; SpookyHash *myhash=new SpookyHash(); diff --git a/lib/MySQL_Protocol.cpp b/lib/MySQL_Protocol.cpp index f00e8a8b10..8b3b989f5d 100644 --- a/lib/MySQL_Protocol.cpp +++ b/lib/MySQL_Protocol.cpp @@ -1361,7 +1361,7 @@ bool MySQL_Protocol::verify_user_pass( ret=proxy_scramble_sha1((char *)pass,(*myds)->myconn->scramble_buff,password+1, reply); if (ret) { if (sha1_pass==NULL) { - GloMyAuth->set_SHA1((char *)user, cred_scope_for_session(session_type),reply); + GloMyAuth->set_SHA1(user, cred_scope_for_session(session_type),reply); } if (userinfo->sha1_pass) free(userinfo->sha1_pass); userinfo->sha1_pass=sha1_pass_hex(reply); @@ -1379,7 +1379,7 @@ bool MySQL_Protocol::verify_user_pass( if (strcasecmp(double_hashed_password,password)==0) { ret = true; if (sha1_pass==NULL) { - GloMyAuth->set_SHA1((char *)user, cred_scope_for_session(session_type),md1_buf); + GloMyAuth->set_SHA1(user, cred_scope_for_session(session_type),md1_buf); } if (userinfo->sha1_pass) free(userinfo->sha1_pass);