-
Notifications
You must be signed in to change notification settings - Fork 0
fix(tls): enforce signature provider contracts #11
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
Changes from 5 commits
86ae015
eda00c3
ddd0b2f
094eead
bbba835
cddb6ca
d132442
ecc0b81
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,9 +35,14 @@ pub static SUPPORTED_SIG_ALGS: WebPkiSupportedAlgorithms = WebPkiSupportedAlgori | |
| RSA_PSS_SHA512, | ||
| RSA_PSS_SHA384, | ||
| RSA_PSS_SHA256, | ||
| // RFC 4055 requires accepting sha*WithRSAEncryption AlgorithmIdentifiers both with | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar-ish content about linking to first-party sources... maybe the specific section in the RFC?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [GPT 5.5] Added a more specific source note in cddb6ca. RFC 4055 section 2.1 says implementations MUST accept both NULL and absent parameters as legal/equivalent encodings for these hash AlgorithmIdentifiers. rustls-webpki also registers both present-parameter and
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [GPT 5.5] Updated in d132442 to include direct links in the code comment: RFC 4055 section 2.1 for accepting NULL and absent parameters (https://www.rfc-editor.org/rfc/rfc4055.html#section-2.1), plus rustls-webpki’s |
||
| // explicit NULL parameters and with parameters absent in certificate signatures. | ||
| RSA_PKCS1_SHA512, | ||
| RSA_PKCS1_SHA512_ABSENT_PARAMS, | ||
| RSA_PKCS1_SHA384, | ||
| RSA_PKCS1_SHA384_ABSENT_PARAMS, | ||
| RSA_PKCS1_SHA256, | ||
| RSA_PKCS1_SHA256_ABSENT_PARAMS, | ||
| ], | ||
| mapping: &[ | ||
| //Note: for TLS1.2 the curve is not fixed by SignatureScheme. For TLS1.3 it is. | ||
|
|
@@ -69,6 +74,18 @@ pub(crate) static RSA_PKCS1_SHA256: &dyn SignatureVerificationAlgorithm = &Verif | |
| params: Params::Rsa(RsaPadding::PKCS1), | ||
| }; | ||
|
|
||
| /// RSA PKCS#1 1.5 signatures using SHA-256 with absent AlgorithmIdentifier parameters. | ||
| pub(crate) static RSA_PKCS1_SHA256_ABSENT_PARAMS: &dyn SignatureVerificationAlgorithm = | ||
| &VerificationAlgorithm { | ||
| display_name: "RSA_PKCS1_SHA256_ABSENT_PARAMS", | ||
| public_key_alg_id: alg_id::RSA_ENCRYPTION, | ||
| signature_alg_id: AlgorithmIdentifier::from_slice(&[ | ||
| 0x30, 0x0b, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x0b, | ||
| ]), | ||
| hash: SHA256, | ||
| params: Params::Rsa(RsaPadding::PKCS1), | ||
| }; | ||
|
|
||
| /// RSA PKCS#1 1.5 signatures using SHA-384. | ||
| pub(crate) static RSA_PKCS1_SHA384: &dyn SignatureVerificationAlgorithm = &VerificationAlgorithm { | ||
| display_name: "RSA_PKCS1_SHA384", | ||
|
|
@@ -78,6 +95,18 @@ pub(crate) static RSA_PKCS1_SHA384: &dyn SignatureVerificationAlgorithm = &Verif | |
| params: Params::Rsa(RsaPadding::PKCS1), | ||
| }; | ||
|
|
||
| /// RSA PKCS#1 1.5 signatures using SHA-384 with absent AlgorithmIdentifier parameters. | ||
| pub(crate) static RSA_PKCS1_SHA384_ABSENT_PARAMS: &dyn SignatureVerificationAlgorithm = | ||
| &VerificationAlgorithm { | ||
| display_name: "RSA_PKCS1_SHA384_ABSENT_PARAMS", | ||
| public_key_alg_id: alg_id::RSA_ENCRYPTION, | ||
| signature_alg_id: AlgorithmIdentifier::from_slice(&[ | ||
| 0x30, 0x0b, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x0c, | ||
| ]), | ||
| hash: SHA384, | ||
| params: Params::Rsa(RsaPadding::PKCS1), | ||
| }; | ||
|
|
||
| /// RSA PKCS#1 1.5 signatures using SHA-512. | ||
| pub(crate) static RSA_PKCS1_SHA512: &dyn SignatureVerificationAlgorithm = &VerificationAlgorithm { | ||
| display_name: "RSA_PKCS1_SHA512", | ||
|
|
@@ -87,6 +116,18 @@ pub(crate) static RSA_PKCS1_SHA512: &dyn SignatureVerificationAlgorithm = &Verif | |
| params: Params::Rsa(RsaPadding::PKCS1), | ||
| }; | ||
|
|
||
| /// RSA PKCS#1 1.5 signatures using SHA-512 with absent AlgorithmIdentifier parameters. | ||
| pub(crate) static RSA_PKCS1_SHA512_ABSENT_PARAMS: &dyn SignatureVerificationAlgorithm = | ||
| &VerificationAlgorithm { | ||
| display_name: "RSA_PKCS1_SHA512_ABSENT_PARAMS", | ||
| public_key_alg_id: alg_id::RSA_ENCRYPTION, | ||
| signature_alg_id: AlgorithmIdentifier::from_slice(&[ | ||
| 0x30, 0x0b, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x0d, | ||
| ]), | ||
| hash: SHA512, | ||
| params: Params::Rsa(RsaPadding::PKCS1), | ||
| }; | ||
|
|
||
| /// RSA PSS signatures using SHA-256. | ||
| pub(crate) static RSA_PSS_SHA256: &dyn SignatureVerificationAlgorithm = &VerificationAlgorithm { | ||
| display_name: "RSA_PSS_SHA256", | ||
|
|
@@ -210,6 +251,24 @@ enum Params { | |
| unsafe impl Send for Params {} | ||
| unsafe impl Sync for Params {} | ||
|
|
||
| const RSA_MIN_MODULUS_BITS: usize = 2048; | ||
| const RSA_MAX_MODULUS_BITS: usize = 8192; | ||
|
Comment on lines
+259
to
+260
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we have the agent try and provide first-party documentation sources for stuff like this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [GPT 5.5] Added a source note in cddb6ca. The first-party source I used is rustls-webpki itself: its RSA verification algorithms are named/defined for 2048-8192-bit keys, e.g.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [GPT 5.5] Updated in d132442 to include a direct source link in the code comment. It now points at rustls-webpki 0.103.13’s first-party RSA verification algorithm definitions for 2048-8192-bit keys: https://docs.rs/rustls-webpki/0.103.13/src/rustls_webpki/aws_lc_rs_algs.rs.html#162-182
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It took three tries, but it got there. |
||
|
|
||
| fn rsa_public_key_allowed_by_webpki(key: &RsaPublicKey<'_>) -> bool { | ||
| (RSA_MIN_MODULUS_BITS..=RSA_MAX_MODULUS_BITS) | ||
| .contains(&rsa_modulus_bit_len(key.modulus.as_bytes())) | ||
| } | ||
|
|
||
| fn rsa_modulus_bit_len(modulus: &[u8]) -> usize { | ||
| let Some(first) = modulus.first() else { | ||
| return 0; | ||
| }; | ||
|
|
||
| let first_byte_bits = | ||
| usize::try_from(u8::BITS - first.leading_zeros()).expect("u8 bit width fits in usize"); | ||
| (modulus.len() - 1) * 8 + first_byte_bits | ||
| } | ||
|
|
||
| #[derive(Debug)] | ||
| enum RsaPadding { | ||
| PKCS1, | ||
|
|
@@ -236,6 +295,9 @@ impl<const HASH_SIZE: usize> SignatureVerificationAlgorithm for VerificationAlgo | |
| match &self.params { | ||
| Params::Rsa(padding) => { | ||
| let key = RsaPublicKey::try_from(public_key).map_err(|_| InvalidSignature)?; | ||
| if !rsa_public_key_allowed_by_webpki(&key) { | ||
| return Err(InvalidSignature); | ||
| } | ||
| let handle = import_rsa_public_key(&key).map_err(|_| InvalidSignature)?; | ||
|
|
||
| match padding { | ||
|
|
@@ -343,6 +405,91 @@ mod tests { | |
| use super::*; | ||
| use wycheproof::TestResult; | ||
|
|
||
| const RSA_PKCS1_SHA256_ABSENT_PARAMS_DER: &[u8] = &[ | ||
| 0x30, 0x0b, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x0b, | ||
| ]; | ||
| const RSA_PKCS1_SHA384_ABSENT_PARAMS_DER: &[u8] = &[ | ||
| 0x30, 0x0b, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x0c, | ||
| ]; | ||
| const RSA_PKCS1_SHA512_ABSENT_PARAMS_DER: &[u8] = &[ | ||
| 0x30, 0x0b, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x0d, | ||
| ]; | ||
|
|
||
| #[test] | ||
| fn supported_algorithms_include_rsa_pkcs1_absent_parameter_variants() { | ||
| for signature_alg_id in [ | ||
| AlgorithmIdentifier::from_slice(RSA_PKCS1_SHA256_ABSENT_PARAMS_DER), | ||
| AlgorithmIdentifier::from_slice(RSA_PKCS1_SHA384_ABSENT_PARAMS_DER), | ||
| AlgorithmIdentifier::from_slice(RSA_PKCS1_SHA512_ABSENT_PARAMS_DER), | ||
| ] { | ||
| assert!(SUPPORTED_SIG_ALGS | ||
| .all | ||
| .iter() | ||
| .any(|alg| alg.signature_alg_id() == signature_alg_id)); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn rsa_public_key_policy_matches_webpki_2048_to_8192_bit_bounds() { | ||
| let key_2047 = rsa_public_key_with_modulus(&modulus_with_bit_len(2047)); | ||
| let key_2048 = rsa_public_key_with_modulus(&modulus_with_bit_len(2048)); | ||
| let key_8192 = rsa_public_key_with_modulus(&modulus_with_bit_len(8192)); | ||
| let key_8193 = rsa_public_key_with_modulus(&modulus_with_bit_len(8193)); | ||
|
|
||
| assert!(!rsa_public_key_allowed_by_webpki(&key_2047)); | ||
| assert!(rsa_public_key_allowed_by_webpki(&key_2048)); | ||
| assert!(rsa_public_key_allowed_by_webpki(&key_8192)); | ||
| assert!(!rsa_public_key_allowed_by_webpki(&key_8193)); | ||
| } | ||
|
|
||
| fn rsa_public_key_with_modulus(modulus: &[u8]) -> RsaPublicKey<'static> { | ||
| let mut der = Vec::new(); | ||
| append_der_integer(&mut der, modulus); | ||
| append_der_integer(&mut der, &[0x01, 0x00, 0x01]); | ||
|
|
||
| let mut sequence = Vec::new(); | ||
| sequence.push(0x30); | ||
| append_der_len(&mut sequence, der.len()); | ||
| sequence.extend_from_slice(&der); | ||
|
|
||
| let sequence: &'static [u8] = Box::leak(sequence.into_boxed_slice()); | ||
| RsaPublicKey::try_from(sequence).unwrap() | ||
| } | ||
|
|
||
| fn modulus_with_bit_len(bit_len: usize) -> Vec<u8> { | ||
| let len = bit_len.div_ceil(8); | ||
| let mut modulus = vec![0xff; len]; | ||
| modulus[0] = 1 << ((bit_len - 1) % 8); | ||
| modulus | ||
| } | ||
|
|
||
| fn append_der_integer(der: &mut Vec<u8>, value: &[u8]) { | ||
| der.push(0x02); | ||
| let needs_leading_zero = value.first().is_some_and(|byte| byte & 0x80 != 0); | ||
| append_der_len(der, value.len() + usize::from(needs_leading_zero)); | ||
| if needs_leading_zero { | ||
| der.push(0); | ||
| } | ||
| der.extend_from_slice(value); | ||
| } | ||
|
|
||
| fn append_der_len(der: &mut Vec<u8>, len: usize) { | ||
| if len < 128 { | ||
| der.push(u8::try_from(len).expect("short-form DER length fits in u8")); | ||
| return; | ||
| } | ||
|
|
||
| let len_bytes = len.to_be_bytes(); | ||
| let first = len_bytes | ||
| .iter() | ||
| .position(|byte| *byte != 0) | ||
| .unwrap_or(len_bytes.len() - 1); | ||
| let len_len = | ||
| u8::try_from(len_bytes.len() - first).expect("usize DER length-of-length fits in u8"); | ||
| der.push(0x80 | len_len); | ||
| der.extend_from_slice(&len_bytes[first..]); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_open_ssl_algorithm_debug() { | ||
| assert_eq!( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not actually supported, and GPT claims that's fine from a FIPS perspective. This removal stops advertising it as supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, yeah, like this actually seems bad... like fundamentally not possible since the whole thing is that ED25519 is a different scheme from ECDSA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[GPT 5.5] Evaluated against the current provider: Ed25519 is widely used, but this CNG-backed provider does not implement Ed25519 signing or verification (
src/signer/ec.rsexplicitly notes CNG does not support it here, andsrc/verify.rshas Ed25519 commented out). So I kept the removal from the TLS 1.2 ECDSA scheme list; advertising it would imply a capability the provider cannot satisfy.