ban apksigner v33, it has bugs verifying APKs with v3/v3.1 sigs

.gitlab-ci.yml

@@ -168,7 +168,7 @@ ubuntu_jammy_pip:
     # back to bare machine to act as user's install machine
     - export ANDROID_HOME=/opt/android-sdk
     - $pip install sdkmanager
-    - sdkmanager 'build-tools;33.0.0'
+    - sdkmanager 'build-tools;35.0.0'
 
     # Install extras_require.optional from setup.py
     - $pip install biplist pycountry
@@ -176,7 +176,7 @@ ubuntu_jammy_pip:
     - $pip install dist/fdroidserver-*.tar.gz
     - tar xzf dist/fdroidserver-*.tar.gz
     - cd fdroidserver-*
-    - export PATH=$PATH:$ANDROID_HOME/build-tools/33.0.0
+    - export PATH=$PATH:$ANDROID_HOME/build-tools/35.0.0
     - fdroid=`which fdroid` ./tests/run-tests
 
     # check localization was properly installed

fdroidserver/common.py

@@ -85,6 +85,7 @@ DEFAULT_LOCALE = 'en-US'
 # this is the build-tools version, aapt has a separate version that
 # has to be manually set in test_aapt_version()
 MINIMUM_AAPT_BUILD_TOOLS_VERSION = '26.0.0'
+# 33.0.x has a bug that verifies APKs it shouldn't https://gitlab.com/fdroid/fdroidserver/-/issues/1253
 # 31.0.0 is the first version to support --v4-signing-enabled.
 # we only require 30.0.0 for now as that's the version in buster-backports, see also signindex.py
 # 26.0.2 is the first version recognizing md5 based signatures as valid again
@@ -841,7 +842,15 @@ def find_apksigner(config):
         if not os.path.isdir(os.path.join(build_tools_path, f)):
             continue
         try:
-            if LooseVersion(f) < LooseVersion(MINIMUM_APKSIGNER_BUILD_TOOLS_VERSION):
+            version = LooseVersion(f)
+            if version >= LooseVersion('33') and version < LooseVersion('34'):
+                logging.warning(
+                    _('apksigner in build-tools;{version} passes APKs with invalid v3 signatures, ignoring.').format(
+                        version=version
+                    )
+                )
+                continue
+            if version < LooseVersion(MINIMUM_APKSIGNER_BUILD_TOOLS_VERSION):
                 logging.debug("Local Android SDK only has outdated apksigner versions")
                 return
         except TypeError:

tests/test_common.py

@@ -3282,6 +3282,55 @@ class SignerExtractionTest(unittest.TestCase):
             )
 
 
+class IgnoreApksignerV33Test(CommonTest):
+    """apksigner v33 should be entirely ignored
+
+    https://gitlab.com/fdroid/fdroidserver/-/issues/1253
+    """
+
+    BAD_VERSIONS = [
+        '33.0.0-rc1',
+        '33.0.0-rc2',
+        '33.0.0-rc3',
+        '33.0.0-rc4',
+        '33.0.0',
+        '33.0.1',
+        '33.0.2',
+        '33.0.3',
+    ]
+
+    def setUp(self):
+        super().setUp()
+        self.config = {'sdk_path': self.testdir}
+
+    def _create_fake_build_tools(self, version):
+        for v in self.BAD_VERSIONS + [version]:
+            apksigner = os.path.join(self.testdir, 'build-tools', v, 'apksigner')
+            os.makedirs(os.path.dirname(apksigner))
+            with open(apksigner, 'w') as fp:
+                fp.write(f'#!/bin/sh\necho {v}[\n')
+            os.chmod(apksigner, 0o0755)  # nosec B103
+
+    def test_find_apksigner_choose_version_32_over_any_33(self):
+        good = '32.0.0'
+        self._create_fake_build_tools(good)
+        with mock.patch.dict(os.environ, clear=True):
+            os.environ['PATH'] = '/fake/path/to/avoid/conflicts'
+            fdroidserver.common.find_apksigner(self.config)
+            self.assertEqual(
+                os.path.join(self.testdir, 'build-tools', good, 'apksigner'),
+                self.config.get('apksigner'),
+            )
+
+    def test_find_apksigner_choose_no_version_over_any_33(self):
+        """apksigner v33 should be entirely ignored"""
+        self._create_fake_build_tools('29.0.0')  # too old a version
+        with mock.patch.dict(os.environ, clear=True):
+            os.environ['PATH'] = '/fake/path/to/avoid/conflicts'
+            fdroidserver.common.find_apksigner(self.config)
+            self.assertIsNone(self.config.get('apksigner'))
+
+
 class ConfigOptionsScopeTest(unittest.TestCase):
     """Test assumptions about variable scope for "config" and "options".