support negative serials in certificate parsing (#6626)

* support negative serials in certificate parsing

but raise a warning every time we see it. also proactively raise on
initial parse of the certificate, not just when accessing the
serial_number attribute

* cargo fmt

* review feedback and changelog

* pssh

* Update CHANGELOG.rst

Co-authored-by: Alex Gaynor <alex.gaynor@gmail.com>

Co-authored-by: Alex Gaynor <alex.gaynor@gmail.com>

8 files changed, 60 insertions, 26 deletions

diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 3ce8283b4..1c88a69d2 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -28,6 +28,10 @@ Changelog
   in the same file.
 * Fixed parsing of :class:`~cryptography.x509.CertificatePolicies` extensions
   containing legacy ``BMPString`` values in their ``explicitText``.
+* Allow parsing of negative serial numbers in certificates. Negative serial
+  numbers are prohibited by :rfc:`5280` so a deprecation warning will be
+  raised whenever they are encountered. A future version of ``cryptography``
+  will drop support for parsing them.
 * Added support for parsing PKCS12 files with friendly names for all
   certificates with
   :func:`~cryptography.hazmat.primitives.serialization.pkcs12.load_pkcs12`,
diff --git a/src/rust/src/asn1.rs b/src/rust/src/asn1.rs
index 49f44ee45..efb340f04 100644
--- a/src/rust/src/asn1.rs
+++ b/src/rust/src/asn1.rs
@@ -4,6 +4,7 @@
 
 use crate::x509::Name;
 use pyo3::basic::CompareOp;
+use pyo3::types::IntoPyDict;
 use pyo3::ToPyObject;
 
 pub enum PyAsn1Error {
@@ -77,12 +78,13 @@ struct DssSignature<'a> {
     s: asn1::BigUint<'a>,
 }
 
-pub(crate) fn big_asn1_uint_to_py<'p>(
+pub(crate) fn big_byte_slice_to_py_int<'p>(
     py: pyo3::Python<'p>,
-    v: asn1::BigUint<'_>,
+    v: &'_ [u8],
 ) -> pyo3::PyResult<&'p pyo3::PyAny> {
     let int_type = py.get_type::<pyo3::types::PyLong>();
-    int_type.call_method1("from_bytes", (v.as_bytes(), "big"))
+    let kwargs = [("signed", true)].into_py_dict(py);
+    int_type.call_method("from_bytes", (v, "big"), Some(kwargs))
 }
 
 #[pyo3::prelude::pyfunction]
@@ -90,8 +92,8 @@ fn decode_dss_signature(py: pyo3::Python<'_>, data: &[u8]) -> Result<pyo3::PyObj
     let sig = asn1::parse_single::<DssSignature<'_>>(data)?;
 
     Ok((
-        big_asn1_uint_to_py(py, sig.r)?,
-        big_asn1_uint_to_py(py, sig.s)?,
+        big_byte_slice_to_py_int(py, sig.r.as_bytes())?,
+        big_byte_slice_to_py_int(py, sig.s.as_bytes())?,
     )
         .to_object(py))
 }
diff --git a/src/rust/src/x509/certificate.rs b/src/rust/src/x509/certificate.rs
index b582640a2..4ab8d3702 100644
--- a/src/rust/src/x509/certificate.rs
+++ b/src/rust/src/x509/certificate.rs
@@ -2,7 +2,9 @@
 // 2.0, and the BSD License. See the LICENSE file in the root of this repository
 // for complete details.
 
-use crate::asn1::{big_asn1_uint_to_py, py_uint_to_big_endian_bytes, PyAsn1Error, PyAsn1Result};
+use crate::asn1::{
+    big_byte_slice_to_py_int, py_uint_to_big_endian_bytes, PyAsn1Error, PyAsn1Result,
+};
 use crate::x509;
 use crate::x509::{crl, extensions, oid, sct};
 use chrono::Datelike;
@@ -23,7 +25,7 @@ pub(crate) struct TbsCertificate<'a> {
     #[explicit(0)]
     #[default(0)]
     version: u8,
-    pub(crate) serial: asn1::BigUint<'a>,
+    pub(crate) serial: asn1::BigInt<'a>,
     signature_alg: x509::AlgorithmIdentifier<'a>,
 
     pub(crate) issuer: x509::Name<'a>,
@@ -178,10 +180,9 @@ impl Certificate {
 
     #[getter]
     fn serial_number<'p>(&self, py: pyo3::Python<'p>) -> Result<&'p pyo3::PyAny, PyAsn1Error> {
-        Ok(big_asn1_uint_to_py(
-            py,
-            self.raw.borrow_value().tbs_cert.serial,
-        )?)
+        let bytes = self.raw.borrow_value().tbs_cert.serial.as_bytes();
+        warn_if_negative_serial(py, bytes)?;
+        Ok(big_byte_slice_to_py_int(py, bytes)?)
     }
 
     #[getter]
