From 3182b77d180b2313f4fdb101af96c035380abfd7 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Mon, 23 May 2022 23:08:16 +0200 Subject: [PATCH 1/2] use apksigner to sign index-v2 with modern, supported algorithms The current signing method uses apksigner to sign the JAR so that it will automatically select algorithms that are compatible with Android SDK 23, which added the most recent algorithms: https://developer.android.com/reference/java/security/Signature This signing method uses then inherits the default signing algothim settings, since Java and Android both maintain those. That helps avoid a repeat of being stuck on an old signing algorithm. That means specifically that this call to apksigner does not specify any of the algorithms. The old indexes must be signed by SHA1withRSA otherwise they will no longer be compatible with old Androids. apksigner 30.0.0+ is available in Debian/bullseye, Debian/buster-backports, Ubuntu 21.10, and Ubuntu 20.04 from the fdroid PPA. Here's a quick way to test: for f in `ls -1 /opt/android-sdk/build-tools/*/apksigner | sort ` /usr/bin/apksigner; do printf "$f : "; $f sign --v4-signing-enabled false; done closes #1005 --- .gitlab-ci.yml | 7 +++- fdroidserver/common.py | 26 +++++++----- fdroidserver/index.py | 4 +- fdroidserver/signindex.py | 85 ++++++++++++++++++++++++++------------- tests/common.TestCase | 51 +++++++++++++++++++++-- tests/signindex.TestCase | 31 +++++++++++++- 6 files changed, 158 insertions(+), 46 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 97e799c5..84644945 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -125,7 +125,7 @@ ubuntu_lts_ppa: - ./run-tests -# Test using Ubuntu/bionic LTS (supported til 2022) with all depends +# Test using Ubuntu/bionic LTS (supported til April, 2023) with all depends # from pypi. The venv is used to isolate the dist tarball generation # environment from the clean install environment. ubuntu_bionic_pip: @@ -145,6 +145,11 @@ ubuntu_bionic_pip: - tar tzf dist/fdroidserver-*.tar.gz # back to bare machine to act as user's install machine - $pip install --upgrade pip setuptools wheel # make this go away: "error: invalid command 'bdist_wheel'" + + - export ANDROID_HOME=/opt/android-sdk + - $pip install sdkmanager + - sdkmanager 'build-tools;30.0.0' + - $pip install dist/fdroidserver-*.tar.gz - tar xzf dist/fdroidserver-*.tar.gz - cd fdroidserver-* diff --git a/fdroidserver/common.py b/fdroidserver/common.py index cf490682..3bdf573a 100644 --- a/fdroidserver/common.py +++ b/fdroidserver/common.py @@ -87,9 +87,10 @@ FDROID_PATH = os.path.realpath(os.path.join(os.path.dirname(__file__), '..')) # 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' +# 30.0.0 is the first version to support --v4-signing-enabled. # 26.0.2 is the first version recognizing md5 based signatures as valid again # (as does android, so we want that) -MINIMUM_APKSIGNER_BUILD_TOOLS_VERSION = '26.0.2' +MINIMUM_APKSIGNER_BUILD_TOOLS_VERSION = '30.0.0' VERCODE_OPERATION_RE = re.compile(r'^([ 0-9/*+-]|%c)+$') @@ -3412,6 +3413,18 @@ def get_min_sdk_version(apk): return 1 +def get_apksigner_smartcardoptions(smartcardoptions): + if '-providerName' in smartcardoptions.copy(): + pos = smartcardoptions.index('-providerName') + # remove -providerName and it's argument + del smartcardoptions[pos] + del smartcardoptions[pos] + replacements = {'-storetype': '--ks-type', + '-providerClass': '--provider-class', + '-providerArg': '--provider-arg'} + return [replacements.get(n, n) for n in smartcardoptions] + + def sign_apk(unsigned_path, signed_path, keyalias): """Sign and zipalign an unsigned APK, then save to a new file, deleting the unsigned. @@ -3429,16 +3442,7 @@ def sign_apk(unsigned_path, signed_path, keyalias): """ if config['keystore'] == 'NONE': - apksigner_smartcardoptions = config['smartcardoptions'].copy() - if '-providerName' in apksigner_smartcardoptions: - pos = config['smartcardoptions'].index('-providerName') - # remove -providerName and it's argument - del apksigner_smartcardoptions[pos] - del apksigner_smartcardoptions[pos] - replacements = {'-storetype': '--ks-type', - '-providerClass': '--provider-class', - '-providerArg': '--provider-arg'} - signing_args = [replacements.get(n, n) for n in apksigner_smartcardoptions] + signing_args = get_apksigner_smartcardoptions(config['smartcardoptions']) else: signing_args = ['--key-pass', 'env:FDROID_KEY_PASS'] apksigner = config.get('apksigner', '') diff --git a/fdroidserver/index.py b/fdroidserver/index.py index 24d4655d..5536fe74 100644 --- a/fdroidserver/index.py +++ b/fdroidserver/index.py @@ -858,7 +858,7 @@ def make_v2(apps, packages, repodir, repodict, requestsdict, fdroid_signing_key_ logging.debug(_('index-v2 must have a signature, use `fdroid signindex` to create it!')) else: signindex.config = common.config - signindex.sign_index(repodir, json_name, signindex.HashAlg.SHA256) + signindex.sign_index(repodir, json_name) def make_v1(apps, packages, repodir, repodict, requestsdict, fdroid_signing_key_fingerprints): @@ -1345,7 +1345,7 @@ def make_v0(apps, apks, repodir, repodict, requestsdict, fdroid_signing_key_fing os.remove(signed) else: signindex.config = common.config - signindex.sign_jar(signed) + signindex.sign_jar(signed, use_old_algs=True) # Copy the repo icon into the repo directory... icon_dir = os.path.join(repodir, 'icons') diff --git a/fdroidserver/signindex.py b/fdroidserver/signindex.py index 3e532f17..62d40640 100644 --- a/fdroidserver/signindex.py +++ b/fdroidserver/signindex.py @@ -21,7 +21,6 @@ import os import time import zipfile from argparse import ArgumentParser -from enum import Enum import logging from . import _ @@ -34,29 +33,27 @@ options = None start_timestamp = time.gmtime() -HashAlg = Enum("SHA1", "SHA256") +def sign_jar(jar, use_old_algs=False): + """Sign a JAR file with the best available algorithm. + The current signing method uses apksigner to sign the JAR so that + it will automatically select algorithms that are compatible with + Android SDK 23, which added the most recent algorithms: + https://developer.android.com/reference/java/security/Signature -def sign_jar(jar, hash_algorithm=None): - """Sign a JAR file with Java's jarsigner. + This signing method uses then inherits the default signing + algothim settings, since Java and Android both maintain those. + That helps avoid a repeat of being stuck on an old signing + algorithm. That means specifically that this call to apksigner + does not specify any of the algorithms. + + The old indexes must be signed by SHA1withRSA otherwise they will + no longer be compatible with old Androids. This method requires a properly initialized config object. + """ - if hash_algorithm == HashAlg.SHA256: - args = [ - config['jarsigner'], - '-keystore', - config['keystore'], - '-storepass:env', - 'FDROID_KEY_STORE_PASS', - '-digestalg', - 'SHA-256', - '-sigalg', - 'SHA256withRSA', - jar, - config['repo_keyalias'], - ] - else: + if use_old_algs: # This does use old hashing algorithms, i.e. SHA1, but that's not # broken yet for file verification. This could be set to SHA256, # but then Android < 4.3 would not be able to verify it. @@ -74,20 +71,50 @@ def sign_jar(jar, hash_algorithm=None): jar, config['repo_keyalias'], ] - if config['keystore'] == 'NONE': - args += config['smartcardoptions'] - else: # smardcards never use -keypass - args += ['-keypass:env', 'FDROID_KEY_PASS'] + if config['keystore'] == 'NONE': + args += config['smartcardoptions'] + else: # smardcards never use -keypass + args += ['-keypass:env', 'FDROID_KEY_PASS'] + else: + # https://developer.android.com/studio/command-line/apksigner + args = [ + config['apksigner'], + 'sign', + '--min-sdk-version', + '23', # enable all current algorithms + '--max-sdk-version', + '24', # avoid future incompatible algorithms + # disable all APK signature types, only use JAR sigs aka v1 + '--v1-signing-enabled', + 'true', + '--v2-signing-enabled', + 'false', + '--v3-signing-enabled', + 'false', + '--v4-signing-enabled', + 'false', + '--ks', + config['keystore'], + '--ks-pass', + 'env:FDROID_KEY_STORE_PASS', + '--ks-key-alias', + config['repo_keyalias'], + ] + if config['keystore'] == 'NONE': + args += common.get_apksigner_smartcardoptions(config['smartcardoptions']) + else: # smardcards never use --key-pass + args += ['--key-pass', 'env:FDROID_KEY_PASS'] + args += [jar] env_vars = { 'FDROID_KEY_STORE_PASS': config['keystorepass'], 'FDROID_KEY_PASS': config.get('keypass', ""), } p = common.FDroidPopen(args, envs=env_vars) if p.returncode != 0: - raise FDroidException("Failed to sign %s!" % jar) + raise FDroidException("Failed to sign %s: %s" % (jar, p.output)) -def sign_index(repodir, json_name, hash_algorithm=None): +def sign_index(repodir, json_name): """Sign index-v1.json to make index-v1.jar. This is a bit different than index.jar: instead of their being index.xml @@ -109,7 +136,11 @@ def sign_index(repodir, json_name, hash_algorithm=None): jar_file = os.path.join(repodir, name + '.jar') with zipfile.ZipFile(jar_file, 'w', zipfile.ZIP_DEFLATED) as jar: jar.write(index_file, json_name) - sign_jar(jar_file, hash_algorithm) + + if json_name in ('index.xml', 'index-v1.json'): + sign_jar(jar_file, use_old_algs=True) + else: + sign_jar(jar_file) def status_update_json(signed): @@ -165,7 +196,7 @@ def main(): json_name = 'entry.json' index_file = os.path.join(output_dir, json_name) if os.path.exists(index_file): - sign_index(output_dir, json_name, HashAlg.SHA256) + sign_index(output_dir, json_name) logging.info('Signed ' + index_file) signed.append(index_file) diff --git a/tests/common.TestCase b/tests/common.TestCase index c511901d..6c22c9e6 100755 --- a/tests/common.TestCase +++ b/tests/common.TestCase @@ -450,7 +450,7 @@ class CommonTest(unittest.TestCase): sourcefile = os.path.join(sourcedir, f) testfile = os.path.join(testsdir, f) shutil.copy(sourcefile, testsdir) - fdroidserver.signindex.sign_jar(testfile) + fdroidserver.signindex.sign_jar(testfile, use_old_algs=True) # these should be resigned, and therefore different self.assertNotEqual( open(sourcefile, 'rb').read(), open(testfile, 'rb').read() @@ -872,6 +872,9 @@ class CommonTest(unittest.TestCase): self.assertFalse(os.path.isfile(unsigned)) self.assertTrue(fdroidserver.common.verify_apk_signature(signed)) + @unittest.skipUnless( + os.path.exists('tests/SystemWebView-repack.apk'), "file too big for sdist" + ) def test_resign_apk(self): """When using apksigner, it should resign signed APKs""" config = fdroidserver.common.read_config(fdroidserver.common.options) @@ -2455,8 +2458,42 @@ class CommonTest(unittest.TestCase): self.assertTrue(os.path.exists(f), f + ' was created') self.assertFalse(is_repo_file(f), f + ' not repo file') + def test_get_apksigner_smartcardoptions(self): + testdir = tempfile.mkdtemp( + prefix=inspect.currentframe().f_code.co_name, dir=self.tmpdir + ) + os.chdir(testdir) + with open('config.yml', 'w') as fp: + d = { + 'smartcardoptions': '-storetype PKCS11' + ' -providerName SunPKCS11-OpenSC' + ' -providerClass sun.security.pkcs11.SunPKCS11' + ' -providerArg opensc-fdroid.cfg' + } + yaml.dump(d, fp) + config = fdroidserver.common.read_config() + fdroidserver.common.config = config + self.assertTrue(isinstance(d['smartcardoptions'], str)) + self.assertTrue(isinstance(config['smartcardoptions'], list)) + self.assertEqual( + [ + '--ks-type', + 'PKCS11', + '--provider-class', + 'sun.security.pkcs11.SunPKCS11', + '--provider-arg', + 'opensc-fdroid.cfg', + ], + fdroidserver.common.get_apksigner_smartcardoptions( + config['smartcardoptions'] + ), + ) + def test_get_smartcardoptions_list(self): - os.chdir(self.tmpdir) + testdir = tempfile.mkdtemp( + prefix=inspect.currentframe().f_code.co_name, dir=self.tmpdir + ) + os.chdir(testdir) with open('config.yml', 'w') as fp: fp.write( textwrap.dedent( @@ -2491,7 +2528,10 @@ class CommonTest(unittest.TestCase): ) def test_get_smartcardoptions_spaces(self): - os.chdir(self.tmpdir) + testdir = tempfile.mkdtemp( + prefix=inspect.currentframe().f_code.co_name, dir=self.tmpdir + ) + os.chdir(testdir) with open('config.yml', 'w') as fp: fp.write( textwrap.dedent( @@ -2519,7 +2559,10 @@ class CommonTest(unittest.TestCase): ) def test_get_smartcardoptions_config_py(self): - os.chdir(self.tmpdir) + testdir = tempfile.mkdtemp( + prefix=inspect.currentframe().f_code.co_name, dir=self.tmpdir + ) + os.chdir(testdir) with open('config.py', 'w') as fp: fp.write( textwrap.dedent( diff --git a/tests/signindex.TestCase b/tests/signindex.TestCase index 79977e8d..f51d7e11 100755 --- a/tests/signindex.TestCase +++ b/tests/signindex.TestCase @@ -6,6 +6,7 @@ import logging import optparse import os import shutil +import subprocess import sys import tempfile import unittest @@ -17,7 +18,7 @@ print('localmodule: ' + localmodule) if localmodule not in sys.path: sys.path.insert(0, localmodule) -from fdroidserver import common, signindex, update +from fdroidserver import apksigcopier, common, signindex, update from pathlib import Path from unittest.mock import patch @@ -96,6 +97,34 @@ class SignindexTest(unittest.TestCase): signindex.main() for f in index_files: self.assertTrue(f.exists(), '%s should exist!' % f) + self.assertFalse(os.path.exists('index-v2.jar')) # no JAR version of this file + + # index.jar aka v0 must by signed by SHA1withRSA + f = 'repo/index.jar' + common.verify_jar_signature(f) + self.assertIsNone(apksigcopier.extract_v2_sig(f, expected=False)) + cp = subprocess.run( + ['jarsigner', '-verify', '-verbose', f], stdout=subprocess.PIPE + ) + self.assertTrue(b'SHA1withRSA' in cp.stdout) + + # index-v1.jar must by signed by SHA1withRSA + f = 'repo/index-v1.jar' + common.verify_jar_signature(f) + self.assertIsNone(apksigcopier.extract_v2_sig(f, expected=False)) + cp = subprocess.run( + ['jarsigner', '-verify', '-verbose', f], stdout=subprocess.PIPE + ) + self.assertTrue(b'SHA1withRSA' in cp.stdout) + + # entry.jar aka index v2 must by signed by a modern algorithm + f = 'repo/entry.jar' + common.verify_jar_signature(f) + self.assertIsNone(apksigcopier.extract_v2_sig(f, expected=False)) + cp = subprocess.run( + ['jarsigner', '-verify', '-verbose', f], stdout=subprocess.PIPE + ) + self.assertFalse(b'SHA1withRSA' in cp.stdout) if __name__ == "__main__": From fe22958476cf53169800e53c8d96875d677cdcfb Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Wed, 25 May 2022 09:44:39 +0200 Subject: [PATCH 2/2] run-tests: skip tests that require apksigner when running on Java8 The buildserver VM has not been upgraded yet to bullseye, so it is still on Debian/stretch. The buildserver VM does not need to run `fdroid update`, `fdroid signindex`, etc. so this new apksigner requirement should not affect app builds even though they are stuck on Debian/stretch. --- tests/run-tests | 11 +++++++++++ tests/signindex.TestCase | 2 ++ tests/update.TestCase | 2 ++ 3 files changed, 15 insertions(+) diff --git a/tests/run-tests b/tests/run-tests index 59fb08e5..408c72a2 100755 --- a/tests/run-tests +++ b/tests/run-tests @@ -202,6 +202,17 @@ if use_apksigner; then grep -F '