diff --git a/fdroidserver/build.py b/fdroidserver/build.py index e2866fbd..d4f50005 100644 --- a/fdroidserver/build.py +++ b/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) diff --git a/fdroidserver/update.py b/fdroidserver/update.py index 02382d2b..782d18fc 100644 --- a/fdroidserver/update.py +++ b/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) diff --git a/tests/janus.apk b/tests/janus.apk new file mode 100644 index 00000000..ed4eed96 Binary files /dev/null and b/tests/janus.apk differ diff --git a/tests/run-tests b/tests/run-tests index 696bcd75..af29f471 100755 --- a/tests/run-tests +++ b/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 diff --git a/tests/update.TestCase b/tests/update.TestCase index da573714..db463a89 100755 --- a/tests/update.TestCase +++ b/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()