Merge branch 'security-fixes' into 'master'

security fixes for Janus and image metadata exploits

See merge request fdroid/fdroidserver!409

fdroidserver/build.py

@@ -414,7 +414,12 @@ def build_local(app, build, vcs, build_dir, output_dir, log_dir, srclib_dir, ext
                 raise BuildException("Error running sudo command for %s:%s" %
                                      (app.id, build.versionName), p.output)
 
-        p = FDroidPopen(['sudo', 'apt-get', '-y', 'purge', 'sudo'])
+        p = FDroidPopen(['sudo', 'passwd', '--lock', 'root'])
+        if p.returncode != 0:
+            raise BuildException("Error locking root account for %s:%s" %
+                                 (app.id, build.versionName), p.output)
+
+        p = FDroidPopen(['sudo', 'SUDO_FORCE_REMOVE=yes', 'apt-get', '-y', 'purge', 'sudo'])
         if p.returncode != 0:
             raise BuildException("Error removing sudo for %s:%s" %
                                  (app.id, build.versionName), p.output)

fdroidserver/update.py

@@ -35,7 +35,7 @@ from argparse import ArgumentParser
 import collections
 from binascii import hexlify
 
-from PIL import Image
+from PIL import Image, PngImagePlugin
 import logging
 
 from . import _
@@ -84,6 +84,8 @@ GRAPHIC_NAMES = ('featureGraphic', 'icon', 'promoGraphic', 'tvBanner')
 SCREENSHOT_DIRS = ('phoneScreenshots', 'sevenInchScreenshots',
                    'tenInchScreenshots', 'tvScreenshots', 'wearScreenshots')
 
+BLANK_PNG_INFO = PngImagePlugin.PngInfo()
+
 
 def dpi_to_px(density):
     return (int(density) * 48) / 160
@@ -371,7 +373,8 @@ def resize_icon(iconpath, density):
             im.thumbnail((size, size), Image.ANTIALIAS)
             logging.debug("%s was too large at %s - new size is %s" % (
                 iconpath, oldsize, im.size))
-            im.save(iconpath, "PNG")
+            im.save(iconpath, "PNG", optimize=True,
+                    pnginfo=BLANK_PNG_INFO, icc_profile=None)
 
     except Exception as e:
         logging.error(_("Failed resizing {path}: {error}".format(path=iconpath, error=e)))
@@ -496,14 +499,25 @@ def has_known_vulnerability(filename):
 
     Checks whether there are more than one classes.dex or AndroidManifest.xml
     files, which is invalid and an essential part of the "Master Key" attack.
-
     http://www.saurik.com/id/17
+
+    Janus is similar to Master Key but is perhaps easier to scan for.
+    https://www.guardsquare.com/en/blog/new-android-vulnerability-allows-attackers-modify-apps-without-affecting-their-signatures
     """
 
+    found_vuln = False
+
     # statically load this pattern
     if not hasattr(has_known_vulnerability, "pattern"):
         has_known_vulnerability.pattern = re.compile(b'.*OpenSSL ([01][0-9a-z.-]+)')
 
+    with open(filename.encode(), 'rb') as fp:
+        first4 = fp.read(4)
+    if first4 != b'\x50\x4b\x03\x04':
+        raise FDroidException(_('{path} has bad file signature "{pattern}", possible Janus exploit!')
+                              .format(path=filename, pattern=first4.decode().replace('\n', ' ')) + '\n'
+                              + 'https://www.guardsquare.com/en/blog/new-android-vulnerability-allows-attackers-modify-apps-without-affecting-their-signatures')
+
     files_in_apk = set()
     with zipfile.ZipFile(filename) as zf:
         for name in zf.namelist():
@@ -524,14 +538,15 @@ def has_known_vulnerability(filename):
                         else:
                             logging.warning(_('"{path}" contains outdated {name} ({version})')
                                             .format(path=filename, name=name, version=version))
-                            return True
+                            found_vuln = True
                         break
             elif name == 'AndroidManifest.xml' or name == 'classes.dex' or name.endswith('.so'):
                 if name in files_in_apk:
-                    return True
+                    logging.warning(_('{apkfilename} has multiple {name} files, looks like Master Key exploit!')
+                                    .format(apkfilename=filename, name=name))
+                    found_vuln = True
                 files_in_apk.add(name)
-
-    return False
+    return found_vuln
 
 
 def insert_obbs(repodir, apps, apks):
@@ -660,6 +675,35 @@ def _set_author_entry(app, key, f):
             app[key] = text
 
 
+def _strip_and_copy_image(inpath, outpath):
+    """Remove any metadata from image and copy it to new path
+
+    Sadly, image metadata like EXIF can be used to exploit devices.
+    It is not used at all in the F-Droid ecosystem, so its much safer
+    just to remove it entirely.
+
+    """
+
+    extension = common.get_extension(inpath)[1]
+    if os.path.isdir(outpath):
+        outpath = os.path.join(outpath, os.path.basename(inpath))
+    if extension == 'png':
+        with open(inpath, 'rb') as fp:
+            in_image = Image.open(fp)
+            in_image.save(outpath, "PNG", optimize=True,
+                          pnginfo=BLANK_PNG_INFO, icc_profile=None)
+    elif extension == 'jpg' or extension == 'jpeg':
+        with open(inpath, 'rb') as fp:
+            in_image = Image.open(fp)
+            data = list(in_image.getdata())
+            out_image = Image.new(in_image.mode, in_image.size)
+        out_image.putdata(data)
+        out_image.save(outpath, "JPEG", optimize=True)
+    else:
+        raise FDroidException(_('Unsupported file type "{extension}" for repo graphic')
+                              .format(extension=extension))
+
+
 def copy_triple_t_store_metadata(apps):
     """Include store metadata from the app's source repo
 
@@ -732,7 +776,7 @@ def copy_triple_t_store_metadata(apps):
                         sourcefile = os.path.join(root, f)
                         destfile = os.path.join(destdir, os.path.basename(f))
                         logging.debug('copying ' + sourcefile + ' ' + destfile)
-                        shutil.copy(sourcefile, destfile)
+                        _strip_and_copy_image(sourcefile, destfile)
 
 
 def insert_localized_app_metadata(apps):
@@ -830,7 +874,7 @@ def insert_localized_app_metadata(apps):
                 if base in GRAPHIC_NAMES and extension in ALLOWED_EXTENSIONS:
                     os.makedirs(destdir, mode=0o755, exist_ok=True)
                     logging.debug('copying ' + os.path.join(root, f) + ' ' + destdir)
-                    shutil.copy(os.path.join(root, f), destdir)
+                    _strip_and_copy_image(os.path.join(root, f), destdir)
             for d in dirs:
                 if d in SCREENSHOT_DIRS:
                     if locale == 'images':
@@ -842,7 +886,7 @@ def insert_localized_app_metadata(apps):
                             screenshotdestdir = os.path.join(destdir, d)
                             os.makedirs(screenshotdestdir, mode=0o755, exist_ok=True)
                             logging.debug('copying ' + f + ' ' + screenshotdestdir)
-                            shutil.copy(f, screenshotdestdir)
+                            _strip_and_copy_image(f, screenshotdestdir)
 
     repofiles = sorted(glob.glob(os.path.join('repo', '[A-Za-z]*', '[a-z][a-z][A-Z-.@]*')))
     for d in repofiles:
@@ -1311,10 +1355,13 @@ def process_apk(apkcache, apkfilename, repodir, knownapks, use_date_from_apk=Fal
         apkzip = zipfile.ZipFile(apkfile, 'r')
 
         manifest = apkzip.getinfo('AndroidManifest.xml')
-        if manifest.date_time[1] == 0:  # month can't be zero
-            logging.debug(_('AndroidManifest.xml has no date'))
-        else:
-            common.check_system_clock(datetime(*manifest.date_time), apkfilename)
+        # 1980-0-0 means zeroed out, any other invalid date should trigger a warning
+        if (1980, 0, 0) != manifest.date_time[0:3]:
+            try:
+                common.check_system_clock(datetime(*manifest.date_time), apkfilename)
+            except ValueError as e:
+                logging.warning(_("{apkfilename}'s AndroidManifest.xml has a bad date: ")
+                                .format(apkfilename=apkfile) + str(e))
 
         # extract icons from APK zip file
         iconfilename = "%s.%s.png" % (apk['packageName'], apk['versionCode'])
@@ -1443,6 +1490,8 @@ def extract_apk_icons(icon_filename, apk, apkzip, repo_dir):
         except Exception as e:
             logging.warning(_("Failed reading {path}: {error}")
                             .format(path=icon_path, error=e))
+        finally:
+            im.close()
 
     if apk['icons']:
         apk['icon'] = icon_filename
@@ -1479,7 +1528,8 @@ def fill_missing_icon_densities(empty_densities, icon_filename, apk, repo_dir):
             size = dpi_to_px(density)
 
             im.thumbnail((size, size), Image.ANTIALIAS)
-            im.save(icon_path, "PNG")
+            im.save(icon_path, "PNG", optimize=True,
+                    pnginfo=BLANK_PNG_INFO, icc_profile=None)
             empty_densities.remove(density)
         except Exception as e:
             logging.warning("Invalid image file at %s: %s", last_icon_path, e)

tests/run-tests

@@ -9,7 +9,7 @@ echo_header() {
 copy_apks_into_repo() {
     set +x
     find $APKDIR -type f -name '*.apk' -print0 | while IFS= read -r -d '' f; do
-        echo $f | grep -F -v -e unaligned -e unsigned -e badsig -e badcert -e bad-unicode || continue
+        echo $f | grep -F -v -e unaligned -e unsigned -e badsig -e badcert -e bad-unicode -e janus.apk || continue
         apk=`$aapt dump badging "$f" | sed -n "s,^package: name='\(.*\)' versionCode='\([0-9][0-9]*\)' .*,\1_\2.apk,p"`
         test "$f" -nt repo/$apk && rm -f repo/$apk  # delete existing if $f is newer
         if [ ! -e repo/$apk ] && [ ! -e archive/$apk ]; then

tests/update.TestCase

@@ -32,6 +32,14 @@ from fdroidserver.common import FDroidPopen
 class UpdateTest(unittest.TestCase):
     '''fdroid update'''
 
+    def setUp(self):
+        logging.basicConfig(level=logging.INFO)
+        self.basedir = os.path.join(localmodule, 'tests')
+        self.tmpdir = os.path.abspath(os.path.join(self.basedir, '..', '.testfiles'))
+        if not os.path.exists(self.tmpdir):
+            os.makedirs(self.tmpdir)
+        os.chdir(self.basedir)
+
     def testInsertStoreMetadata(self):
         config = dict()
         fdroidserver.common.fill_config_defaults(config)
@@ -117,15 +125,13 @@ class UpdateTest(unittest.TestCase):
                 self.assertEqual('Conversations', app['localized']['en-US']['name'])
 
     def test_insert_triple_t_metadata(self):
-        importer = os.path.join(localmodule, 'tests', 'tmp', 'importer')
+        importer = os.path.join(self.basedir, 'tmp', 'importer')
         packageName = 'org.fdroid.ci.test.app'
         if not os.path.isdir(importer):
             logging.warning('skipping test_insert_triple_t_metadata, import.TestCase must run first!')
             return
-        tmpdir = os.path.join(localmodule, '.testfiles')
-        if not os.path.exists(tmpdir):
-            os.makedirs(tmpdir)
-        tmptestsdir = tempfile.mkdtemp(prefix=inspect.currentframe().f_code.co_name, dir=tmpdir)
+        tmptestsdir = tempfile.mkdtemp(prefix=inspect.currentframe().f_code.co_name,
+                                       dir=self.tmpdir)
         packageDir = os.path.join(tmptestsdir, 'build', packageName)
         shutil.copytree(importer, packageDir)
 
@@ -387,10 +393,6 @@ class UpdateTest(unittest.TestCase):
             self.assertEqual(apk, frompickle)
 
     def test_process_apk_signed_by_disabled_algorithms(self):
-        os.chdir(os.path.join(localmodule, 'tests'))
-        if os.path.basename(os.getcwd()) != 'tests':
-            raise Exception('This test must be run in the "tests/" subdir')
-
         config = dict()
         fdroidserver.common.fill_config_defaults(config)
         fdroidserver.update.config = config
@@ -408,12 +410,9 @@ class UpdateTest(unittest.TestCase):
         fdroidserver.update.options.allow_disabled_algorithms = False
 
         knownapks = fdroidserver.common.KnownApks()
-        apksourcedir = os.getcwd()
-        tmpdir = os.path.join(localmodule, '.testfiles')
-        if not os.path.exists(tmpdir):
-            os.makedirs(tmpdir)
+
         tmptestsdir = tempfile.mkdtemp(prefix=inspect.currentframe().f_code.co_name,
-                                       dir=tmpdir)
+                                       dir=self.tmpdir)
         print('tmptestsdir', tmptestsdir)
         os.chdir(tmptestsdir)
         os.mkdir('repo')
@@ -424,7 +423,7 @@ class UpdateTest(unittest.TestCase):
 
         disabledsigs = ['org.bitbucket.tickytacky.mirrormirror_2.apk', ]
         for apkName in disabledsigs:
-            shutil.copy(os.path.join(apksourcedir, apkName),
+            shutil.copy(os.path.join(self.basedir, apkName),
                         os.path.join(tmptestsdir, 'repo'))
 
             skip, apk, cachechanged = fdroidserver.update.process_apk({}, apkName, 'repo',
@@ -475,7 +474,7 @@ class UpdateTest(unittest.TestCase):
 
         badsigs = ['urzip-badcert.apk', 'urzip-badsig.apk', 'urzip-release-unsigned.apk', ]
         for apkName in badsigs:
-            shutil.copy(os.path.join(apksourcedir, apkName),
+            shutil.copy(os.path.join(self.basedir, apkName),
                         os.path.join(tmptestsdir, 'repo'))
 
             skip, apk, cachechanged = fdroidserver.update.process_apk({}, apkName, 'repo',
@@ -539,11 +538,8 @@ class UpdateTest(unittest.TestCase):
         self.assertTrue(foundtest)
 
     def test_create_metadata_from_template(self):
-        tmpdir = os.path.join(localmodule, '.testfiles')
-        if not os.path.exists(tmpdir):
-            os.makedirs(tmpdir)
         tmptestsdir = tempfile.mkdtemp(prefix=inspect.currentframe().f_code.co_name,
-                                       dir=tmpdir)
+                                       dir=self.tmpdir)
         print('tmptestsdir', tmptestsdir)
         os.chdir(tmptestsdir)
         os.mkdir('repo')
@@ -605,6 +601,35 @@ class UpdateTest(unittest.TestCase):
         self.assertEqual('urzip', data['Name'])
         self.assertEqual('urzip', data['Summary'])
 
+    def test_has_known_vulnerability(self):
+        good = [
+            'org.bitbucket.tickytacky.mirrormirror_1.apk',
+            'org.bitbucket.tickytacky.mirrormirror_2.apk',
+            'org.bitbucket.tickytacky.mirrormirror_3.apk',
+            'org.bitbucket.tickytacky.mirrormirror_4.apk',
+            'org.dyndns.fules.ck_20.apk',
+            'urzip.apk',
+            'urzip-badcert.apk',
+            'urzip-badsig.apk',
+            'urzip-release.apk',
+            'urzip-release-unsigned.apk',
+            'repo/com.politedroid_3.apk',
+            'repo/com.politedroid_4.apk',
+            'repo/com.politedroid_5.apk',
+            'repo/com.politedroid_6.apk',
+            'repo/obb.main.oldversion_1444412523.apk',
+            'repo/obb.mainpatch.current_1619_another-release-key.apk',
+            'repo/obb.mainpatch.current_1619.apk',
+            'repo/obb.main.twoversions_1101613.apk',
+            'repo/obb.main.twoversions_1101615.apk',
+            'repo/obb.main.twoversions_1101617.apk',
+            'repo/urzip-; Рахма́нинов, [rɐxˈmanʲɪnəf] سيرجي_رخمانينوف 谢尔盖·.apk',
+        ]
+        for f in good:
+            self.assertFalse(fdroidserver.update.has_known_vulnerability(f))
+        with self.assertRaises(fdroidserver.exception.FDroidException):
+            fdroidserver.update.has_known_vulnerability('janus.apk')
+
 
 if __name__ == "__main__":
     parser = optparse.OptionParser()