diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 6175dcd3..a5d78efe 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -10,6 +10,11 @@ The third digit is only for regressions. Backward-incompatible changes: ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +- ``OpenSSL.crypto.X509.set_serial_number()`` now raises ``ValueError`` for a negative serial number, which + `RFC 5280 section 4.1.2.2 `_ does not permit. + Passing one previously crashed the interpreter. + [`#1534 `_] + Deprecations: ^^^^^^^^^^^^^ diff --git a/src/OpenSSL/crypto.py b/src/OpenSSL/crypto.py index a7e8de69..c4cfff3b 100644 --- a/src/OpenSSL/crypto.py +++ b/src/OpenSSL/crypto.py @@ -1004,14 +1004,18 @@ def set_serial_number(self, serial: int) -> None: if not isinstance(serial, int): raise TypeError("serial must be an integer") - hex_serial = hex(serial)[2:] + if serial < 0: + raise ValueError("serial must be non-negative") + + hex_serial = f"{serial:x}" hex_serial_bytes = hex_serial.encode("ascii") bignum_serial = _ffi.new("BIGNUM**") - # BN_hex2bn stores the result in &bignum. + # BN_hex2bn returns the count of characters consumed, or 0 on error -- + # not a pointer, so it must not be compared against NULL. result = _lib.BN_hex2bn(bignum_serial, hex_serial_bytes) - _openssl_assert(result != _ffi.NULL) + _openssl_assert(result != 0) asn1_serial = _lib.BN_to_ASN1_INTEGER(bignum_serial[0], _ffi.NULL) _lib.BN_free(bignum_serial[0]) diff --git a/tests/test_crypto.py b/tests/test_crypto.py index 7508d1d0..5db93427 100644 --- a/tests/test_crypto.py +++ b/tests/test_crypto.py @@ -1471,6 +1471,23 @@ def test_serial_number(self) -> None: certificate.set_serial_number(2**128 + 1) assert certificate.get_serial_number() == 2**128 + 1 + def test_negative_serial_number(self) -> None: + """ + `X509.set_serial_number` rejects a negative serial number, which + RFC 5280 section 4.1.2.2 does not permit. Before this was checked, + a negative value crashed the interpreter. + """ + certificate = X509() + for serial in [-1, -255, -(2**128 + 1)]: + with pytest.raises(ValueError): + certificate.set_serial_number(serial) + + # A serial that was set earlier is left alone by the rejected call. + certificate.set_serial_number(1) + with pytest.raises(ValueError): + certificate.set_serial_number(-1) + assert certificate.get_serial_number() == 1 + def _setBoundTest( self, get: typing.Callable[[X509], bytes | None],