Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

end_entity: add verify_private_key function. #67

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
164 changes: 164 additions & 0 deletions src/der.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@

use crate::{calendar, time, Error};
pub(crate) use ring::io::der::{CONSTRUCTED, CONTEXT_SPECIFIC};
#[cfg(feature = "alloc")]
use ring::signature::{
EcdsaSigningAlgorithm, ECDSA_P256_SHA256_ASN1_SIGNING, ECDSA_P384_SHA384_ASN1_SIGNING,
};

// Copied (and extended) from ring's src/der.rs
#[allow(clippy::upper_case_acronyms)]
Expand Down Expand Up @@ -223,6 +227,93 @@ macro_rules! oid {
)
}

/// helper function for converting DER encoded [RFC 5915] format "Sec1" elliptic curve
/// private key material into a more standard DER encoded PKCS8 format.
///
/// Requires `alloc` feature.
///
/// This can be removed once https://github.com/briansmith/ring/pull/1456
/// (or equivalent) is landed.
///
/// [RFC 5915]: <https://datatracker.ietf.org/doc/html/rfc5915>
#[cfg(feature = "alloc")]
pub(crate) fn convert_sec1_to_pkcs8(
sigalg: &EcdsaSigningAlgorithm,
maybe_sec1_der: &[u8],
) -> Result<Vec<u8>, Error> {
let pkcs8_prefix = if sigalg == &ECDSA_P256_SHA256_ASN1_SIGNING {
PKCS8_PREFIX_ECDSA_NISTP256
} else if sigalg == &ECDSA_P384_SHA384_ASN1_SIGNING {
PKCS8_PREFIX_ECDSA_NISTP384
} else {
return Err(Error::UnsupportedSignatureAlgorithm);
};

// wrap sec1 encoding in an OCTET STRING
let mut sec1_wrap = Vec::with_capacity(maybe_sec1_der.len() + 8);
sec1_wrap.extend_from_slice(maybe_sec1_der);
wrap_in_asn1_len(&mut sec1_wrap);
#[allow(clippy::as_conversions)] // Infallible conversion.
sec1_wrap.insert(0, Tag::OctetString as u8);

let mut pkcs8 = Vec::with_capacity(pkcs8_prefix.len() + sec1_wrap.len() + 4);
pkcs8.extend_from_slice(pkcs8_prefix);
pkcs8.extend_from_slice(&sec1_wrap);
wrap_in_sequence(&mut pkcs8);

Ok(pkcs8)
}

#[cfg(feature = "alloc")]
pub(crate) fn wrap_in_asn1_len(bytes: &mut Vec<u8>) {
let len = bytes.len();

if len <= 0x7f {
#[allow(clippy::as_conversions)] // Infallible conversion.
bytes.insert(0, len as u8);
} else {
bytes.insert(0, 0x80u8);
let mut left = len;
while left > 0 {
#[allow(clippy::as_conversions)] // Safe to truncate.
let byte: u8 = (left & 0xff) as u8;
bytes.insert(1, byte);
bytes[0] += 1;
left >>= 8;
}
}
}

/// Prepend stuff to `bytes` to put it in a DER SEQUENCE.
#[cfg(feature = "alloc")]
pub(crate) fn wrap_in_sequence(bytes: &mut Vec<u8>) {
wrap_in_asn1_len(bytes);
#[allow(clippy::as_conversions)] // Infallible conversion.
bytes.insert(0, Tag::Sequence as u8);
}

// This is (line-by-line):
// - INTEGER Version = 0
// - SEQUENCE (privateKeyAlgorithm)
// - id-ecPublicKey OID
// - prime256v1 OID
#[cfg(feature = "alloc")]
const PKCS8_PREFIX_ECDSA_NISTP256: &[u8] = b"\x02\x01\x00\
\x30\x13\
\x06\x07\x2a\x86\x48\xce\x3d\x02\x01\
\x06\x08\x2a\x86\x48\xce\x3d\x03\x01\x07";

// This is (line-by-line):
// - INTEGER Version = 0
// - SEQUENCE (privateKeyAlgorithm)
// - id-ecPublicKey OID
// - secp384r1 OID
#[cfg(feature = "alloc")]
const PKCS8_PREFIX_ECDSA_NISTP384: &[u8] = b"\x02\x01\x00\
\x30\x10\
\x06\x07\x2a\x86\x48\xce\x3d\x02\x01\
\x06\x05\x2b\x81\x04\x00\x22";

#[cfg(test)]
mod tests {
#[test]
Expand Down Expand Up @@ -288,3 +379,76 @@ mod tests {
return untrusted::Reader::new(untrusted::Input::from(bytes));
}
}

