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

@@ -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)

pmb/build/_package.py

@@ -19,7 +19,6 @@ along with pmbootstrap.  If not, see <http://www.gnu.org/licenses/>.
 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 = []
+    cmd = ["abuild"]
-    for key, value in env.items():
-        cmd += [key + "=" + shlex.quote(value)]
-    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, ["abuild", "undeps"], suffix, "/home/pmos/build",
-        pmb.chroot.user(args, cmd, suffix, "/home/pmos/build")
+                        env={"SUDO_APK": "abuild-apk --no-progress"})
 
 
 def package(args, pkgname, arch=None, force=False, strict=False,

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",
+    pmb.chroot.user(args, ["abuild", "prepare"], "native",
-                    "/home/pmos/build", log=False)
+                    "/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

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 <http://www.gnu.org/licenses/>.
 """
-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_",
+                # Wrap the index command with sh so we can use '*.apk'
-                 "--rewrite-arch", path_arch, "*.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"]
             ]

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
+    # Write down the arch and cmdline
-    # environment variables, /proc/$pid/cmdline does not!)
     info = configparser.ConfigParser()
     info["distccd"] = {}
     info["distccd"]["arch"] = arch

pmb/chroot/root.py

@@ -18,7 +18,6 @@ along with pmbootstrap.  If not, see <http://www.gnu.org/licenses/>.
 """
 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 cmd: command as list, e.g. ["echo", "string with spaces"]
-    :param auto_init: Automatically initialize the chroot
+    :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
+    # Readable log message (without all the escaping)
-    # variables
+    msg = "(" + suffix + ") % "
-    executables = executables_absolute_path()
+    for key, value in env.items():
-    for i in range(len(cmd)):
+        msg += key + "=" + value + " "
-        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 + ") % "
     if working_dir != "/":
-        log_message += "cd " + working_dir + " && "
+        msg += "cd " + working_dir + "; "
-    log_message += " ".join(cmd)
+    msg += " ".join(cmd)
 
-    # Run the command
+    # Merge env with defaults into env_all
-    return pmb.helpers.run.core(args, cmd_full, log_message, log,
+    env_all = {"CHARSET": "UTF-8",
-                                return_stdout, check)
+               "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)

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 <http://www.gnu.org/licenses/>.
 """
 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
+    Run a command inside a chroot as "user". We always use the BusyBox
-    BusyBox implementation of 'su', because other implementations
+    implementation of 'su', because other implementations may override the PATH
-    may override the PATH environment variable (#1071).
+    environment variable (#1071).
 
-    :param log: When set to true, redirect all output to the logfile
+    :param cmd: command as list, e.g. ["echo", "string with spaces"]
-    :param auto_init: Automatically initialize the chroot
+    :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)
 

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 <http://www.gnu.org/licenses/>.
 """
+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,
+def flat_cmd(cmd, working_dir=None, env={}):
-         check=True, background=False):
+    """
+    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)
+        ret = "cd " + shlex.quote(working_dir) + ";" + ret
-    else:
-        msg = "% " + " ".join(cmd)
 
-    # 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)

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 = ["abuild", "-d"]
-    cmd = ["CARCH=armhf", sudo_apk, "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-",
+    cmd = ["abuild", "-d"]
-           "CC=armv6-alpine-linux-muslgnueabihf-gcc", "abuild", "-d"]
     assert func(args, apkbuild, "armhf", cross="native") == (output, cmd, env)
 
     # cross=distcc

test/test_shell_escape.py

@@ -41,26 +41,92 @@ def args(request):
 
 
 def test_shell_escape(args):
-    cmds = {
+    cmds = {"test\n": ["echo", "test"],
-        "test\n": ["echo", "test"],
+            "test && test\n": ["echo", "test", "&&", "test"],
-        "test && test\n": ["echo", "test", "&&", "test"],
+            "test ; test\n": ["echo", "test", ";", "test"],
-        "test ; test\n": ["echo", "test", ";", "test"],
+            "'test\"test\\'\n": ["echo", "'test\"test\\'"],
-        "'test\"test\\'\n": ["echo", "'test\"test\\'"],
+            "*\n": ["echo", "*"],
-        "*\n": ["echo", "*"],
+            "$PWD\n": ["echo", "$PWD"],
-        "$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