[reprotest] 01/01: Properly sanitize artifact_pattern to avoid arbitrary shell execution

Ximin Luo infinity0 at debian.org
Fri Sep 15 15:15:59 UTC 2017


This is an automated email from the git hooks/post-receive script.

infinity0 pushed a commit to branch master
in repository reprotest.

commit f8e260bf94fa7fea21ce45ae24c852c60c71b2d1
Author: Ximin Luo <infinity0 at debian.org>
Date:   Fri Sep 15 17:13:57 2017 +0200

    Properly sanitize artifact_pattern to avoid arbitrary shell execution
---
 debian/changelog       |  1 +
 reprotest/__init__.py  |  7 ++--
 reprotest/shell_syn.py | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/test_shell.py    | 37 ++++++++++++++++++++
 4 files changed, 135 insertions(+), 3 deletions(-)

diff --git a/debian/changelog b/debian/changelog
index 82d4217..75ad952 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -12,6 +12,7 @@ reprotest (0.7) UNRELEASED; urgency=medium
     different variations, and to make it easier to configure further builds.
   * Deprecate the --dont-vary flag, add a --vary flag for better composability.
   * Support >2 builds using the new --extra-build flag.
+  * Properly sanitize artifact_pattern to avoid arbitrary shell execution.
 
   [ Mattia Rizzolo ]
   * Bump Standards-Version to 4.0.0.
diff --git a/reprotest/__init__.py b/reprotest/__init__.py
index 16b4fc9..9ef5f18 100644
--- a/reprotest/__init__.py
+++ b/reprotest/__init__.py
@@ -21,7 +21,7 @@ import pkg_resources
 from reprotest.lib import adtlog
 from reprotest.lib import adt_testbed
 from reprotest.build import Build, VariationSpec, Variations
-from reprotest import presets
+from reprotest import presets, shell_syn
 
 
 VIRT_PREFIX = "autopkgtest-virt-"
