From 6e2a7f5bd2c06ab0656d51267f89e0d8a5eebd1e Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 16 Dec 2022 10:04:32 -0500 Subject: [PATCH 1/3] Fix CRL nextUpdate handling. When setting the nextUpdate field of a CRL, this code grabbed the nextUpdate ASN1_TIME field from the CRL and set its time. But nextUpdate is optional in a CRL so that field is usually NULL. But OpenSSL's ASN1_TIME_set_string succeeds when the destination argument is NULL, so it was silently a no-op. Given that, the call in a test to set the nextUpdate field suddenly starts working and sets the time to 2018, thus causing the CRL to be considered expired and breaking the test. So this change also changes the expiry year far into the future. Additionally, the other CRL and Revoked setters violate const in the API. Fixes #1168. --- src/OpenSSL/crypto.py | 43 ++++++++++++++++++++++++++++++++++--------- tests/test_crypto.py | 4 +++- 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/src/OpenSSL/crypto.py b/src/OpenSSL/crypto.py index 4d7d03a49..4a9fa0184 100644 --- a/src/OpenSSL/crypto.py +++ b/src/OpenSSL/crypto.py @@ -168,12 +168,35 @@ def _set_asn1_time(boundary: Any, when: bytes) -> None: """ if not isinstance(when, bytes): raise TypeError("when must be a byte string") + if boundary == _ffi.NULL: + # ASN1_TIME_set_string validates the string without writing anything + # when the destination is NULL. + raise ValueError("boundary must not be NULL") set_result = _lib.ASN1_TIME_set_string(boundary, when) if set_result == 0: raise ValueError("Invalid string") +def _new_asn1_time(when: bytes) -> Any: + """ + Behaves like _set_asn1_time bit returns a new ASN1_TIME object. + + @param when: A string representation of the desired time value. + + @raise TypeError: If C{when} is not a L{bytes} string. + @raise ValueError: If C{when} does not represent a time in the required + format. + @raise RuntimeError: If the time value cannot be set for some other + (unspecified) reason. + """ + ret = _lib.ASN1_TIME_new() + _openssl_assert(ret != _ffi.NULL) + ret = _ffi.gc(ret, _lib.ASN1_TIME_free) + _set_asn1_time(ret, when) + return ret + + def _get_asn1_time(timestamp: Any) -> Optional[bytes]: """ Retrieve the time value of an ASN1 time object. @@ -2283,8 +2306,11 @@ def set_rev_date(self, when: bytes) -> None: as ASN.1 TIME. :return: ``None`` """ - dt = _lib.X509_REVOKED_get0_revocationDate(self._revoked) - return _set_asn1_time(dt, when) + revocationDate = _new_asn1_time(when) + ret = _lib.X509_REVOKED_set_revocationDate( + self._revoked, revocationDate + ) + _openssl_assert(ret == 1) def get_rev_date(self) -> Optional[bytes]: """ @@ -2406,11 +2432,6 @@ def set_version(self, version: int) -> None: """ _openssl_assert(_lib.X509_CRL_set_version(self._crl, version) != 0) - def _set_boundary_time( - self, which: Callable[..., Any], when: bytes - ) -> None: - return _set_asn1_time(which(self._crl), when) - def set_lastUpdate(self, when: bytes) -> None: """ Set when the CRL was last updated. @@ -2424,7 +2445,9 @@ def set_lastUpdate(self, when: bytes) -> None: :param bytes when: A timestamp string. :return: ``None`` """ - return self._set_boundary_time(_lib.X509_CRL_get0_lastUpdate, when) + lastUpdate = _new_asn1_time(when) + ret = _lib.X509_CRL_set1_lastUpdate(self._crl, lastUpdate) + _openssl_assert(ret == 1) def set_nextUpdate(self, when: bytes) -> None: """ @@ -2439,7 +2462,9 @@ def set_nextUpdate(self, when: bytes) -> None: :param bytes when: A timestamp string. :return: ``None`` """ - return self._set_boundary_time(_lib.X509_CRL_get0_nextUpdate, when) + nextUpdate = _new_asn1_time(when) + ret = _lib.X509_CRL_set1_nextUpdate(self._crl, nextUpdate) + _openssl_assert(ret == 1) def sign(self, issuer_cert: X509, issuer_key: PKey, digest: bytes) -> None: """ diff --git a/tests/test_crypto.py b/tests/test_crypto.py index 88756f04b..44bbd0f2a 100644 --- a/tests/test_crypto.py +++ b/tests/test_crypto.py @@ -3850,7 +3850,9 @@ def _make_test_crl(self, issuer_cert, issuer_key, certs=()): crl.add_revoked(revoked) crl.set_version(1) crl.set_lastUpdate(b"20140601000000Z") - crl.set_nextUpdate(b"20180601000000Z") + # The year 5000 is far into the future so that this CRL isn't + # considered to have expired. + crl.set_nextUpdate(b"50000601000000Z") crl.sign(issuer_cert, issuer_key, digest=b"sha512") return crl From fa495a94d0f78ba78f8c0cda26e82f2ac7fa3b6f Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 16 Dec 2022 10:22:38 -0500 Subject: [PATCH 2/3] Replace self-check with an assert for coverage --- src/OpenSSL/crypto.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/OpenSSL/crypto.py b/src/OpenSSL/crypto.py index 4a9fa0184..5943bcb6e 100644 --- a/src/OpenSSL/crypto.py +++ b/src/OpenSSL/crypto.py @@ -168,10 +168,9 @@ def _set_asn1_time(boundary: Any, when: bytes) -> None: """ if not isinstance(when, bytes): raise TypeError("when must be a byte string") - if boundary == _ffi.NULL: - # ASN1_TIME_set_string validates the string without writing anything - # when the destination is NULL. - raise ValueError("boundary must not be NULL") + # ASN1_TIME_set_string validates the string without writing anything + # when the destination is NULL. + _openssl_assert(boundary != _ffi.NULL) set_result = _lib.ASN1_TIME_set_string(boundary, when) if set_result == 0: From 22e7a557c0fa779f53a6c2375e4365a799e3802f Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 16 Dec 2022 10:36:08 -0500 Subject: [PATCH 3/3] Update src/OpenSSL/crypto.py Co-authored-by: Alex Gaynor --- src/OpenSSL/crypto.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenSSL/crypto.py b/src/OpenSSL/crypto.py index 5943bcb6e..a7c214080 100644 --- a/src/OpenSSL/crypto.py +++ b/src/OpenSSL/crypto.py @@ -179,7 +179,7 @@ def _set_asn1_time(boundary: Any, when: bytes) -> None: def _new_asn1_time(when: bytes) -> Any: """ - Behaves like _set_asn1_time bit returns a new ASN1_TIME object. + Behaves like _set_asn1_time but returns a new ASN1_TIME object. @param when: A string representation of the desired time value.