Merge branch 'bandit-scanner-and-fixes' into 'master'

Bandit scanner and fixes

See merge request fdroid/fdroidserver!567

.gitlab-ci.yml

@@ -119,16 +119,22 @@ pip_install:
     - fdroid readmeta
     - fdroid update --help
 
-lint_format_safety_checks:
+lint_format_safety_bandit_checks:
   image: alpine:3.7
   variables:
     LANG: C.UTF-8
   script:
     - apk add --no-cache bash dash ca-certificates python3
     - python3 -m ensurepip
-    - pip3 install pycodestyle pyflakes 'pylint<2.0' safety
+    - pip3 install bandit pycodestyle pyflakes 'pylint<2.0' safety
     - export EXITVALUE=0
     - ./hooks/pre-commit || export EXITVALUE=1
+    - bandit
+        -ii
+        -s B110,B310,B322,B404,B408,B410,B603,B607
+        -x fdroidserver/dscanner.py,docker/install_agent.py,docker/drozer.py
+        -r $CI_PROJECT_DIR
+        || export EXITVALUE=1
     - safety check --full-report || export EXITVALUE=1
     - pylint --rcfile=.pylint-rcfile --output-format=colorized --reports=n
             fdroid

fdroidserver/btlog.py

@@ -28,6 +28,7 @@
 
 
 import collections
+import defusedxml.minidom
 import git
 import glob
 import os
@@ -36,7 +37,6 @@ import logging
 import requests
 import shutil
 import tempfile
-import xml.dom.minidom
 import zipfile
 from argparse import ArgumentParser
 
@@ -94,7 +94,7 @@ For more info on this idea:
                 continue
             dest = os.path.join(cpdir, f)
             if f.endswith('.xml'):
-                doc = xml.dom.minidom.parse(repof)
+                doc = defusedxml.minidom.parse(repof)
                 output = doc.toprettyxml(encoding='utf-8')
                 with open(dest, 'wb') as f:
                     f.write(output)

fdroidserver/build.py

@@ -21,6 +21,7 @@ import os
 import shutil
 import glob
 import subprocess
+import posixpath
 import re
 import resource
 import sys
@@ -92,7 +93,7 @@ def build_server(app, build, vcs, build_dir, output_dir, log_dir, force):
                      port=sshinfo['port'], timeout=300,
                      look_for_keys=False, key_filename=sshinfo['idfile'])
 
-        homedir = '/home/' + sshinfo['user']
+        homedir = posixpath.join('/home', sshinfo['user'])
 
         # Get an SFTP connection...
         ftp = sshs.open_sftp()
@@ -126,8 +127,8 @@ def build_server(app, build, vcs, build_dir, output_dir, log_dir, force):
         ftp.chdir('fdroidserver')
         ftp.put(os.path.join(serverpath, '..', 'fdroid'), 'fdroid')
         ftp.put(os.path.join(serverpath, '..', 'gradlew-fdroid'), 'gradlew-fdroid')
-        ftp.chmod('fdroid', 0o755)
-        ftp.chmod('gradlew-fdroid', 0o755)
+        ftp.chmod('fdroid', 0o755)  # nosec B103 permissions are appropriate
+        ftp.chmod('gradlew-fdroid', 0o755)  # nosec B103 permissions are appropriate
         send_dir(os.path.join(serverpath))
         ftp.chdir(homedir)
 
@@ -159,7 +160,7 @@ def build_server(app, build, vcs, build_dir, output_dir, log_dir, force):
         ftp.mkdir('srclib')
         # Copy any extlibs that are required...
         if build.extlibs:
-            ftp.chdir(homedir + '/build/extlib')
+            ftp.chdir(posixpath.join(homedir, 'build', 'extlib'))
             for lib in build.extlibs:
                 lib = lib.strip()
                 libsrc = os.path.join('build/extlib', lib)
@@ -186,20 +187,20 @@ def build_server(app, build, vcs, build_dir, output_dir, log_dir, force):
             srclibpaths.append(basesrclib)
         for name, number, lib in srclibpaths:
             logging.info("Sending srclib '%s'" % lib)
-            ftp.chdir(homedir + '/build/srclib')
+            ftp.chdir(posixpath.join(homedir, 'build', 'srclib'))
             if not os.path.exists(lib):
                 raise BuildException("Missing srclib directory '" + lib + "'")
             fv = '.fdroidvcs-' + name
             ftp.put(os.path.join('build/srclib', fv), fv)
             send_dir(lib)
             # Copy the metadata file too...
