MDEV-40027 : Galera Cluster-peer > Donor command execution#5234
Open
janlindstrom wants to merge 2 commits into
Open
MDEV-40027 : Galera Cluster-peer > Donor command execution#5234janlindstrom wants to merge 2 commits into
janlindstrom wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces input validation for the wsrep_sst_auth variable to prevent shell injection, along with corresponding test cases. However, several issues were identified in the review: the shell_char function uses a blacklist approach that is vulnerable to command injection and should be replaced with a strict whitelist; the safe function in the mariabackup script is incorrectly passed the variable name instead of its value; and multiple temporary debug logs (JAN:JAN) need to be removed before merging.
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.
janlindstrom
force-pushed
the
10.6-MDEV-40027
branch
2 times, most recently
from
6599821 to
c03f58c
Compare
hemantdangi-gc
requested changes
Jun 16, 2026
janlindstrom
force-pushed
the
10.6-MDEV-40027
branch
2 times, most recently
from
7312085 to
d53a22a
Compare
An appropriately privileged user (with SUPER privileges) could execute shell commands as the uid of the mariadbd process because the values of the system variable wsrep_sst_auth, which can be modified at runtime, were not properly sanitized when used to construct a shell command. Combined rsync and mariabackup test cases and added test case for incorrect values for wsrep_sst_auth.
Fix potential issue with wsrep_node_address by allowing only correctly constructed address.
janlindstrom
force-pushed
the
10.6-MDEV-40027
branch
from
d53a22a to
a5a2571
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses a Galera SST command-injection risk by adding server-side validation of wsrep-related dynamic variables (notably wsrep_sst_auth) and tightening how SST scripts consume user-provided values, alongside updating/reshaping regression tests to cover unsafe inputs.
Changes:
- Centralizes/introduces wsrep string-validation helpers (
wsrep_check_request_str, character allowlists) and applies them to wsrep node/SST variable checks and donor request parsing. - Hardens mariabackup SST script argument construction by sanitizing the
--uservalue viasafe. - Updates and restructures MTR coverage for unsafe wsrep values and CN/remote-auth injection scenarios (including new combinations coverage).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sql/wsrep_var.cc | Adds character-level validation for wsrep_node_address and adjusts buffer safety for cluster address checks. |
| sql/wsrep_utils.h | Exposes new wsrep validation helper prototypes. |
| sql/wsrep_utils.cc | Implements shared character-check helpers and a generic request-string validator. |
| sql/wsrep_sst.cc | Replaces local validators with shared helpers; adds wsrep_sst_auth validation and remote-auth validation in donate path. |
| scripts/wsrep_sst_mariabackup.sh | Sanitizes WSREP_SST_OPT_USER before embedding into mariadb-backup command args. |
| mysql-test/suite/sys_vars/t/wsrep_sst_auth_basic.test | Adds negative tests for unsafe wsrep_sst_auth values and suppression for new warning text. |
| mysql-test/suite/sys_vars/r/wsrep_sst_auth_basic.result | Updates expected output for the added wsrep_sst_auth negative tests. |
| mysql-test/suite/sys_vars/t/wsrep_node_address_basic.test | Adds negative tests for unsafe wsrep_node_address characters. |
| mysql-test/suite/sys_vars/r/wsrep_node_address_basic.result | Updates expected output for the added wsrep_node_address negative tests. |
| mysql-test/suite/galera/t/galera_sst_rsync_encrypt_with_key_server.test | Removed legacy rsync-only CN injection regression test. |
| mysql-test/suite/galera/r/galera_sst_rsync_encrypt_with_key_server.result | Removes corresponding expected output for the deleted test. |
| mysql-test/suite/galera/t/galera_sst_mariabackup_encrypt_with_key_server.cnf | Removes legacy mariabackup-only config in favor of combination coverage. |
| mysql-test/suite/galera/t/galera_sst_cn_injection.test | Updates CN/remote-auth injection test to assert new server-side rejection behavior/logs. |
| mysql-test/suite/galera/t/galera_sst_cn_injection.combinations | Adds combinations to run CN injection test with both rsync and mariabackup SST methods. |
| mysql-test/suite/galera/t/galera_sst_cn_injection.cnf | Adjusts config to rely on combinations for SST method selection. |
| mysql-test/suite/galera/r/galera_sst_cn_injection.result | Adds expected output for the updated CN injection test. |
Comments suppressed due to low confidence (1)
mysql-test/suite/galera/t/galera_sst_cn_injection.test:26
- The suppression pattern
".*State transfer to:*"is not doing what it looks like: in a regex,*repeats the preceding token (:), so this only matches zero-or-more colons afterto, not arbitrary text. This can lead to test flakes due to unsuppressed error-log lines.
Use .* to match arbitrary characters (or match the known message more precisely).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+604
to
+607
| bool wsrep_filename_char(int const c) | ||
| { | ||
| return isalnum(c) || (c == '-') || (c == '_') || (c == '.'); | ||
| } |
Comment on lines
+609
to
+613
| /* return true if string is comma seprated list */ | ||
| bool wsrep_comma_char(int const c) | ||
| { | ||
| return (c == ','); | ||
| } |
Comment on lines
45
to
46
| # The values being assigned to wsrep_node_address are not verified so the | ||
| # following alues are currently valid too. |
Comment on lines
+622
to
+625
| bool wsrep_shell_char(int const c) | ||
| { | ||
| return (c != '`') && (c != '\'') && (c != '$') && (c != ' '); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An appropriately privileged user (with SUPER privileges) could execute shell commands as the uid of the mariadbd process because the values of the system variable wsrep_sst_auth, which can be modified at runtime, were not properly sanitized when used to construct a shell command.