diff --git a/CHANGELOG.rst b/CHANGELOG.rst index da9a312c266b..7eca8c6795ed 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -13,6 +13,9 @@ Changelog silently ignoring them. * Added support for using :class:`~cryptography.x509.Name` as a field type in the :doc:`/hazmat/asn1/index` module. +* Loading a public key or an EC private key now rejects DER where the + ``subjectPublicKey`` (or EC ``publicKey``) ``BIT STRING`` declares a non-zero + number of unused bits, instead of silently ignoring it. .. _v49-0-0: diff --git a/src/rust/cryptography-key-parsing/src/ec.rs b/src/rust/cryptography-key-parsing/src/ec.rs index f33844f3b12a..6f1a2760fe7e 100644 --- a/src/rust/cryptography-key-parsing/src/ec.rs +++ b/src/rust/cryptography-key-parsing/src/ec.rs @@ -155,6 +155,11 @@ pub fn parse_pkcs1_private_key( let private_number = openssl::bn::BigNum::from_slice(ec_private_key.private_key)?; let mut bn_ctx = openssl::bn::BigNumContext::new()?; let public_point = if let Some(point_bytes) = ec_private_key.public_key { + // The publicKey BIT STRING holds an octet-aligned EC point, so a + // non-zero unused-bits count is a malformed encoding. + if point_bytes.padding_bits() != 0 { + return Err(crate::KeyParsingError::InvalidKey); + } openssl::ec::EcPoint::from_bytes(&group, point_bytes.as_bytes(), &mut bn_ctx) .map_err(|_| crate::KeyParsingError::InvalidKey)? } else { diff --git a/src/rust/cryptography-key-parsing/src/spki.rs b/src/rust/cryptography-key-parsing/src/spki.rs index 81ec7ff83901..5eba0dfd024b 100644 --- a/src/rust/cryptography-key-parsing/src/spki.rs +++ b/src/rust/cryptography-key-parsing/src/spki.rs @@ -12,6 +12,13 @@ use crate::{KeyParsingError, KeyParsingResult, KeySerializationResult, ParsedPub pub fn parse_public_key(data: &[u8]) -> KeyParsingResult { let k = asn1::parse_single::>(data)?; + // The subjectPublicKey BIT STRING wraps whole octets for every key type + // we support, so a non-zero unused-bits count is a malformed encoding. + // `parse_spki_for_data` already rejects this; do the same here. + if k.subject_public_key.padding_bits() != 0 { + return Err(KeyParsingError::InvalidKey); + } + match k.algorithm.params { AlgorithmParameters::Ec(ec_params) => { let group = crate::ec::ec_params_to_group(&ec_params)?; diff --git a/tests/hazmat/primitives/test_serialization.py b/tests/hazmat/primitives/test_serialization.py index f4bc7084b1c8..ef65f18f7eec 100644 --- a/tests/hazmat/primitives/test_serialization.py +++ b/tests/hazmat/primitives/test_serialization.py @@ -362,6 +362,48 @@ def test_load_der_invalid_public_key(self, backend): with pytest.raises(ValueError): load_der_public_key(b"invalid data", backend) + def test_load_der_public_key_bit_string_padding(self, backend): + # The subjectPublicKey BIT STRING must use 0 unused bits. Take a valid + # Ed25519 SPKI and bump the unused-bits count; the last data bit is 0 + # so it is still a well-formed BIT STRING, just an invalid encoding for + # a public key. + for _ in range(2000): + pub = ed25519.Ed25519PrivateKey.generate().public_key() + spki = pub.public_bytes( + Encoding.DER, PublicFormat.SubjectPublicKeyInfo + ) + if spki[-1] & 0x07 == 0: + break + else: + pytest.fail("could not generate a suitable key") + + data = bytearray(spki) + idx = data.rfind(b"\x03\x21\x00") + assert idx != -1 + data[idx + 2] = 0x01 + + with pytest.raises(ValueError): + load_der_public_key(bytes(data), backend) + + def test_load_der_ec_private_key_bit_string_padding(self, backend): + _skip_curve_unsupported(backend, ec.SECP256R1()) + # The publicKey BIT STRING in an EC private key must use 0 unused bits. + for _ in range(3000): + der = ec.generate_private_key(ec.SECP256R1()).private_bytes( + Encoding.DER, PrivateFormat.TraditionalOpenSSL, NoEncryption() + ) + idx = der.find(b"\x03\x42\x00\x04") + if idx != -1 and der[-1] & 0x07 == 0: + break + else: + pytest.fail("could not generate a suitable key") + + data = bytearray(der) + data[idx + 2] = 0x01 + + with pytest.raises(ValueError): + load_der_private_key(bytes(data), password=None, backend=backend) + @pytest.mark.supported( only_if=lambda backend: backend.dsa_supported(), skip_message="Does not support DSA.",