diff --git a/docs/changelog.rst b/docs/changelog.rst index 1b89508..cefd0a7 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -6,7 +6,7 @@ v0.7.0 (unreleased) ------------------- * Remove dependency of ``pyasn1`` and ``pyasn1-modules`` entirely to provide more robust parsing of ASN.1 structures, adding the ability to parse structures independent of - RFC version. Certain bugs bugs we've encountered in the past, have now been resolved + RFC version. Certain bugs we've encountered in the past, have now been resolved as a result of this. On top of that, structures defined in the replacement, ``asn1crypto``, are a lot more Pythonic, and parsing speed has been sliced in more than half. @@ -37,6 +37,12 @@ v0.7.0 (unreleased) * Add support for ``SignerInfo`` versions other than v1 * Fix bug that could cause out-of-bound reads during parsing of the PE file's certificate table +* Correctly handle the lifetime-signing EKU (OID 1.3.6.1.4.1.311.10.3.13) by ignoring + the countersignature's timestamp during verification of the certification chain when + this is set on the end-entity's certificate. Note that the private + ``SignerInfo._verify_issuer`` has slightly changed semantics based on this. +* Return the certificate chain(s) in ``AuthenticodeSignedData.verify`` and + the used ``AuthenticodeSignedData`` and chains in ``SignedPEFile.verify`` * Parse the ``SpcPeImageData`` as part of the SpcInfo. This adds the attributes ``image_flags`` and ``image_publisher``, although this information is never used. diff --git a/signify/authenticode/signed_pe.py b/signify/authenticode/signed_pe.py index ea54b52..8c980cc 100644 --- a/signify/authenticode/signed_pe.py +++ b/signify/authenticode/signed_pe.py @@ -45,6 +45,7 @@ from signify.asn1.hashing import ACCEPTED_DIGEST_ALGORITHMS from signify.authenticode import structures from signify.exceptions import AuthenticodeNotSignedError, SignedPEParseError +from signify.x509 import Certificate logger = logging.getLogger(__name__) @@ -365,7 +366,7 @@ def verify( expected_hashes: dict[str, bytes] | None = None, ignore_parse_errors: bool = True, **kwargs: Any, - ) -> bool: + ) -> list[tuple[structures.AuthenticodeSignedData, Iterable[list[Certificate]]]]: """Verifies the SignedData structures. This is a little bit more efficient than calling all verify-methods separately. @@ -398,6 +399,8 @@ def verify( When :const:`False`, this will raise the :exc:`SignedPEParseError` as soon as one occurs. This often occurs before :exc:`AuthenticodeNotSignedError` is potentially raised. + :return: the used structure(s) in validation, as a list of tuples, in the form + (signed data object, certificate chain) :raises AuthenticodeVerificationError: when the verification failed :raises SignedPEParseError: for parse errors in the PEFile """ @@ -430,13 +433,21 @@ def verify( expected_hashes = self._calculate_expected_hashes(signed_datas, expected_hashes) # Now iterate over all SignedDatas + chains = [] last_error = None assert signed_datas for signed_data in signed_datas: try: - signed_data.verify( - expected_hash=expected_hashes[signed_data.digest_algorithm().name], - **kwargs, + chains.append( + ( + signed_data, + signed_data.verify( + expected_hash=expected_hashes[ + signed_data.digest_algorithm().name + ], + **kwargs, + ), + ) ) except Exception as e: # noqa: PERF203 # best and any are interpreted as any; first doesn't matter either way, @@ -446,9 +457,10 @@ def verify( last_error = e else: if multi_verify_mode not in ("all", "first"): - return True + # only return the last one, as we are in mode any/best + return chains[-1:] if last_error is None: - return True + return chains raise last_error def explain_verify( diff --git a/signify/authenticode/structures.py b/signify/authenticode/structures.py index 1fa9f0a..20708b3 100644 --- a/signify/authenticode/structures.py +++ b/signify/authenticode/structures.py @@ -275,6 +275,22 @@ def _parse(self) -> None: self.countersigner = RFC3161SignedData(ts_data["content"]) + def _verify_issuer( + self, + issuer: Certificate, + context: VerificationContext, + signing_time: datetime.datetime | None = None, + ) -> list[Certificate]: + """Check whether the lifetime signing EKU is set. if that is the case, we can + only use the timestamp for revocation checking, not for extending the lifetime + of the signature. Revocation checking currently does not work. + """ + if "microsoft_lifetime_signing" in issuer.extensions.get( + "extended_key_usage", [] + ): + signing_time = None + return super()._verify_issuer(issuer, context, signing_time) + class SpcInfo: """The Authenticode's SpcIndirectDataContent information, and their children. This @@ -417,7 +433,7 @@ def verify( trusted_certificate_store: CertificateStore = TRUSTED_CERTIFICATE_STORE, verification_context_kwargs: dict[str, Any] | None = None, countersignature_mode: Literal["strict", "permit", "ignore"] = "strict", - ) -> None: + ) -> Iterable[list[Certificate]]: """Verifies the SignedData structure: * Verifies that the digest algorithms match across the structure @@ -468,7 +484,7 @@ def verify( checked, but when it errors, it is verified as if the countersignature was never set. When set to 'ignore', countersignatures are never checked. :raises AuthenticodeVerificationError: when the verification failed - :return: :const:`None` + :return: A list of valid certificate chains for this SignedData. """ if verification_context_kwargs is None: @@ -532,6 +548,7 @@ def verify( # Can't check authAttr hash against encrypted hash, done implicitly in # M2's pubkey.verify. + signing_time = None if self.signer_info.countersigner and countersignature_mode != "ignore": assert cs_verification_context is not None @@ -565,11 +582,9 @@ def verify( else: # If no errors occur, we should be fine setting the timestamp to the # countersignature's timestamp - verification_context.timestamp = ( - self.signer_info.countersigner.signing_time - ) + signing_time = self.signer_info.countersigner.signing_time - self.signer_info.verify(verification_context) + return self.signer_info.verify(verification_context, signing_time) def explain_verify( self, *args: Any, **kwargs: Any diff --git a/signify/pkcs7/signerinfo.py b/signify/pkcs7/signerinfo.py index 99ccd92..948a292 100644 --- a/signify/pkcs7/signerinfo.py +++ b/signify/pkcs7/signerinfo.py @@ -268,16 +268,17 @@ def _encode_attributes(cls, data: cms.CMSAttributes) -> bytes: new_attrs = type(data)(contents=data.contents) return cast(bytes, new_attrs.dump()) - def _verify_issuer(self, issuer: Certificate, context: VerificationContext) -> None: - """Verifies whether the given issuer is valid for the given context. Similar to - :meth:`Certificate._verify_issuer`. Does not support legacy verification method. + def _verify_issuer_signature( + self, issuer: Certificate, context: VerificationContext + ) -> None: + """Check the issuer signature against the information in the class. Use + :meth:`_verify_issuer` for full verification. :param Certificate issuer: The Certificate to verify - :param VerificationContext context: The + :param VerificationContext context: The context for verification + :raises SignerInfoVerificationError: If the issuer signature is invalid """ - issuer.verify(context) - try: issuer.verify_signature( self.encrypted_digest, @@ -291,9 +292,31 @@ def _verify_issuer(self, issuer: Certificate, context: VerificationContext) -> N f" attributes in {type(self).__name__}: {e}" ) + def _verify_issuer( + self, + issuer: Certificate, + context: VerificationContext, + signing_time: datetime.datetime | None = None, + ) -> list[Certificate]: + """Verifies whether the given issuer is valid for this :class:`SignerInfo`, + and valid in the given context. Similar to :meth:`Certificate._verify_issuer`. + + It adds the ``signing_time`` to the context if necessary. + """ + + # _verify_issuer_signature may fail when it is not a valid issuer for + # this SignedInfo + self._verify_issuer_signature(issuer, context) + + if signing_time is not None: + context.timestamp = signing_time + return context.verify(issuer) + def _build_chain( - self, context: VerificationContext - ) -> Iterable[Iterable[Certificate]]: + self, + context: VerificationContext, + signing_time: datetime.datetime | None = None, + ) -> Iterable[list[Certificate]]: """Given a context, builds a chain up to a trusted certificate. This is a generator function, generating all valid chains. @@ -303,6 +326,7 @@ def _build_chain( :param VerificationContext context: The context for building the chain. Most importantly, contains all certificates to build the chain from, but also their properties are relevant. + :param signing_time: The time to be used as timestamp when creating the chain :return: Iterable of all of the valid chains from this SignedInfo up to and including a trusted anchor. Note that this may be an empty iteration if no candidate parent certificate was found. @@ -323,12 +347,8 @@ def _build_chain( issuer=self.issuer, serial_number=self.serial_number ): try: - # _verify_issuer may fail when it is not a valid issuer for this - # SignedInfo - self._verify_issuer(issuer, context) - - # _build_chain may fail when anywhere up its chain an error occurs - yield context.verify(issuer) + # may fail when anywhere up its chain an error occurs + yield self._verify_issuer(issuer, context, signing_time) except VerificationError as e: # noqa: PERF203 if first_error is None: first_error = e @@ -338,18 +358,23 @@ def _build_chain( if first_error: raise first_error - def verify(self, context: VerificationContext) -> Iterable[Iterable[Certificate]]: + def verify( + self, + context: VerificationContext, + signing_time: datetime.datetime | None = None, + ) -> Iterable[list[Certificate]]: """Verifies that this :class:`SignerInfo` verifies up to a chain with the root of a trusted certificate. :param VerificationContext context: The context for verifying the SignerInfo. + :param signing_time: The time to be used as timestamp when creating the chain :return: A list of valid certificate chains for this SignerInfo. :rtype: Iterable[Iterable[Certificate]] :raises AuthenticodeVerificationError: When the SignerInfo could not be verified. """ - chains = list(self._build_chain(context)) + chains = list(self._build_chain(context, signing_time)) if not chains: raise SignerInfoVerificationError( @@ -361,7 +386,7 @@ def verify(self, context: VerificationContext) -> Iterable[Iterable[Certificate] def potential_chains( self, context: VerificationContext - ) -> Iterable[Iterable[Certificate]]: + ) -> Iterable[list[Certificate]]: """Retrieves all potential chains from this SignerInfo instance. :param VerificationContext context: The context diff --git a/signify/x509/context.py b/signify/x509/context.py index e1632f8..1350799 100644 --- a/signify/x509/context.py +++ b/signify/x509/context.py @@ -310,7 +310,7 @@ def potential_chains( else: yield [*chain, certificate] - def verify(self, certificate: Certificate) -> Iterable[Certificate]: + def verify(self, certificate: Certificate) -> list[Certificate]: """Verifies the certificate, and its chain. :param Certificate certificate: The certificate to verify diff --git a/tests/test_authenticode.py b/tests/test_authenticode.py index 57e2644..0194e3a 100644 --- a/tests/test_authenticode.py +++ b/tests/test_authenticode.py @@ -33,12 +33,13 @@ ) from signify.authenticode.signed_pe import SignedPEFile from signify.exceptions import ( + AuthenticodeNotSignedError, AuthenticodeVerificationError, SignedPEParseError, VerificationError, - AuthenticodeNotSignedError, ) from signify.fingerprinter import AuthenticodeFingerprinter +from signify.x509 import Certificate from signify.x509.context import ( CertificateStore, FileSystemCertificateStore, @@ -340,6 +341,45 @@ def test_signerinfo_v0(self): pefile = SignedPEFile(f) pefile.verify() + def test_lifetime_signing(self): + """this tests a sample that has a valid countersignature and a lifetime signing + flag. it is self-signed, so we need to load that one as well + """ + certificate_store = CertificateStore( + list(TRUSTED_CERTIFICATE_STORE_NO_CTL) + + list( + Certificate.from_pems( + (root_dir / "test_data" / "kdbazis.dll.crt").read_bytes() + ) + ), + trusted=True, + ) + + with open( + str(root_dir / "test_data" / "kdbazis.dll"), + "rb", + ) as f: + pefile = SignedPEFile(f) + # should verify on this date + pefile.verify( + trusted_certificate_store=certificate_store, + verification_context_kwargs={ + "timestamp": datetime.datetime( + 2024, 1, 1, tzinfo=datetime.timezone.utc + ) + }, + ) + # should not verify on this date + with self.assertRaises(VerificationError): + pefile.verify( + trusted_certificate_store=certificate_store, + verification_context_kwargs={ + "timestamp": datetime.datetime( + 2060, 1, 1, tzinfo=datetime.timezone.utc + ) + }, + ) + class CertificateTestCase(unittest.TestCase): def test_all_trusted_certificates_are_trusted(self): diff --git a/tests/test_data/kdbazis.dll b/tests/test_data/kdbazis.dll new file mode 100644 index 0000000..93f920b Binary files /dev/null and b/tests/test_data/kdbazis.dll differ diff --git a/tests/test_data/kdbazis.dll.crt b/tests/test_data/kdbazis.dll.crt new file mode 100644 index 0000000..2577956 --- /dev/null +++ b/tests/test_data/kdbazis.dll.crt @@ -0,0 +1,30 @@ +-----BEGIN CERTIFICATE----- +MIIFJzCCAw+gAwIBAgIQNbHe1o7U14hPfhG6QHfQ0jANBgkqhkiG9w0BAQsFADAd +MRswGQYDVQQDExJWaXJ0dWFsS0QtUmVkdXggQ0EwHhcNMjAxMTA3MTUyMDA2WhcN +MzkxMjMxMjM1OTU5WjAdMRswGQYDVQQDExJWaXJ0dWFsS0QtUmVkdXggQ0EwggIi +MA0GCSqGSIb3DQEBAQUAA4ICDwAwggIKAoICAQCw4HhzBkL9X8i8mIa5Xol9L2Il +T289kTPS91Im0iJuMCdJanzCqPj/WjiVqL0ptjun0GNnPu3N26cImD+bdO/xEr0/ +ESnYzR6xgqbNEEL7mw9Jw6iIV91XBSguOCu8Rhibrcddh85qvcwDiUQjANeMxPd8 +dUNajJaDAg8bdu2EKXQkO6VAHf6J1RvBJW2jjNu8R6Tjyj+HdkS4zedD5t/aJ8wI +TPasW843xqPOzUknzzYK2MW+IeXzmWU82T4QvNpJQnFF7ChB+y9x0IxfklddBWUN +ZTNo2Dc4i3VSB41jxfmF96Hy8lT7TgKB+ab8hjHT0japbSOMgFh/jsjUIOo/uglW +9EIbWPh0gL2kdGs8qPBPRO20vXrOnjbeh8s83Wd6GLxTTXTwuMaOY8A47zkCZlWE +f0rf4kix7MNVXNvsNvOjT0FUQiE/OhhdFKRgViaQsKIcQtKVyL9Osr5hygOtLvkx +/17nd7+NaZkzPIIagUd1YJN9JhlCs7KAuyGeWcsFKKDNxGScJrLrZvick8eL10/6 +GR4lJfUKSIx+1+MT6hvB5incWLhaKKcUF2vPib+x7TCabtZMymq0IdHdpiKcPzlZ +ZnTQQAKHW1HYr6o+sNJRR8QKd2T1JnGLQ0Vg/I0/OJnIu5HkFrtbpuFKVKnG5LeR +3b7VmxjPGev2fkIm7QIDAQABo2MwYTAPBgNVHRMBAf8EBTADAQH/ME4GA1UdAQRH +MEWAEIXZMI6puXhWlnEv8ouzxC+hHzAdMRswGQYDVQQDExJWaXJ0dWFsS0QtUmVk +dXggQ0GCEDWx3taO1NeIT34RukB30NIwDQYJKoZIhvcNAQELBQADggIBAHUC3v/L +YIFpT4MISPMmMnLMVv8ra53Qvt2Y3ykqCcq3SO7jDAseMk27oNzdRW7hsP66BfO5 +uYLAyjAsgw+wta2h5L6jWlvPZQFEXgKhzd4cPnFZnQs4KxaUabW2HIrNHnJkgmss +tNmT74YrwTHbg5oI/+RVf+24VLunmnngqmTS37R3bGMZtrmi0lTkTIQ1NrWASoAG +zZTKQQiF1dUvDkC3uNQfgJ2b/sYVohUIJiVncLWnXA30ElX6wlhuZNnr72dCzbD/ ++23uCWHY2h1FQvVzxFo+tInsz0ijjXAOAwYtjQm1jw7QwbWIQVSvARAMsD0coM9V +LgZ1G+hdOeQjt/h3CJ1I7jWDaDY9ssgVqEAEr8eYHkUvmJBzdX07tutytCT+aFQP +/D8Fsj/5KHw5f0zwEoUJjzgrIksaMS048ERifSkrE1UmQCbcmDW8VJM+p6hUC5/z +bkBPHJrDcauc64f5SmIPqeP3fPQYykvI1DrVa31JmP8yITWEvzETetA21BHk3ArR +PqRHYepdLPkpBfwiW5nHUln118aajp4B1npMGli2rSpXW0sTVjLQQaqVO5YsW8ea +pG3p7LV39NWVJ8BjCQCTEXnkgn7PW6n3QV5kNz2vHE4PC4AhqUN1CXV6B0k3X2tR +wocCMeN+1tepOsR9Ut/zIf2ISb8AF+NH3/3Z +-----END CERTIFICATE----- diff --git a/tests/test_fingerprinter.py b/tests/test_fingerprinter.py index 0347e21..d3ccac1 100644 --- a/tests/test_fingerprinter.py +++ b/tests/test_fingerprinter.py @@ -22,6 +22,7 @@ import hashlib import io import json +import os.path import pathlib import unittest @@ -40,7 +41,9 @@ class FingerPrinterTestCase(unittest.TestCase): def test_entire_blobs(self): for filename in (root_dir / "test_data").iterdir(): - if str(filename).endswith(".res") or str(filename).endswith("README.rst"): + if str(filename).endswith(".res") or not os.path.exists( + str(filename) + ".res" + ): continue with self.subTest(filename): with open(str(filename), "rb") as file_obj: