From 998b6245e93ce35ba0a7ff85f64b720cb5674691 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Mon, 19 Dec 2016 16:54:32 +0100 Subject: [PATCH 1/5] verify: ensure only a single signature is in compared APK The ZIP format allows multiple entries with the exact same filename, and on top of that, it does not allow deleting or updating entries. To make the `fdroid verify` procedure failsafe, it needs to create a new temporary APK that is made up on the contents of the "unsigned APK" and the signature from the "signed APK". Since it would be possible to give a signed APK as in the unsigned one's position, `fdroid verify` was not able to update the signature since it was just adding the new signature to the end of the ZIP file. When reading a ZIP, the first entry is used. --- fdroidserver/common.py | 45 ++++++++++++++++++++++++++++-------------- tests/common.TestCase | 41 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 15 deletions(-) diff --git a/fdroidserver/common.py b/fdroidserver/common.py index 31c97380..103ce449 100644 --- a/fdroidserver/common.py +++ b/fdroidserver/common.py @@ -2012,28 +2012,43 @@ def verify_apks(signed_apk, unsigned_apk, tmp_dir): One of the inputs is signed, the other is unsigned. The signature metadata is transferred from the signed to the unsigned apk, and then jarsigner is used to verify that the signature from the signed apk is also varlid for - the unsigned one. + the unsigned one. If the APK given as unsigned actually does have a + signature, it will be stripped out and ignored. :param signed_apk: Path to a signed apk file :param unsigned_apk: Path to an unsigned apk file expected to match it :param tmp_dir: Path to directory for temporary files :returns: None if the verification is successful, otherwise a string describing what went wrong. """ - with ZipFile(signed_apk) as signed_apk_as_zip: - meta_inf_files = ['META-INF/MANIFEST.MF'] - for f in signed_apk_as_zip.namelist(): - if apk_sigfile.match(f): - meta_inf_files.append(f) - if len(meta_inf_files) < 3: - return "Signature files missing from {0}".format(signed_apk) - signed_apk_as_zip.extractall(tmp_dir, meta_inf_files) - with ZipFile(unsigned_apk, mode='a') as unsigned_apk_as_zip: - for meta_inf_file in meta_inf_files: - unsigned_apk_as_zip.write(os.path.join(tmp_dir, meta_inf_file), arcname=meta_inf_file) - if subprocess.call([config['jarsigner'], '-verify', unsigned_apk]) != 0: - logging.info("...NOT verified - {0}".format(signed_apk)) - return compare_apks(signed_apk, unsigned_apk, tmp_dir) + signed = ZipFile(signed_apk, 'r') + meta_inf_files = ['META-INF/MANIFEST.MF'] + for f in signed.namelist(): + if apk_sigfile.match(f): + meta_inf_files.append(f) + if len(meta_inf_files) < 3: + return "Signature files missing from {0}".format(signed_apk) + + tmp_apk = os.path.join(tmp_dir, 'sigcp_' + os.path.basename(unsigned_apk)) + unsigned = ZipFile(unsigned_apk, 'r') + # only read the signature from the signed APK, everything else from unsigned + with ZipFile(tmp_apk, 'w') as tmp: + for filename in meta_inf_files: + tmp.writestr(signed.getinfo(filename), signed.read(filename)) + for info in unsigned.infolist(): + if info.filename in meta_inf_files: + logging.warning('Ignoring ' + info.filename + ' from ' + unsigned_apk) + continue + if info.filename in tmp.namelist(): + return "duplicate filename found: " + info.filename + tmp.writestr(info, unsigned.read(info.filename)) + unsigned.close() + signed.close() + + if subprocess.call([config['jarsigner'], '-verify', tmp_apk]) != 0: + logging.info("...NOT verified - {0}".format(unsigned_apk)) + return compare_apks(signed_apk, tmp_apk, tmp_dir) + logging.info("...successfully verified") return None diff --git a/tests/common.TestCase b/tests/common.TestCase index 98939985..63550e47 100755 --- a/tests/common.TestCase +++ b/tests/common.TestCase @@ -10,6 +10,7 @@ import shutil import sys import tempfile import unittest +from zipfile import ZipFile localmodule = os.path.realpath( os.path.join(os.path.dirname(inspect.getfile(inspect.currentframe())), '..')) @@ -177,6 +178,46 @@ class CommonTest(unittest.TestCase): # these should be resigned, and therefore different self.assertNotEqual(open(sourcefile, 'rb').read(), open(testfile, 'rb').read()) + def test_verify_apks(self): + fdroidserver.common.config = None + config = fdroidserver.common.read_config(fdroidserver.common.options) + config['jarsigner'] = fdroidserver.common.find_sdk_tools_cmd('jarsigner') + fdroidserver.common.config = config + + basedir = os.path.dirname(__file__) + sourceapk = os.path.join(basedir, 'urzip.apk') + + tmpdir = os.path.join(basedir, '..', '.testfiles') + if not os.path.exists(tmpdir): + os.makedirs(tmpdir) + testdir = tempfile.mkdtemp(prefix='test_verify_apks', dir=tmpdir) + print('testdir', testdir) + + copyapk = os.path.join(testdir, 'urzip-copy.apk') + shutil.copy(sourceapk, copyapk) + self.assertTrue(fdroidserver.common.verify_apk_signature(copyapk)) + self.assertIsNone(fdroidserver.common.verify_apks(sourceapk, copyapk, tmpdir)) + + unsignedapk = os.path.join(testdir, 'urzip-unsigned.apk') + with ZipFile(sourceapk, 'r') as apk: + with ZipFile(unsignedapk, 'w') as testapk: + for info in apk.infolist(): + if not info.filename.startswith('META-INF/'): + testapk.writestr(info, apk.read(info.filename)) + self.assertIsNone(fdroidserver.common.verify_apks(sourceapk, unsignedapk, tmpdir)) + + twosigapk = os.path.join(testdir, 'urzip-twosig.apk') + otherapk = ZipFile(os.path.join(basedir, 'urzip-release.apk'), 'r') + with ZipFile(sourceapk, 'r') as apk: + with ZipFile(twosigapk, 'w') as testapk: + for info in apk.infolist(): + testapk.writestr(info, apk.read(info.filename)) + if info.filename.startswith('META-INF/'): + testapk.writestr(info, otherapk.read(info.filename)) + otherapk.close() + self.assertFalse(fdroidserver.common.verify_apk_signature(twosigapk)) + self.assertIsNone(fdroidserver.common.verify_apks(sourceapk, twosigapk, tmpdir)) + if __name__ == "__main__": parser = optparse.OptionParser() From 364e609ebe2ee867b2d156bc96be135ec8da385a Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Tue, 20 Dec 2016 14:09:45 +0100 Subject: [PATCH 2/5] make `fdroid verify` use common.verify_apks() This makes the jarsigner the ultimate and only judge of whether two APKs match. This is the best tool since APK signatures are jar signatures. This should be eventually updated to use the official Android APK signing tool called apksigner. https://android.googlesource.com/platform/tools/apksig/ --- fdroidserver/verify.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fdroidserver/verify.py b/fdroidserver/verify.py index c13055a0..cac31a9c 100644 --- a/fdroidserver/verify.py +++ b/fdroidserver/verify.py @@ -78,9 +78,9 @@ def main(): logging.info("...retrieving " + url) net.download_file(url, dldir=tmp_dir) - compare_result = common.compare_apks( - os.path.join(unsigned_dir, apkfilename), + compare_result = common.verify_apks( remoteapk, + os.path.join(unsigned_dir, apkfilename), tmp_dir) if compare_result: raise FDroidException(compare_result) From 70915a7445ba249aec9782b1a58a151939d96c34 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Mon, 9 Jan 2017 15:10:54 +0100 Subject: [PATCH 3/5] verify: fdroidserverid and buildserverid are part of the sig There are two SHA1 git commit IDs that fdroidserver includes in the builds it makes: fdroidserverid and buildserverid. Originally, these were inserted into AndroidManifest.xml, but that makes the build not reproducible. So instead they are included as separate files in the APK's META-INF/ folder. If those files exist in the signed APK, they will be part of the signature and need to also be included in the unsigned APK for it to validate. --- fdroidserver/common.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/fdroidserver/common.py b/fdroidserver/common.py index 103ce449..cf43769f 100644 --- a/fdroidserver/common.py +++ b/fdroidserver/common.py @@ -2014,6 +2014,14 @@ def verify_apks(signed_apk, unsigned_apk, tmp_dir): used to verify that the signature from the signed apk is also varlid for the unsigned one. If the APK given as unsigned actually does have a signature, it will be stripped out and ignored. + + There are two SHA1 git commit IDs that fdroidserver includes in the builds + it makes: fdroidserverid and buildserverid. Originally, these were inserted + into AndroidManifest.xml, but that makes the build not reproducible. So + instead they are included as separate files in the APK's META-INF/ folder. + If those files exist in the signed APK, they will be part of the signature + and need to also be included in the unsigned APK for it to validate. + :param signed_apk: Path to a signed apk file :param unsigned_apk: Path to an unsigned apk file expected to match it :param tmp_dir: Path to directory for temporary files @@ -2024,7 +2032,8 @@ def verify_apks(signed_apk, unsigned_apk, tmp_dir): signed = ZipFile(signed_apk, 'r') meta_inf_files = ['META-INF/MANIFEST.MF'] for f in signed.namelist(): - if apk_sigfile.match(f): + if apk_sigfile.match(f) \ + or f in ['META-INF/fdroidserverid', 'META-INF/buildserverid']: meta_inf_files.append(f) if len(meta_inf_files) < 3: return "Signature files missing from {0}".format(signed_apk) From ffd490d8da2548879a1cb2042345a2e49a379999 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Mon, 9 Jan 2017 15:21:05 +0100 Subject: [PATCH 4/5] set_command_in_config() for finding CLI tools to run --- fdroidserver/common.py | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/fdroidserver/common.py b/fdroidserver/common.py index cf43769f..c765e005 100644 --- a/fdroidserver/common.py +++ b/fdroidserver/common.py @@ -2076,12 +2076,7 @@ def compare_apks(apk1, apk2, tmp_dir): absapk1 = os.path.abspath(apk1) absapk2 = os.path.abspath(apk2) - # try to find diffoscope in the path, if it hasn't been manually configed - if 'diffoscope' not in config: - tmp = find_command('diffoscope') - if tmp is not None: - config['diffoscope'] = tmp - if 'diffoscope' in config: + if set_command_in_config('diffoscope'): htmlfile = absapk1 + '.diffoscope.html' textfile = absapk1 + '.diffoscope.txt' if subprocess.call([config['diffoscope'], @@ -2107,12 +2102,7 @@ def compare_apks(apk1, apk2, tmp_dir): cwd=os.path.join(apk2dir, 'jar-xf')) != 0: return("Failed to unpack " + apk2) - # try to find apktool in the path, if it hasn't been manually configed - if 'apktool' not in config: - tmp = find_command('apktool') - if tmp is not None: - config['apktool'] = tmp - if 'apktool' in config: + if set_command_in_config('apktool'): if subprocess.call([config['apktool'], 'd', os.path.abspath(apk1), '--output', 'apktool'], cwd=apk1dir) != 0: return("Failed to unpack " + apk1) @@ -2136,6 +2126,22 @@ def compare_apks(apk1, apk2, tmp_dir): return None +def set_command_in_config(command): + '''Try to find specified command in the path, if it hasn't been + manually set in config.py. If found, it is added to the config + dict. The return value says whether the command is available. + + ''' + if command in config: + return True + else: + tmp = find_command(command) + if tmp is not None: + config[command] = tmp + return True + return False + + def find_command(command): '''find the full path of a command, or None if it can't be found in the PATH''' From 223c7932011b11993e466db44f94881592e438b7 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Mon, 9 Jan 2017 17:35:58 +0100 Subject: [PATCH 5/5] prefer apksigner if installed, jarsigner sucks Google has their own utility for verifying APK signatures on a desktop machine since Java's jarsigner is bad for the task. For example, it acts as if an unsigned APK validates. And to check whether an APK is unsigned using jarsigner is difficult. apksigner also does the v2 signatures, so it will have to be used eventually anyway. It is already in Debian/stretch and can be available in jessie-backports if need be. https://android.googlesource.com/platform/tools/apksig https://packages.debian.org/apksigner --- fdroidserver/common.py | 33 +++++++++++++++++++++++++++------ tests/common.TestCase | 12 ++++++++++++ 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/fdroidserver/common.py b/fdroidserver/common.py index c765e005..cae27803 100644 --- a/fdroidserver/common.py +++ b/fdroidserver/common.py @@ -2054,18 +2054,35 @@ def verify_apks(signed_apk, unsigned_apk, tmp_dir): unsigned.close() signed.close() - if subprocess.call([config['jarsigner'], '-verify', tmp_apk]) != 0: - logging.info("...NOT verified - {0}".format(unsigned_apk)) - return compare_apks(signed_apk, tmp_apk, tmp_dir) + verified = verify_apk_signature(tmp_apk) + + if not verified: + logging.info("...NOT verified - {0}".format(tmp_apk)) + return compare_apks(signed_apk, tmp_apk, tmp_dir, os.path.dirname(unsigned_apk)) logging.info("...successfully verified") return None +def verify_apk_signature(apk): + """verify the signature on an APK + + Try to use apksigner whenever possible since jarsigner is very + shitty: unsigned APKs pass as "verified"! So this has to turn on + -strict then check for result 4. + + """ + if set_command_in_config('apksigner'): + return subprocess.call([config['apksigner'], 'verify', apk]) == 0 + else: + logging.warning("Using Java's jarsigner, not recommended for verifying APKs! Use apksigner") + return subprocess.call([config['jarsigner'], '-strict', '-verify', apk]) == 4 + + apk_badchars = re.compile('''[/ :;'"]''') -def compare_apks(apk1, apk2, tmp_dir): +def compare_apks(apk1, apk2, tmp_dir, log_dir=None): """Compare two apks Returns None if the apk content is the same (apart from the signing key), @@ -2073,12 +2090,16 @@ def compare_apks(apk1, apk2, tmp_dir): trying to do the comparison. """ + if not log_dir: + log_dir = tmp_dir + absapk1 = os.path.abspath(apk1) absapk2 = os.path.abspath(apk2) if set_command_in_config('diffoscope'): - htmlfile = absapk1 + '.diffoscope.html' - textfile = absapk1 + '.diffoscope.txt' + logfilename = os.path.join(log_dir, os.path.basename(absapk1)) + htmlfile = logfilename + '.diffoscope.html' + textfile = logfilename + '.diffoscope.txt' if subprocess.call([config['diffoscope'], '--max-report-size', '12345678', '--max-diff-block-lines', '100', '--html', htmlfile, '--text', textfile, diff --git a/tests/common.TestCase b/tests/common.TestCase index 63550e47..db0be6d0 100755 --- a/tests/common.TestCase +++ b/tests/common.TestCase @@ -178,6 +178,18 @@ class CommonTest(unittest.TestCase): # these should be resigned, and therefore different self.assertNotEqual(open(sourcefile, 'rb').read(), open(testfile, 'rb').read()) + def test_verify_apk_signature(self): + fdroidserver.common.config = None + config = fdroidserver.common.read_config(fdroidserver.common.options) + config['jarsigner'] = fdroidserver.common.find_sdk_tools_cmd('jarsigner') + fdroidserver.common.config = config + + self.assertTrue(fdroidserver.common.verify_apk_signature('urzip.apk')) + self.assertFalse(fdroidserver.common.verify_apk_signature('urzip-badcert.apk')) + self.assertFalse(fdroidserver.common.verify_apk_signature('urzip-badsig.apk')) + self.assertTrue(fdroidserver.common.verify_apk_signature('urzip-release.apk')) + self.assertFalse(fdroidserver.common.verify_apk_signature('urzip-release-unsigned.apk')) + def test_verify_apks(self): fdroidserver.common.config = None config = fdroidserver.common.read_config(fdroidserver.common.options)