@@ -127,8 +127,6 @@ class BuildContext(collections.namedtuple('_BuildContext', 'testbed_root local_d
             self.testbed_src, artifact_pattern)
         # remove any existing artifact, in case the build script doesn't overwrite
         # it e.g. like how make(1) sometimes works.
-        if re.search(r"""(^| )['"]*/""", artifact_pattern):
-            raise ValueError("artifact_pattern is possibly dangerous; maybe use a relative path instead?")
         testbed.check_exec(
             ['sh', '-ec', 'cd "%s" && rm -rf %s' %
             (self.testbed_src, artifact_pattern)])
@@ -184,6 +182,9 @@ def check(build_command, artifact_pattern, virtual_server_args, source_root,
     if os.path.isfile(source_root):
         source_root = os.path.normpath(os.path.dirname(source_root))
 
+    artifact_pattern = shell_syn.sanitize_globs(artifact_pattern)
+    logging.debug("artifact_pattern sanitized to: %s", artifact_pattern)
+
     if store_dir:
         store_dir = str(store_dir)
         if not os.path.exists(store_dir):
diff --git a/reprotest/shell_syn.py b/reprotest/shell_syn.py
new file mode 100644
index 0000000..98a6ab5
--- /dev/null
+++ b/reprotest/shell_syn.py
@@ -0,0 +1,93 @@
+# Licensed under the GPL: https://www.gnu.org/licenses/gpl-3.0.en.html
+# For details: reprotest/debian/copyright
+
+# http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html
+# Special chars except allowed ones <space> " ' \
+special_except_quotes=r"""
+	|&;<>()$`"""
+special_in_double_quotes=r"$`"
+escaped_in_double_quotes=r"""$`"\
+"""
+
+def sanitize_globs(s, join=True, forcerel=True):
+    """Ensure that a shell snippet only contains glob expressions, and nothing
+    that can be executed.
+
+    Args:
+
+    s: The snippet to sanitize
+    join: If true, returns a single shell snippet string, else a list of shell
+        snippet words (that include quotes and other limit) that may be joined
+        by a single space into a single shell snippet. Default: true.
+    forcerel: If true, prepends ./ to all snippet words. This ensures that they
+        can't be accidentally interpreted as options to most commands.
+    """
+    # output state
+    words = []
+    cw = None
+    def next_word():
+        nonlocal cw, words
+        if cw is not None:
+            words.append(cw)
+            cw = None
+
+    # parse state
+    in_quote = False
+    escaped = False
+
+    for c in s:
+        #print(c, in_quote, escaped)
+        if not in_quote:
+            if escaped:
+                escaped = False
+            else:
+                if c in special_except_quotes:
+                    raise ValueError("not a shell-glob pattern: %s" % s)
+                elif c == "\\":
+                    escaped = True
+                elif c in "'\"":
+                    in_quote = c
+                elif c == " ":
+                    next_word()
+                    continue
+
+        elif in_quote == "'":
+            if c == "'":
+                in_quote = False
+
+        elif in_quote == "\"":
+            if escaped:
+                if c not in escaped_in_double_quotes:
+                    # as per the spec, these chars retain the backslash, so
+                    # append that to cw before we append c as described below
+                    cw = "\\" if cw is None else cw + "\\"
+                escaped = False
+            else:
+                if c in special_in_double_quotes:
+                    raise ValueError("not a shell-glob pattern: %s" % s)
+                elif c == "\\":
+                    escaped = True
+                elif c == "\"":
+                    in_quote = False
+
+        else:
+            assert False
+
+        # append c onto cw. we do this uncondtionally because we're only
+        # sanitising the string and not parsing it; we want to keep it in a
+        # form that the shell can parse
+        cw = c if cw is None else cw + c
+
+    if in_quote or escaped:
+        raise ValueError("unclosed escape or quote: %s" % s)
+
+    next_word()
+
+    words = ["./" + w for w in words] if forcerel else words
+    return " ".join(words) if join else words
+
+
+if __name__ == "__main__":
+    import sys
+    for a in sys.argv[1:]:
+        print(sanitize_globs(a))
diff --git a/tests/test_shell.py b/tests/test_shell.py
new file mode 100644
index 0000000..1788412
--- /dev/null
+++ b/tests/test_shell.py
@@ -0,0 +1,37 @@
+# Licensed under the GPL: https://www.gnu.org/licenses/gpl-3.0.en.html
+# For details: reprotest/debian/copyright
+
+import pytest
+
+import random
+import shlex
+import string
+
+from reprotest.shell_syn import *
+
+
+def test_basic():
+    assert (sanitize_globs("""lol \$ *"\$ s" "" '' ' ' " " '   '   "" """)
+        == '''./lol ./\$ ./*"\$ s" ./"" ./'' ./' ' ./" " ./'   ' ./""''')
+    assert (sanitize_globs("""*"*"'*' ../hm wut???""")
+        == '''./*"*"'*' ./../hm ./wut???''')
+
+    with pytest.raises(ValueError):
+        sanitize_globs('; sudo rm -rf /')
+    with pytest.raises(ValueError):
+        sanitize_globs('`sudo rm -rf /`')
+    with pytest.raises(ValueError):
+        sanitize_globs('<(sudo rm -rf /)')
+    with pytest.raises(ValueError):
+        sanitize_globs('$(sudo rm -rf /)')
+    with pytest.raises(ValueError):
+        sanitize_globs('ok; sudo rm -rf /')
+
+    assert sanitize_globs('-rf') == './-rf'
+    assert sanitize_globs('/') == './/'
+
+def test_shlex_quote():
+    for _ in range(65536):
+        x = ''.join(random.choice(string.printable) for _ in range(random.randint(0, 32)))
+        assert len(sanitize_globs(shlex.quote(x), False)) == 1
+        assert len(shlex.split(sanitize_globs(shlex.quote(x)))) == 1

-- 
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/reproducible/reprotest.git



More information about the Reproducible-commits mailing list