aherman
/
proxysql
mirror of https://github.com/sysown/proxysql
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix: Address coderabbitai review - use-after-free, missing responses, SQL injection

Browse Source 
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)
pull/5310/head^2
Rene Cannao 1 month ago
parent 5ece563514
commit ffe5690360
2 changed files with 31 additions and 16 deletions
Show Stats Download Patch File Download Diff File

22
lib/Admin_Handler.cpp
Unescape View File

@ -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

25
lib/ProxySQL_Admin_Stats.cpp
Unescape View File

@ -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<SQLite3_row*>::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;
}

Write Preview
Loading…
Cancel
Save