From a3360813a63215273a66578e159a9c0142e407fd Mon Sep 17 00:00:00 2001 From: Michał Górny Date: Wed, 15 Feb 2023 10:17:27 +0100 Subject: openpgp: Do not reject signatures made prior to key expiration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the key is expired, reject it only if the signature was made after the key expired. This only works in isolated environments where we control key trust explicitly — as GnuPG will consider all expired keys untrusted. Signed-off-by: Michał Górny --- gemato/cli.py | 2 ++ gemato/openpgp.py | 32 +++++++++++++++++++++++++++----- tests/test_openpgp.py | 40 ++++++++++++++++++++++++++++++++++------ 3 files changed, 63 insertions(+), 11 deletions(-) diff --git a/gemato/cli.py b/gemato/cli.py index 7e90afa..b404da4 100644 --- a/gemato/cli.py +++ b/gemato/cli.py @@ -171,6 +171,8 @@ class VerifyingOpenPGPMixin(BaseOpenPGPMixin): f"{sig.primary_key_fingerprint}") logging.info(f"- subkey: {sig.fingerprint}") logging.info(f"- timestamp: {sig.timestamp} UTC") + if sig.key_expiration is not None: + logging.info(f"- key expiration: {sig.key_expiration} UTC") class BaseManifestLoaderMixin: diff --git a/gemato/openpgp.py b/gemato/openpgp.py index cce47ee..e2f89ed 100644 --- a/gemato/openpgp.py +++ b/gemato/openpgp.py @@ -68,6 +68,7 @@ class OpenPGPSignatureData: sig_status: typing.Optional[OpenPGPSignatureStatus] = None valid_sig: bool = False trusted_sig: bool = False + key_expiration: typing.Optional[datetime.datetime] = None class OpenPGPSignatureList(list[OpenPGPSignatureData]): @@ -111,10 +112,9 @@ class SystemGPGEnvironment: (user's home directory or GNUPGHOME). """ - __slots__ = ['debug'] - def __init__(self, debug=False, proxy=None): self.debug = debug + self._trusted_keys = set() def __enter__(self): return self @@ -220,17 +220,40 @@ class SystemGPGEnvironment: b'TRUST_FULL', b'TRUST_ULTIMATE'): sig_list[-1].trusted_sig = True + elif line.startswith(b"[GNUPG:] KEYEXPIRED"): + # TODO: will the "correct" key be emitted last? + assert sig_list + spl = line.split(b" ", 3) + assert len(spl) >= 3 + sig_list[-1].key_expiration = ( + self._parse_gpg_ts(spl[2].decode("utf8"))) if not sig_list: raise OpenPGPUnknownSigFailure( err.decode('utf8', errors='backslashreplace')) - # bad signature causes failure even without require_all_good for sig in sig_list: + # bad signature causes failure even without require_all_good if sig.sig_status == OpenPGPSignatureStatus.BAD: raise OpenPGPVerificationFailure( err.decode("utf8", errors="backslashreplace"), sig) + if sig.sig_status == OpenPGPSignatureStatus.EXPIRED_KEY: + # if the signature was done using a key that's expired *now*, + # check whether it was expired prior to the signature. + assert sig.timestamp is not None + assert sig.key_expiration is not None + if sig.timestamp < sig.key_expiration: + sig.sig_status = OpenPGPSignatureStatus.GOOD + # we need to check the trust explicitly since GPG + # doesn't check trust on expired keys + if sig.primary_key_fingerprint in self._trusted_keys: + sig.trusted_sig = True + else: + # remove key_expiration if it the signature was not done + # by an expired key + sig.key_expiration = None + if not require_all_good: if any(x.sig_status == OpenPGPSignatureStatus.GOOD and x.valid_sig and x.trusted_sig for x in sig_list): @@ -373,8 +396,6 @@ class IsolatedGPGEnvironment(SystemGPGEnvironment): or use as a context manager (via 'with'). """ - __slots__ = ['_home', 'proxy'] - def __init__(self, debug=False, proxy=None): super().__init__(debug=debug) self.proxy = proxy @@ -443,6 +464,7 @@ debug-level guru for line in out.splitlines(): if line.startswith(b'[GNUPG:] IMPORT_OK'): fprs.add(line.split(b' ')[3].decode('ASCII')) + self._trusted_keys.update(fprs) ownertrust = ''.join(f'{fpr}:6:\n' for fpr in fprs).encode('utf8') exitst, out, err = self._spawn_gpg( diff --git a/tests/test_openpgp.py b/tests/test_openpgp.py index aa6d2d7..0c93839 100644 --- a/tests/test_openpgp.py +++ b/tests/test_openpgp.py @@ -106,6 +106,24 @@ mkkhTd2Auao4D2K74BePBuiZ9+eDQA== -----END PGP SIGNATURE----- """ +POST_EXPIRATION_SIGNED_MANIFEST = f""" +-----BEGIN PGP SIGNED MESSAGE----- +Hash: SHA256 + +{COMMON_MANIFEST_TEXT} +-----BEGIN PGP SIGNATURE----- + +iQEzBAEBCAAdFiEEgeEsFr2NzWC+GAhFE2iA5yp7E4QFAmPsj28ACgkQE2iA5yp7 +E4R0xAf8CC6uh8VMmv8xlFePEoBYEuSUtDa2hWHJv1sMn90QnszHGG6oo32g2Lje +H9NRyjOltAG9t0siF/pf57EiKCs9B+Z9zLGYuWlK4gvkHjMHzsoTipUymm2/saEo +AuoeZvhqNtfU0hCIJsWENtdyMb/hsJIxIOwBjVS/JT5cZlOGjhlyxVO0CS/7FsCp +GZCeLYPdYXPw2em2DR3Q3NDuNmUY7W3WhJCL14uC+AkU64SnHc13xQ9/go6TQ2ho +783Jm2f/4ZREYpKMvCgUJvOADSqnfY89hc6B/9JCXn+Zm8a31zgENlJ8DEhN0JMN +le/JaXEH/AhO6xCOmk8tNQ3QXcNF5w== +=UGgA +-----END PGP SIGNATURE----- +""" + DASH_ESCAPED_SIGNED_MANIFEST = ''' -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 @@ -251,6 +269,7 @@ _ = VALID_KEY_SUBKEY "MODIFIED_SIGNED_MANIFEST", "EXPIRED_SIGNED_MANIFEST", "TWO_SIGNATURE_MANIFEST", + "POST_EXPIRATION_SIGNED_MANIFEST", ]) def test_noverify_goodish_manifest_load(manifest_var): """Test Manifest files that should succeed (OpenPGP disabled)""" @@ -385,6 +404,9 @@ MANIFEST_VARIANTS = [ ('SIGNED_MANIFEST', 'COMBINED_PUBLIC_KEYS', None), ('DASH_ESCAPED_SIGNED_MANIFEST', 'VALID_PUBLIC_KEY', None), ('SUBKEY_SIGNED_MANIFEST', 'VALID_KEY_SUBKEY', None), + ("POST_EXPIRATION_SIGNED_MANIFEST", "VALID_PUBLIC_KEY", None), + # == Manifest signed before the key expired == + ("SIGNED_MANIFEST", "EXPIRED_PUBLIC_KEY", None), # == Manifest with two signatures == ("TWO_SIGNATURE_MANIFEST", "TWO_SIGNATURE_PUBLIC_KEYS", None), ("TWO_SIGNATURE_MANIFEST", "VALID_PUBLIC_KEY", OpenPGPVerificationFailure), @@ -402,7 +424,7 @@ MANIFEST_VARIANTS = [ # == bad keys == ('SIGNED_MANIFEST', None, OpenPGPVerificationFailure), - ('SIGNED_MANIFEST', 'EXPIRED_PUBLIC_KEY', + ("POST_EXPIRATION_SIGNED_MANIFEST", "EXPIRED_PUBLIC_KEY", OpenPGPExpiredKeyFailure), ('SIGNED_MANIFEST', 'REVOKED_PUBLIC_KEY', OpenPGPRevokedKeyFailure), @@ -451,6 +473,12 @@ def assert_signature(sig: OpenPGPSignatureList, assert sig.timestamp == SUBKEY_SIG_TIMESTAMP assert sig.expire_timestamp is None assert sig.primary_key_fingerprint == KEY_FINGERPRINT + elif manifest_var == "POST_EXPIRATION_SIGNED_MANIFEST": + assert len(sig) == 1 + assert sig.fingerprint == KEY_FINGERPRINT + assert sig.timestamp == datetime.datetime(2023, 2, 15, 7, 53, 19) + assert sig.expire_timestamp is None + assert sig.primary_key_fingerprint == KEY_FINGERPRINT else: assert len(sig) == 1 assert sig.fingerprint == KEY_FINGERPRINT @@ -482,7 +510,7 @@ def test_verify_manifest(openpgp_env, manifest_var, key_var, expected): with io.BytesIO(globals()[key_var]) as kf: openpgp_env.import_key(kf) - openpgp_env.verify_file(f) + print(openpgp_env.verify_file(f)) except OpenPGPNoImplementation as e: pytest.skip(str(e)) @@ -825,16 +853,16 @@ REFRESH_VARIANTS = [ ('SIGNED_MANIFEST', 'EXPIRED_PUBLIC_KEY', KEY_FINGERPRINT, 'UNEXPIRE_PUBLIC_KEY', None), # ...but only with a new signature - ('SIGNED_MANIFEST', 'EXPIRED_PUBLIC_KEY', KEY_FINGERPRINT, - 'OLD_UNEXPIRE_PUBLIC_KEY', OpenPGPExpiredKeyFailure), + ("POST_EXPIRATION_SIGNED_MANIFEST", "EXPIRED_PUBLIC_KEY", KEY_FINGERPRINT, + "OLD_UNEXPIRE_PUBLIC_KEY", OpenPGPExpiredKeyFailure), # make sure server can't malicously inject or replace key ('SIGNED_MANIFEST', 'OTHER_VALID_PUBLIC_KEY', OTHER_KEY_FINGERPRINT, 'VALID_PUBLIC_KEY', OpenPGPKeyRefreshError), ('SIGNED_MANIFEST', 'OTHER_VALID_PUBLIC_KEY', OTHER_KEY_FINGERPRINT, 'COMBINED_PUBLIC_KEYS', OpenPGPRuntimeError), # test that forged keys are rejected - ('SIGNED_MANIFEST', 'EXPIRED_PUBLIC_KEY', KEY_FINGERPRINT, - 'FORGED_UNEXPIRE_KEY', OpenPGPExpiredKeyFailure), + ("POST_EXPIRATION_SIGNED_MANIFEST", "EXPIRED_PUBLIC_KEY", KEY_FINGERPRINT, + "FORGED_UNEXPIRE_KEY", OpenPGPExpiredKeyFailure), ('SUBKEY_SIGNED_MANIFEST', 'VALID_PUBLIC_KEY', KEY_FINGERPRINT, 'FORGED_SUBKEY', OpenPGPVerificationFailure), ('SUBKEY_SIGNED_MANIFEST', 'VALID_PUBLIC_KEY', KEY_FINGERPRINT, -- cgit v1.2.3