-            ftp.chdir(homedir + '/srclibs')
+            ftp.chdir(posixpath.join(homedir, 'srclibs'))
             ftp.put(os.path.join('srclibs', name + '.txt'),
                     name + '.txt')
         # Copy the main app source code
         # (no need if it's a srclib)
         if (not basesrclib) and os.path.exists(build_dir):
-            ftp.chdir(homedir + '/build')
+            ftp.chdir(posixpath.join(homedir, 'build'))
             fv = '.fdroidvcs-' + app.id
             ftp.put(os.path.join('build', fv), fv)
             send_dir(build_dir)
@@ -208,7 +209,7 @@ def build_server(app, build, vcs, build_dir, output_dir, log_dir, force):
         logging.info("Starting build...")
         chan = sshs.get_transport().open_session()
         chan.get_pty()
-        cmdline = os.path.join(homedir, 'fdroidserver', 'fdroid')
+        cmdline = posixpath.join(homedir, 'fdroidserver', 'fdroid')
         cmdline += ' build --on-server'
         if force:
             cmdline += ' --force --test'
@@ -219,7 +220,7 @@ def build_server(app, build, vcs, build_dir, output_dir, log_dir, force):
         if options.notarball:
             cmdline += ' --no-tarball'
         cmdline += " %s:%s" % (app.id, build.versionCode)
-        chan.exec_command('bash --login -c "' + cmdline + '"')
+        chan.exec_command('bash --login -c "' + cmdline + '"')  # nosec B601 inputs are sanitized
 
         # Fetch build process output ...
         try:
@@ -255,7 +256,7 @@ def build_server(app, build, vcs, build_dir, output_dir, log_dir, force):
         # Retreive logs...
         toolsversion_log = common.get_toolsversion_logname(app, build)
         try:
-            ftp.chdir(os.path.join(homedir, log_dir))
+            ftp.chdir(posixpath.join(homedir, log_dir))
             ftp.get(toolsversion_log, os.path.join(log_dir, toolsversion_log))
             logging.debug('retrieved %s', toolsversion_log)
         except Exception as e:
@@ -264,9 +265,9 @@ def build_server(app, build, vcs, build_dir, output_dir, log_dir, force):
         # Retrieve the built files...
         logging.info("Retrieving build output...")
         if force:
-            ftp.chdir(homedir + '/tmp')
+            ftp.chdir(posixpath.join(homedir, 'tmp'))
         else:
-            ftp.chdir(homedir + '/unsigned')
+            ftp.chdir(posixpath.join(homedir, 'unsigned'))
         apkfile = common.get_release_filename(app, build)
         tarball = common.getsrcname(app, build)
         try:

fdroidserver/checkupdates.py

@@ -57,7 +57,7 @@ def check_http(app):
             if not parsed.netloc or not parsed.scheme or parsed.scheme != 'https':
                 raise FDroidException(_('UpdateCheckData has invalid URL: {url}').format(url=urlcode))
 
-        vercode = "99999999"
+        vercode = None
         if len(urlcode) > 0:
             logging.debug("...requesting {0}".format(urlcode))
             req = urllib.request.Request(urlcode, None)

fdroidserver/common.py

@@ -39,7 +39,7 @@ import base64
 import zipfile
 import tempfile
 import json
-import xml.etree.ElementTree as XMLElementTree
+import defusedxml.ElementTree as XMLElementTree
 
 from binascii import hexlify
 from datetime import datetime, timedelta
@@ -71,7 +71,9 @@ CERT_PATH_REGEX = re.compile(r'^META-INF/.*\.(DSA|EC|RSA)$')
 APK_NAME_REGEX = re.compile(r'^([a-zA-Z][\w.]*)_(-?[0-9]+)_?([0-9a-f]{7})?\.apk')
 STANDARD_FILE_NAME_REGEX = re.compile(r'^(\w[\w.]*)_(-?[0-9]+)\.\w+')
 
-XMLElementTree.register_namespace('android', 'http://schemas.android.com/apk/res/android')
+MAX_VERSION_CODE = 0x7fffffff  # Java's Integer.MAX_VALUE (2147483647)
+
+XMLNS_ANDROID = '{http://schemas.android.com/apk/res/android}'
 
 config = None
 options = None
@@ -281,7 +283,7 @@ def read_config(opts, config_file='config.py'):
         logging.debug(_("Reading '{config_file}'").format(config_file=config_file))
         with io.open(config_file, "rb") as f:
             code = compile(f.read(), config_file, 'exec')
-            exec(code, None, config)
+            exec(code, None, config)  # nosec TODO switch to YAML file
     else:
         logging.warning(_("No 'config.py' found, using defaults."))
 