@@ -350,12 +351,31 @@ fn load_der_x509_certificate(py: pyo3::Python<'_>, data: &[u8]) -> PyAsn1Result<
     let raw = OwnedRawCertificate::try_new(Arc::from(data), |data| asn1::parse_single(data))?;
     // Parse cert version immediately so we can raise error on parse if it is invalid.
     cert_version(py, raw.borrow_value().tbs_cert.version)?;
+    // determine if the serial is negative and raise a warning if it is. We want to drop support
+    // for this sort of invalid encoding eventually.
+    warn_if_negative_serial(py, raw.borrow_value().tbs_cert.serial.as_bytes())?;
+
     Ok(Certificate {
         raw,
         cached_extensions: None,
     })
 }
 
+fn warn_if_negative_serial(py: pyo3::Python<'_>, bytes: &'_ [u8]) -> pyo3::PyResult<()> {
+    if bytes[0] & 0x80 != 0 {
+        let cryptography_warning = py.import("cryptography.utils")?.getattr("DeprecatedIn36")?;
+        let warnings = py.import("warnings")?;
+        warnings.call_method1(
+            "warn",
+            (
+                "Parsed a negative serial number, which is disallowed by RFC 5280.",
+                cryptography_warning,
+            ),
+        )?;
+    }
+    Ok(())
+}
+
 // Needed due to clippy type complexity warning.
 type SequenceOfPolicyQualifiers<'a> = x509::Asn1ReadableOrWritable<
     'a,
