From: Hans-Christoph Steiner <hans@eds.org>
Date: Thu, 1 Jun 2017 14:24:31 +0000 (+0200)
Subject: check signature and OpenSSL after APK has proven valid
X-Git-Tag: 0.8~49^2
X-Git-Url: http://www.chiark.greenend.org.uk/ucgi/~ianmdlvl/git?a=commitdiff_plain;h=8776221988803ac27a42febe475503546a04752a;p=fdroidserver.git

check signature and OpenSSL after APK has proven valid

If working with a random grabbag of APKs, there can be all sorts of
issues like corrupt entries in the ZIP, bad signatures, signatures that
are invalid since they use MD5, etc.  Moving these two checks later means
that the APKs can be renamed still.

This does change how common.getsig() works.  For years, it returned
None if the signature check failed.  Now that I've started working
with giant APK collections gathered from the wild, I can see that
`fdroid update` needs to be able to first index what's there, then
make decisions based on that information.  So that means separating
the getsig() fingerprint fetching from the APK signature verification.

This is not hugely security sensitive, since the APKs still have to
get past the Android checks, e.g. update signature checks.  Plus the
APK hash is already included in the signed index.
---

diff --git a/fdroidserver/update.py b/fdroidserver/update.py
index 614ae07f..faec9144 100644
--- a/fdroidserver/update.py
+++ b/fdroidserver/update.py
@@ -402,10 +402,6 @@ def getsig(apkpath):
               if an error occurred.
     """
 
-    # verify the jar signature is correct
-    if not common.verify_apk_signature(apkpath):
-        return None
-
     with zipfile.ZipFile(apkpath, 'r') as apk:
         certs = [n for n in apk.namelist() if common.CERT_PATH_REGEX.match(n)]
 
@@ -1118,8 +1114,6 @@ def scan_apk(apkcache, apkfilename, repodir, knownapks, use_date_from_apk):
         apk['icons_src'] = {}
         apk['icons'] = {}
         apk['antiFeatures'] = set()
-        if has_old_openssl(apkfile):
-            apk['antiFeatures'].add('KnownVuln')
 
         try:
             if SdkToolsPopen(['aapt', 'version'], output=False):
@@ -1173,6 +1167,13 @@ def scan_apk(apkcache, apkfilename, repodir, knownapks, use_date_from_apk):
             apk['srcname'] = srcfilename
         apk['size'] = os.path.getsize(apkfile)
 
+        # verify the jar signature is correct
+        if not common.verify_apk_signature(apkfile):
+            return True, None, False
+
+        if has_old_openssl(apkfile):
+            apk['antiFeatures'].add('KnownVuln')
+
         apkzip = zipfile.ZipFile(apkfile, 'r')
 
         # if an APK has files newer than the system time, suggest updating
diff --git a/tests/update.TestCase b/tests/update.TestCase
index b5020877..7e1626c5 100755
--- a/tests/update.TestCase
+++ b/tests/update.TestCase
@@ -169,21 +169,21 @@ class UpdateTest(unittest.TestCase):
             self.assertTrue(False, 'TypeError!')
 
     def testBadGetsig(self):
+        """getsig() should still be able to fetch the fingerprint of bad signatures"""
         # config needed to use jarsigner and keytool
         config = dict()
         fdroidserver.common.fill_config_defaults(config)
         fdroidserver.update.config = config
+
         apkfile = os.path.join(os.path.dirname(__file__), 'urzip-badsig.apk')
-        sig = self.javagetsig(apkfile)
-        self.assertIsNone(sig, "sig should be None: " + str(sig))
-        pysig = fdroidserver.update.getsig(apkfile)
-        self.assertIsNone(pysig, "python sig should be None: " + str(sig))
+        sig = fdroidserver.update.getsig(apkfile)
+        self.assertEqual(sig, 'e0ecb5fc2d63088e4a07ae410a127722',
+                         "python sig should be: " + str(sig))
 
         apkfile = os.path.join(os.path.dirname(__file__), 'urzip-badcert.apk')
-        sig = self.javagetsig(apkfile)
-        self.assertIsNone(sig, "sig should be None: " + str(sig))
-        pysig = fdroidserver.update.getsig(apkfile)
-        self.assertIsNone(pysig, "python sig should be None: " + str(sig))
+        sig = fdroidserver.update.getsig(apkfile)
+        self.assertEqual(sig, 'e0ecb5fc2d63088e4a07ae410a127722',
+                         "python sig should be: " + str(sig))
 
     def testScanApksAndObbs(self):
         os.chdir(os.path.join(localmodule, 'tests'))