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

Fix critical double-free bug, SQL injection vulnerability, and hardcoded path

Browse Source 
This commit addresses three issues identified by code review:

1. CRITICAL: Fix double-free bug in MCP server restart logic
   - Remove manual handler deletions in Admin_FlushVariables.cpp
   - ProxySQL_MCP_Server destructor already properly cleans up all handlers
   - Previously caused crashes when toggling SSL mode or changing port
   - Simplified restart: delete server (destructor cleanup) → create new server
   - Verified with 10+ rapid SSL toggles without crashes

2. HIGH: Fix SQL injection vulnerability in catalog search
   - Rewrite MySQL_Catalog::search() to use prepared statements
   - Use parameter binding (proxy_sqlite3_bind_text/bind_int) for user input
   - Escape single quotes in FTS5 MATCH clause (doesn't support parameters)
   - Tested against multiple injection attempts (single quote, backslash,
     comments, UNION SELECT, kind/tags parameter injection)
   - All 21 catalog tests still pass with new implementation

3. MEDIUM: Fix hardcoded user-specific path in config
   - Revert datadir from user-specific absolute path to /var/lib/proxysql
   - Ensures portability across different environments

Testing:
- SSL toggle: 7 tests passed (HTTP↔HTTPS, port changes, stress test)
- SQL injection: 10 tests passed (various injection attempts blocked)
- Catalog functionality: 21 tests passed (FTS5, BM25 ranking, etc.)
- Total: 38 tests passed, 0 failed

Fixes issues identified in GitHub PR #16 review.
pull/5310/head
Rahim Kanji 4 months ago
parent f7397f633c
commit 9b66224df1
2 changed files with 80 additions and 100 deletions
Show Stats Download Patch File Download Diff File

84
lib/Admin_FlushVariables.cpp
Unescape View File

@ -1612,83 +1612,27 @@ void ProxySQL_Admin::flush_mcp_variables___runtime_to_database(SQLite3DB* db, bo
}
} else {
// Server is already running - need to stop, delete server, and recreate everything
proxy_info("MCP: Server already running, reinitializing MySQL tool handler\n");
proxy_info("MCP: Server already running, reinitializing\n");
// 1. Delete Query_Tool_Handler first (server destructor doesn't delete this)
if (GloMCPH->query_tool_handler) {
proxy_info("MCP: Deleting old Query Tool Handler\n");
delete GloMCPH->query_tool_handler;
GloMCPH->query_tool_handler = NULL;
}
// 2. Stop and delete the server (server destructor also deletes MySQL_Tool_Handler)
// Delete the old server - its destructor will clean up all handlers
// (mysql_tool_handler, config_tool_handler, query_tool_handler,
// admin_tool_handler, cache_tool_handler, observe_tool_handler)
proxy_info("MCP: Stopping and deleting old server\n");
delete GloMCPH->mcp_server;
GloMCPH->mcp_server = NULL;
// Note: mysql_tool_handler is already deleted by server destructor and set to NULL
// All handlers are now deleted and set to NULL by the destructor
proxy_info("MCP: Old server deleted\n");
// 3. Delete other handlers that were created by old server
// The server destructor doesn't clean these up, so we need to do it manually
if (GloMCPH->config_tool_handler) {
delete GloMCPH->config_tool_handler;
GloMCPH->config_tool_handler = NULL;
}
if (GloMCPH->admin_tool_handler) {
delete GloMCPH->admin_tool_handler;
GloMCPH->admin_tool_handler = NULL;
}
if (GloMCPH->cache_tool_handler) {
delete GloMCPH->cache_tool_handler;
GloMCPH->cache_tool_handler = NULL;
}
if (GloMCPH->observe_tool_handler) {
delete GloMCPH->observe_tool_handler;
GloMCPH->observe_tool_handler = NULL;
}
// 4. Create new MySQL_Tool_Handler with current configuration
proxy_info("MCP: Creating new MySQL Tool Handler with updated configuration\n");
GloMCPH->mysql_tool_handler = new MySQL_Tool_Handler(
GloMCPH->variables.mcp_mysql_hosts ? GloMCPH->variables.mcp_mysql_hosts : "",
GloMCPH->variables.mcp_mysql_ports ? GloMCPH->variables.mcp_mysql_ports : "",
GloMCPH->variables.mcp_mysql_user ? GloMCPH->variables.mcp_mysql_user : "",
GloMCPH->variables.mcp_mysql_password ? GloMCPH->variables.mcp_mysql_password : "",
GloMCPH->variables.mcp_mysql_schema ? GloMCPH->variables.mcp_mysql_schema : "",
GloMCPH->variables.mcp_catalog_path ? GloMCPH->variables.mcp_catalog_path : ""
);
if (GloMCPH->mysql_tool_handler->init() != 0) {
proxy_error("MCP: Failed to initialize new MySQL Tool Handler\n");
delete GloMCPH->mysql_tool_handler;
GloMCPH->mysql_tool_handler = NULL;
} else {
proxy_info("MCP: New MySQL Tool Handler initialized successfully\n");
// 5. Create new Query_Tool_Handler that wraps the new MySQL_Tool_Handler
GloMCPH->query_tool_handler = new Query_Tool_Handler(GloMCPH->mysql_tool_handler);
if (GloMCPH->query_tool_handler->init() != 0) {
proxy_error("MCP: Failed to initialize new Query Tool Handler\n");
delete GloMCPH->query_tool_handler;
GloMCPH->query_tool_handler = NULL;
} else {
proxy_info("MCP: New Query Tool Handler initialized successfully\n");
}
}
// 6. Create and start new server (which will recreate all handlers including config/admin/cache/observe)
if (GloMCPH->mysql_tool_handler && GloMCPH->query_tool_handler) {
proxy_info("MCP: Creating and starting new server\n");
int port = GloMCPH->variables.mcp_port;
GloMCPH->mcp_server = new ProxySQL_MCP_Server(port, GloMCPH);
if (GloMCPH->mcp_server) {
GloMCPH->mcp_server->start();
proxy_info("MCP: New server created and started successfully\n");
} else {
proxy_error("MCP: Failed to create new server instance\n");
}
// Create and start new server with current configuration
// The server constructor will recreate all handlers with updated settings
proxy_info("MCP: Creating and starting new server\n");
int port = GloMCPH->variables.mcp_port;
GloMCPH->mcp_server = new ProxySQL_MCP_Server(port, GloMCPH);
if (GloMCPH->mcp_server) {
GloMCPH->mcp_server->start();
proxy_info("MCP: New server created and started successfully\n");
} else {
proxy_error("MCP: Server not created due to handler initialization failure\n");
proxy_error("MCP: Failed to create new server instance\n");
}
}
} else {

96
lib/MySQL_Catalog.cpp
Unescape View File

@ -194,57 +194,93 @@ std::string MySQL_Catalog::search(
return "[]";
}
std::ostringstream sql;
char* error = NULL;
int cols = 0, affected = 0;
SQLite3_result* resultset = NULL;
// Helper lambda to escape single quotes for SQLite SQL literals
auto escape_sql = [](const std::string& str) -> std::string {
std::string result;
result.reserve(str.length() * 2); // Reserve space for potential escaping
for (char c : str) {
if (c == '\'') {
result += '\''; // Escape single quote by doubling it
}
result += c;
}
return result;
};
// FTS5 search with BM25 ranking
// Escape query for use in FTS5 MATCH (MATCH doesn't support parameter binding)
std::string escaped_query = escape_sql(query);
// Build SQL query with placeholders for parameters
std::ostringstream sql;
sql << "SELECT c.kind, c.key, c.document, c.tags, c.links "
<< "FROM catalog c "
<< "INNER JOIN catalog_fts f ON c.id = f.rowid "
<< "WHERE catalog_fts MATCH '" << query << "'";
<< "WHERE catalog_fts MATCH '" << escaped_query << "'";
// Add kind filter
int param_count = 1; // Track parameter binding position
// Add kind filter with parameter placeholder
if (!kind.empty()) {
sql << " AND c.kind = '" << kind << "'";
sql << " AND c.kind = ?";
}
// Add tags filter
// Add tags filter with parameter placeholder
if (!tags.empty()) {
sql << " AND c.tags LIKE '%" << tags << "%'";
sql << " AND c.tags LIKE ?";
}
// Order by relevance (BM25) and recency
sql << " ORDER BY bm25(f) ASC, c.updated_at DESC LIMIT " << limit << " OFFSET " << offset;
sql << " ORDER BY bm25(f) ASC, c.updated_at DESC LIMIT ? OFFSET ?";
db->execute_statement(sql.str().c_str(), &error, &cols, &affected, &resultset);
if (error) {
proxy_error("Catalog search error: %s\n", error);
// Prepare the statement
sqlite3_stmt* stmt = NULL;
int rc = db->prepare_v2(sql.str().c_str(), &stmt);
if (rc != SQLITE_OK) {
proxy_error("Catalog search: Failed to prepare statement: %d\n", rc);
return "[]";
}
// Build JSON result
// Bind parameters
param_count = 1;
if (!kind.empty()) {
(*proxy_sqlite3_bind_text)(stmt, param_count++, kind.c_str(), -1, SQLITE_TRANSIENT);
}
if (!tags.empty()) {
// Add wildcards for LIKE search
std::string tags_pattern = "%" + tags + "%";
(*proxy_sqlite3_bind_text)(stmt, param_count++, tags_pattern.c_str(), -1, SQLITE_TRANSIENT);
}
(*proxy_sqlite3_bind_int)(stmt, param_count++, limit);
(*proxy_sqlite3_bind_int)(stmt, param_count, offset);
// Execute query and build JSON result
std::ostringstream json;
json << "[";
bool first = true;
if (resultset) {
for (std::vector<SQLite3_row*>::iterator it = resultset->rows.begin();
it != resultset->rows.end(); ++it) {
SQLite3_row* row = *it;
if (!first) json << ",";
first = false;
while ((rc = (*proxy_sqlite3_step)(stmt)) == SQLITE_ROW) {
if (!first) json << ",";
first = false;
const char* kind_val = (const char*)(*proxy_sqlite3_column_text)(stmt, 0);
const char* key_val = (const char*)(*proxy_sqlite3_column_text)(stmt, 1);
const char* doc_val = (const char*)(*proxy_sqlite3_column_text)(stmt, 2);
const char* tags_val = (const char*)(*proxy_sqlite3_column_text)(stmt, 3);
const char* links_val = (const char*)(*proxy_sqlite3_column_text)(stmt, 4);
json << "{"
<< "\"kind\":\"" << (kind_val ? kind_val : "") << "\","
<< "\"key\":\"" << (key_val ? key_val : "") << "\","
<< "\"document\":" << (doc_val ? doc_val : "null") << ","
<< "\"tags\":\"" << (tags_val ? tags_val : "") << "\","
<< "\"links\":\"" << (links_val ? links_val : "") << "\""
<< "}";
}
json << "{"
<< "\"kind\":\"" << (row->fields[0] ? row->fields[0] : "") << "\","
<< "\"key\":\"" << (row->fields[1] ? row->fields[1] : "") << "\","
<< "\"document\":" << (row->fields[2] ? row->fields[2] : "null") << ","
<< "\"tags\":\"" << (row->fields[3] ? row->fields[3] : "") << "\","
<< "\"links\":\"" << (row->fields[4] ? row->fields[4] : "") << "\""
<< "}";
}
delete resultset;
(*proxy_sqlite3_finalize)(stmt);
if (rc != SQLITE_DONE && rc != SQLITE_ROW) {
proxy_error("Catalog search: Error executing query: %d\n", rc);
}
json << "]";

Write Preview
Loading…
Cancel
Save