From d2f0aec1033181ab2f256e78915cdfdccc31b977 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 16 Dec 2022 10:44:52 -0500 Subject: Fix CRL nextUpdate handling. (#1169) * 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. * Replace self-check with an assert for coverage * Update src/OpenSSL/crypto.py Co-authored-by: Alex Gaynor Co-authored-by: Alex Gaynor --- src/OpenSSL/crypto.py | 42 +++++++++++++++++++++++++++++++++--------- tests/test_crypto.py | 4 +++- 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/src/OpenSSL/crypto.py b/src/OpenSSL/crypto.py index 3224103..17fd522 100644 --- a/src/OpenSSL/crypto.py +++ b/src/OpenSSL/crypto.py @@ -168,12 +168,34 @@ def _set_asn1_time(boundary: Any, when: bytes) -> None: """ if not isinstance(when, bytes): raise TypeError("when must be a byte string") + # 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: raise ValueError("Invalid string") +def _new_asn1_time(when: bytes) -> Any: + """ + Behaves like _set_asn1_time but 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 +2305,11 @@ class Revoked: 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 +2431,6 @@ class CRL: """ _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 +2444,9 @@ class CRL: :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 +2461,9 @@ class CRL: :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 88756f0..44bbd0f 100644 --- a/tests/test_crypto.py +++ b/tests/test_crypto.py @@ -3850,7 +3850,9 @@ class TestCRL: 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 -- cgit v1.2.1