@@ -1287,9 +1289,9 @@ def fetch_real_name(app_dir, flavours):
         app = xml.find('application')
         if app is None:
             continue
-        if "{http://schemas.android.com/apk/res/android}label" not in app.attrib:
+        if XMLNS_ANDROID + "label" not in app.attrib:
             continue
-        label = app.attrib["{http://schemas.android.com/apk/res/android}label"]
+        label = app.attrib[XMLNS_ANDROID + "label"]
         result = retrieve_string_singleline(app_dir, label)
         if result:
             result = result.strip()
@@ -1469,12 +1471,12 @@ def parse_androidmanifests(paths, app):
                     s = xml.attrib["package"]
                     if app_matches_packagename(app, s):
                         package = s
-                if "{http://schemas.android.com/apk/res/android}versionName" in xml.attrib:
-                    version = xml.attrib["{http://schemas.android.com/apk/res/android}versionName"]
+                if XMLNS_ANDROID + "versionName" in xml.attrib:
+                    version = xml.attrib[XMLNS_ANDROID + "versionName"]
                     base_dir = os.path.dirname(path)
                     version = retrieve_string_singleline(base_dir, version)
-                if "{http://schemas.android.com/apk/res/android}versionCode" in xml.attrib:
-                    a = xml.attrib["{http://schemas.android.com/apk/res/android}versionCode"]
+                if XMLNS_ANDROID + "versionCode" in xml.attrib:
+                    a = xml.attrib[XMLNS_ANDROID + "versionCode"]
                     if string_is_integer(a):
                         vercode = a
             except Exception:
@@ -3275,8 +3277,12 @@ def get_git_describe_link():
 
 
 def calculate_math_string(expr):
-    ops = {ast.Add: operator.add, ast.Sub: operator.sub,
-           ast.Mult: operator.mul}
+    ops = {
+        ast.Add: operator.add,
+        ast.Mult: operator.mul,
+        ast.Sub: operator.sub,
+        ast.USub: operator.neg,
+    }
 
     def execute_ast(node):
         if isinstance(node, ast.Num):  # <number>
@@ -3285,7 +3291,7 @@ def calculate_math_string(expr):
             return ops[type(node.op)](execute_ast(node.left),
                                       execute_ast(node.right))
         elif isinstance(node, ast.UnaryOp):  # <operator> <operand> e.g., -1
-            return ops[type(node.op)](eval(node.operand))
+            return ops[type(node.op)](ast.literal_eval(node.operand))
         else:
             raise SyntaxError(node)
 

fdroidserver/publish.py

@@ -60,12 +60,12 @@ def key_alias(appid):
         # For this particular app, the key alias is overridden...
         keyalias = config['keyaliases'][appid]
         if keyalias.startswith('@'):
-            m = hashlib.md5()
+            m = hashlib.md5()  # nosec just used to generate a keyalias
             m.update(keyalias[1:].encode('utf-8'))
             keyalias = m.hexdigest()[:8]
         return keyalias
     else:
-        m = hashlib.md5()
+        m = hashlib.md5()  # nosec just used to generate a keyalias
         m.update(appid.encode('utf-8'))
         return m.hexdigest()[:8]
 
@@ -197,7 +197,7 @@ def main():
     vercodes = common.read_pkg_args(options.appid, True)
     allaliases = []
     for appid in allapps:
-        m = hashlib.md5()
+        m = hashlib.md5()  # nosec just used to generate a keyalias
         m.update(appid.encode('utf-8'))
         keyalias = m.hexdigest()[:8]
         if keyalias in allaliases:
@@ -307,11 +307,11 @@ def main():
                     # For this particular app, the key alias is overridden...
                     keyalias = config['keyaliases'][appid]
                     if keyalias.startswith('@'):
-                        m = hashlib.md5()
+                        m = hashlib.md5()  # nosec just used to generate a keyalias
                         m.update(keyalias[1:].encode('utf-8'))
                         keyalias = m.hexdigest()[:8]
                 else:
-                    m = hashlib.md5()
+                    m = hashlib.md5()  # nosec just used to generate a keyalias
                     m.update(appid.encode('utf-8'))
                     keyalias = m.hexdigest()[:8]
                 logging.info("Key alias: " + keyalias)

fdroidserver/server.py

@@ -192,7 +192,7 @@ def update_awsbucket_libcloud(repo_section):
                     upload = True
                 else:
                     # if the sizes match, then compare by MD5
-                    md5 = hashlib.md5()
+                    md5 = hashlib.md5()  # nosec AWS uses MD5
                     with open(file_to_upload, 'rb') as f:
                         while True:
                             data = f.read(8192)

