MDEV-40151: Galera SST shell command injection via path parameters#5283
MDEV-40151: Galera SST shell command injection via path parameters#5283hemantdangi-gc wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances security in Galera SST by validating directory and path parameters (such as datadir and innodb-data-home-dir) against shell-unsafe characters in both C++ and Bash before they are interpolated into shell commands. Feedback on these changes highlights a discrepancy between C++ and Bash validation where Bash's safe_path rejects UTF-8 characters allowed by C++, potentially breaking SST for non-ASCII paths. Additionally, potential buffer truncation issues were identified in snprintf formatting for ib_data_home_opt which could result in missing closing single quotes and command execution failures.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| safe_path() | ||
| { | ||
| if [[ "${!1}" = *[!a-zA-Z0-9/\\._:\ +=,@%-]* ]]; then | ||
| wsrep_log_error "Invalid characters in $1: ${!1}" | ||
| exit 22 | ||
| fi | ||
| echo "${!1}" | ||
| } |
There was a problem hiding this comment.
There is a discrepancy between the C++ validation (wsrep_path_char) and the Bash validation (safe_path).
In C++, wsrep_path_char explicitly allows all bytes >= 0x80 (which covers UTF-8 multi-byte characters) to support non-ASCII paths:
if (c >= 0x80) return true;However, the Bash safe_path function uses a strict ASCII-only character class [!a-zA-Z0-9/\\._:\\ +=,@%-]. Any byte >= 0x80 will be matched by this negated class, causing safe_path to reject the path and abort the SST with exit code 22. This will break Galera SST for any database cluster where paths (such as datadir, innodb-data-home-dir, etc.) contain non-ASCII/UTF-8 characters.
To fix this, we should allow bytes in the range 0x80-0xFF in the Bash character class, and set LC_ALL=C locally within the function to ensure consistent byte-based range matching across different system locales.
| safe_path() | |
| { | |
| if [[ "${!1}" = *[!a-zA-Z0-9/\\._:\ +=,@%-]* ]]; then | |
| wsrep_log_error "Invalid characters in $1: ${!1}" | |
| exit 22 | |
| fi | |
| echo "${!1}" | |
| } | |
| safe_path() | |
| { | |
| local LC_ALL=C | |
| if [[ "${!1}" = *[!a-zA-Z0-9/\\._:\\ +=,@%-$'\x80'-$'\xFF'-]* ]]; then | |
| wsrep_log_error "Invalid characters in $1: ${!1}" | |
| exit 22 | |
| fi | |
| echo "${!1}" | |
| } |
| char ib_data_home_opt[FN_REFLEN + 32]; | ||
| ib_data_home_opt[0]= '\0'; | ||
| if (data_home_dir) | ||
| snprintf(ib_data_home_opt, sizeof(ib_data_home_opt), | ||
| WSREP_SST_OPT_INNODB_DATA_HOME_DIR " '%s' ", data_home_dir); |
There was a problem hiding this comment.
The stack buffer ib_data_home_opt has a fixed size of FN_REFLEN + 32. If data_home_dir is very long (e.g., close to FN_REFLEN or up to PATH_MAX on systems where paths can be longer than 512 characters), snprintf will truncate the formatted string.
Because the path is interpolated single-quoted ('%s'), truncation will result in a missing closing single quote ' at the end of the option. This will cause the shell to fail to parse the command, leading to a mysterious SST failure.
We should check the return value of snprintf to detect truncation and return an error if the path is too long to fit.
char ib_data_home_opt[FN_REFLEN + 32];
ib_data_home_opt[0]= '\0';
if (data_home_dir)
{
int printed= snprintf(ib_data_home_opt, sizeof(ib_data_home_opt),
WSREP_SST_OPT_INNODB_DATA_HOME_DIR " '%s' ", data_home_dir);
if (printed < 0 || (size_t)printed >= sizeof(ib_data_home_opt))
{
WSREP_ERROR("Refusing SST: innodb-data-home-dir path is too long.");
return -EINVAL;
}
}| char ib_data_home_opt[FN_REFLEN + 32]; | ||
| ib_data_home_opt[0]= '\0'; | ||
| if (data_home_dir) | ||
| snprintf(ib_data_home_opt, sizeof(ib_data_home_opt), | ||
| WSREP_SST_OPT_INNODB_DATA_HOME_DIR " '%s' ", data_home_dir); |
There was a problem hiding this comment.
Similar to the check in sst_prepare_other, we should check the return value of snprintf here to detect truncation of ib_data_home_opt and return an error if the path is too long to fit.
char ib_data_home_opt[FN_REFLEN + 32];
ib_data_home_opt[0]= '\0';
if (data_home_dir)
{
int printed= snprintf(ib_data_home_opt, sizeof(ib_data_home_opt),
WSREP_SST_OPT_INNODB_DATA_HOME_DIR " '%s' ", data_home_dir);
if (printed < 0 || (size_t)printed >= sizeof(ib_data_home_opt))
{
WSREP_ERROR("Refusing SST: innodb-data-home-dir path is too long.");
return -EINVAL;
}
}Issue: The datadir and InnoDB/Aria path parameters were interpolated single-quoted into shell commands run via sh -c (mysqld) and eval (rsync FILTER, mariabackup INNOEXTRA) without validation, so a single quote in a value broke out of the quoting and ran arbitrary commands as the mysql OS user. Solution: Add allowlist validators wsrep_path_char() (C++) and safe_path() (shell) that permit path characters incl. spaces but reject shell-breakout characters, and validate every such value before it reaches the command/eval. innodb-data-home-dir is now also passed on the SST command line (top priority) with env as fallback.
b475ac5 to
d5abe6d
Compare
Issue:
The datadir and InnoDB/Aria path parameters were interpolated single-quoted into shell commands run via sh -c (mysqld) and eval (rsync FILTER, mariabackup INNOEXTRA) without validation, so a single quote in a value broke out of the quoting and ran arbitrary commands as the mysql OS user.
Solution:
Add allowlist validators wsrep_path_char() (C++) and safe_path() (shell) that permit path characters incl. spaces but reject shell-breakout characters, and validate every such value before it reaches the command/eval. innodb-data-home-dir is now also passed on the SST command line (top priority) with env as fallback.