From a57f17b2765a7475eeaac27c8b72a539148c5d05 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Tue, 23 Jan 2018 09:35:41 +0100 Subject: [PATCH 1/7] wiki: include per-app link to all related activity on gitlab.com --- fdroidserver/update.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/fdroidserver/update.py b/fdroidserver/update.py index 9b97fdb0..0e46f7ac 100644 --- a/fdroidserver/update.py +++ b/fdroidserver/update.py @@ -140,7 +140,7 @@ def update_wiki(apps, sortedids, apks): requiresroot = 'Yes' else: requiresroot = 'No' - wikidata += '{{App|id=%s|name=%s|added=%s|lastupdated=%s|source=%s|tracker=%s|web=%s|changelog=%s|donate=%s|flattr=%s|liberapay=%s|bitcoin=%s|litecoin=%s|license=%s|root=%s|author=%s|email=%s}}\n' % ( + wikidata += '{{App|id=%s|name=%s|added=%s|lastupdated=%s|source=%s|tracker=%s|web=%s|changelog=%s|donate=%s|flattr=%s|liberapay=%s|bitcoin=%s|litecoin=%s|license=%s|root=%s|author=%s|email=%s|activity=%s}}\n' % ( appid, app.Name, app.added.strftime('%Y-%m-%d') if app.added else '', @@ -157,7 +157,9 @@ def update_wiki(apps, sortedids, apks): app.License, requiresroot, app.AuthorName, - app.AuthorEmail) + app.AuthorEmail, + 'https://gitlab.com/search?group_id=28397&scope=issues&search=' + appid, + ) if app.Provides: wikidata += "This app provides: %s" % ', '.join(app.Summary.split(',')) From f0940540eeca42dadd7f2b7db05497229f640a91 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Tue, 23 Jan 2018 17:12:32 +0100 Subject: [PATCH 2/7] buildserver: include python3-git for future use We should be replacing all our custom git shell commands with python3-git, since it is a common library for doing that. It will receive a lot more attention and maintenance than our code for doing it. For example, we should not ever use shell=True, since that opens up a lot of security risks. --- buildserver/provision-apt-get-install | 3 +++ 1 file changed, 3 insertions(+) diff --git a/buildserver/provision-apt-get-install b/buildserver/provision-apt-get-install index 87a4f879..c6b40e5e 100644 --- a/buildserver/provision-apt-get-install +++ b/buildserver/provision-apt-get-install @@ -89,10 +89,13 @@ packages=" python-lxml python-magic python-setuptools + python3-git/jessie-backports + python3-gitdb/jessie-backports python3-gnupg python3-pyasn1 python3-pyasn1-modules python3-requests + python3-smmap/jessie-backports python3-yaml python3-ruamel.yaml quilt From 53f603bf3026a66b0e31abffb74da6921d1e9a34 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Tue, 23 Jan 2018 17:13:49 +0100 Subject: [PATCH 3/7] lint: check description for forbidden HTML tags: iframe, link, script, etc. --- fdroidserver/lint.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/fdroidserver/lint.py b/fdroidserver/lint.py index a5ed3637..0f6252b0 100644 --- a/fdroidserver/lint.py +++ b/fdroidserver/lint.py @@ -164,6 +164,10 @@ regex_checks = { _("Unnecessary leading space")), (re.compile(r'.*\s$'), _("Unnecessary trailing space")), + (re.compile(r'.*<(iframe|link|script).*'), + _("Forbidden HTML tags")), + (re.compile(r'''.*\s+src=["']javascript:.*'''), + _("Javascript in HTML src attributes")), ], } From 32213ef040a39bab88f44d1227f13a4c07ed4fae Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Tue, 23 Jan 2018 21:55:37 +0100 Subject: [PATCH 4/7] scanner: allow running without versionCode and as API This lets `fdroid scanner my.package.name` run without requiring that the versionCode is also specified. It also allows scanner.scan_source() to be called as a function in the public API of fdroidserver. --- fdroidserver/scanner.py | 43 +++++++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/fdroidserver/scanner.py b/fdroidserver/scanner.py index 3886d0f8..30552989 100644 --- a/fdroidserver/scanner.py +++ b/fdroidserver/scanner.py @@ -40,7 +40,7 @@ def get_gradle_compile_commands(build): return [re.compile(r'\s*' + c, re.IGNORECASE) for c in compileCommands] -def scan_source(build_dir, build): +def scan_source(build_dir, build=metadata.Build()): """Scan the source code in the given directory (and all subdirectories) and return the number of fatal problems encountered """ @@ -294,19 +294,25 @@ def main(): if app.Disabled: logging.info(_("Skipping {appid}: disabled").format(appid=appid)) continue - if not app.builds: - logging.info(_("Skipping {appid}: no builds specified").format(appid=appid)) - continue - - logging.info(_("Processing {appid}").format(appid=appid)) try: - if app.RepoType == 'srclib': build_dir = os.path.join('build', 'srclib', app.Repo) else: build_dir = os.path.join('build', appid) + if app.builds: + logging.info(_("Processing {appid}").format(appid=appid)) + else: + logging.info(_("{appid}: no builds specified, running on current source state") + .format(appid=appid)) + count = scan_source(build_dir) + if count > 0: + logging.warn(_('Scanner found {count} problems in {appid}:') + .format(count=count, appid=appid)) + probcount += count + continue + # Set up vcs interface and make sure we have the latest code... vcs = common.getvcs(app.RepoType, app.Repo, build_dir) @@ -315,20 +321,19 @@ def main(): if build.disable: logging.info("...skipping version %s - %s" % ( build.versionName, build.get('disable', build.commit[1:]))) - else: - logging.info("...scanning version " + build.versionName) + continue - # Prepare the source code... - common.prepare_source(vcs, app, build, - build_dir, srclib_dir, - extlib_dir, False) + logging.info("...scanning version " + build.versionName) + # Prepare the source code... + common.prepare_source(vcs, app, build, + build_dir, srclib_dir, + extlib_dir, False) - # Do the scan... - count = scan_source(build_dir, build) - if count > 0: - logging.warn('Scanner found %d problems in %s (%s)' % ( - count, appid, build.versionCode)) - probcount += count + count = scan_source(build_dir, build) + if count > 0: + logging.warn(_('Scanner found {count} problems in {appid}:{versionCode}:') + .format(count=count, appid=appid, versionCode=build.versionCode)) + probcount += count except BuildException as be: logging.warn("Could not scan app %s due to BuildException: %s" % ( From 07cdf848d71fc5cd4a3cc4ceda1ecf2b3b8c5a99 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Tue, 23 Jan 2018 22:42:32 +0100 Subject: [PATCH 5/7] use '--' in source vcs calls to protect against malicious input This is a quick and very incomplete addition of '--' to command line calls to source VCSs like git and hg that could manipulated by malicious tag/branch names or other vectors. These were all manually tested by calling the command lines on my own machine. --- fdroidserver/common.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/fdroidserver/common.py b/fdroidserver/common.py index 5ca32df2..352b22b1 100644 --- a/fdroidserver/common.py +++ b/fdroidserver/common.py @@ -849,7 +849,7 @@ class vcs_git(vcs): def gotorevisionx(self, rev): if not os.path.exists(self.local): # Brand new checkout - p = self.git(['clone', self.remote, self.local]) + p = self.git(['clone', '--', self.remote, self.local]) if p.returncode != 0: self.clone_failed = True raise VCSException("Git clone failed", p.output) @@ -882,7 +882,8 @@ class vcs_git(vcs): if 'Multiple remote HEAD branches' not in lines[0]: raise VCSException(_("Git remote set-head failed"), p.output) branch = lines[1].split(' ')[-1] - p2 = FDroidPopen(['git', 'remote', 'set-head', 'origin', branch], cwd=self.local, output=False) + p2 = FDroidPopen(['git', 'remote', 'set-head', 'origin', '--', branch], + cwd=self.local, output=False) if p2.returncode != 0: raise VCSException(_("Git remote set-head failed"), p.output + '\n' + p2.output) self.refreshed = True @@ -1090,7 +1091,8 @@ class vcs_hg(vcs): def gotorevisionx(self, rev): if not os.path.exists(self.local): - p = FDroidPopen(['hg', 'clone', '--ssh', 'false', self.remote, self.local], output=False) + p = FDroidPopen(['hg', 'clone', '--ssh', 'false', '--', self.remote, self.local], + output=False) if p.returncode != 0: self.clone_failed = True raise VCSException("Hg clone failed", p.output) @@ -1101,7 +1103,7 @@ class vcs_hg(vcs): for line in p.output.splitlines(): if not line.startswith('? '): raise VCSException("Unexpected output from hg status -uS: " + line) - FDroidPopen(['rm', '-rf', line[2:]], cwd=self.local, output=False) + FDroidPopen(['rm', '-rf', '--', line[2:]], cwd=self.local, output=False) if not self.refreshed: p = FDroidPopen(['hg', 'pull', '--ssh', 'false'], cwd=self.local, output=False) if p.returncode != 0: @@ -1111,7 +1113,7 @@ class vcs_hg(vcs): rev = rev or 'default' if not rev: return - p = FDroidPopen(['hg', 'update', '-C', rev], cwd=self.local, output=False) + p = FDroidPopen(['hg', 'update', '-C', '--', rev], cwd=self.local, output=False) if p.returncode != 0: raise VCSException("Hg checkout of '%s' failed" % rev, p.output) p = FDroidPopen(['hg', 'purge', '--all'], cwd=self.local, output=False) @@ -1511,7 +1513,7 @@ def getsrclib(spec, srclib_dir, subdir=None, basepath=False, if srclib["Prepare"]: cmd = replace_config_vars(srclib["Prepare"], build) - p = FDroidPopen(['bash', '-x', '-c', cmd], cwd=libdir) + p = FDroidPopen(['bash', '-x', '-c', '--', cmd], cwd=libdir) if p.returncode != 0: raise BuildException("Error running prepare command for srclib %s" % name, p.output) @@ -1566,7 +1568,7 @@ def prepare_source(vcs, app, build, build_dir, srclib_dir, extlib_dir, onserver= cmd = replace_config_vars(build.init, build) logging.info("Running 'init' commands in %s" % root_dir) - p = FDroidPopen(['bash', '-x', '-c', cmd], cwd=root_dir) + p = FDroidPopen(['bash', '-x', '-c', '--', cmd], cwd=root_dir) if p.returncode != 0: raise BuildException("Error running init command for %s:%s" % (app.id, build.versionName), p.output) @@ -1724,7 +1726,7 @@ def prepare_source(vcs, app, build, build_dir, srclib_dir, extlib_dir, onserver= libpath = os.path.relpath(libpath, root_dir) cmd = cmd.replace('$$' + name + '$$', libpath) - p = FDroidPopen(['bash', '-x', '-c', cmd], cwd=root_dir) + p = FDroidPopen(['bash', '-x', '-c', '--', cmd], cwd=root_dir) if p.returncode != 0: raise BuildException("Error running prebuild command for %s:%s" % (app.id, build.versionName), p.output) From b851d49d245b289b8e447e4211f78b203bbbd4c9 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Tue, 23 Jan 2018 23:56:15 +0100 Subject: [PATCH 6/7] 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. --- fdroidserver/build.py | 8 ++++---- fdroidserver/vmtools.py | 16 +++++++++------- hooks/pre-commit | 4 ++++ 3 files changed, 17 insertions(+), 11 deletions(-) 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 From d3caf094216ba5e69b8abc2774f09ad24018cfb7 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Wed, 24 Jan 2018 06:39:23 +0100 Subject: [PATCH 7/7] use standard User-Agent in check-fdroid-apk --- tests/check-fdroid-apk | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/check-fdroid-apk b/tests/check-fdroid-apk index bd1121ae..2290cfdc 100755 --- a/tests/check-fdroid-apk +++ b/tests/check-fdroid-apk @@ -35,8 +35,8 @@ apk="${dldir}/FDroid.apk" asc="${apk}.asc" log=/var/www/html/check-fdroid-apk/`date +%s`.txt -$curl https://f-droid.org/FDroid.apk > $apk -$curl https://f-droid.org/FDroid.apk.asc > $asc +$curl --user-agent F-Droid https://f-droid.org/FDroid.apk > $apk +$curl --user-agent F-Droid https://f-droid.org/FDroid.apk.asc > $asc fingerprint=37D2C98789D8311948394E3E41E7044E1DBA2E89