From ffe56903605c3eeb6d09724e95bee29c8dad2a23 Mon Sep 17 00:00:00 2001 From: Rene Cannao Date: Thu, 22 Jan 2026 00:35:57 +0000 Subject: [PATCH] fix: Address coderabbitai review - use-after-free, missing responses, SQL injection Fix issues identified by coderabbitai review: 1. Admin_Handler.cpp: Fix typo in strncasecmp for LOAD MCP QUERY RULES - Line 2365 had "LOAD MCP RULES FROM DISK" instead of "LOAD MCP QUERY RULES FROM DISK" 2. Admin_Handler.cpp: Fix use-after-free and missing client response - Removed l_free(*ql,*q) which freed *q before caller used it - Added send_error_msg_to_client calls on all error paths - Added send_ok_msg_to_client call on success path - Changed return value from true to false (to match handler pattern) - Applied to both LOAD MCP QUERY RULES and SAVE MCP QUERY RULES handlers 3. ProxySQL_Admin_Stats.cpp: Fix sprintf SQL injection in stats___mcp_query_rules - Replaced sprintf with prepared statement using positional parameters - Changed from char* query with malloc/free to sqlite3_stmt with prepare_v2 - Both columns now bound as parameters (?1, ?2) --- lib/Admin_Handler.cpp | 22 +++++++++++++++------- lib/ProxySQL_Admin_Stats.cpp | 25 ++++++++++++++++--------- 2 files changed, 31 insertions(+), 16 deletions(-) diff --git a/lib/Admin_Handler.cpp b/lib/Admin_Handler.cpp index dd88c229a..d586a4566 100644 --- a/lib/Admin_Handler.cpp +++ b/lib/Admin_Handler.cpp @@ -2362,18 +2362,17 @@ bool admin_handler_command_load_or_save(char *query_no_space, unsigned int query // LOAD MCP QUERY RULES FROM DISK / TO MEMORY // Copies rules from persistent storage (disk.mcp_query_rules) to working memory (main.mcp_query_rules) if ( - (query_no_space_length == strlen("LOAD MCP QUERY RULES FROM DISK") && !strncasecmp("LOAD MCP RULES FROM DISK", query_no_space, query_no_space_length)) + (query_no_space_length == strlen("LOAD MCP QUERY RULES FROM DISK") && !strncasecmp("LOAD MCP QUERY RULES FROM DISK", query_no_space, query_no_space_length)) || (query_no_space_length == strlen("LOAD MCP QUERY RULES TO MEMORY") && !strncasecmp("LOAD MCP QUERY RULES TO MEMORY", query_no_space, query_no_space_length)) ) { - l_free(*ql,*q); - ProxySQL_Admin *SPA=(ProxySQL_Admin *)pa; // Execute as transaction to ensure both statements run atomically // Begin transaction if (!SPA->admindb->execute("BEGIN")) { proxy_error("Failed to BEGIN transaction for LOAD MCP QUERY RULES\n"); + SPA->send_error_msg_to_client(sess, (char *)"Failed to BEGIN transaction"); return false; } @@ -2381,6 +2380,7 @@ bool admin_handler_command_load_or_save(char *query_no_space, unsigned int query if (!SPA->admindb->execute("DELETE FROM main.mcp_query_rules")) { proxy_error("Failed to DELETE from main.mcp_query_rules\n"); SPA->admindb->execute("ROLLBACK"); + SPA->send_error_msg_to_client(sess, (char *)"Failed to DELETE from main.mcp_query_rules"); return false; } @@ -2388,16 +2388,20 @@ bool admin_handler_command_load_or_save(char *query_no_space, unsigned int query if (!SPA->admindb->execute("INSERT OR REPLACE INTO main.mcp_query_rules SELECT * FROM disk.mcp_query_rules")) { proxy_error("Failed to INSERT into main.mcp_query_rules\n"); SPA->admindb->execute("ROLLBACK"); + SPA->send_error_msg_to_client(sess, (char *)"Failed to INSERT into main.mcp_query_rules"); return false; } // Commit transaction if (!SPA->admindb->execute("COMMIT")) { proxy_error("Failed to COMMIT transaction for LOAD MCP QUERY RULES\n"); + SPA->send_error_msg_to_client(sess, (char *)"Failed to COMMIT transaction"); return false; } - return true; + proxy_info("Received %s command\n", query_no_space); + SPA->send_ok_msg_to_client(sess, NULL, 0, query_no_space); + return false; } // SAVE MCP QUERY RULES TO DISK @@ -2405,14 +2409,13 @@ bool admin_handler_command_load_or_save(char *query_no_space, unsigned int query if ( (query_no_space_length == strlen("SAVE MCP QUERY RULES TO DISK") && !strncasecmp("SAVE MCP QUERY RULES TO DISK", query_no_space, query_no_space_length)) ) { - l_free(*ql,*q); - ProxySQL_Admin *SPA=(ProxySQL_Admin *)pa; // Execute as transaction to ensure both statements run atomically // Begin transaction if (!SPA->admindb->execute("BEGIN")) { proxy_error("Failed to BEGIN transaction for SAVE MCP QUERY RULES TO DISK\n"); + SPA->send_error_msg_to_client(sess, (char *)"Failed to BEGIN transaction"); return false; } @@ -2420,6 +2423,7 @@ bool admin_handler_command_load_or_save(char *query_no_space, unsigned int query if (!SPA->admindb->execute("DELETE FROM disk.mcp_query_rules")) { proxy_error("Failed to DELETE from disk.mcp_query_rules\n"); SPA->admindb->execute("ROLLBACK"); + SPA->send_error_msg_to_client(sess, (char *)"Failed to DELETE from disk.mcp_query_rules"); return false; } @@ -2427,16 +2431,20 @@ bool admin_handler_command_load_or_save(char *query_no_space, unsigned int query if (!SPA->admindb->execute("INSERT OR REPLACE INTO disk.mcp_query_rules SELECT * FROM main.mcp_query_rules")) { proxy_error("Failed to INSERT into disk.mcp_query_rules\n"); SPA->admindb->execute("ROLLBACK"); + SPA->send_error_msg_to_client(sess, (char *)"Failed to INSERT into disk.mcp_query_rules"); return false; } // Commit transaction if (!SPA->admindb->execute("COMMIT")) { proxy_error("Failed to COMMIT transaction for SAVE MCP QUERY RULES TO DISK\n"); + SPA->send_error_msg_to_client(sess, (char *)"Failed to COMMIT transaction"); return false; } - return true; + proxy_info("Received %s command\n", query_no_space); + SPA->send_ok_msg_to_client(sess, NULL, 0, query_no_space); + return false; } // SAVE MCP QUERY RULES FROM RUNTIME / TO MEMORY diff --git a/lib/ProxySQL_Admin_Stats.cpp b/lib/ProxySQL_Admin_Stats.cpp index 29ea8a74f..f8e0bbfc1 100644 --- a/lib/ProxySQL_Admin_Stats.cpp +++ b/lib/ProxySQL_Admin_Stats.cpp @@ -2683,18 +2683,25 @@ void ProxySQL_Admin::stats___mcp_query_rules() { statsdb->execute("BEGIN"); statsdb->execute("DELETE FROM stats_mcp_query_rules"); - char* a = (char*)"INSERT INTO stats_mcp_query_rules VALUES (\"%s\",\"%s\")"; + // Use prepared statement to prevent SQL injection + const char* query_str = "INSERT INTO stats_mcp_query_rules VALUES (?1, ?2)"; + sqlite3_stmt* statement = nullptr; + int rc = statsdb->prepare_v2(query_str, &statement); + ASSERT_SQLITE_OK(rc, statsdb); + for (std::vector::iterator it = resultset->rows.begin(); it != resultset->rows.end(); ++it) { SQLite3_row* r = *it; - int arg_len = 0; - for (int i = 0; i < 2; i++) { - arg_len += strlen(r->fields[i]); - } - char* query = (char*)malloc(strlen(a) + arg_len + 32); - sprintf(query, a, r->fields[0], r->fields[1]); - statsdb->execute(query); - free(query); + + // Bind both columns using positional parameters + rc = (*proxy_sqlite3_bind_text)(statement, 1, r->fields[0], -1, SQLITE_TRANSIENT); ASSERT_SQLITE_OK(rc, statsdb); + rc = (*proxy_sqlite3_bind_text)(statement, 2, r->fields[1], -1, SQLITE_TRANSIENT); ASSERT_SQLITE_OK(rc, statsdb); + + SAFE_SQLITE3_STEP2(statement); + rc = (*proxy_sqlite3_clear_bindings)(statement); ASSERT_SQLITE_OK(rc, statsdb); + rc = (*proxy_sqlite3_reset)(statement); ASSERT_SQLITE_OK(rc, statsdb); } + + (*proxy_sqlite3_finalize)(statement); statsdb->execute("COMMIT"); delete resultset; }