From d0d44071fa7655194f8784c0e8f3cf66e34fb1b9 Mon Sep 17 00:00:00 2001 From: Facundo Tuesca Date: Mon, 22 Jan 2024 12:36:03 +0100 Subject: [PATCH 1/3] Migrate PKCS7 backend to Rust --- .../hazmat/backends/openssl/backend.py | 57 +-------------- .../hazmat/bindings/_rust/pkcs7.pyi | 6 ++ .../hazmat/primitives/serialization/pkcs7.py | 14 +--- src/rust/cryptography-openssl/Cargo.toml | 2 +- src/rust/src/pkcs7.rs | 70 ++++++++++++++++++- src/rust/src/x509/certificate.rs | 2 +- 6 files changed, 79 insertions(+), 72 deletions(-) diff --git a/src/cryptography/hazmat/backends/openssl/backend.py b/src/cryptography/hazmat/backends/openssl/backend.py index 1cb68c33ac74..5d9eb2768dfb 100644 --- a/src/cryptography/hazmat/backends/openssl/backend.py +++ b/src/cryptography/hazmat/backends/openssl/backend.py @@ -10,7 +10,7 @@ import typing from cryptography import utils, x509 -from cryptography.exceptions import UnsupportedAlgorithm, _Reasons +from cryptography.exceptions import UnsupportedAlgorithm from cryptography.hazmat.backends.openssl import aead from cryptography.hazmat.backends.openssl.ciphers import _CipherContext from cryptography.hazmat.bindings._rust import openssl as rust_openssl @@ -863,61 +863,6 @@ def poly1305_supported(self) -> bool: def pkcs7_supported(self) -> bool: return not self._lib.CRYPTOGRAPHY_IS_BORINGSSL - def load_pem_pkcs7_certificates( - self, data: bytes - ) -> list[x509.Certificate]: - utils._check_bytes("data", data) - bio = self._bytes_to_bio(data) - p7 = self._lib.PEM_read_bio_PKCS7( - bio.bio, self._ffi.NULL, self._ffi.NULL, self._ffi.NULL - ) - if p7 == self._ffi.NULL: - self._consume_errors() - raise ValueError("Unable to parse PKCS7 data") - - p7 = self._ffi.gc(p7, self._lib.PKCS7_free) - return self._load_pkcs7_certificates(p7) - - def load_der_pkcs7_certificates( - self, data: bytes - ) -> list[x509.Certificate]: - utils._check_bytes("data", data) - bio = self._bytes_to_bio(data) - p7 = self._lib.d2i_PKCS7_bio(bio.bio, self._ffi.NULL) - if p7 == self._ffi.NULL: - self._consume_errors() - raise ValueError("Unable to parse PKCS7 data") - - p7 = self._ffi.gc(p7, self._lib.PKCS7_free) - return self._load_pkcs7_certificates(p7) - - def _load_pkcs7_certificates(self, p7) -> list[x509.Certificate]: - nid = self._lib.OBJ_obj2nid(p7.type) - self.openssl_assert(nid != self._lib.NID_undef) - if nid != self._lib.NID_pkcs7_signed: - raise UnsupportedAlgorithm( - "Only basic signed structures are currently supported. NID" - f" for this data was {nid}", - _Reasons.UNSUPPORTED_SERIALIZATION, - ) - - if p7.d.sign == self._ffi.NULL: - raise ValueError( - "The provided PKCS7 has no certificate data, but a cert " - "loading method was called." - ) - - sk_x509 = p7.d.sign.cert - num = self._lib.sk_X509_num(sk_x509) - certs: list[x509.Certificate] = [] - for i in range(num): - x509 = self._lib.sk_X509_value(sk_x509, i) - self.openssl_assert(x509 != self._ffi.NULL) - cert = self._ossl2cert(x509) - certs.append(cert) - - return certs - class GetCipherByName: def __init__(self, fmt: str): diff --git a/src/cryptography/hazmat/bindings/_rust/pkcs7.pyi b/src/cryptography/hazmat/bindings/_rust/pkcs7.pyi index 32c21c4c5439..a84978246572 100644 --- a/src/cryptography/hazmat/bindings/_rust/pkcs7.pyi +++ b/src/cryptography/hazmat/bindings/_rust/pkcs7.pyi @@ -13,3 +13,9 @@ def sign_and_serialize( encoding: serialization.Encoding, options: typing.Iterable[pkcs7.PKCS7Options], ) -> bytes: ... +def load_pem_pkcs7_certificates( + data: bytes, +) -> list[x509.Certificate]: ... +def load_der_pkcs7_certificates( + data: bytes, +) -> list[x509.Certificate]: ... diff --git a/src/cryptography/hazmat/primitives/serialization/pkcs7.py b/src/cryptography/hazmat/primitives/serialization/pkcs7.py index cd6c904df0ea..bae35c5f5988 100644 --- a/src/cryptography/hazmat/primitives/serialization/pkcs7.py +++ b/src/cryptography/hazmat/primitives/serialization/pkcs7.py @@ -17,22 +17,12 @@ from cryptography.hazmat.primitives.asymmetric import ec, padding, rsa from cryptography.utils import _check_byteslike +load_pem_pkcs7_certificates = rust_pkcs7.load_pem_pkcs7_certificates -def load_pem_pkcs7_certificates(data: bytes) -> list[x509.Certificate]: - from cryptography.hazmat.backends.openssl.backend import backend - - return backend.load_pem_pkcs7_certificates(data) - - -def load_der_pkcs7_certificates(data: bytes) -> list[x509.Certificate]: - from cryptography.hazmat.backends.openssl.backend import backend - - return backend.load_der_pkcs7_certificates(data) - +load_der_pkcs7_certificates = rust_pkcs7.load_der_pkcs7_certificates serialize_certificates = rust_pkcs7.serialize_certificates - PKCS7HashTypes = typing.Union[ hashes.SHA224, hashes.SHA256, diff --git a/src/rust/cryptography-openssl/Cargo.toml b/src/rust/cryptography-openssl/Cargo.toml index 9de75a80c88f..3a35c9fcaa2d 100644 --- a/src/rust/cryptography-openssl/Cargo.toml +++ b/src/rust/cryptography-openssl/Cargo.toml @@ -9,6 +9,6 @@ rust-version = "1.63.0" [dependencies] openssl = "0.10.63" -ffi = { package = "openssl-sys", version = "0.9.91" } +ffi = { package = "openssl-sys", version = "0.9.99" } foreign-types = "0.3" foreign-types-shared = "0.1" diff --git a/src/rust/src/pkcs7.rs b/src/rust/src/pkcs7.rs index b7f6af216e49..30bb94d5f40f 100644 --- a/src/rust/src/pkcs7.rs +++ b/src/rust/src/pkcs7.rs @@ -9,11 +9,14 @@ use std::ops::Deref; use cryptography_x509::csr::Attribute; use cryptography_x509::{common, oid, pkcs7}; use once_cell::sync::Lazy; +use openssl::pkcs7::Pkcs7; +use pyo3::IntoPy; use crate::asn1::encode_der_data; use crate::buf::CffiBuf; -use crate::error::CryptographyResult; -use crate::{types, x509}; +use crate::error::{CryptographyError, CryptographyResult}; +use crate::x509::certificate::load_der_x509_certificate; +use crate::{exceptions, types, x509}; const PKCS7_CONTENT_TYPE_OID: asn1::ObjectIdentifier = asn1::oid!(1, 2, 840, 113549, 1, 9, 3); const PKCS7_MESSAGE_DIGEST_OID: asn1::ObjectIdentifier = asn1::oid!(1, 2, 840, 113549, 1, 9, 4); @@ -290,11 +293,74 @@ fn smime_canonicalize(data: &[u8], text_mode: bool) -> (Cow<'_, [u8]>, Cow<'_, [ } } +fn load_pkcs7_certificates( + py: pyo3::Python<'_>, + pkcs7: Pkcs7, +) -> CryptographyResult> { + let nid = pkcs7.type_().map(|t| t.nid()); + if nid != Some(openssl::nid::Nid::PKCS7_SIGNED) { + let nid_string = nid.map_or("empty".to_string(), |n| n.as_raw().to_string()); + return Err(CryptographyError::from( + exceptions::UnsupportedAlgorithm::new_err(( + format!("Only basic signed structures are currently supported. NID for this data was {}", nid_string), + exceptions::Reasons::UNSUPPORTED_SERIALIZATION, + )), + )); + } + + let signed_certificates = pkcs7.signed().and_then(|x| x.certificates()); + match signed_certificates { + None => Err(CryptographyError::from( + pyo3::exceptions::PyValueError::new_err( + "The provided PKCS7 has no certificate data, but a cert loading method was called.", + ), + )), + Some(c) => c + .iter() + .map(|c| { + load_der_x509_certificate( + py, + pyo3::types::PyBytes::new(py, c.to_der()?.as_slice()).into_py(py), + None, + ) + }) + .collect(), + } +} + +#[pyo3::prelude::pyfunction] +fn load_pem_pkcs7_certificates( + py: pyo3::Python<'_>, + data: &[u8], +) -> CryptographyResult> { + let pkcs7_decoded = openssl::pkcs7::Pkcs7::from_pem(data).map_err(|_| { + CryptographyError::from(pyo3::exceptions::PyValueError::new_err( + "Unable to parse PKCS7 data", + )) + })?; + load_pkcs7_certificates(py, pkcs7_decoded) +} + +#[pyo3::prelude::pyfunction] +fn load_der_pkcs7_certificates( + py: pyo3::Python<'_>, + data: &[u8], +) -> CryptographyResult> { + let pkcs7_decoded = openssl::pkcs7::Pkcs7::from_der(data).map_err(|_| { + CryptographyError::from(pyo3::exceptions::PyValueError::new_err( + "Unable to parse PKCS7 data", + )) + })?; + load_pkcs7_certificates(py, pkcs7_decoded) +} + pub(crate) fn create_submodule(py: pyo3::Python<'_>) -> pyo3::PyResult<&pyo3::prelude::PyModule> { let submod = pyo3::prelude::PyModule::new(py, "pkcs7")?; submod.add_function(pyo3::wrap_pyfunction!(serialize_certificates, submod)?)?; submod.add_function(pyo3::wrap_pyfunction!(sign_and_serialize, submod)?)?; + submod.add_function(pyo3::wrap_pyfunction!(load_pem_pkcs7_certificates, submod)?)?; + submod.add_function(pyo3::wrap_pyfunction!(load_der_pkcs7_certificates, submod)?)?; Ok(submod) } diff --git a/src/rust/src/x509/certificate.rs b/src/rust/src/x509/certificate.rs index bc40fc846ef4..552f4eda7d81 100644 --- a/src/rust/src/x509/certificate.rs +++ b/src/rust/src/x509/certificate.rs @@ -378,7 +378,7 @@ fn load_pem_x509_certificates( } #[pyo3::prelude::pyfunction] -fn load_der_x509_certificate( +pub(crate) fn load_der_x509_certificate( py: pyo3::Python<'_>, data: pyo3::Py, backend: Option<&pyo3::PyAny>, From 6efad7fe7f373f64fa51e3b95639c142959a78af Mon Sep 17 00:00:00 2001 From: Facundo Tuesca Date: Mon, 22 Jan 2024 18:32:16 +0100 Subject: [PATCH 2/3] Disable PKCS7 functions under BoringSSL --- src/rust/src/pkcs7.rs | 50 ++++++++++++++++++++------- tests/hazmat/primitives/test_pkcs7.py | 13 +++++++ 2 files changed, 51 insertions(+), 12 deletions(-) diff --git a/src/rust/src/pkcs7.rs b/src/rust/src/pkcs7.rs index 30bb94d5f40f..e743e3080fb0 100644 --- a/src/rust/src/pkcs7.rs +++ b/src/rust/src/pkcs7.rs @@ -9,12 +9,15 @@ use std::ops::Deref; use cryptography_x509::csr::Attribute; use cryptography_x509::{common, oid, pkcs7}; use once_cell::sync::Lazy; +#[cfg(not(CRYPTOGRAPHY_IS_BORINGSSL))] use openssl::pkcs7::Pkcs7; +#[cfg(not(CRYPTOGRAPHY_IS_BORINGSSL))] use pyo3::IntoPy; use crate::asn1::encode_der_data; use crate::buf::CffiBuf; use crate::error::{CryptographyError, CryptographyResult}; +#[cfg(not(CRYPTOGRAPHY_IS_BORINGSSL))] use crate::x509::certificate::load_der_x509_certificate; use crate::{exceptions, types, x509}; @@ -293,6 +296,7 @@ fn smime_canonicalize(data: &[u8], text_mode: bool) -> (Cow<'_, [u8]>, Cow<'_, [ } } +#[cfg(not(CRYPTOGRAPHY_IS_BORINGSSL))] fn load_pkcs7_certificates( py: pyo3::Python<'_>, pkcs7: Pkcs7, @@ -333,12 +337,23 @@ fn load_pem_pkcs7_certificates( py: pyo3::Python<'_>, data: &[u8], ) -> CryptographyResult> { - let pkcs7_decoded = openssl::pkcs7::Pkcs7::from_pem(data).map_err(|_| { - CryptographyError::from(pyo3::exceptions::PyValueError::new_err( - "Unable to parse PKCS7 data", - )) - })?; - load_pkcs7_certificates(py, pkcs7_decoded) + cfg_if::cfg_if! { + if #[cfg(not(CRYPTOGRAPHY_IS_BORINGSSL))] { + let pkcs7_decoded = openssl::pkcs7::Pkcs7::from_pem(data).map_err(|_| { + CryptographyError::from(pyo3::exceptions::PyValueError::new_err( + "Unable to parse PKCS7 data", + )) + })?; + load_pkcs7_certificates(py, pkcs7_decoded) + } else { + return Err(CryptographyError::from( + exceptions::UnsupportedAlgorithm::new_err(( + "PKCS#7 is not supported by this backend.", + exceptions::Reasons::BACKEND_MISSING_INTERFACE, + )), + )); + } + } } #[pyo3::prelude::pyfunction] @@ -346,12 +361,23 @@ fn load_der_pkcs7_certificates( py: pyo3::Python<'_>, data: &[u8], ) -> CryptographyResult> { - let pkcs7_decoded = openssl::pkcs7::Pkcs7::from_der(data).map_err(|_| { - CryptographyError::from(pyo3::exceptions::PyValueError::new_err( - "Unable to parse PKCS7 data", - )) - })?; - load_pkcs7_certificates(py, pkcs7_decoded) + cfg_if::cfg_if! { + if #[cfg(not(CRYPTOGRAPHY_IS_BORINGSSL))] { + let pkcs7_decoded = openssl::pkcs7::Pkcs7::from_der(data).map_err(|_| { + CryptographyError::from(pyo3::exceptions::PyValueError::new_err( + "Unable to parse PKCS7 data", + )) + })?; + load_pkcs7_certificates(py, pkcs7_decoded) + } else { + return Err(CryptographyError::from( + exceptions::UnsupportedAlgorithm::new_err(( + "PKCS#7 is not supported by this backend.", + exceptions::Reasons::BACKEND_MISSING_INTERFACE, + )), + )); + } + } } pub(crate) fn create_submodule(py: pyo3::Python<'_>) -> pyo3::PyResult<&pyo3::prelude::PyModule> { diff --git a/tests/hazmat/primitives/test_pkcs7.py b/tests/hazmat/primitives/test_pkcs7.py index dffc4ab2c1d0..35583abac2ae 100644 --- a/tests/hazmat/primitives/test_pkcs7.py +++ b/tests/hazmat/primitives/test_pkcs7.py @@ -922,3 +922,16 @@ def test_invalid_types(self): certs, "not an encoding", # type: ignore[arg-type] ) + + +@pytest.mark.supported( + only_if=lambda backend: not backend.pkcs7_supported(), + skip_message="Requires OpenSSL without PKCS7 support (BoringSSL)", +) +class TestPKCS7Unsupported: + def test_pkcs7_functions_unsupported(self): + with raises_unsupported_algorithm(_Reasons.BACKEND_MISSING_INTERFACE): + pkcs7.load_der_pkcs7_certificates(b"nonsense") + + with raises_unsupported_algorithm(_Reasons.BACKEND_MISSING_INTERFACE): + pkcs7.load_pem_pkcs7_certificates(b"nonsense") From e27843607a017f7a6e62ec65e0e826b9186a7539 Mon Sep 17 00:00:00 2001 From: Facundo Tuesca Date: Mon, 22 Jan 2024 21:12:34 +0100 Subject: [PATCH 3/3] Misc PKCS7 fixes --- src/rust/src/pkcs7.rs | 37 +++++++++++++-------------- tests/hazmat/primitives/test_pkcs7.py | 4 +-- 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/src/rust/src/pkcs7.rs b/src/rust/src/pkcs7.rs index e743e3080fb0..f307cf483ad7 100644 --- a/src/rust/src/pkcs7.rs +++ b/src/rust/src/pkcs7.rs @@ -300,7 +300,7 @@ fn smime_canonicalize(data: &[u8], text_mode: bool) -> (Cow<'_, [u8]>, Cow<'_, [ fn load_pkcs7_certificates( py: pyo3::Python<'_>, pkcs7: Pkcs7, -) -> CryptographyResult> { +) -> CryptographyResult<&pyo3::types::PyList> { let nid = pkcs7.type_().map(|t| t.nid()); if nid != Some(openssl::nid::Nid::PKCS7_SIGNED) { let nid_string = nid.map_or("empty".to_string(), |n| n.as_raw().to_string()); @@ -319,24 +319,23 @@ fn load_pkcs7_certificates( "The provided PKCS7 has no certificate data, but a cert loading method was called.", ), )), - Some(c) => c - .iter() - .map(|c| { - load_der_x509_certificate( - py, - pyo3::types::PyBytes::new(py, c.to_der()?.as_slice()).into_py(py), - None, - ) - }) - .collect(), + Some(certificates) => { + let result = pyo3::types::PyList::empty(py); + for c in certificates { + let cert_der = pyo3::types::PyBytes::new(py, c.to_der()?.as_slice()).into_py(py); + let cert = load_der_x509_certificate(py, cert_der, None)?; + result.append(cert.into_py(py))?; + } + Ok(result) + } } } #[pyo3::prelude::pyfunction] -fn load_pem_pkcs7_certificates( - py: pyo3::Python<'_>, +fn load_pem_pkcs7_certificates<'p>( + py: pyo3::Python<'p>, data: &[u8], -) -> CryptographyResult> { +) -> CryptographyResult<&'p pyo3::types::PyList> { cfg_if::cfg_if! { if #[cfg(not(CRYPTOGRAPHY_IS_BORINGSSL))] { let pkcs7_decoded = openssl::pkcs7::Pkcs7::from_pem(data).map_err(|_| { @@ -349,7 +348,7 @@ fn load_pem_pkcs7_certificates( return Err(CryptographyError::from( exceptions::UnsupportedAlgorithm::new_err(( "PKCS#7 is not supported by this backend.", - exceptions::Reasons::BACKEND_MISSING_INTERFACE, + exceptions::Reasons::UNSUPPORTED_SERIALIZATION, )), )); } @@ -357,10 +356,10 @@ fn load_pem_pkcs7_certificates( } #[pyo3::prelude::pyfunction] -fn load_der_pkcs7_certificates( - py: pyo3::Python<'_>, +fn load_der_pkcs7_certificates<'p>( + py: pyo3::Python<'p>, data: &[u8], -) -> CryptographyResult> { +) -> CryptographyResult<&'p pyo3::types::PyList> { cfg_if::cfg_if! { if #[cfg(not(CRYPTOGRAPHY_IS_BORINGSSL))] { let pkcs7_decoded = openssl::pkcs7::Pkcs7::from_der(data).map_err(|_| { @@ -373,7 +372,7 @@ fn load_der_pkcs7_certificates( return Err(CryptographyError::from( exceptions::UnsupportedAlgorithm::new_err(( "PKCS#7 is not supported by this backend.", - exceptions::Reasons::BACKEND_MISSING_INTERFACE, + exceptions::Reasons::UNSUPPORTED_SERIALIZATION, )), )); } diff --git a/tests/hazmat/primitives/test_pkcs7.py b/tests/hazmat/primitives/test_pkcs7.py index 35583abac2ae..03b04cd389e5 100644 --- a/tests/hazmat/primitives/test_pkcs7.py +++ b/tests/hazmat/primitives/test_pkcs7.py @@ -930,8 +930,8 @@ def test_invalid_types(self): ) class TestPKCS7Unsupported: def test_pkcs7_functions_unsupported(self): - with raises_unsupported_algorithm(_Reasons.BACKEND_MISSING_INTERFACE): + with raises_unsupported_algorithm(_Reasons.UNSUPPORTED_SERIALIZATION): pkcs7.load_der_pkcs7_certificates(b"nonsense") - with raises_unsupported_algorithm(_Reasons.BACKEND_MISSING_INTERFACE): + with raises_unsupported_algorithm(_Reasons.UNSUPPORTED_SERIALIZATION): pkcs7.load_pem_pkcs7_certificates(b"nonsense")