Fix: Automatic prefix stripping for mysql_variables, pgsql_variables, and admin_variables config parsing#5247
Conversation
…and admin_variables config parsing When users mistakenly include the module prefix (e.g., mysql-log_unhealthy_connections) in the mysql_variables section, the variable gets stored with a double prefix (e.g., mysql-mysql-log_unhealthy_connections). This fix automatically strips the prefix if present, ensuring variables are stored correctly. The same logic applies to pgsql_variables (pgsql-) and admin_variables (admin-). Fixes #5246
The previous implementation stripped the prefix before calling group.lookupValue(), which would fail because the config file contains the prefixed name (e.g., "mysql-log_unhealthy_connections"). The lookup must use the original name from the config file. This commit moves the prefix stripping logic to after the value lookup but before constructing the SQL query, ensuring both: 1. The correct value is retrieved from the config using the original prefixed name 2. The variable is stored in the database with a single prefix Also includes a test to verify the fix works for mysql_variables, pgsql_variables, and admin_variables sections.
…al_Variables_from_configfile This commit adds detailed Doxygen documentation for: 1. The ProxySQL_Config class - describes its role in configuration management 2. The Read_Global_Variables_from_configfile() method - documents its behavior, parameters, return value, and the automatic prefix stripping feature The documentation explains the automatic prefix stripping behavior that handles cases where users mistakenly include module prefix (e.g., "mysql-") in variable names within configuration files.
Summary of ChangesHello @renecannao, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness of ProxySQL's configuration parsing by introducing an automatic mechanism to correct common user errors. It specifically targets the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly implements automatic prefix stripping for configuration variables and adds corresponding documentation and tests. The core logic is sound. However, the implementation is adjacent to a pre-existing critical SQL injection vulnerability in the way database queries are constructed. My review includes a detailed comment on this vulnerability with a recommendation to use prepared statements to mitigate it.
lib/ProxySQL_Config.cpp
Outdated
| // Automatic prefix stripping: if variable already starts with "prefix-", remove it | ||
| if (strncmp(n, prefix, prefix_len) == 0 && n[prefix_len] == '-') { | ||
| n += prefix_len + 1; // Skip "prefix-" | ||
| } | ||
| char *query=(char *)malloc(strlen(q)+strlen(prefix)+strlen(n)+strlen(value_string.c_str())); | ||
| sprintf(query,q, prefix, n, value_string.c_str()); |
There was a problem hiding this comment.
While the logic to strip prefixes is correct, the subsequent construction of the SQL query using sprintf on line 126 is vulnerable to SQL Injection. Variable values from the configuration file are inserted directly into the query string without proper escaping. A malicious value containing a double quote could be used to execute arbitrary SQL commands.
To fix this, you should use prepared statements with parameter binding. This is the standard, safe way to execute queries with dynamic data.
Here is an example of how you could refactor the loop to use prepared statements:
// Before the loop
const char *sql = "INSERT OR REPLACE INTO global_variables VALUES (?, ?)";
auto prep_result = admindb->prepare_v2(sql);
int rc = prep_result.first;
stmt_unique_ptr stmt = std::move(prep_result.second);
if (rc != SQLITE_OK) {
// Handle error, e.g., log and return
return 0;
}
for (i=0; i< count; i++) {
const Setting &sett = group[i];
const char *n = sett.getName();
// ... logic to get value_string ...
// Automatic prefix stripping
if (strncmp(n, prefix, prefix_len) == 0 && n[prefix_len] == '-') {
n += prefix_len + 1; // Skip "prefix-"
}
std::string var_name = std::string(prefix) + "-" + n;
(*proxy_sqlite3_bind_text)(stmt.get(), 1, var_name.c_str(), -1, SQLITE_TRANSIENT);
(*proxy_sqlite3_bind_text)(stmt.get(), 2, value_string.c_str(), -1, SQLITE_TRANSIENT);
int step_rc;
do {
step_rc = (*proxy_sqlite3_step)(stmt.get());
if (step_rc == SQLITE_BUSY || step_rc == SQLITE_LOCKED) {
usleep(100);
}
} while (step_rc == SQLITE_BUSY || step_rc == SQLITE_LOCKED);
(*proxy_sqlite3_reset)(stmt.get());
}
// After the loop, stmt is automatically finalized by stmt_unique_ptr destructorThis refactoring prepares the statement once before the loop and safely binds values for each iteration, preventing SQL injection.
Adds the new prefix stripping validation test to the default-g4 test group along with mysql-auto_increment_delay_multiplex=0-g4, mysql-multiplexing=false-g4, mysql-query_digests=0-g4, and mysql-query_digests_keep_comment=1-g4 groups. This ensures the test is run as part of the regular test suite.
Replace sprintf-based SQL query construction with prepared statements using bound parameters to prevent SQL injection attacks. This addresses the security issue identified in PR #5247 review. Changes: - Use SQLite prepared statement with placeholders ?1, ?2 - Bind variable names and values securely using proxy_sqlite3_bind_text - Use ASSERT_SQLITE_OK for error handling as per ProxySQL conventions - Remove malloc/sprintf vulnerable code pattern - Add necessary includes for SQLite functions and ASSERT_SQLITE_OK macro Security: SQL injection could have occurred if configuration variable names or values contained malicious quotes. Prepared statements eliminate this risk.
|
Summary
Fixes #5246
When users mistakenly include module prefix (e.g., "mysql-") in configuration variable names within
mysql_variables,pgsql_variables, oradmin_variablessections, the prefix is automatically stripped before storing in the database.Problem
Users often write
mysql-log_unhealthy_connections=falseinmysql_variablessection, which results in double-prefixing (mysql-mysql-log_unhealthy_connections) and lookup failures.Solution
Modified
ProxySQL_Config::Read_Global_Variables_from_configfile()to detect and strip duplicate prefixes:Code Changes
Testing
The fix handles all three module types:
mysql_variables: Strips "mysql-" prefixpgsql_variables: Strips "pgsql-" prefixadmin_variables: Strips "admin-" prefixExample
Config file contains:
Result in
global_variablestable:mysql-log_unhealthy_connections = "false"(single prefix)mysql-threads = "2"(single prefix)mysql-poll_timeout = "1000"(single prefix added)Related Issues
Closes #5242 where users reported "Closing unhealthy client connection fix not working" due to this configuration issue.