From 8a0b7e5b1bb0fd9b4720978e3812fb970a0062ed Mon Sep 17 00:00:00 2001 From: "Jason A. Donenfeld" Date: Tue, 18 Apr 2023 13:24:58 +0200 Subject: [PATCH 1/5] lint: `binary` or `Binaries` requires `AllowedAPKSigningKeys` Per fdroiddata!12911 the linter should error out if somebody uses `binary` or `Binaries` without supplying an `AllowedAPKSigningKeys`. There are two reasons for this: - Security: this allows full verification that the binaries built match the developers, not just what happened to get uploaded onto github at some later point in time. - Reliable updates: if the signing key changes, users won't be able to update, so this is something we should learn about when upstreams send in commits changing their signing key, rather than just leaving it to chance. --- fdroidserver/lint.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/fdroidserver/lint.py b/fdroidserver/lint.py index bc165f30..fb94e258 100644 --- a/fdroidserver/lint.py +++ b/fdroidserver/lint.py @@ -696,6 +696,25 @@ def check_updates_ucm_http_aum_pattern(app): # noqa: D403 yield _("AutoUpdateMode with UpdateCheckMode: HTTP must have a pattern.") +def check_certificate_pinned_binaries(app): + if len(app.get('AllowedAPKSigningKeys')) > 0: + return + if app.get('Binaries') is not None: + yield _( + 'App has Binaries but does not have corresponding AllowedAPKSigningKeys to pin certificate.' + ) + return + builds = app.get('Builds') + if builds is None: + return + for build in builds: + if build.get('binary') is not None: + yield _( + 'App version has binary but does not have corresponding AllowedAPKSigningKeys to pin certificate.' + ) + return + + def main(): global config, options @@ -803,6 +822,7 @@ def main(): check_current_version_code, check_updates_expected, check_updates_ucm_http_aum_pattern, + check_certificate_pinned_binaries, ] for check_func in app_check_funcs: From 26472c22cecaab2f70cef2af67ec37f14fdd07e5 Mon Sep 17 00:00:00 2001 From: "Jason A. Donenfeld" Date: Tue, 18 Apr 2023 14:38:58 +0200 Subject: [PATCH 2/5] build: check `AllowedAPKSigningKeys` in reproducible build scenario The builder should check the `AllowedAPKSigningKeys` at build time, so that the CI can check if somebody gives a wrong value that doesn't match a compared RB binary. In the event it fails, it gives useful information, and in the event it succeeds, it makes it clear that this build has verification back to the developer's original key. Also, add tests for this to the test suite. --- fdroidserver/build.py | 28 ++++++++++++++++++++++++ tests/build.TestCase | 51 +++++++++++++++++++++++++++++++++++++++---- 2 files changed, 75 insertions(+), 4 deletions(-) diff --git a/fdroidserver/build.py b/fdroidserver/build.py index 83065dbb..9078cd04 100644 --- a/fdroidserver/build.py +++ b/fdroidserver/build.py @@ -1112,6 +1112,34 @@ def main(): 'supplied reference binary ' 'successfully') + used_key = common.apk_signer_fingerprint(of) + expected_keys = app['AllowedAPKSigningKeys'] + if used_key is None: + logging.warn(_('reference binary missing ' + 'signature')) + elif len(expected_keys) == 0: + logging.warn(_('AllowedAPKSigningKeys missing ' + 'but reference binary supplied')) + elif used_key not in expected_keys: + if options.test: + logging.warning(_('Keeping failed build "{apkfilename}"') + .format(apkfilename=unsigned_apk)) + else: + logging.debug('removing %s', unsigned_apk) + os.remove(unsigned_apk) + logging.debug('removing %s', of) + os.remove(of) + raise FDroidException('supplied reference ' + 'binary signed with ' + '{signer} instead of ' + 'with {expected}'. + format(signer=used_key, + expected=expected_keys)) + else: + logging.info(_('supplied reference binary has ' + 'allowed signer {signer}'). + format(signer=used_key)) + build_succeeded_ids.append([app['id'], build.versionCode]) if not options.onserver: diff --git a/tests/build.TestCase b/tests/build.TestCase index a00f4f75..0e4906f5 100755 --- a/tests/build.TestCase +++ b/tests/build.TestCase @@ -627,6 +627,9 @@ class BuildTest(unittest.TestCase): } ) app['Builds'] = [build] + expected_key = 'a' * 64 + bogus_key = 'b' * 64 + app['AllowedAPKSigningKeys'] = [expected_key] fdroidserver.metadata.write_metadata(metadata_file, app) os.makedirs(os.path.join('unsigned', 'binaries')) @@ -657,13 +660,32 @@ class BuildTest(unittest.TestCase): a, b, c, d, e, f # silence linters' "unused" warnings with mock.patch('sys.argv', ['fdroid build', appid]): - # successful comparison + # successful comparison, successful signer open(production_result, 'w').close() open(production_compare_file, 'w').close() - with mock.patch('fdroidserver.common.verify_apks', lambda *args: None): + with mock.patch( + 'fdroidserver.common.verify_apks', lambda *args: None + ) as g, mock.patch( + 'fdroidserver.common.apk_signer_fingerprint', + lambda *args: expected_key, + ) as h: + g, h fdroidserver.build.main() self.assertTrue(os.path.exists(production_result)) self.assertTrue(os.path.exists(production_compare_file)) + # successful comparison, failed signer + open(production_result, 'w').close() + open(production_compare_file, 'w').close() + with mock.patch( + 'fdroidserver.common.verify_apks', lambda *args: None + ) as g, mock.patch( + 'fdroidserver.common.apk_signer_fingerprint', + lambda *args: bogus_key, + ) as h: + g, h + fdroidserver.build.main() + self.assertFalse(os.path.exists(production_result)) + self.assertFalse(os.path.exists(production_compare_file)) # failed comparison open(production_result, 'w').close() open(production_compare_file, 'w').close() @@ -675,15 +697,36 @@ class BuildTest(unittest.TestCase): self.assertFalse(os.path.exists(production_compare_file)) with mock.patch('sys.argv', ['fdroid build', '--test', appid]): - # successful comparison + # successful comparison, successful signer open(test_result, 'w').close() open(test_compare_file, 'w').close() - with mock.patch('fdroidserver.common.verify_apks', lambda *args: None): + with mock.patch( + 'fdroidserver.common.verify_apks', lambda *args: None + ) as g, mock.patch( + 'fdroidserver.common.apk_signer_fingerprint', + lambda *args: expected_key, + ) as h: + g, h fdroidserver.build.main() self.assertTrue(os.path.exists(test_result)) self.assertTrue(os.path.exists(test_compare_file)) self.assertFalse(os.path.exists(production_result)) self.assertFalse(os.path.exists(production_compare_file)) + # successful comparison, failed signer + open(test_result, 'w').close() + open(test_compare_file, 'w').close() + with mock.patch( + 'fdroidserver.common.verify_apks', lambda *args: None + ) as g, mock.patch( + 'fdroidserver.common.apk_signer_fingerprint', + lambda *args: bogus_key, + ) as h: + g, h + fdroidserver.build.main() + self.assertTrue(os.path.exists(test_result)) + self.assertFalse(os.path.exists(test_compare_file)) + self.assertFalse(os.path.exists(production_result)) + self.assertFalse(os.path.exists(production_compare_file)) # failed comparison open(test_result, 'w').close() open(test_compare_file, 'w').close() From 386fb55b9939cbbe13b611adfb1f7b25a58560c8 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Fri, 5 May 2023 09:31:45 +0200 Subject: [PATCH 3/5] keep old test case intact --- tests/build.TestCase | 97 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) diff --git a/tests/build.TestCase b/tests/build.TestCase index 0e4906f5..ffaaee53 100755 --- a/tests/build.TestCase +++ b/tests/build.TestCase @@ -608,6 +608,103 @@ class BuildTest(unittest.TestCase): os.chmod('config.yml', 0o600) fdroidserver.common.build = fdroidserver.common.read_config() + os.mkdir('metadata') + appid = 'info.guardianproject.checkey' + metadata_file = os.path.join('metadata', appid + '.yml') + shutil.copy(os.path.join(self.basedir, metadata_file), 'metadata') + with open(metadata_file) as fp: + app = fdroidserver.metadata.App(yaml.safe_load(fp)) + app['RepoType'] = 'git' + app[ + 'Binaries' + ] = 'https://example.com/fdroid/repo/info.guardianproject.checkey_%v.apk' + build = fdroidserver.metadata.Build( + { + 'versionCode': 123, + 'versionName': '1.2.3', + 'commit': '1.2.3', + 'disable': False, + } + ) + app['Builds'] = [build] + fdroidserver.metadata.write_metadata(metadata_file, app) + + os.makedirs(os.path.join('unsigned', 'binaries')) + production_result = os.path.join( + 'unsigned', '%s_%d.apk' % (appid, build['versionCode']) + ) + production_compare_file = os.path.join( + 'unsigned', 'binaries', '%s_%d.binary.apk' % (appid, build['versionCode']) + ) + os.makedirs(os.path.join('tmp', 'binaries')) + test_result = os.path.join('tmp', '%s_%d.apk' % (appid, build['versionCode'])) + test_compare_file = os.path.join( + 'tmp', 'binaries', '%s_%d.binary.apk' % (appid, build['versionCode']) + ) + with mock.patch( + 'fdroidserver.common.force_exit', lambda *args: None + ) as a, mock.patch( + 'fdroidserver.common.get_android_tools_version_log', lambda: 'fake' + ) as b, mock.patch( + 'fdroidserver.common.FDroidPopen', FakeProcess + ) as c, mock.patch( + 'fdroidserver.build.FDroidPopen', FakeProcess + ) as d, mock.patch( + 'fdroidserver.build.trybuild', lambda *args: True + ) as e, mock.patch( + 'fdroidserver.net.download_file', lambda *args, **kwargs: None + ) as f: + a, b, c, d, e, f # silence linters' "unused" warnings + + with mock.patch('sys.argv', ['fdroid build', appid]): + # successful comparison + open(production_result, 'w').close() + open(production_compare_file, 'w').close() + with mock.patch('fdroidserver.common.verify_apks', lambda *args: None): + fdroidserver.build.main() + self.assertTrue(os.path.exists(production_result)) + self.assertTrue(os.path.exists(production_compare_file)) + # failed comparison + open(production_result, 'w').close() + open(production_compare_file, 'w').close() + with mock.patch( + 'fdroidserver.common.verify_apks', lambda *args: 'failed' + ): + fdroidserver.build.main() + self.assertFalse(os.path.exists(production_result)) + self.assertFalse(os.path.exists(production_compare_file)) + + with mock.patch('sys.argv', ['fdroid build', '--test', appid]): + # successful comparison + open(test_result, 'w').close() + open(test_compare_file, 'w').close() + with mock.patch('fdroidserver.common.verify_apks', lambda *args: None): + fdroidserver.build.main() + self.assertTrue(os.path.exists(test_result)) + self.assertTrue(os.path.exists(test_compare_file)) + self.assertFalse(os.path.exists(production_result)) + self.assertFalse(os.path.exists(production_compare_file)) + # failed comparison + open(test_result, 'w').close() + open(test_compare_file, 'w').close() + with mock.patch( + 'fdroidserver.common.verify_apks', lambda *args: 'failed' + ): + fdroidserver.build.main() + self.assertTrue(os.path.exists(test_result)) + self.assertFalse(os.path.exists(test_compare_file)) + self.assertFalse(os.path.exists(production_result)) + self.assertFalse(os.path.exists(production_compare_file)) + + def test_failed_allowedapksigningkeys_are_not_in_unsigned(self): + os.chdir(self.testdir) + sdk_path = os.path.join(self.testdir, 'android-sdk') + self.create_fake_android_home(sdk_path) + with open('config.yml', 'w') as fp: + yaml.dump({'sdk_path': sdk_path}, fp) + os.chmod('config.yml', 0o600) + fdroidserver.common.build = fdroidserver.common.read_config() + os.mkdir('metadata') appid = 'info.guardianproject.checkey' metadata_file = os.path.join('metadata', appid + '.yml') From 1e4e2489aa4717382aebc1d1e21d70cb5870b024 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Fri, 5 May 2023 09:27:46 +0200 Subject: [PATCH 4/5] add keep_when_not_allowed config/option --- fdroidserver/build.py | 13 ++++++++++- fdroidserver/common.py | 1 + tests/build.TestCase | 52 +++++++++++++++++++++++++++++++++++++++++- 3 files changed, 64 insertions(+), 2 deletions(-) diff --git a/fdroidserver/build.py b/fdroidserver/build.py index 9078cd04..71fd20bc 100644 --- a/fdroidserver/build.py +++ b/fdroidserver/build.py @@ -826,6 +826,15 @@ def force_halt_build(timeout): vm.destroy() +def keep_when_not_allowed(): + """Control if APKs signed by keys not in AllowedAPKSigningKeys are removed.""" + return ( + (options is not None and options.keep_when_not_allowed) + or (config is not None and config.get('keep_when_not_allowed')) + or common.default_config['keep_when_not_allowed'] + ) + + def parse_commandline(): """Parse the command line. @@ -863,6 +872,8 @@ def parse_commandline(): help=_("Force build of disabled apps, and carries on regardless of scan problems. Only allowed in test mode.")) parser.add_argument("-a", "--all", action="store_true", default=False, help=_("Build all applications available")) + parser.add_argument("--keep-when-not-allowed", default=False, action="store_true", + help=argparse.SUPPRESS) parser.add_argument("-w", "--wiki", default=False, action="store_true", help=argparse.SUPPRESS) metadata.add_metadata_arguments(parser) @@ -1121,7 +1132,7 @@ def main(): logging.warn(_('AllowedAPKSigningKeys missing ' 'but reference binary supplied')) elif used_key not in expected_keys: - if options.test: + if options.test or keep_when_not_allowed(): logging.warning(_('Keeping failed build "{apkfilename}"') .format(apkfilename=unsigned_apk)) else: diff --git a/fdroidserver/common.py b/fdroidserver/common.py index a6039dcb..550e7b5a 100644 --- a/fdroidserver/common.py +++ b/fdroidserver/common.py @@ -128,6 +128,7 @@ default_config = { 'gradle': os.path.join(FDROID_PATH, 'gradlew-fdroid'), 'sync_from_local_copy_dir': False, 'allow_disabled_algorithms': False, + 'keep_when_not_allowed': False, 'per_app_repos': False, 'make_current_version_link': False, 'current_version_name_source': 'Name', diff --git a/tests/build.TestCase b/tests/build.TestCase index ffaaee53..c6cc0570 100755 --- a/tests/build.TestCase +++ b/tests/build.TestCase @@ -40,6 +40,10 @@ class FakeProcess: print('FakeFDroidPopen', args, kwargs) +class Options: + keep_when_not_allowed = False + + class BuildTest(unittest.TestCase): '''fdroidserver/build.py''' @@ -51,6 +55,7 @@ class BuildTest(unittest.TestCase): os.chdir(self.basedir) fdroidserver.common.config = None fdroidserver.build.config = None + fdroidserver.build.options = None self._td = mkdtemp() self.testdir = self._td.name @@ -604,7 +609,7 @@ class BuildTest(unittest.TestCase): sdk_path = os.path.join(self.testdir, 'android-sdk') self.create_fake_android_home(sdk_path) with open('config.yml', 'w') as fp: - yaml.dump({'sdk_path': sdk_path}, fp) + yaml.dump({'sdk_path': sdk_path, 'keep_when_not_allowed': True}, fp) os.chmod('config.yml', 0o600) fdroidserver.common.build = fdroidserver.common.read_config() @@ -920,6 +925,51 @@ class BuildTest(unittest.TestCase): test_flag = ('--skip-scan', True) fdroidserver.build.build_server(app, build, vcs, '', '', '', False) + def test_keep_when_not_allowed_default(self): + self.assertFalse(fdroidserver.build.keep_when_not_allowed()) + + def test_keep_when_not_allowed_config_true(self): + fdroidserver.build.config = {'keep_when_not_allowed': True} + self.assertTrue(fdroidserver.build.keep_when_not_allowed()) + + def test_keep_when_not_allowed_config_false(self): + fdroidserver.build.config = {'keep_when_not_allowed': False} + self.assertFalse(fdroidserver.build.keep_when_not_allowed()) + + def test_keep_when_not_allowed_options_true(self): + fdroidserver.build.options = Options + fdroidserver.build.options.keep_when_not_allowed = True + self.assertTrue(fdroidserver.build.keep_when_not_allowed()) + + def test_keep_when_not_allowed_options_false(self): + fdroidserver.build.options = Options + fdroidserver.build.options.keep_when_not_allowed = False + self.assertFalse(fdroidserver.build.keep_when_not_allowed()) + + def test_keep_when_not_allowed_options_true_override_config(self): + fdroidserver.build.options = Options + fdroidserver.build.options.keep_when_not_allowed = True + fdroidserver.build.config = {'keep_when_not_allowed': False} + self.assertTrue(fdroidserver.build.keep_when_not_allowed()) + + def test_keep_when_not_allowed_options_default_does_not_override(self): + fdroidserver.build.options = Options + fdroidserver.build.options.keep_when_not_allowed = False + fdroidserver.build.config = {'keep_when_not_allowed': True} + self.assertTrue(fdroidserver.build.keep_when_not_allowed()) + + def test_keep_when_not_allowed_all_true(self): + fdroidserver.build.options = Options + fdroidserver.build.options.keep_when_not_allowed = True + fdroidserver.build.config = {'keep_when_not_allowed': True} + self.assertTrue(fdroidserver.build.keep_when_not_allowed()) + + def test_keep_when_not_allowed_all_false(self): + fdroidserver.build.options = Options + fdroidserver.build.options.keep_when_not_allowed = False + fdroidserver.build.config = {'keep_when_not_allowed': False} + self.assertFalse(fdroidserver.build.keep_when_not_allowed()) + if __name__ == "__main__": os.chdir(os.path.dirname(__file__)) From 43b278b9d68fe8ffd51ea31a74b060255f9d36e6 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Fri, 5 May 2023 09:46:38 +0200 Subject: [PATCH 5/5] build: fix loading config files in tests The new test case works with the default config. --- tests/build.TestCase | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/tests/build.TestCase b/tests/build.TestCase index c6cc0570..128b3f4c 100755 --- a/tests/build.TestCase +++ b/tests/build.TestCase @@ -611,7 +611,7 @@ class BuildTest(unittest.TestCase): with open('config.yml', 'w') as fp: yaml.dump({'sdk_path': sdk_path, 'keep_when_not_allowed': True}, fp) os.chmod('config.yml', 0o600) - fdroidserver.common.build = fdroidserver.common.read_config() + fdroidserver.build.config = fdroidserver.common.read_config() os.mkdir('metadata') appid = 'info.guardianproject.checkey' @@ -703,13 +703,6 @@ class BuildTest(unittest.TestCase): def test_failed_allowedapksigningkeys_are_not_in_unsigned(self): os.chdir(self.testdir) - sdk_path = os.path.join(self.testdir, 'android-sdk') - self.create_fake_android_home(sdk_path) - with open('config.yml', 'w') as fp: - yaml.dump({'sdk_path': sdk_path}, fp) - os.chmod('config.yml', 0o600) - fdroidserver.common.build = fdroidserver.common.read_config() - os.mkdir('metadata') appid = 'info.guardianproject.checkey' metadata_file = os.path.join('metadata', appid + '.yml')