From 01fc9fb6bcf4a9e8a034c3262b0c0dbb10cf2bdc Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Wed, 3 Jan 2024 09:02:55 -0500 Subject: [PATCH] Migrate RSA PKCS#1 parsing to pure-Rust (#10104) We no longer let OpenSSL parse anything. --- src/rust/Cargo.lock | 9 +++++ src/rust/Cargo.toml | 2 + src/rust/cryptography-key-parsing/Cargo.toml | 12 ++++++ src/rust/cryptography-key-parsing/src/lib.rs | 39 ++++++++++++++++++++ src/rust/cryptography-key-parsing/src/rsa.rs | 23 ++++++++++++ src/rust/src/backend/keys.rs | 35 +++++++----------- src/rust/src/backend/rsa.rs | 5 ++- src/rust/src/error.rs | 24 ++++++++++++ 8 files changed, 126 insertions(+), 23 deletions(-) create mode 100644 src/rust/cryptography-key-parsing/Cargo.toml create mode 100644 src/rust/cryptography-key-parsing/src/lib.rs create mode 100644 src/rust/cryptography-key-parsing/src/rsa.rs diff --git a/src/rust/Cargo.lock b/src/rust/Cargo.lock index e502b141c..06fb324df 100644 --- a/src/rust/Cargo.lock +++ b/src/rust/Cargo.lock @@ -70,6 +70,14 @@ dependencies = [ "pyo3", ] +[[package]] +name = "cryptography-key-parsing" +version = "0.1.0" +dependencies = [ + "asn1", + "openssl", +] + [[package]] name = "cryptography-openssl" version = "0.1.0" @@ -88,6 +96,7 @@ dependencies = [ "cc", "cfg-if", "cryptography-cffi", + "cryptography-key-parsing", "cryptography-openssl", "cryptography-x509", "cryptography-x509-verification", diff --git a/src/rust/Cargo.toml b/src/rust/Cargo.toml index 13e35e298..d816efc29 100644 --- a/src/rust/Cargo.toml +++ b/src/rust/Cargo.toml @@ -13,6 +13,7 @@ cfg-if = "1" pyo3 = { version = "0.20", features = ["abi3"] } asn1 = { version = "0.15.5", default-features = false } cryptography-cffi = { path = "cryptography-cffi" } +cryptography-key-parsing = { path = "cryptography-key-parsing" } cryptography-x509 = { path = "cryptography-x509" } cryptography-x509-verification = { path = "cryptography-x509-verification" } cryptography-openssl = { path = "cryptography-openssl" } @@ -39,6 +40,7 @@ overflow-checks = true [workspace] members = [ "cryptography-cffi", + "cryptography-key-parsing", "cryptography-openssl", "cryptography-x509", "cryptography-x509-verification", diff --git a/src/rust/cryptography-key-parsing/Cargo.toml b/src/rust/cryptography-key-parsing/Cargo.toml new file mode 100644 index 000000000..a6fed36e2 --- /dev/null +++ b/src/rust/cryptography-key-parsing/Cargo.toml @@ -0,0 +1,12 @@ +[package] +name = "cryptography-key-parsing" +version = "0.1.0" +authors = ["The cryptography developers "] +edition = "2021" +publish = false +# This specifies the MSRV +rust-version = "1.63.0" + +[dependencies] +asn1 = { version = "0.15.5", default-features = false } +openssl = "0.10.62" diff --git a/src/rust/cryptography-key-parsing/src/lib.rs b/src/rust/cryptography-key-parsing/src/lib.rs new file mode 100644 index 000000000..a5f7bc1d5 --- /dev/null +++ b/src/rust/cryptography-key-parsing/src/lib.rs @@ -0,0 +1,39 @@ +// This file is dual licensed under the terms of the Apache License, Version +// 2.0, and the BSD License. See the LICENSE file in the root of this repository +// for complete details. + +pub mod rsa; + +pub enum KeyParsingError { + Parse(asn1::ParseError), + OpenSSL(openssl::error::ErrorStack), +} + +impl From for KeyParsingError { + fn from(e: asn1::ParseError) -> KeyParsingError { + KeyParsingError::Parse(e) + } +} + +impl From for KeyParsingError { + fn from(e: openssl::error::ErrorStack) -> KeyParsingError { + KeyParsingError::OpenSSL(e) + } +} + +pub type KeyParsingResult = Result; + +#[cfg(test)] +mod tests { + use super::KeyParsingError; + + #[test] + fn test_key_parsing_error_from() { + let e = openssl::error::ErrorStack::get(); + + assert!(matches!( + KeyParsingError::from(e), + KeyParsingError::OpenSSL(_) + )); + } +} diff --git a/src/rust/cryptography-key-parsing/src/rsa.rs b/src/rust/cryptography-key-parsing/src/rsa.rs new file mode 100644 index 000000000..b1bbe2c13 --- /dev/null +++ b/src/rust/cryptography-key-parsing/src/rsa.rs @@ -0,0 +1,23 @@ +// This file is dual licensed under the terms of the Apache License, Version +// 2.0, and the BSD License. See the LICENSE file in the root of this repository +// for complete details. + +use crate::KeyParsingResult; + +#[derive(asn1::Asn1Read)] +struct Pksc1RsaPublicKey<'a> { + n: asn1::BigUint<'a>, + e: asn1::BigUint<'a>, +} + +pub fn parse_pkcs1_rsa_public_key( + data: &[u8], +) -> KeyParsingResult> { + let k = asn1::parse_single::(data)?; + + let n = openssl::bn::BigNum::from_slice(k.n.as_bytes())?; + let e = openssl::bn::BigNum::from_slice(k.e.as_bytes())?; + + let rsa = openssl::rsa::Rsa::from_public_components(n, e)?; + Ok(openssl::pkey::PKey::from_rsa(rsa)?) +} diff --git a/src/rust/src/backend/keys.rs b/src/rust/src/backend/keys.rs index 5775f538f..094bb20f3 100644 --- a/src/rust/src/backend/keys.rs +++ b/src/rust/src/backend/keys.rs @@ -6,8 +6,8 @@ use foreign_types_shared::ForeignTypeRef; use pyo3::IntoPy; use crate::buf::CffiBuf; -use crate::error::{CryptographyError, CryptographyResult}; -use crate::{error, exceptions, types}; +use crate::error::{self, CryptographyError, CryptographyResult}; +use crate::{exceptions, types}; #[pyo3::prelude::pyfunction] fn private_key_from_ptr( @@ -86,14 +86,7 @@ pub(crate) fn load_der_public_key_bytes( // It's not a (RSA/DSA/ECDSA) subjectPublicKeyInfo, but we still need to // check to see if it is a pure PKCS1 RSA public key (not embedded in a // subjectPublicKeyInfo) - let rsa = openssl::rsa::Rsa::public_key_from_der_pkcs1(data).or_else(|e| { - let errors = error::list_from_openssl_error(py, e); - Err(types::BACKEND_HANDLE_KEY_LOADING_ERROR - .get(py)? - .call1((errors,)) - .unwrap_err()) - })?; - let pkey = openssl::pkey::PKey::from_rsa(rsa)?; + let pkey = cryptography_key_parsing::rsa::parse_pkcs1_rsa_public_key(data)?; public_key_from_pkey(py, &pkey, pkey.id()) } @@ -104,18 +97,18 @@ fn load_pem_public_key( ) -> CryptographyResult { let p = pem::parse(data.as_bytes())?; let pkey = match p.tag() { - "RSA PUBLIC KEY" => openssl::rsa::Rsa::public_key_from_der_pkcs1(p.contents()) - .and_then(openssl::pkey::PKey::from_rsa), - "PUBLIC KEY" => openssl::pkey::PKey::public_key_from_der(p.contents()), + "RSA PUBLIC KEY" => { + cryptography_key_parsing::rsa::parse_pkcs1_rsa_public_key(p.contents())? + } + "PUBLIC KEY" => openssl::pkey::PKey::public_key_from_der(p.contents()).or_else(|e| { + let errors = error::list_from_openssl_error(py, e); + Err(types::BACKEND_HANDLE_KEY_LOADING_ERROR + .get(py)? + .call1((errors,)) + .unwrap_err()) + })?, _ => return Err(CryptographyError::from(pem::PemError::MalformedFraming)), - } - .or_else(|e| { - let errors = error::list_from_openssl_error(py, e); - Err(types::BACKEND_HANDLE_KEY_LOADING_ERROR - .get(py)? - .call1((errors,)) - .unwrap_err()) - })?; + }; public_key_from_pkey(py, &pkey, pkey.id()) } diff --git a/src/rust/src/backend/rsa.rs b/src/rust/src/backend/rsa.rs index 4fdcde2ec..35dd1053f 100644 --- a/src/rust/src/backend/rsa.rs +++ b/src/rust/src/backend/rsa.rs @@ -2,11 +2,12 @@ // 2.0, and the BSD License. See the LICENSE file in the root of this repository // for complete details. +use std::collections::hash_map::DefaultHasher; +use std::hash::{Hash, Hasher}; + use crate::backend::{hashes, utils}; use crate::error::{CryptographyError, CryptographyResult}; use crate::{exceptions, types}; -use std::collections::hash_map::DefaultHasher; -use std::hash::{Hash, Hasher}; #[pyo3::prelude::pyclass( frozen, diff --git a/src/rust/src/error.rs b/src/rust/src/error.rs index 57648bf23..79ca0ea63 100644 --- a/src/rust/src/error.rs +++ b/src/rust/src/error.rs @@ -9,6 +9,7 @@ use crate::exceptions; pub enum CryptographyError { Asn1Parse(asn1::ParseError), Asn1Write(asn1::WriteError), + KeyParsing(asn1::ParseError), Py(pyo3::PyErr), OpenSSL(openssl::error::ErrorStack), } @@ -51,6 +52,15 @@ impl From for CryptographyError { } } +impl From for CryptographyError { + fn from(e: cryptography_key_parsing::KeyParsingError) -> CryptographyError { + match e { + cryptography_key_parsing::KeyParsingError::Parse(e) => CryptographyError::KeyParsing(e), + cryptography_key_parsing::KeyParsingError::OpenSSL(e) => CryptographyError::OpenSSL(e), + } + } +} + pub(crate) fn list_from_openssl_error( py: pyo3::Python<'_>, error_stack: openssl::error::ErrorStack, @@ -78,6 +88,9 @@ impl From for pyo3::PyErr { "failed to allocate memory while performing ASN.1 serialization", ) } + CryptographyError::KeyParsing(asn1_error) => pyo3::exceptions::PyValueError::new_err( + format!("Could not deserialize key data. The data may be in an incorrect format, it may be encrypted with an unsupported algorithm, or it may be an unsupported key type (e.g. EC curves with explicit parameters). Details: {asn1_error}"), + ), CryptographyError::Py(py_error) => py_error, CryptographyError::OpenSSL(error_stack) => pyo3::Python::with_gil(|py| { let errors = list_from_openssl_error(py, error_stack); @@ -103,6 +116,7 @@ impl CryptographyError { match self { CryptographyError::Py(e) => CryptographyError::Py(e), CryptographyError::Asn1Parse(e) => CryptographyError::Asn1Parse(e.add_location(loc)), + CryptographyError::KeyParsing(e) => CryptographyError::KeyParsing(e.add_location(loc)), CryptographyError::Asn1Write(e) => CryptographyError::Asn1Write(e), CryptographyError::OpenSSL(e) => CryptographyError::OpenSSL(e), } @@ -184,6 +198,12 @@ mod tests { let e: CryptographyError = pyo3::PyDowncastError::new(py.None().as_ref(py), "abc").into(); assert!(matches!(e, CryptographyError::Py(_))); + + let e = cryptography_key_parsing::KeyParsingError::OpenSSL( + openssl::error::ErrorStack::get(), + ) + .into(); + assert!(matches!(e, CryptographyError::OpenSSL(_))); }) } @@ -198,5 +218,9 @@ mod tests { let openssl_error = openssl::error::ErrorStack::get(); CryptographyError::from(openssl_error).add_location(asn1::ParseLocation::Field("meh")); + + let asn1_parse_error = asn1::ParseError::new(asn1::ParseErrorKind::InvalidValue); + CryptographyError::KeyParsing(asn1_parse_error) + .add_location(asn1::ParseLocation::Field("meh")); } }