MDEV-40151: Galera SST: InnoDB/Aria directory & buffer-pool parameters interpolated unsanitized into eval-ed shell commands - #5570
Open
hemantdangi-gc wants to merge 7 commits into
Conversation
| @@ -0,0 +1,6 @@ | |||
| !include ../galera_2nodes.cnf | |||
Contributor
There was a problem hiding this comment.
Can you explain why these test are run only for either rsync or mariabackup not both and especially not for mysqldump?
Contributor
Author
There was a problem hiding this comment.
Why rsync-only for some, mariabackup-only for others?
- galera_sst_datadir_spaces and galera_sst_rsyncd_conf_injection test FILTER (the rsync donor's transfer exclude-list) and the rsyncd.conf heredoc - code that only exists in wsrep_sst_rsync.sh. mariabackup and mysqldump have no equivalent construct, so there's nothing for those methods to exercise here.
- galera_sst_buffer_pool_injection tests INNOEXTRA/setup_commands, which is mariadb-backup-specific - innodb-buffer-pool-filename is only ever consumed by mariadb-backup; rsync and mysqldump never read it.
- The datadir_injection/mysqld_args_injection these two test C++ code (sst_prepare_other's datadir check, copy_orig_argv's escaping in sql/wsrep_sst.cc) that's shared verbatim between rsync and mariabackup - method is just a string substituted into the command template, the validation logic itself doesn't branch on it. The rsync was picked as one of representative method rather than duplicating the same C++-level test per script; re-running with mariabackup would exercise the identical check.
And for why mysqldump specifically is missing?
- sst_prepare_mysqldump() (the joiner side, used only for wsrep_sst_method=mysqldump) never builds a shell command at all: no sh -c, no --datadir, no copy_orig_argv. It just formats an addr:port string and starts a monitor thread. So all four of my reproduction tests, which put the malicious value on the joiner and force it to request SST, have nothing to exercise on mysqldump's joiner side - there's no sink there.
Issue: mysqld builds the wsrep_sst_<method> command line with datadir and the socket path single-quoted, then runs it via sh -c. A single quote in either value breaks out of the quoting and runs the remainder as a shell command. Solution: Add an allowlist validator, wsrep_path_char(), and reject datadir, the socket path, and the defaults-file/suffix values before the command line is built.
Issue: copy_orig_argv() appends mysqld's own arguments to the SST command line that is run via sh -c. A backquote only caused the argument to be enclosed in double quotes, which does not stop command substitution, so a backquote in any forwarded argument ran as a shell command. Solution: Escape the backquote with a backslash instead of relying on the enclosing double quotes, which also keeps it literal when the argument is not quoted.
Issue: INNOEXTRA embedded aria-log-dir-path, innodb-data-home-dir, innodb-log-group-home-dir, innodb-undo-directory, innodb-buffer-pool-filename and innodb-buffer-pool-size single-quoted into a string; INNOAPPLY/INNOMOVE/INNOBACKUP embedded that string and were run via eval. A single quote in any of the six values broke out of the quoting and ran as a shell command. Solution: Build INNOEXTRA and the mariadb-backup commands as bash arrays so each value is a literal argv element, and drop the eval from setup_commands and its callers. The forwarded --mysqld-args, the [sst] inno-*-opts, the defaults file and the tmpdir are converted the same way, since they are assembled in the same eval'd command and any eval left there would still be the injection mechanism the sink relies on. WSREP_SST_OPT_MYSQLD keeps its historical string format for other scripts that source wsrep_sst_common, now with embedded quotes escaped.
Issue: datadir and the InnoDB/Aria directory values are written unquoted into the generated rsyncd.conf as "path = <value>" lines. A directory name containing a newline splices in an extra rsync daemon directive. Solution: Reject a newline or carriage return in those values before writing the config file. The config format has no quoting, so this is checked directly rather than passed through a shell construct.
Issue: FILTER embedded directory values single-quoted into a string that was run via eval. On the donor these values always equal the datadir (create_dirs(), which resolves them from the individual InnoDB/Aria directory options, only runs on the joiner), so this is not independently reachable given the datadir check added for the mysqld sh -c sink, but the construct is still the same shell-injection pattern the SST scripts should not use anywhere. Solution: Build FILTER as a bash array so each value is a literal argv element, and call rsync directly instead of through eval.
Issue: sst_donate_other()'s shell-unsafe datadir/socket check (added in a prior commit) only had joiner-side test coverage; the donor path was untested. Solution: add a regression test that forces node_1 to act as donor with a malicious datadir, confirming sst_donate_other() rejects it.
Issue: sst_donate_mysqldump()'s shell-unsafe datadir/socket check has its own copy of the validation logic used elsewhere, but had no test coverage. Solution: add a regression test that forces node_1 to donate via mysqldump with a malicious datadir, confirming sst_donate_mysqldump() rejects it.
hemantdangi-gc
force-pushed
the
10.11-MDEV-40151
branch
from
August 19, 2026 06:30
5bf1621 to
f426ca1
Compare
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
3 participants
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.
Issue:
Datadir and InnoDB/Aria/buffer-pool parameters were interpolated single-quoted
into shell strings run via eval or sh -c, so a quote in any of them broke out
and ran as a shell command; a backquote in a forwarded mysqld argument was
insufficiently escaped for the same sh -c string, and a directory name with a
newline could inject a directive into the generated rsyncd.conf.
Solution:
Reject shell-unsafe datadir/socket values in C++ before the command line is
built, escape backquotes in forwarded arguments, build the rsync and
mariadb-backup commands from bash arrays instead of eval, and reject a
newline before it reaches rsyncd.conf.