#[cfg(test)]
#[cfg(feature = "alloc")]
mod der_helper_tests {
use crate::der::wrap_in_sequence;

#[test]
fn test_empty() {
let mut val = Vec::new();
wrap_in_sequence(&mut val);
assert_eq!(vec![0x30, 0x00], val);
}

#[test]
fn test_small() {
let mut val = Vec::new();
val.insert(0, 0x00);
val.insert(1, 0x11);
val.insert(2, 0x22);
val.insert(3, 0x33);
wrap_in_sequence(&mut val);
assert_eq!(vec![0x30, 0x04, 0x00, 0x11, 0x22, 0x33], val);
}

#[test]
fn test_medium() {
let mut val = Vec::new();
val.resize(255, 0x12);
wrap_in_sequence(&mut val);
assert_eq!(vec![0x30, 0x81, 0xff, 0x12, 0x12, 0x12], val[..6].to_vec());
}

#[test]
fn test_large() {
let mut val = Vec::new();
val.resize(4660, 0x12);
wrap_in_sequence(&mut val);
assert_eq!(vec![0x30, 0x82, 0x12, 0x34, 0x12, 0x12], val[..6].to_vec());
}

#[test]
fn test_huge() {
let mut val = Vec::new();
val.resize(0xffff, 0x12);
wrap_in_sequence(&mut val);
assert_eq!(vec![0x30, 0x82, 0xff, 0xff, 0x12, 0x12], val[..6].to_vec());
assert_eq!(val.len(), 0xffff + 4);
}

#[test]
fn test_gigantic() {
let mut val = Vec::new();
val.resize(0x100000, 0x12);
wrap_in_sequence(&mut val);
assert_eq!(
vec![0x30, 0x83, 0x10, 0x00, 0x00, 0x12, 0x12],
val[..7].to_vec()
);
assert_eq!(val.len(), 0x100000 + 5);
}

#[test]
fn test_ludicrous() {
let mut val = Vec::new();
val.resize(0x1000000, 0x12);
wrap_in_sequence(&mut val);
assert_eq!(
vec![0x30, 0x84, 0x01, 0x00, 0x00, 0x00, 0x12, 0x12],
val[..8].to_vec()
);
assert_eq!(val.len(), 0x1000000 + 6);
}
}
108 changes: 105 additions & 3 deletions src/end_entity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,21 @@
// ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
// OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.

#[cfg(feature = "alloc")]
use crate::subject_name::GeneralDnsNameRef;
use crate::{
cert, signed_data, subject_name, verify_cert, Error, SignatureAlgorithm, SubjectNameRef, Time,
cert,
signed_data::{self, parse_spki_value},
subject_name, verify_cert, Error, SignatureAlgorithm, SubjectNameRef, Time,
TlsClientTrustAnchors, TlsServerTrustAnchors,
};
#[cfg(feature = "alloc")]
use crate::{der, subject_name::GeneralDnsNameRef};

use ring::signature::{
EcdsaKeyPair, Ed25519KeyPair, KeyPair, ECDSA_P256_SHA256_ASN1_SIGNING,
ECDSA_P384_SHA384_ASN1_SIGNING,
};
#[cfg(feature = "alloc")]
use ring::signature::{EcdsaSigningAlgorithm, RsaKeyPair};

/// An end-entity certificate.
///
Expand Down Expand Up @@ -185,4 +194,97 @@ impl<'a> EndEntityCert<'a> {
pub fn dns_names(&'a self) -> Result<impl Iterator<Item = GeneralDnsNameRef<'a>>, Error> {
subject_name::list_cert_dns_names(self)
}

/// This function tries to verify that the DER encoded `private_key_der` bytes correspond
/// to the public key described in the end-entity certificate's subject public key information.
/// If the provided key isn't usable for this EE certificate [`Error::CertPrivateKeyMismatch`]
/// will be returned.
///
/// This function supports the following private key algorithms and encodings (matching the
/// supported formats used by Rustls):
/// Key algorithms:
/// * RSA
/// * ECDSA (P-256 or P-384)
/// * Ed25519
/// Encodings:
/// * PKCS8v1 (RSA, ECDSA, Ed25519)
/// * PKCS8v2 (Ed25519 only)
/// * PKCS1 (RSA only)
/// * Sec1 (ECDSA only)
pub fn verify_private_key(&self, private_key_der: &[u8]) -> Result<(), Error> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking aloud: if we restate this API as "is this the right public key for the certificate?" then we'd get some good advantages:

  • it would unduplicate all the private key handling from here,
  • webpki as a crate would maintain its "never deals with secrets" property,
  • the result would be more linker friendly (eg, a program that otherwise only uses RSA but uses this function ends up with ECDSA and ed25519 inside)

I guess that would be on the rustls side we'd add a fn public_key(&self) -> &[u8] to SigningKey and then do the key identity validation in cross_check_end_entity_cert?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are compelling advantages 👍 In retrospect I was probably overfitting Brian's suggestion on Rustls#618 by jumping straight to this design.

// Parse the SPKI of the EE cert and extract the DER encoded bytes of the public key.
let cert_pub_key = parse_spki_value(self.inner.spki.value())?
.key_value
.as_slice_less_safe();

#[cfg(feature = "alloc")]
if let Some(result) = extract_and_compare(private_key_der, rsa_from_der, cert_pub_key) {
return result;
}

if let Some(result) = extract_and_compare(private_key_der, ecdsa_from_pkcs8, cert_pub_key) {
return result;
}

#[cfg(feature = "alloc")]
if let Some(result) = extract_and_compare(private_key_der, ecdsa_from_sec1, cert_pub_key) {
return result;
}

if let Some(result) = extract_and_compare(private_key_der, ed25519_from_pkcs8, cert_pub_key)
{
return result;
}

Err(Error::CertPrivateKeyMismatch)
}
}

