From 3666388619b9f625be56ef8af8d7fd216111a69a Mon Sep 17 00:00:00 2001 From: Oliver Smith Date: Sat, 10 Mar 2018 22:58:39 +0000 Subject: [PATCH] Properly escape commands in pmb.chroot.user() (#1316) ## Introduction In #1302 we noticed that `pmb.chroot.user()` does not escape commands properly: When passing one string with spaces, it would pass them as two strings to the chroot. The use case is passing a description with a space inside to `newapkbuild` with `pmboostrap newapkbuild`. This is not a security issue, as we don't pass strings from untrusted input to this function. ## Functions for running commands in pmbootstrap To put the rest of the description in context: We have four high level functions that run commands: * `pmb.helpers.run.user()` * `pmb.helpers.run.root()` * `pmb.chroot.root()` * `pmb.chroot.user()` In addition, one low level function that the others invoke: * `pmb.helpers.run.core()` ## Flawed test case The issue described above did not get detected for so long, because we have a test case in place since day one, which verifies that all of the functions above escape everything properly: * `test/test_shell_escape.py` So the test case ran a given command through all these functions, and compared the result each time. However, `pmb.chroot.root()` modified the command variable (passed by reference) and did the escaping already, which means `pmb.chroot.user()` running directly afterwards only returns the right output when *not* doing any escaping. Without questioning the accuracy of the test case, I've escaped commands and environment variables with `shlex.quote()` *before* passing them to `pmb.chroot.user()`. In retrospective this does not make sense at all and is reverted with this commit. ## Environment variables By coincidence, we have only passed custom environment variables to `pmb.chroot.user()`, never to the other high level functions. This only worked, because we did not do any escaping and the passed line gets executed as shell command: ``` $ MYENV=test echo test2 test 2 ``` If it was properly escaped as one shell command: ``` $ 'MYENV=test echo test2' sh: MYENV=test echo test2: not found ``` So doing that clearly doesn't work anymore. I have added a new `env` parameter to `pmb.chroot.user()` (and to all other high level functions for consistency), where environment variables can be passed as a dictionary. Then the function knows what to do and we end up with properly escaped commands and environment variables. ## Details * Add new `env` parameter to all high level command execution functions * New `pmb.helpers.run.flat_cmd()` function, that takes a command as list and environment variables as dict, and creates a properly escaped flat string from the input. * Use that function for proper escaping in all high level exec funcs * Don't escape commands *before* passing them to `pmb.chroot.user()` * Describe parameters of the command execution functions * `pmbootstrap -v` writes the exact command to the log that was executed (in addition to the simplified form we always write down for readability) * `test_shell_escape.py`: verify that the command passed by reference has not been modified, add a new test for strings with spaces, add tests for new function `pmb.helpers.run.flat_cmd()` * Remove obsolete commend in `pmb.chroot.distccd` about environment variables, because we don't use any there anymore * Add `TERM=xterm` to default environment variables in the chroot, so running ncurses applications like `menuconfig` and `nano` works out of the box --- pmb/aportgen/device.py | 2 +- pmb/build/_package.py | 14 +++--- pmb/build/menuconfig.py | 12 ++--- pmb/build/other.py | 10 +++-- pmb/chroot/distccd.py | 3 +- pmb/chroot/root.py | 71 ++++++++++++++++-------------- pmb/chroot/user.py | 26 ++++++++--- pmb/helpers/run.py | 90 +++++++++++++++++++++++++++++++++----- test/test_build_package.py | 11 ++--- test/test_shell_escape.py | 84 +++++++++++++++++++++++++++++++---- 10 files changed, 236 insertions(+), 87 deletions(-) diff --git a/pmb/aportgen/device.py b/pmb/aportgen/device.py index 6e7c2eaa..fb76ea66 100644 --- a/pmb/aportgen/device.py +++ b/pmb/aportgen/device.py @@ -88,7 +88,7 @@ def ask_for_bootimg(args): while True: path = os.path.expanduser(pmb.helpers.cli.ask(args, "Path", None, "", False)) - if not len(path): + if not path: return None try: return pmb.parse.bootimg(args, path) diff --git a/pmb/build/_package.py b/pmb/build/_package.py index 49074f6a..4a9e34f7 100644 --- a/pmb/build/_package.py +++ b/pmb/build/_package.py @@ -19,7 +19,6 @@ along with pmbootstrap. If not, see . import datetime import logging import os -import shlex import pmb.build import pmb.build.autodetect @@ -305,7 +304,7 @@ def override_source(args, apkbuild, pkgver, src, suffix="native"): apkbuild_path = "/home/pmos/build/APKBUILD" shell_cmd = ("cat " + apkbuild_path + " " + append_path + " > " + append_path + "_") - pmb.chroot.user(args, ["sh", "-c", shlex.quote(shell_cmd)], suffix) + pmb.chroot.user(args, ["sh", "-c", shell_cmd], suffix) pmb.chroot.user(args, ["mv", append_path + "_", apkbuild_path], suffix) @@ -352,10 +351,7 @@ def run_abuild(args, apkbuild, arch, strict=False, force=False, cross=None, env["DISTCC_HOSTS"] = "127.0.0.1:" + args.port_distccd # Build the abuild command - cmd = [] - for key, value in env.items(): - cmd += [key + "=" + shlex.quote(value)] - cmd += ["abuild"] + cmd = ["abuild"] if strict: cmd += ["-r"] # install depends with abuild else: @@ -366,7 +362,7 @@ def run_abuild(args, apkbuild, arch, strict=False, force=False, cross=None, # Copy the aport to the chroot and build it pmb.build.copy_to_buildpath(args, apkbuild["pkgname"], suffix) override_source(args, apkbuild, pkgver, src, suffix) - pmb.chroot.user(args, cmd, suffix, "/home/pmos/build") + pmb.chroot.user(args, cmd, suffix, "/home/pmos/build", env=env) return (output, cmd, env) @@ -388,8 +384,8 @@ def finish(args, apkbuild, arch, output, strict=False, suffix="native"): # Uninstall build dependencies (strict mode) if strict: logging.info("(" + suffix + ") uninstall build dependencies") - cmd = ["SUDO_APK='abuild-apk --no-progress'", "abuild", "undeps"] - pmb.chroot.user(args, cmd, suffix, "/home/pmos/build") + pmb.chroot.user(args, ["abuild", "undeps"], suffix, "/home/pmos/build", + env={"SUDO_APK": "abuild-apk --no-progress"}) def package(args, pkgname, arch=None, force=False, strict=False, diff --git a/pmb/build/menuconfig.py b/pmb/build/menuconfig.py index aca7740b..c7fe4b51 100644 --- a/pmb/build/menuconfig.py +++ b/pmb/build/menuconfig.py @@ -78,17 +78,13 @@ def menuconfig(args, pkgname): logging.info("(native) extract kernel source") pmb.chroot.user(args, ["abuild", "unpack"], "native", "/home/pmos/build") logging.info("(native) apply patches") - pmb.chroot.user(args, ["CARCH=" + arch, "abuild", "prepare"], "native", - "/home/pmos/build", log=False) + pmb.chroot.user(args, ["abuild", "prepare"], "native", + "/home/pmos/build", log=False, env={"CARCH": arch}) # Run abuild menuconfig - cmd = [] - environment = {"CARCH": arch, "TERM": "xterm"} - for key, value in environment.items(): - cmd += [key + "=" + value] - cmd += ["abuild", "-d", "menuconfig"] logging.info("(native) run menuconfig") - pmb.chroot.user(args, cmd, "native", "/home/pmos/build", log=False) + pmb.chroot.user(args, ["abuild", "-d", "menuconfig"], "native", + "/home/pmos/build", log=False, env={"CARCH": arch}) # Update config + checksums config = "config-" + apkbuild["_flavor"] + "." + arch diff --git a/pmb/build/other.py b/pmb/build/other.py index bcc31547..8612c04a 100644 --- a/pmb/build/other.py +++ b/pmb/build/other.py @@ -16,9 +16,10 @@ GNU General Public License for more details. You should have received a copy of the GNU General Public License along with pmbootstrap. If not, see . """ -import os -import logging import glob +import logging +import os +import shlex import pmb.build.other import pmb.chroot @@ -156,8 +157,9 @@ def index_repo(args, arch=None): path_repo_chroot = "/home/pmos/packages/pmos/" + path_arch logging.debug("(native) index " + path_arch + " repository") commands = [ - ["apk", "-q", "index", "--output", "APKINDEX.tar.gz_", - "--rewrite-arch", path_arch, "*.apk"], + # Wrap the index command with sh so we can use '*.apk' + ["sh", "-c", "apk -q index --output APKINDEX.tar.gz_" + " --rewrite-arch " + shlex.quote(path_arch) + " *.apk"], ["abuild-sign", "APKINDEX.tar.gz_"], ["mv", "APKINDEX.tar.gz_", "APKINDEX.tar.gz"] ] diff --git a/pmb/chroot/distccd.py b/pmb/chroot/distccd.py index b9a03705..441841d6 100644 --- a/pmb/chroot/distccd.py +++ b/pmb/chroot/distccd.py @@ -122,8 +122,7 @@ def start(args, arch): args.port_distccd) pmb.chroot.user(args, cmdline) - # Write down the arch and cmdline (which also contains the relevant - # environment variables, /proc/$pid/cmdline does not!) + # Write down the arch and cmdline info = configparser.ConfigParser() info["distccd"] = {} info["distccd"]["arch"] = arch diff --git a/pmb/chroot/root.py b/pmb/chroot/root.py index e4b06fc1..637d40ff 100644 --- a/pmb/chroot/root.py +++ b/pmb/chroot/root.py @@ -18,7 +18,6 @@ along with pmbootstrap. If not, see . """ import os import shutil -import shlex import pmb.config import pmb.chroot @@ -41,46 +40,54 @@ def executables_absolute_path(): def root(args, cmd, suffix="native", working_dir="/", log=True, - auto_init=True, return_stdout=False, check=True): + auto_init=True, return_stdout=False, check=True, env={}): """ Run a command inside a chroot as root. - :param log: When set to true, redirect all output to the logfile - :param auto_init: Automatically initialize the chroot + :param cmd: command as list, e.g. ["echo", "string with spaces"] + :param suffix: of the chroot to execute code in + :param working_dir: path inside chroot where the command should run + :param log: when set to true, redirect all output to the logfile + :param auto_init: automatically initialize the chroot + :param return_stdout: write stdout to a buffer and return it as string when + the command is through + :param check: raise an exception, when the command fails + :param env: dict of environment variables to be passed to the command, e.g. + {"JOBS": "5"} + :returns: * stdout when return_stdout is True + * None otherwise """ - # Get and verify chroot folder + # Initialize chroot chroot = args.work + "/chroot_" + suffix if not auto_init and not os.path.islink(chroot + "/bin/sh"): raise RuntimeError("Chroot does not exist: " + chroot) - if auto_init: pmb.chroot.init(args, suffix) - # Run the args with sudo chroot, and with cleaned environment - # variables - executables = executables_absolute_path() - for i in range(len(cmd)): - cmd[i] = shlex.quote(cmd[i]) - cmd_inner_shell = ("cd " + shlex.quote(working_dir) + ";" + - " ".join(cmd)) - - cmd_full = ["sudo", executables["sh"], "-c", - "env -i" + # unset all - " CHARSET=UTF-8" + - " PATH=" + pmb.config.chroot_path + - " SHELL=/bin/ash" + - " HISTFILE=~/.ash_history" + - " " + executables["chroot"] + - " " + chroot + - " sh -c " + shlex.quote(cmd_inner_shell) - ] - - # Generate log message - log_message = "(" + suffix + ") % " + # Readable log message (without all the escaping) + msg = "(" + suffix + ") % " + for key, value in env.items(): + msg += key + "=" + value + " " if working_dir != "/": - log_message += "cd " + working_dir + " && " - log_message += " ".join(cmd) + msg += "cd " + working_dir + "; " + msg += " ".join(cmd) - # Run the command - return pmb.helpers.run.core(args, cmd_full, log_message, log, - return_stdout, check) + # Merge env with defaults into env_all + env_all = {"CHARSET": "UTF-8", + "HISTFILE": "~/.ash_history", + "PATH": pmb.config.chroot_path, + "SHELL": "/bin/ash", + "TERM": "xterm"} + for key, value in env.items(): + env_all[key] = value + + # Build the command in steps and run it, e.g.: + # cmd: ["echo", "test"] + # cmd_chroot: ["/sbin/chroot", "/..._native", "/bin/sh", "-c", "echo test"] + # cmd_sudo: ["sudo", "env", "-i", "sh", "-c", "PATH=... /sbin/chroot ..."] + executables = executables_absolute_path() + cmd_chroot = [executables["chroot"], chroot, "/bin/sh", "-c", + pmb.helpers.run.flat_cmd(cmd, working_dir)] + cmd_sudo = ["sudo", "env", "-i", executables["sh"], "-c", + pmb.helpers.run.flat_cmd(cmd_chroot, env=env_all)] + return pmb.helpers.run.core(args, cmd_sudo, msg, log, return_stdout, check) diff --git a/pmb/chroot/user.py b/pmb/chroot/user.py index bfffea2a..d8e8e708 100644 --- a/pmb/chroot/user.py +++ b/pmb/chroot/user.py @@ -17,19 +17,31 @@ You should have received a copy of the GNU General Public License along with pmbootstrap. If not, see . """ import pmb.chroot.root +import pmb.helpers.run def user(args, cmd, suffix="native", working_dir="/", log=True, - auto_init=True, return_stdout=False, check=True): + auto_init=True, return_stdout=False, check=True, env={}): """ - Run a command inside a chroot as "user". We always use the - BusyBox implementation of 'su', because other implementations - may override the PATH environment variable (#1071). + Run a command inside a chroot as "user". We always use the BusyBox + implementation of 'su', because other implementations may override the PATH + environment variable (#1071). - :param log: When set to true, redirect all output to the logfile - :param auto_init: Automatically initialize the chroot + :param cmd: command as list, e.g. ["echo", "string with spaces"] + :param suffix: of the chroot to execute code in + :param working_dir: path inside chroot where the command should run + :param log: when set to true, redirect all output to the logfile + :param auto_init: automatically initialize the chroot + :param return_stdout: write stdout to a buffer and return it as string when + the command is through + :param check: raise an exception, when the command fails + :param env: dict of environment variables to be passed to the command, e.g. + {"JOBS": "5"} + :returns: * stdout when return_stdout is True + * None otherwise """ - cmd = ["busybox", "su", "pmos", "-c", " ".join(cmd)] + flat_cmd = pmb.helpers.run.flat_cmd(cmd, env=env) + cmd = ["busybox", "su", "pmos", "-c", flat_cmd] return pmb.chroot.root(args, cmd, suffix, working_dir, log, auto_init, return_stdout, check) diff --git a/pmb/helpers/run.py b/pmb/helpers/run.py index f0645195..374e9311 100644 --- a/pmb/helpers/run.py +++ b/pmb/helpers/run.py @@ -16,6 +16,7 @@ GNU General Public License for more details. You should have received a copy of the GNU General Public License along with pmbootstrap. If not, see . """ +import shlex import subprocess import logging import os @@ -23,19 +24,34 @@ import os def core(args, cmd, log_message, log, return_stdout, check=True, working_dir=None, background=False): - logging.debug(log_message) """ Run the command and write the output to the log. + :param cmd: command as list, e.g. ["echo", "string with spaces"] + :param log_message: simplified and more readable form of the command, e.g. + "(native) % echo test" instead of the full command with + entering the chroot and more escaping + :param log: * True: write stdout and stderr of the running process into + the log file (read with "pmbootstrap log"). + * False: redirect stdout and stderr to pmbootstrap stdout + :param return_stdout: write stdout to a buffer and return it as string when + the command is through :param check: raise an exception, when the command fails + :param working_dir: path in host system where the command should run + :param background: run the process in the background and return the process + handler + :returns: * stdout when return_stdout is True + * process handler when background is True + * None otherwise """ + logging.debug(log_message) + logging.verbose("run: " + str(cmd)) if working_dir: working_dir_old = os.getcwd() os.chdir(working_dir) ret = None - if background: if log: ret = subprocess.Popen(cmd, stdout=args.logfd, stderr=args.logfd) @@ -72,23 +88,77 @@ def core(args, cmd, log_message, log, return_stdout, check=True, return ret -def user(args, cmd, log=True, working_dir=None, return_stdout=False, - check=True, background=False): +def flat_cmd(cmd, working_dir=None, env={}): + """ + Convert a shell command passed as list into a flat shell string with + proper escaping. + :param cmd: command as list, e.g. ["echo", "string with spaces"] + :param working_dir: when set, prepend "cd ...;" to execute the command + in the given working directory + :param env: dict of environment variables to be passed to the command, e.g. + {"JOBS": "5"} + :returns: the flat string, e.g. + echo 'string with spaces' + cd /home/pmos;echo 'string with spaces' + """ + # Merge env and cmd into escaped list + escaped = [] + for key, value in env.items(): + escaped.append(key + "=" + shlex.quote(value)) + for i in range(len(cmd)): + escaped.append(shlex.quote(cmd[i])) + + # Prepend working dir + ret = " ".join(escaped) if working_dir: - msg = "% cd " + working_dir + " && " + " ".join(cmd) - else: - msg = "% " + " ".join(cmd) + ret = "cd " + shlex.quote(working_dir) + ";" + ret - # TODO: maintain and check against a whitelist + return ret + + +def user(args, cmd, log=True, working_dir=None, return_stdout=False, + check=True, background=False, env={}): + """ + Run a command on the host system as user. + + :param cmd: command as list, e.g. ["echo", "string with spaces"] + :param log: when set to true, redirect all output to the logfile + :param working_dir: path in host system where the command should run + :param return_stdout: write stdout to a buffer and return it as string when + the command is through + :param check: raise an exception, when the command fails + :param background: run the process in the background and return the process + handler + :param env: dict of environment variables to be passed to the command, e.g. + {"JOBS": "5"} + :returns: * stdout when return_stdout is True + * process handler when background is True + * None otherwise + """ + # Readable log message (without all the escaping) + msg = "% " + for key, value in env.items(): + msg += key + "=" + value + " " + if working_dir: + msg += "cd " + working_dir + "; " + msg += " ".join(cmd) + + # Add environment variables and run + if env: + cmd = ["sh", "-c", flat_cmd(cmd, env=env)] return core(args, cmd, msg, log, return_stdout, check, working_dir, background) def root(args, cmd, log=True, working_dir=None, return_stdout=False, - check=True, background=False): + check=True, background=False, env={}): """ - :param working_dir: defaults to args.work + Run a command on the host system as root, with sudo. + + NOTE: See user() above for parameter descriptions. """ + if env: + cmd = ["sh", "-c", flat_cmd(cmd, env=env)] cmd = ["sudo"] + cmd return user(args, cmd, log, working_dir, return_stdout, check, background) diff --git a/test/test_build_package.py b/test/test_build_package.py index 0d5a090f..0f65e438 100644 --- a/test/test_build_package.py +++ b/test/test_build_package.py @@ -181,6 +181,9 @@ def test_is_necessary_warn_depends(args, monkeypatch): def test_init_buildenv(args, monkeypatch): + # First init native chroot buildenv properly without patched functions + pmb.build.init(args) + # Disable effects of functions we don't want to test here monkeypatch.setattr(pmb.build._package, "build_depends", return_fake_build_depends) @@ -227,12 +230,11 @@ def test_run_abuild(args, monkeypatch): # Normal run output = "armhf/test-1-r2.apk" env = {"CARCH": "armhf", "SUDO_APK": "abuild-apk --no-progress"} - sudo_apk = "SUDO_APK='abuild-apk --no-progress'" - cmd = ["CARCH=armhf", sudo_apk, "abuild", "-d"] + cmd = ["abuild", "-d"] assert func(args, apkbuild, "armhf") == (output, cmd, env) # Force and strict - cmd = ["CARCH=armhf", sudo_apk, "abuild", "-r", "-f"] + cmd = ["abuild", "-r", "-f"] assert func(args, apkbuild, "armhf", True, True) == (output, cmd, env) # cross=native @@ -240,8 +242,7 @@ def test_run_abuild(args, monkeypatch): "SUDO_APK": "abuild-apk --no-progress", "CROSS_COMPILE": "armv6-alpine-linux-muslgnueabihf-", "CC": "armv6-alpine-linux-muslgnueabihf-gcc"} - cmd = ["CARCH=armhf", sudo_apk, "CROSS_COMPILE=armv6-alpine-linux-muslgnueabihf-", - "CC=armv6-alpine-linux-muslgnueabihf-gcc", "abuild", "-d"] + cmd = ["abuild", "-d"] assert func(args, apkbuild, "armhf", cross="native") == (output, cmd, env) # cross=distcc diff --git a/test/test_shell_escape.py b/test/test_shell_escape.py index fea0d44a..33afb655 100644 --- a/test/test_shell_escape.py +++ b/test/test_shell_escape.py @@ -41,26 +41,92 @@ def args(request): def test_shell_escape(args): - cmds = { - "test\n": ["echo", "test"], - "test && test\n": ["echo", "test", "&&", "test"], - "test ; test\n": ["echo", "test", ";", "test"], - "'test\"test\\'\n": ["echo", "'test\"test\\'"], - "*\n": ["echo", "*"], - "$PWD\n": ["echo", "$PWD"], - } + cmds = {"test\n": ["echo", "test"], + "test && test\n": ["echo", "test", "&&", "test"], + "test ; test\n": ["echo", "test", ";", "test"], + "'test\"test\\'\n": ["echo", "'test\"test\\'"], + "*\n": ["echo", "*"], + "$PWD\n": ["echo", "$PWD"], + "hello world\n": ["printf", "%s world\n", "hello"]} for expected, cmd in cmds.items(): - core = pmb.helpers.run.core(args, cmd, "test", True, True) + copy = list(cmd) + core = pmb.helpers.run.core(args, cmd, str(cmd), True, True) assert expected == core + assert cmd == copy user = pmb.helpers.run.user(args, cmd, return_stdout=True) assert expected == user + assert cmd == copy root = pmb.helpers.run.root(args, cmd, return_stdout=True) assert expected == root + assert cmd == copy chroot_root = pmb.chroot.root(args, cmd, return_stdout=True) assert expected == chroot_root + assert cmd == copy chroot_user = pmb.chroot.user(args, cmd, return_stdout=True) assert expected == chroot_user + assert cmd == copy + + +def test_shell_escape_env(args): + key = "PMBOOTSTRAP_TEST_ENVIRONMENT_VARIABLE" + value = "long value with spaces and special characters: '\"\\!$test" + env = {key: value} + cmd = ["sh", "-c", "env | grep " + key + " | grep -v SUDO_COMMAND"] + ret = key + "=" + value + "\n" + + copy = list(cmd) + func = pmb.helpers.run.user + assert func(args, cmd, return_stdout=True, env=env) == ret + assert cmd == copy + + func = pmb.helpers.run.root + assert func(args, cmd, return_stdout=True, env=env) == ret + assert cmd == copy + + func = pmb.chroot.root + assert func(args, cmd, return_stdout=True, env=env) == ret + assert cmd == copy + + func = pmb.chroot.user + assert func(args, cmd, return_stdout=True, env=env) == ret + assert cmd == copy + + +def test_flat_cmd_simple(): + func = pmb.helpers.run.flat_cmd + cmd = ["echo", "test"] + working_dir = None + ret = "echo test" + env = {} + assert func(cmd, working_dir, env) == ret + + +def test_flat_cmd_wrap_shell_string_with_spaces(): + func = pmb.helpers.run.flat_cmd + cmd = ["echo", "string with spaces"] + working_dir = None + ret = "echo 'string with spaces'" + env = {} + assert func(cmd, working_dir, env) == ret + + +def test_flat_cmd_wrap_env_simple(): + func = pmb.helpers.run.flat_cmd + cmd = ["echo", "test"] + working_dir = None + ret = "JOBS=5 echo test" + env = {"JOBS": "5"} + assert func(cmd, working_dir, env) == ret + + +def test_flat_cmd_wrap_env_spaces(): + func = pmb.helpers.run.flat_cmd + cmd = ["echo", "test"] + working_dir = None + ret = "JOBS=5 TEST='spaces string' echo test" + env = {"JOBS": "5", "TEST": "spaces string"} + assert func(cmd, working_dir, env) == ret