From 7513197fc63f5ad1bdf2e1fe4d7cd36cf433606a Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Sun, 6 Feb 2022 22:42:56 -0500 Subject: [PATCH] fixes #6804 -- improve error message quality with invalid characters in name attributes (#6843) --- docs/development/test-vectors.rst | 2 ++ src/rust/src/asn1.rs | 20 +++++++++++++++++++ src/rust/src/x509/certificate.rs | 10 ++++++++-- src/rust/src/x509/common.rs | 18 ++++++++--------- src/rust/src/x509/crl.rs | 5 ++++- src/rust/src/x509/csr.rs | 5 ++++- src/rust/src/x509/ocsp_resp.rs | 2 +- tests/x509/test_x509.py | 11 ++++++++++ .../x509/custom/invalid_utf8_common_name.pem | 17 ++++++++++++++++ 9 files changed, 76 insertions(+), 14 deletions(-) create mode 100644 vectors/cryptography_vectors/x509/custom/invalid_utf8_common_name.pem diff --git a/docs/development/test-vectors.rst b/docs/development/test-vectors.rst index 200797c12..93bc1ebcd 100644 --- a/docs/development/test-vectors.rst +++ b/docs/development/test-vectors.rst @@ -282,6 +282,8 @@ Custom X.509 Vectors * ``utf8_common_name.pem`` - An RSA 2048 bit self-signed CA certificate generated using OpenSSL that contains a UTF8String common name with the value "We heart UTF8!™". +* ``invalid_utf8_common_name.pem`` - A certificate that contains a + ``UTF8String`` common name with an invalid UTF-8 byte sequence. * ``two_basic_constraints.pem`` - An RSA 2048 bit self-signed certificate containing two basic constraints extensions. * ``basic_constraints_not_critical.pem`` - An RSA 2048 bit self-signed diff --git a/src/rust/src/asn1.rs b/src/rust/src/asn1.rs index 65105b11c..d68e61e66 100644 --- a/src/rust/src/asn1.rs +++ b/src/rust/src/asn1.rs @@ -45,6 +45,15 @@ impl From for pyo3::PyErr { } } +impl PyAsn1Error { + pub(crate) fn add_location(self, loc: asn1::ParseLocation) -> Self { + match self { + PyAsn1Error::Py(e) => PyAsn1Error::Py(e), + PyAsn1Error::Asn1(e) => PyAsn1Error::Asn1(e.add_location(loc)), + } + } +} + // The primary purpose of this alias is for brevity to keep function signatures // to a single-line as a work around for coverage issues. See // https://github.com/pyca/cryptography/pull/6173 @@ -218,3 +227,14 @@ pub(crate) fn create_submodule(py: pyo3::Python<'_>) -> pyo3::PyResult<&pyo3::pr Ok(submod) } + +#[cfg(test)] +mod tests { + use super::PyAsn1Error; + + #[test] + fn test_pyasn1error_add_location() { + let py_err = pyo3::PyErr::new::("Error!"); + PyAsn1Error::Py(py_err).add_location(asn1::ParseLocation::Field("meh")); + } +} diff --git a/src/rust/src/x509/certificate.rs b/src/rust/src/x509/certificate.rs index 4ab8d3702..88d4523c6 100644 --- a/src/rust/src/x509/certificate.rs +++ b/src/rust/src/x509/certificate.rs @@ -193,12 +193,18 @@ impl Certificate { #[getter] fn issuer<'p>(&self, py: pyo3::Python<'p>) -> pyo3::PyResult<&'p pyo3::PyAny> { - x509::parse_name(py, &self.raw.borrow_value().tbs_cert.issuer) + Ok( + x509::parse_name(py, &self.raw.borrow_value().tbs_cert.issuer) + .map_err(|e| e.add_location(asn1::ParseLocation::Field("issuer")))?, + ) } #[getter] fn subject<'p>(&self, py: pyo3::Python<'p>) -> pyo3::PyResult<&'p pyo3::PyAny> { - x509::parse_name(py, &self.raw.borrow_value().tbs_cert.subject) + Ok( + x509::parse_name(py, &self.raw.borrow_value().tbs_cert.subject) + .map_err(|e| e.add_location(asn1::ParseLocation::Field("subject")))?, + ) } #[getter] diff --git a/src/rust/src/x509/common.rs b/src/rust/src/x509/common.rs index 4772040e5..f4f17e7ad 100644 --- a/src/rust/src/x509/common.rs +++ b/src/rust/src/x509/common.rs @@ -328,14 +328,14 @@ pub(crate) struct Extension<'a> { pub(crate) fn parse_name<'p>( py: pyo3::Python<'p>, name: &Name<'_>, -) -> pyo3::PyResult<&'p pyo3::PyAny> { +) -> Result<&'p pyo3::PyAny, PyAsn1Error> { let x509_module = py.import("cryptography.x509")?; let py_rdns = pyo3::types::PyList::empty(py); for rdn in name.unwrap_read().clone() { let py_rdn = parse_rdn(py, &rdn)?; py_rdns.append(py_rdn)?; } - x509_module.call_method1("Name", (py_rdns,)) + Ok(x509_module.call_method1("Name", (py_rdns,))?) } fn parse_name_attribute( @@ -714,6 +714,13 @@ impl<'a, T: asn1::SimpleAsn1Writable<'a>, U: asn1::SimpleAsn1Writable<'a>> } } +pub(crate) fn add_to_module(module: &pyo3::prelude::PyModule) -> pyo3::PyResult<()> { + module.add_wrapped(pyo3::wrap_pyfunction!(encode_extension_value))?; + module.add_wrapped(pyo3::wrap_pyfunction!(encode_name_bytes))?; + + Ok(()) +} + #[cfg(test)] mod tests { use super::{Asn1ReadableOrWritable, RawTlv}; @@ -736,10 +743,3 @@ mod tests { assert!(RawTlv::can_parse(123)); } } - -pub(crate) fn add_to_module(module: &pyo3::prelude::PyModule) -> pyo3::PyResult<()> { - module.add_wrapped(pyo3::wrap_pyfunction!(encode_extension_value))?; - module.add_wrapped(pyo3::wrap_pyfunction!(encode_name_bytes))?; - - Ok(()) -} diff --git a/src/rust/src/x509/crl.rs b/src/rust/src/x509/crl.rs index e76fd740c..06e8e9988 100644 --- a/src/rust/src/x509/crl.rs +++ b/src/rust/src/x509/crl.rs @@ -231,7 +231,10 @@ impl CertificateRevocationList { #[getter] fn issuer<'p>(&self, py: pyo3::Python<'p>) -> pyo3::PyResult<&'p pyo3::PyAny> { - x509::parse_name(py, &self.raw.borrow_value().tbs_cert_list.issuer) + Ok(x509::parse_name( + py, + &self.raw.borrow_value().tbs_cert_list.issuer, + )?) } #[getter] diff --git a/src/rust/src/x509/csr.rs b/src/rust/src/x509/csr.rs index 45c107778..4d478574f 100644 --- a/src/rust/src/x509/csr.rs +++ b/src/rust/src/x509/csr.rs @@ -128,7 +128,10 @@ impl CertificateSigningRequest { #[getter] fn subject<'p>(&self, py: pyo3::Python<'p>) -> pyo3::PyResult<&'p pyo3::PyAny> { - x509::parse_name(py, &self.raw.borrow_value().csr_info.subject) + Ok(x509::parse_name( + py, + &self.raw.borrow_value().csr_info.subject, + )?) } #[getter] diff --git a/src/rust/src/x509/ocsp_resp.rs b/src/rust/src/x509/ocsp_resp.rs index d19393703..663be2873 100644 --- a/src/rust/src/x509/ocsp_resp.rs +++ b/src/rust/src/x509/ocsp_resp.rs @@ -135,7 +135,7 @@ impl OCSPResponse { fn responder_name<'p>(&self, py: pyo3::Python<'p>) -> pyo3::PyResult<&'p pyo3::PyAny> { let resp = self.requires_successful_response()?; match resp.tbs_response_data.responder_id { - ResponderId::ByName(ref name) => x509::parse_name(py, name), + ResponderId::ByName(ref name) => Ok(x509::parse_name(py, name)?), ResponderId::ByKey(_) => Ok(py.None().into_ref(py)), } } diff --git a/tests/x509/test_x509.py b/tests/x509/test_x509.py index 261f87b85..119fd60d6 100644 --- a/tests/x509/test_x509.py +++ b/tests/x509/test_x509.py @@ -981,6 +981,17 @@ class TestRSACertificate: x509.NameAttribute(NameOID.COMMON_NAME, "We heart UTF8!\u2122") ] + def test_invalid_unicode_name(self, backend): + cert = _load_cert( + os.path.join("x509", "custom", "invalid_utf8_common_name.pem"), + x509.load_pem_x509_certificate, + backend, + ) + with pytest.raises(ValueError, match="subject"): + cert.subject + with pytest.raises(ValueError, match="issuer"): + cert.issuer + def test_non_ascii_dns_name(self, backend): cert = _load_cert( os.path.join("x509", "utf8-dnsname.pem"), diff --git a/vectors/cryptography_vectors/x509/custom/invalid_utf8_common_name.pem b/vectors/cryptography_vectors/x509/custom/invalid_utf8_common_name.pem new file mode 100644 index 000000000..05a5e8a24 --- /dev/null +++ b/vectors/cryptography_vectors/x509/custom/invalid_utf8_common_name.pem @@ -0,0 +1,17 @@ +-----BEGIN CERTIFICATE----- +MIICsjCCAZoCCQDfCSnalLBhujANBgkqhkiG9w0BAQUFADAbMRkwFwYDVQQDDBBX +ZSBoZWFydCBVVEY4IeKEMB4XDTE1MDExODAzNTM1NFoXDTE1MDIxNzAzNTM1NFow +GzEZMBcGA1UEAwwQV2UgaGVhcnQgVVRGOCHihDCCASIwDQYJKoZIhvcNAQEBBQAD +ggEPADCCAQoCggEBAOBG4RmNZtn5lqI9PawhjCTHQ3aEBv3G4o/gKu+dqWzhweC7 +Dl2u7Wc9NUJzGLBjqhCMCt/HysbO5rJj0DRQA+uyspe+7SCORHg29NpRCH735nq7 +wdaExkZ/wsnp9l3BVggq9x7tbAmEi383sFTLPcjId3kN6/RTaihWSiOKUc/J2Rzu +vpcpbqbkzmHAMR3wHccuZO3OsSibO59NUR1ogYiTEid51L4glV2PsEDWxB7Epbdv +/Hsf1GY9yMQpBGGQ7YwZ30FKAzWg5sWrWgHdpM4rvnFlta1T4pEqIDmVm13H3TtN +1hTwBJP7tNZcg304vSa55xOz3NbQUEjuiIUZhS0CAwEAATANBgkqhkiG9w0BAQUF +AAOCAQEAEQ2bnpTYlnWzDGI7sTwTOBDzc8WlbPzF+2ZkbtRI3GfCeAdp2+UgBJgC +pIzOMzn73hHhCZ8/4Ca9v2KumetCZJh38uIjzzzTre8yfSAZBHD8QpYKhsR4RYl6 +syWitP5j49W860C5SKGAio8tSuJpSzpwaF8xl/UoyAthBu7xTPtEmWsvUJLLW23C +qSpXCA8iefePk2l/Y7iCIZrv4QB85pTKlL+LVcaTFOHRi8sHwFrA/O2hbdUp8YKt +SdYV1ERjwfNYy7Ci3MamIaVlIxsuJSo8wY0aXcoqqAy3c33vdrvUP+HQiAQSUt56 +1KhGQMAo+zuCVuI18WZ0gsaAYWL+CA== +-----END CERTIFICATE-----