From: Hans-Christoph Steiner <hans@eds.org>
Date: Tue, 23 Jan 2018 22:56:15 +0000 (+0100)
Subject: shell=True is too dangerous to allow; there are unfiltered user inputs
X-Git-Tag: 1.0.1~29^2~1
X-Git-Url: https://www.chiark.greenend.org.uk/ucgi/~ianmdlvl/git?a=commitdiff_plain;h=b851d49d245b289b8e447e4211f78b203bbbd4c9;p=fdroidserver.git

shell=True is too dangerous to allow; there are unfiltered user inputs

There are all sorts of unfiltered user inputs like tag and branch names in
source repos.  If those names are fed into popen calls that use shell=True,
that opens up a wide range of exploits.  All core operations should never
use shell=True.
---

diff --git a/fdroidserver/build.py b/fdroidserver/build.py
index b8229f16..ba0b0733 100644
--- a/fdroidserver/build.py
+++ b/fdroidserver/build.py
@@ -133,9 +133,9 @@ def build_server(app, build, vcs, build_dir, output_dir, log_dir, force):
         ftp.chmod('config.py', 0o600)
 
         # Copy over the ID (head commit hash) of the fdroidserver in use...
-        subprocess.call('git rev-parse HEAD >' +
-                        os.path.join(os.getcwd(), 'tmp', 'fdroidserverid'),
-                        shell=True, cwd=serverpath)
+        with open(os.path.join(os.getcwd(), 'tmp', 'fdroidserverid'), 'wb') as fp:
+            fp.write(subprocess.check_output(['git', 'rev-parse', 'HEAD'],
+                                             cwd=serverpath))
         ftp.put('tmp/fdroidserverid', 'fdroidserverid')
 
         # Copy the metadata - just the file for this app...
@@ -1263,7 +1263,7 @@ def main():
             for app in build_succeeded:
 
                 logging.info("Need to sign the app before we can install it.")
-                subprocess.call("fdroid publish {0}".format(app.id), shell=True)
+                subprocess.call("fdroid publish {0}".format(app.id))
 
                 apk_path = None
 
diff --git a/fdroidserver/vmtools.py b/fdroidserver/vmtools.py
index 33544ac5..e8281192 100644
--- a/fdroidserver/vmtools.py
+++ b/fdroidserver/vmtools.py
@@ -88,14 +88,14 @@ def get_clean_builder(serverdir, reset=False):
     return sshinfo
 
 
-def _check_call(cmd, shell=False, cwd=None):
+def _check_call(cmd, cwd=None):
     logger.debug(' '.join(cmd))
-    return subprocess.check_call(cmd, shell=shell, cwd=cwd)
+    return subprocess.check_call(cmd, shell=False, cwd=cwd)
 
 
-def _check_output(cmd, shell=False, cwd=None):
+def _check_output(cmd, cwd=None):
     logger.debug(' '.join(cmd))
-    return subprocess.check_output(cmd, shell=shell, cwd=cwd)
+    return subprocess.check_output(cmd, shell=False, cwd=cwd)
 
 
 def get_build_vm(srvdir, provider=None):
@@ -303,11 +303,13 @@ class FDroidBuildVm():
         """
         import paramiko
         try:
-            _check_call(['vagrant ssh-config > sshconfig'],
-                        cwd=self.srvdir, shell=True)
+            sshconfig_path = os.path.join(self.srvdir, 'sshconfig')
+            with open(sshconfig_path, 'wb') as fp:
+                fp.write(_check_output(['vagrant', 'ssh-config'],
+                                       cwd=self.srvdir))
             vagranthost = 'default'  # Host in ssh config file
             sshconfig = paramiko.SSHConfig()
-            with open(joinpath(self.srvdir, 'sshconfig'), 'r') as f:
+            with open(sshconfig_path, 'r') as f:
                 sshconfig.parse(f)
             sshconfig = sshconfig.lookup(vagranthost)
             idfile = sshconfig['identityfile']
diff --git a/hooks/pre-commit b/hooks/pre-commit
index 1dcc1d5a..5969f196 100755
--- a/hooks/pre-commit
+++ b/hooks/pre-commit
@@ -129,4 +129,8 @@ for f in $RB_FILES; do
 	fi
 done
 
+if grep --line-number 'shell=True' fdroidserver/[a-ce-z]*.py; then
+    err "shell=True is too dangerous, there are unfiltered user inputs!"
+fi
+
 exit 0