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 614ae07f5a51bf9245370547d6b3ca7105f66f27..faec914482b44b6c0d8ed82983f99b89249ad134 100644 (file)
--- 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 b50208775d23d38d4f25a41baa243a92a4232b26..7e1626c549a16cb8e58a92f58fb68be66e48aa6d 100755 (executable)
--- 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'))