fdroidserver/update.py

@@ -27,7 +27,7 @@ import re
 import socket
 import zipfile
 import hashlib
-import pickle
+import pickle  # nosec TODO
 import time
 import copy
 from datetime import datetime
@@ -434,7 +434,7 @@ def getsig(apkpath):
 
     cert_encoded = common.get_certificate(cert)
 
-    return hashlib.md5(hexlify(cert_encoded)).hexdigest()
+    return hashlib.md5(hexlify(cert_encoded)).hexdigest()  # nosec just used as ID for signing key
 
 
 def get_cache_file():
@@ -461,7 +461,7 @@ def get_cache():
     ada = options.allow_disabled_algorithms or config['allow_disabled_algorithms']
     if not options.clean and os.path.exists(apkcachefile):
         with open(apkcachefile, 'rb') as cf:
-            apkcache = pickle.load(cf, encoding='utf-8')
+            apkcache = pickle.load(cf, encoding='utf-8')  # nosec TODO
         if apkcache.get("METADATA_VERSION") != METADATA_VERSION \
            or apkcache.get('allow_disabled_algorithms') != ada:
             apkcache = {}
@@ -1237,7 +1237,7 @@ def scan_apk_androguard(apk, apkfile):
     androidmanifest_xml = apkobject.xml['AndroidManifest.xml']
     if len(xml.nsmap) > 0:
         # one of them surely will be the Android one, or its corrupt
-        xmlns = '{http://schemas.android.com/apk/res/android}'
+        xmlns = common.XMLNS_ANDROID
     else:
         # strange but sometimes the namespace is blank.  This seems to
         # only happen with the Bromite/Chromium APKs

setup.py

@@ -69,6 +69,7 @@ setup(name='fdroidserver',
       install_requires=[
           'androguard >= 3.1.0rc2',
           'clint',
+          'defusedxml',
           'GitPython',
           'mwclient',
           'paramiko',

tests/common.TestCase

@@ -681,6 +681,12 @@ class CommonTest(unittest.TestCase):
         sig = fdroidserver.common.metadata_find_developer_signature('org.smssecure.smssecure')
         self.assertEqual('b30bb971af0d134866e158ec748fcd553df97c150f58b0a963190bbafbeb0868', sig)
 
+    def test_parse_xml(self):
+        manifest = os.path.join('source-files', 'fdroid', 'fdroidclient', 'AndroidManifest.xml')
+        parsed = fdroidserver.common.parse_xml(manifest)
+        self.assertIsNotNone(parsed)
+        self.assertEqual(str(type(parsed)), "<class 'xml.etree.ElementTree.Element'>")
+
     def test_parse_androidmanifests(self):
         app = fdroidserver.metadata.App()
         app.id = 'org.fdroid.fdroid'
@@ -800,13 +806,26 @@ class CommonTest(unittest.TestCase):
                          fdroidserver.common.parse_androidmanifests(paths, app))
 
     def test_calculate_math_string(self):
-        self.assertEqual(1234, fdroidserver.common.calculate_math_string('1234'))
-        self.assertEqual(4, fdroidserver.common.calculate_math_string('(1+1)*2'))
-        self.assertEqual(2, fdroidserver.common.calculate_math_string('(1-1)*2+3*1-1'))
+        self.assertEqual(1234,
+                         fdroidserver.common.calculate_math_string('1234'))
+        self.assertEqual((1 + 1) * 2,
+                         fdroidserver.common.calculate_math_string('(1 + 1) * 2'))
+        self.assertEqual((1 - 1) * 2 + 3 * 1 - 1,
+                         fdroidserver.common.calculate_math_string('(1 - 1) * 2 + 3 * 1 - 1'))
+        self.assertEqual(0 - 12345,
+                         fdroidserver.common.calculate_math_string('0 - 12345'))
+        self.assertEqual(0xffff,
+                         fdroidserver.common.calculate_math_string('0xffff'))
+        self.assertEqual(0xcafe * 123,
+                         fdroidserver.common.calculate_math_string('0xcafe * 123'))
+        self.assertEqual(-1,
+                         fdroidserver.common.calculate_math_string('-1'))
         with self.assertRaises(SyntaxError):
             fdroidserver.common.calculate_math_string('__import__("urllib")')
         with self.assertRaises(SyntaxError):
             fdroidserver.common.calculate_math_string('self')
+        with self.assertRaises(SyntaxError):
+            fdroidserver.common.calculate_math_string('Ox9()')
         with self.assertRaises(SyntaxError):
             fdroidserver.common.calculate_math_string('1+1; print(1)')
         with self.assertRaises(SyntaxError):