#[cfg(feature = "alloc")]
fn rsa_from_der(private_key_der: &[u8]) -> Option<RsaKeyPair> {
RsaKeyPair::from_pkcs8(private_key_der)
.or_else(|_| RsaKeyPair::from_der(private_key_der))
.ok()
}

fn ecdsa_from_pkcs8(pkcs8_der: &[u8]) -> Option<EcdsaKeyPair> {
EcdsaKeyPair::from_pkcs8(&ECDSA_P256_SHA256_ASN1_SIGNING, pkcs8_der)
.ok()
.or_else(|| EcdsaKeyPair::from_pkcs8(&ECDSA_P384_SHA384_ASN1_SIGNING, pkcs8_der).ok())
}

#[cfg(feature = "alloc")]
fn try_sec1_curve(sigalg: &'static EcdsaSigningAlgorithm, sec1_der: &[u8]) -> Option<EcdsaKeyPair> {
der::convert_sec1_to_pkcs8(sigalg, sec1_der)
.and_then(|pkcs8_der| {
EcdsaKeyPair::from_pkcs8(sigalg, &pkcs8_der).map_err(|_| Error::BadDer)
})
.ok()
}

#[cfg(feature = "alloc")]
fn ecdsa_from_sec1(sec1_der: &[u8]) -> Option<EcdsaKeyPair> {
try_sec1_curve(&ECDSA_P256_SHA256_ASN1_SIGNING, sec1_der)
.or_else(|| try_sec1_curve(&ECDSA_P384_SHA384_ASN1_SIGNING, sec1_der))
}

fn ed25519_from_pkcs8(pkcs8_der: &[u8]) -> Option<Ed25519KeyPair> {
Ed25519KeyPair::from_pkcs8_maybe_unchecked(pkcs8_der).ok()
}

// use extractor to try and read a `KeyPair` from the `private_key_der`. If the extraction fails,
// return None indicating the caller should try a different extractor. If the extraction succeeds,
// return a result indicating whether the extracted keypair matches the `cert_pub_key`.
fn extract_and_compare<K: KeyPair>(
private_key_der: &[u8],
extractor: impl Fn(&[u8]) -> Option<K>,
cert_pub_key: &[u8],
) -> Option<Result<(), Error>> {
match extractor(private_key_der) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can replace the outer match with let key_pair = extractor(private_key_der)?;. Also consider extracting the Some from the inner match arms.

Some(keypair) => match cert_pub_key == keypair.public_key().as_ref() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is completely correct, because neither cert_pub_key nor keypair.public_key() expresses the key type. This would mean we could compare as equal (say) a nistp256 and ed25519 key as equal if they had the same encoding, even though they semantically are never equal because they're are not in the same domain.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. We should also be asserting the key types match somewhere in this logic. I'll give this more attention in the reworked formulation. Thanks for calling this out as a potential pitfall.

true => Some(Ok(())),
false => Some(Err(Error::CertPrivateKeyMismatch)),
},
None => None,
}
}
3 changes: 3 additions & 0 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ pub enum Error {
/// for is earlier than the certificate's notBefore time.
CertNotValidYet,

/// The private key provided does not match the subject public key of the certificate.
CertPrivateKeyMismatch,

/// An end-entity certificate is being used as a CA certificate.
EndEntityUsedAsCa,

Expand Down
6 changes: 3 additions & 3 deletions src/signed_data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,16 +165,16 @@ pub(crate) fn verify_signature(
.map_err(|_| Error::InvalidSignatureForPublicKey)
}

struct SubjectPublicKeyInfo<'a> {
pub(crate) struct SubjectPublicKeyInfo<'a> {
algorithm_id_value: untrusted::Input<'a>,
key_value: untrusted::Input<'a>,
pub(crate) key_value: untrusted::Input<'a>,
}

// Parse the public key into an algorithm OID, an optional curve OID, and the
// key value. The caller needs to check whether these match the
// `PublicKeyAlgorithm` for the `SignatureAlgorithm` that is matched when
// parsing the signature.
fn parse_spki_value(input: untrusted::Input) -> Result<SubjectPublicKeyInfo, Error> {
pub(crate) fn parse_spki_value(input: untrusted::Input) -> Result<SubjectPublicKeyInfo, Error> {
input.read_all(Error::BadDer, |input| {
let algorithm_id_value = der::expect_tag_and_get_value(input, der::Tag::Sequence)?;
let key_value = der::bit_string_with_no_unused_bits(input)?;
Expand Down
Loading