@@ -441,7 +461,7 @@ fn parse_user_notice(
             let org = parse_display_text(py, data.organization)?;
             let numbers = pyo3::types::PyList::empty(py);
             for num in data.notice_numbers.unwrap_read().clone() {
-                numbers.append(big_asn1_uint_to_py(py, num)?.to_object(py))?;
+                numbers.append(big_byte_slice_to_py_int(py, num.as_bytes())?.to_object(py))?;
             }
             x509_module
                 .call_method1("NoticeReference", (org, numbers))?
@@ -699,7 +719,7 @@ pub(crate) fn parse_authority_key_identifier<'p>(
     let x509_module = py.import("cryptography.x509")?;
     let aki = asn1::parse_single::<AuthorityKeyIdentifier<'_>>(ext_data)?;
     let serial = match aki.authority_cert_serial_number {
-        Some(biguint) => big_asn1_uint_to_py(py, biguint)?.to_object(py),
+        Some(biguint) => big_byte_slice_to_py_int(py, biguint.as_bytes())?.to_object(py),
         None => py.None(),
     };
     let issuer = match aki.authority_cert_issuer {
@@ -836,7 +856,7 @@ pub fn parse_cert_ext<'p>(
         Ok(Some(x509_module.getattr("OCSPNoCheck")?.call0()?))
     } else if oid == *oid::INHIBIT_ANY_POLICY_OID {
         let bignum = asn1::parse_single::<asn1::BigUint<'_>>(ext_data)?;
-        let pynum = big_asn1_uint_to_py(py, bignum)?;
+        let pynum = big_byte_slice_to_py_int(py, bignum.as_bytes())?;
         Ok(Some(
             x509_module.getattr("InhibitAnyPolicy")?.call1((pynum,))?,
         ))
@@ -909,7 +929,7 @@ fn create_x509_certificate(
 
     let tbs_cert = TbsCertificate {
         version: builder.getattr("_version")?.getattr("value")?.extract()?,
-        serial: asn1::BigUint::new(py_uint_to_big_endian_bytes(py, py_serial)?).unwrap(),
+        serial: asn1::BigInt::new(py_uint_to_big_endian_bytes(py, py_serial)?).unwrap(),
         signature_alg: sigalg.clone(),
         issuer: x509::common::encode_name(py, builder.getattr("_issuer_name")?)?,
         validity: Validity {
diff --git a/src/rust/src/x509/crl.rs b/src/rust/src/x509/crl.rs
index 94117940a..e76fd740c 100644
--- a/src/rust/src/x509/crl.rs
+++ b/src/rust/src/x509/crl.rs
@@ -2,7 +2,9 @@
 // 2.0, and the BSD License. See the LICENSE file in the root of this repository
 // for complete details.
 
-use crate::asn1::{big_asn1_uint_to_py, py_uint_to_big_endian_bytes, PyAsn1Error, PyAsn1Result};
+use crate::asn1::{
+    big_byte_slice_to_py_int, py_uint_to_big_endian_bytes, PyAsn1Error, PyAsn1Result,
+};
 use crate::x509;
 use crate::x509::{certificate, extensions, oid};
 use pyo3::ToPyObject;
@@ -262,11 +264,11 @@ impl CertificateRevocationList {
             |oid, ext_data| {
                 if oid == &*oid::CRL_NUMBER_OID {
                     let bignum = asn1::parse_single::<asn1::BigUint<'_>>(ext_data)?;
-                    let pynum = big_asn1_uint_to_py(py, bignum)?;
+                    let pynum = big_byte_slice_to_py_int(py, bignum.as_bytes())?;
                     Ok(Some(x509_module.getattr("CRLNumber")?.call1((pynum,))?))
                 } else if oid == &*oid::DELTA_CRL_INDICATOR_OID {
                     let bignum = asn1::parse_single::<asn1::BigUint<'_>>(ext_data)?;
-                    let pynum = big_asn1_uint_to_py(py, bignum)?;
+                    let pynum = big_byte_slice_to_py_int(py, bignum.as_bytes())?;
                     Ok(Some(
                         x509_module.getattr("DeltaCRLIndicator")?.call1((pynum,))?,
                     ))
@@ -530,7 +532,7 @@ struct RevokedCertificate {
 impl RevokedCertificate {
     #[getter]
     fn serial_number<'p>(&self, py: pyo3::Python<'p>) -> pyo3::PyResult<&'p pyo3::PyAny> {
-        big_asn1_uint_to_py(py, self.raw.borrow_value().user_certificate)
+        big_byte_slice_to_py_int(py, self.raw.borrow_value().user_certificate.as_bytes())
     }
 
     #[getter]
diff --git a/src/rust/src/x509/ocsp.rs b/src/rust/src/x509/ocsp.rs
index ca340d0b1..0d8e1c071 100644
--- a/src/rust/src/x509/ocsp.rs
+++ b/src/rust/src/x509/ocsp.rs
@@ -33,7 +33,7 @@ pub(crate) struct CertID<'a> {
     pub(crate) hash_algorithm: x509::AlgorithmIdentifier<'a>,
     pub(crate) issuer_name_hash: &'a [u8],
     pub(crate) issuer_key_hash: &'a [u8],
-    pub(crate) serial_number: asn1::BigUint<'a>,
+    pub(crate) serial_number: asn1::BigInt<'a>,
 }
 
 impl CertID<'_> {
diff --git a/src/rust/src/x509/ocsp_req.rs b/src/rust/src/x509/ocsp_req.rs
index a3e161b9f..57b1391c6 100644
--- a/src/rust/src/x509/ocsp_req.rs
+++ b/src/rust/src/x509/ocsp_req.rs
@@ -2,7 +2,7 @@
 // 2.0, and the BSD License. See the LICENSE file in the root of this repository
 // for complete details.
 
-use crate::asn1::{big_asn1_uint_to_py, PyAsn1Error, PyAsn1Result};
+use crate::asn1::{big_byte_slice_to_py_int, PyAsn1Error, PyAsn1Result};
 use crate::x509;
 use crate::x509::{extensions, ocsp, oid};
 use std::sync::Arc;
@@ -95,7 +95,8 @@ impl OCSPRequest {
 
     #[getter]
     fn serial_number<'p>(&self, py: pyo3::Python<'p>) -> Result<&'p pyo3::PyAny, PyAsn1Error> {
-        Ok(big_asn1_uint_to_py(py, self.cert_id()?.serial_number)?)
+        let bytes = self.cert_id()?.serial_number.as_bytes();
+        Ok(big_byte_slice_to_py_int(py, bytes)?)
     }
 
     #[getter]
diff --git a/src/rust/src/x509/ocsp_resp.rs b/src/rust/src/x509/ocsp_resp.rs
index f8df0b282..66b5e2324 100644
--- a/src/rust/src/x509/ocsp_resp.rs
+++ b/src/rust/src/x509/ocsp_resp.rs
@@ -2,7 +2,7 @@
 // 2.0, and the BSD License. See the LICENSE file in the root of this repository
 // for complete details.
 
-use crate::asn1::{big_asn1_uint_to_py, PyAsn1Error, PyAsn1Result};
+use crate::asn1::{big_byte_slice_to_py_int, PyAsn1Error, PyAsn1Result};
 use crate::x509;
 use crate::x509::{certificate, crl, extensions, ocsp, oid, py_to_chrono, sct};
 use std::sync::Arc;
@@ -238,7 +238,7 @@ impl OCSPResponse {
     fn serial_number<'p>(&self, py: pyo3::Python<'p>) -> pyo3::PyResult<&'p pyo3::PyAny> {
         let resp = self.requires_successful_response()?;
         let single_resp = resp.single_response();
-        big_asn1_uint_to_py(py, single_resp.cert_id.serial_number)
+        big_byte_slice_to_py_int(py, single_resp.cert_id.serial_number.as_bytes())
     }
 
     #[getter]
diff --git a/tests/x509/test_x509.py b/tests/x509/test_x509.py
index 9e0a35bde..17076d992 100644
--- a/tests/x509/test_x509.py
+++ b/tests/x509/test_x509.py
@@ -735,13 +735,18 @@ class TestRSACertificate(object):
         assert cert == cert2
 
     def test_negative_serial_number(self, backend):
-        with pytest.raises(ValueError, match="TbsCertificate::serial"):
-            _load_cert(
+        # We load certificates with negative serial numbers but on load
+        # and on access of the attribute we raise a warning
+        with pytest.warns(utils.DeprecatedIn36):
+            cert = _load_cert(
                 os.path.join("x509", "custom", "negative_serial.pem"),
                 x509.load_pem_x509_certificate,
                 backend,
             )
 
+        with pytest.warns(utils.DeprecatedIn36):
+            assert cert.serial_number == -18008675309
+
     def test_alternate_rsa_with_sha1_oid(self, backend):
         cert = _load_cert(
             os.path.join("x509", "custom", "alternate-rsa-sha1-oid.der"),