From 3b178f0447730195441400c05668917e9955cb8c Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 30 Jun 2026 02:31:54 +0300 Subject: [PATCH 1/7] feat(xmldsig): resolve ECKeyValue keys Closes #69 --- README.md | 2 +- src/xmldsig/keys.rs | 130 ++++++++++++++++++++----- src/xmldsig/parse.rs | 220 +++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 322 insertions(+), 30 deletions(-) diff --git a/README.md b/README.md index ee92fdb..231890d 100644 --- a/README.md +++ b/README.md @@ -39,7 +39,7 @@ Currently implemented (core paths): - C14N 1.0, C14N 1.1, and Exclusive C14N - XMLDSig parsing, same-document URI dereference, transform chains, and digest verification - XMLDSig full verify pipeline (`SignedInfo` canonicalization + `SignatureValue` verification) -- Built-in verification-key resolution from X.509, DER, `KeyName`, and RSA `KeyValue` sources +- Built-in verification-key resolution from X.509, DER, `KeyName`, RSA `KeyValue`, and EC `KeyValue` sources - RSA PKCS#1 v1.5 verification helpers for SHA-1 / SHA-256 / SHA-384 / SHA-512 - ECDSA verification helpers for P-256/SHA-256 and P-384/SHA-384 - Opt-in X.509 certificate-chain validation with explicit trust anchors, validity checks, CA constraints, and CRLs diff --git a/src/xmldsig/keys.rs b/src/xmldsig/keys.rs index 2faf45f..f9da752 100644 --- a/src/xmldsig/keys.rs +++ b/src/xmldsig/keys.rs @@ -2,7 +2,7 @@ use std::{collections::HashMap, time::SystemTime}; -use rsa::pkcs8::EncodePublicKey; +use p256::pkcs8::EncodePublicKey; use x509_parser::{ prelude::{FromDer, X509Certificate}, public_key::PublicKey, @@ -177,29 +177,33 @@ impl DefaultKeyResolver { key_value: &KeyValueInfo, algorithm: SignatureAlgorithm, ) -> Result, KeyResolutionError> { - let KeyValueInfo::Rsa { modulus, exponent } = key_value else { - return Ok(None); + let public_key_bytes = match key_value { + KeyValueInfo::Rsa { modulus, exponent } => { + if !matches!( + algorithm, + SignatureAlgorithm::RsaSha1 + | SignatureAlgorithm::RsaSha256 + | SignatureAlgorithm::RsaSha384 + | SignatureAlgorithm::RsaSha512 + ) { + return Err(KeyResolutionError::AlgorithmMismatch); + } + rsa_key_value_to_spki_der(modulus, exponent)? + } + KeyValueInfo::Ec { + curve_oid, + public_key, + } => { + if !matches!( + algorithm, + SignatureAlgorithm::EcdsaP256Sha256 | SignatureAlgorithm::EcdsaP384Sha384 + ) { + return Err(KeyResolutionError::AlgorithmMismatch); + } + ec_key_value_to_spki_der(curve_oid, public_key)? + } + KeyValueInfo::Unsupported { .. } => return Ok(None), }; - if !matches!( - algorithm, - SignatureAlgorithm::RsaSha1 - | SignatureAlgorithm::RsaSha256 - | SignatureAlgorithm::RsaSha384 - | SignatureAlgorithm::RsaSha512 - ) { - return Err(KeyResolutionError::AlgorithmMismatch); - } - - let key = rsa::RsaPublicKey::new( - rsa::BigUint::from_bytes_be(modulus), - rsa::BigUint::from_bytes_be(exponent), - ) - .map_err(|_| KeyResolutionError::InvalidPublicKey)?; - let public_key_bytes = key - .to_public_key_der() - .map_err(|_| KeyResolutionError::InvalidPublicKey)? - .as_bytes() - .to_vec(); validate_spki_algorithm(&public_key_bytes, algorithm)?; Ok(Some(VerificationKey { @@ -260,6 +264,39 @@ impl KeyResolver for DefaultKeyResolver { } } +fn rsa_key_value_to_spki_der( + modulus: &[u8], + exponent: &[u8], +) -> Result, KeyResolutionError> { + let key = rsa::RsaPublicKey::new( + rsa::BigUint::from_bytes_be(modulus), + rsa::BigUint::from_bytes_be(exponent), + ) + .map_err(|_| KeyResolutionError::InvalidPublicKey)?; + key.to_public_key_der() + .map_err(|_| KeyResolutionError::InvalidPublicKey) + .map(|der| der.as_bytes().to_vec()) +} + +fn ec_key_value_to_spki_der( + curve_oid: &str, + public_key: &[u8], +) -> Result, KeyResolutionError> { + match curve_oid { + "1.2.840.10045.3.1.7" => p256::PublicKey::from_sec1_bytes(public_key) + .map_err(|_| KeyResolutionError::InvalidPublicKey)? + .to_public_key_der() + .map_err(|_| KeyResolutionError::InvalidPublicKey) + .map(|der| der.as_bytes().to_vec()), + "1.3.132.0.34" => p384::PublicKey::from_sec1_bytes(public_key) + .map_err(|_| KeyResolutionError::InvalidPublicKey)? + .to_public_key_der() + .map_err(|_| KeyResolutionError::InvalidPublicKey) + .map(|der| der.as_bytes().to_vec()), + _ => Err(KeyResolutionError::InvalidPublicKey), + } +} + fn validate_spki_algorithm( public_key_bytes: &[u8], algorithm: SignatureAlgorithm, @@ -321,6 +358,12 @@ mod tests { const LEGACY_RSA_KEY_VALUE_SIGNATURE: &str = include_str!( "../../tests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-enveloping-rsa.xml" ); + const EC_P256_KEY_VALUE_SIGNATURE: &str = include_str!( + "../../donors/xmlsec/tests/xmldsig11-interop-2012/signature-enveloping-p256_sha256.xml" + ); + const EC_P384_KEY_VALUE_SIGNATURE: &str = include_str!( + "../../donors/xmlsec/tests/xmldsig11-interop-2012/signature-enveloping-p384_sha384.xml" + ); fn replace_key_info(xml: &str, replacement: &str) -> String { let start = xml.find("").expect("fixture has KeyInfo"); @@ -487,6 +530,47 @@ mod tests { )); } + #[test] + fn resolves_ec_p256_key_value_end_to_end() { + // XMLDSig 1.1 ECKeyValue must verify without a preset key or certificate. + let resolver = DefaultKeyResolver::default(); + let result = super::super::VerifyContext::new() + .key_resolver(&resolver) + .verify(EC_P256_KEY_VALUE_SIGNATURE) + .expect("P-256 ECKeyValue should resolve"); + + assert_eq!(result.status, super::super::DsigStatus::Valid); + } + + #[test] + fn resolves_ec_p384_key_value_end_to_end() { + // The donor P-384 vector uses NamedCurve + uncompressed PublicKey. + let resolver = DefaultKeyResolver::default(); + let result = super::super::VerifyContext::new() + .key_resolver(&resolver) + .verify(EC_P384_KEY_VALUE_SIGNATURE) + .expect("P-384 ECKeyValue should resolve"); + + assert_eq!(result.status, super::super::DsigStatus::Valid); + } + + #[test] + fn ec_key_value_rejects_rsa_signature_method() { + // Embedded EC key material must not be relabeled for an RSA SignatureMethod. + let key_info = r#"BJ/yaXNlq4FRObyJCBhb5jAz8GVzinK3bBGLjSDfjbJwNfydtgjnlS4EsDmxSRhWyJWq6GIqy5wvnaiARK04uB4="#; + let xml = replace_unprefixed_key_info(RSA_KEY_VALUE_SIGNATURE, key_info); + let resolver = DefaultKeyResolver::default(); + let error = super::super::VerifyContext::new() + .key_resolver(&resolver) + .verify(&xml) + .expect_err("ECKeyValue must not resolve for RSA"); + + assert!(matches!( + error, + DsigError::KeyResolution(KeyResolutionError::AlgorithmMismatch) + )); + } + #[test] fn chain_verification_rejects_untrusted_embedded_certificate() { // Enabling chain policy must fail closed when no trust anchor is configured. diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index f433d9c..fd4d55c 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -39,6 +39,9 @@ const MAX_DER_ENCODED_KEY_VALUE_BASE64_LEN: usize = MAX_DER_ENCODED_KEY_VALUE_LE const MAX_KEY_NAME_TEXT_LEN: usize = 4096; const MAX_RSA_MODULUS_LEN: usize = 1024; const MAX_RSA_EXPONENT_LEN: usize = 8; +const EC_P256_OID: &str = "1.2.840.10045.3.1.7"; +const EC_P384_OID: &str = "1.3.132.0.34"; +const MAX_EC_PUBLIC_KEY_LEN: usize = 97; const MAX_X509_BASE64_TEXT_LEN: usize = 262_144; const MAX_X509_BASE64_NORMALIZED_LEN: usize = MAX_X509_BASE64_TEXT_LEN; const MAX_X509_DECODED_BINARY_LEN: usize = MAX_X509_BASE64_NORMALIZED_LEN.div_ceil(4) * 3; @@ -167,8 +170,13 @@ pub enum KeyValueInfo { /// RSA public exponent. exponent: Vec, }, - /// `dsig11:ECKeyValue` (the XMLDSig 1.1 namespace form). - EcKeyValue, + /// `dsig11:ECKeyValue` with a supported named curve and SEC1 public point. + Ec { + /// Bare named-curve OID, without the XMLDSig `urn:oid:` prefix. + curve_oid: String, + /// Uncompressed SEC1 point (`0x04 || x || y`). + public_key: Vec, + }, /// Any other `` child not yet supported by this phase. Unsupported { /// Namespace URI of the unsupported child, when present. @@ -420,7 +428,7 @@ pub(crate) fn parse_reference(reference_node: Node) -> Result` -/// - `` (dispatch by child QName; only `dsig11:ECKeyValue` is treated as supported EC) +/// - `` (dispatch by child QName; RSA and `dsig11:ECKeyValue` are parsed) /// - `` /// - `` /// @@ -488,6 +496,26 @@ fn verify_ds_element(node: Node, expected_name: &'static str) -> Result<(), Pars Ok(()) } +/// Verify that a node is a `` element. +fn verify_dsig11_element(node: Node, expected_name: &'static str) -> Result<(), ParseError> { + if !node.is_element() { + return Err(ParseError::InvalidStructure(format!( + "expected element <{expected_name}>, got non-element node" + ))); + } + let tag = node.tag_name(); + if tag.name() != expected_name || tag.namespace() != Some(XMLDSIG11_NS) { + return Err(ParseError::InvalidStructure(format!( + "expected , got <{}{}>", + tag.namespace() + .map(|ns| format!("{{{ns}}}")) + .unwrap_or_default(), + tag.name() + ))); + } + Ok(()) +} + /// Get the required `Algorithm` attribute from an element. fn required_algorithm_attr<'a>( node: Node<'a, 'a>, @@ -548,7 +576,7 @@ fn parse_key_value_dispatch(node: Node) -> Result { first_child.tag_name().name(), ) { (Some(XMLDSIG_NS), "RSAKeyValue") => parse_rsa_key_value(first_child), - (Some(XMLDSIG11_NS), "ECKeyValue") => Ok(KeyValueInfo::EcKeyValue), + (Some(XMLDSIG11_NS), "ECKeyValue") => parse_ec_key_value(first_child), (namespace, child_name) => Ok(KeyValueInfo::Unsupported { namespace: namespace.map(str::to_string), local_name: child_name.to_string(), @@ -556,6 +584,78 @@ fn parse_key_value_dispatch(node: Node) -> Result { } } +fn parse_ec_key_value(node: Node<'_, '_>) -> Result { + verify_dsig11_element(node, "ECKeyValue")?; + ensure_no_non_whitespace_text(node, "ECKeyValue")?; + + let mut children = element_children(node); + let named_curve_node = children.next().ok_or_else(|| { + ParseError::InvalidStructure("ECKeyValue requires NamedCurve and PublicKey".into()) + })?; + verify_dsig11_element(named_curve_node, "NamedCurve")?; + ensure_no_element_children(named_curve_node, "NamedCurve")?; + ensure_no_non_whitespace_text(named_curve_node, "NamedCurve")?; + let curve_oid = parse_ec_named_curve_oid(named_curve_node)?; + let expected_public_key_len = ec_public_key_len(&curve_oid)?; + + let public_key_node = children.next().ok_or_else(|| { + ParseError::InvalidStructure("ECKeyValue requires NamedCurve and PublicKey".into()) + })?; + verify_dsig11_element(public_key_node, "PublicKey")?; + ensure_no_element_children(public_key_node, "PublicKey")?; + if children.next().is_some() { + return Err(ParseError::InvalidStructure( + "ECKeyValue must contain exactly NamedCurve followed by PublicKey".into(), + )); + } + + let public_key = decode_crypto_binary(public_key_node, "PublicKey", MAX_EC_PUBLIC_KEY_LEN)?; + validate_ec_public_key_point(&public_key, expected_public_key_len)?; + + Ok(KeyValueInfo::Ec { + curve_oid, + public_key, + }) +} + +fn parse_ec_named_curve_oid(node: Node<'_, '_>) -> Result { + let uri = node.attribute("URI").ok_or_else(|| { + ParseError::InvalidStructure("ECKeyValue NamedCurve must include URI attribute".into()) + })?; + let curve_oid = uri.strip_prefix("urn:oid:").unwrap_or(uri); + if curve_oid.is_empty() { + return Err(ParseError::InvalidStructure( + "ECKeyValue NamedCurve URI must not be empty".into(), + )); + } + ec_public_key_len(curve_oid)?; + Ok(curve_oid.to_string()) +} + +fn ec_public_key_len(curve_oid: &str) -> Result { + match curve_oid { + EC_P256_OID => Ok(65), + EC_P384_OID => Ok(97), + _ => Err(ParseError::InvalidStructure(format!( + "unsupported ECKeyValue NamedCurve OID {curve_oid}" + ))), + } +} + +fn validate_ec_public_key_point(public_key: &[u8], expected_len: usize) -> Result<(), ParseError> { + if public_key.len() != expected_len { + return Err(ParseError::InvalidStructure( + "ECKeyValue PublicKey length does not match NamedCurve".into(), + )); + } + if public_key.first().copied() != Some(0x04) { + return Err(ParseError::InvalidStructure( + "ECKeyValue PublicKey must be an uncompressed SEC1 point".into(), + )); + } + Ok(()) +} + fn parse_rsa_key_value(node: Node<'_, '_>) -> Result { verify_ds_element(node, "RSAKeyValue")?; ensure_no_non_whitespace_text(node, "RSAKeyValue")?; @@ -2214,21 +2314,129 @@ BA== #[test] fn parse_key_info_dispatches_dsig11_ec_keyvalue() { + let public_key = "BJ/yaXNlq4FRObyJCBhb5jAz8GVzinK3bBGLjSDfjbJwNfydtgjnlS4EsDmxSRhWyJWq6GIqy5wvnaiARK04uB4="; let xml = r#" - + + + BJ/yaXNlq4FRObyJCBhb5jAz8GVzinK3bBGLjSDfjbJwNfydtgjnlS4EsDmxSRhWyJWq6GIqy5wvnaiARK04uB4= + "#; let doc = Document::parse(xml).unwrap(); + let expected_public_key = base64::engine::general_purpose::STANDARD + .decode(public_key) + .expect("fixture EC point must be valid base64"); let key_info = parse_key_info(doc.root_element()).unwrap(); assert_eq!( key_info.sources, - vec![KeyInfoSource::KeyValue(KeyValueInfo::EcKeyValue)] + vec![KeyInfoSource::KeyValue(KeyValueInfo::Ec { + curve_oid: "1.2.840.10045.3.1.7".into(), + public_key: expected_public_key, + })] ); } + #[test] + fn parse_ec_key_value_accepts_bare_curve_oid() { + let xml = r#" + + + + BO/yd/OZzDfjX4qivDY/vsUIuh6KWAxoxW5P4ukvwd+T6pVljWsX2UBJNNy5MdhTwB8e2YwB8kUbJwdsAS/XGi/fz8unFrs+lVlAgIs6s/xBYFbfUoRiAacD2SpVDe6XBA== + + + "#; + let doc = Document::parse(xml).unwrap(); + + let sources = parse_key_info(doc.root_element()).unwrap().sources; + + assert!(matches!( + &sources[0], + KeyInfoSource::KeyValue(KeyValueInfo::Ec { curve_oid, public_key }) + if curve_oid == EC_P384_OID && public_key.len() == 97 + )); + } + + #[test] + fn parse_ec_key_value_rejects_ec_parameters() { + let xml = r#" + + + + BA== + + + "#; + let doc = Document::parse(xml).unwrap(); + + assert!(matches!( + parse_key_info(doc.root_element()), + Err(ParseError::InvalidStructure(_)) + )); + } + + #[test] + fn parse_ec_key_value_rejects_missing_named_curve_uri() { + let xml = r#" + + + + BA== + + + "#; + let doc = Document::parse(xml).unwrap(); + + assert!(matches!( + parse_key_info(doc.root_element()), + Err(ParseError::InvalidStructure(_)) + )); + } + + #[test] + fn parse_ec_key_value_rejects_reordered_children() { + let xml = r#" + + + BJ/yaXNlq4FRObyJCBhb5jAz8GVzinK3bBGLjSDfjbJwNfydtgjnlS4EsDmxSRhWyJWq6GIqy5wvnaiARK04uB4= + + + + "#; + let doc = Document::parse(xml).unwrap(); + + assert!(matches!( + parse_key_info(doc.root_element()), + Err(ParseError::InvalidStructure(_)) + )); + } + + #[test] + fn parse_ec_key_value_rejects_non_uncompressed_point() { + let xml = r#" + + + + Ap/yaXNlq4FRObyJCBhb5jAz8GVzinK3bBGLjSDfjbJwNfydtgjnlS4EsDmxSRhWyJWq6GIqy5wvnaiARK04uB4= + + + "#; + let doc = Document::parse(xml).unwrap(); + + assert!(matches!( + parse_key_info(doc.root_element()), + Err(ParseError::InvalidStructure(_)) + )); + } + #[test] fn parse_key_info_marks_ds_namespace_ec_keyvalue_as_unsupported() { let xml = r#" From f714343a87af233d0b3c88155ab3a1c9f153386f Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 30 Jun 2026 03:00:04 +0300 Subject: [PATCH 2/7] fix(xmldsig): keep ECKeyValue fallback non-fatal --- src/xmldsig/keys.rs | 64 ++++++++++++++--- src/xmldsig/parse.rs | 72 ++++++++++++++----- .../signature-enveloping-p256_sha256.xml | 1 + .../signature-enveloping-p384_sha384.xml | 1 + tests/fixtures_smoke.rs | 4 +- 5 files changed, 114 insertions(+), 28 deletions(-) create mode 100644 tests/fixtures/xmldsig/xmldsig11-interop-2012/signature-enveloping-p256_sha256.xml create mode 100644 tests/fixtures/xmldsig/xmldsig11-interop-2012/signature-enveloping-p384_sha384.xml diff --git a/src/xmldsig/keys.rs b/src/xmldsig/keys.rs index f9da752..fa46ab5 100644 --- a/src/xmldsig/keys.rs +++ b/src/xmldsig/keys.rs @@ -198,7 +198,7 @@ impl DefaultKeyResolver { algorithm, SignatureAlgorithm::EcdsaP256Sha256 | SignatureAlgorithm::EcdsaP384Sha384 ) { - return Err(KeyResolutionError::AlgorithmMismatch); + return Ok(None); } ec_key_value_to_spki_der(curve_oid, public_key)? } @@ -359,10 +359,10 @@ mod tests { "../../tests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-enveloping-rsa.xml" ); const EC_P256_KEY_VALUE_SIGNATURE: &str = include_str!( - "../../donors/xmlsec/tests/xmldsig11-interop-2012/signature-enveloping-p256_sha256.xml" + "../../tests/fixtures/xmldsig/xmldsig11-interop-2012/signature-enveloping-p256_sha256.xml" ); const EC_P384_KEY_VALUE_SIGNATURE: &str = include_str!( - "../../donors/xmlsec/tests/xmldsig11-interop-2012/signature-enveloping-p384_sha384.xml" + "../../tests/fixtures/xmldsig/xmldsig11-interop-2012/signature-enveloping-p384_sha384.xml" ); fn replace_key_info(xml: &str, replacement: &str) -> String { @@ -555,20 +555,64 @@ mod tests { } #[test] - fn ec_key_value_rejects_rsa_signature_method() { + fn ec_key_value_ignored_for_rsa_signature_method() { // Embedded EC key material must not be relabeled for an RSA SignatureMethod. let key_info = r#"BJ/yaXNlq4FRObyJCBhb5jAz8GVzinK3bBGLjSDfjbJwNfydtgjnlS4EsDmxSRhWyJWq6GIqy5wvnaiARK04uB4="#; let xml = replace_unprefixed_key_info(RSA_KEY_VALUE_SIGNATURE, key_info); let resolver = DefaultKeyResolver::default(); - let error = super::super::VerifyContext::new() + let result = super::super::VerifyContext::new() .key_resolver(&resolver) .verify(&xml) - .expect_err("ECKeyValue must not resolve for RSA"); + .expect("incompatible ECKeyValue should be ignored"); - assert!(matches!( - error, - DsigError::KeyResolution(KeyResolutionError::AlgorithmMismatch) - )); + assert_eq!( + result.status, + super::super::DsigStatus::Invalid(super::super::FailureReason::KeyNotFound) + ); + } + + #[test] + fn incompatible_ec_key_value_falls_back_to_later_rsa_key_value() { + // Mixed KeyInfo should keep scanning after an incompatible ECKeyValue source. + let public_key = rsa::RsaPublicKey::from_public_key_pem(RSA_PUBLIC_KEY) + .expect("fixture must contain an RSA public key"); + let key_info = format!( + r#"BJ/yaXNlq4FRObyJCBhb5jAz8GVzinK3bBGLjSDfjbJwNfydtgjnlS4EsDmxSRhWyJWq6GIqy5wvnaiARK04uB4={}{}"#, + STANDARD.encode(public_key.n().to_bytes_be()), + STANDARD.encode(public_key.e().to_bytes_be()), + ); + let xml = replace_unprefixed_key_info(RSA_KEY_VALUE_SIGNATURE, &key_info); + let resolver = DefaultKeyResolver::default(); + let result = super::super::VerifyContext::new() + .key_resolver(&resolver) + .verify(&xml) + .expect("later RSAKeyValue should resolve"); + + assert_eq!(result.status, super::super::DsigStatus::Valid); + } + + #[test] + fn unsupported_ec_key_value_falls_back_to_later_key_name() { + // Unsupported curves are non-fatal so a later compatible source can verify. + let key_info = r#"BA==idp-signing"#; + let xml = replace_key_info(SIGNED_SAML, key_info); + let mut config = KeyResolverConfig::default(); + config.named_keys.insert( + "idp-signing".into(), + VerificationKey { + algorithm: SignatureAlgorithm::EcdsaP256Sha256, + public_key_bytes: public_key_der(SAML_PUBLIC_KEY), + certificate_der: None, + name: Some("idp-signing".into()), + }, + ); + let resolver = DefaultKeyResolver::new(config); + let result = super::super::VerifyContext::new() + .key_resolver(&resolver) + .verify(&xml) + .expect("later KeyName should resolve"); + + assert_eq!(result.status, super::super::DsigStatus::Valid); } #[test] diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index fd4d55c..5a2f4f9 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -592,11 +592,24 @@ fn parse_ec_key_value(node: Node<'_, '_>) -> Result { let named_curve_node = children.next().ok_or_else(|| { ParseError::InvalidStructure("ECKeyValue requires NamedCurve and PublicKey".into()) })?; + if named_curve_node.tag_name().namespace() == Some(XMLDSIG11_NS) + && named_curve_node.tag_name().name() == "ECParameters" + { + return Ok(KeyValueInfo::Unsupported { + namespace: Some(XMLDSIG11_NS.to_string()), + local_name: "ECKeyValue".into(), + }); + } verify_dsig11_element(named_curve_node, "NamedCurve")?; ensure_no_element_children(named_curve_node, "NamedCurve")?; ensure_no_non_whitespace_text(named_curve_node, "NamedCurve")?; - let curve_oid = parse_ec_named_curve_oid(named_curve_node)?; - let expected_public_key_len = ec_public_key_len(&curve_oid)?; + let Some((curve_oid, expected_public_key_len)) = parse_ec_named_curve_oid(named_curve_node)? + else { + return Ok(KeyValueInfo::Unsupported { + namespace: Some(XMLDSIG11_NS.to_string()), + local_name: "ECKeyValue".into(), + }); + }; let public_key_node = children.next().ok_or_else(|| { ParseError::InvalidStructure("ECKeyValue requires NamedCurve and PublicKey".into()) @@ -618,7 +631,7 @@ fn parse_ec_key_value(node: Node<'_, '_>) -> Result { }) } -fn parse_ec_named_curve_oid(node: Node<'_, '_>) -> Result { +fn parse_ec_named_curve_oid(node: Node<'_, '_>) -> Result, ParseError> { let uri = node.attribute("URI").ok_or_else(|| { ParseError::InvalidStructure("ECKeyValue NamedCurve must include URI attribute".into()) })?; @@ -628,17 +641,17 @@ fn parse_ec_named_curve_oid(node: Node<'_, '_>) -> Result { "ECKeyValue NamedCurve URI must not be empty".into(), )); } - ec_public_key_len(curve_oid)?; - Ok(curve_oid.to_string()) + let Some(public_key_len) = ec_public_key_len(curve_oid) else { + return Ok(None); + }; + Ok(Some((curve_oid.to_string(), public_key_len))) } -fn ec_public_key_len(curve_oid: &str) -> Result { +fn ec_public_key_len(curve_oid: &str) -> Option { match curve_oid { - EC_P256_OID => Ok(65), - EC_P384_OID => Ok(97), - _ => Err(ParseError::InvalidStructure(format!( - "unsupported ECKeyValue NamedCurve OID {curve_oid}" - ))), + EC_P256_OID => Some(65), + EC_P384_OID => Some(97), + _ => None, } } @@ -2362,7 +2375,7 @@ BA== } #[test] - fn parse_ec_key_value_rejects_ec_parameters() { + fn parse_ec_key_value_marks_ec_parameters_as_unsupported() { let xml = r#" @@ -2374,10 +2387,37 @@ BA== "#; let doc = Document::parse(xml).unwrap(); - assert!(matches!( - parse_key_info(doc.root_element()), - Err(ParseError::InvalidStructure(_)) - )); + let key_info = parse_key_info(doc.root_element()).unwrap(); + assert_eq!( + key_info.sources, + vec![KeyInfoSource::KeyValue(KeyValueInfo::Unsupported { + namespace: Some(XMLDSIG11_NS.to_string()), + local_name: "ECKeyValue".into(), + })] + ); + } + + #[test] + fn parse_ec_key_value_marks_unsupported_curve_as_unsupported() { + let xml = r#" + + + + BA== + + + "#; + let doc = Document::parse(xml).unwrap(); + + let key_info = parse_key_info(doc.root_element()).unwrap(); + assert_eq!( + key_info.sources, + vec![KeyInfoSource::KeyValue(KeyValueInfo::Unsupported { + namespace: Some(XMLDSIG11_NS.to_string()), + local_name: "ECKeyValue".into(), + })] + ); } #[test] diff --git a/tests/fixtures/xmldsig/xmldsig11-interop-2012/signature-enveloping-p256_sha256.xml b/tests/fixtures/xmldsig/xmldsig11-interop-2012/signature-enveloping-p256_sha256.xml new file mode 100644 index 0000000..0c59f95 --- /dev/null +++ b/tests/fixtures/xmldsig/xmldsig11-interop-2012/signature-enveloping-p256_sha256.xml @@ -0,0 +1 @@ +vIgv7JtPOh3hpedKK0rm8XHtYCSoBX4eEF0YwnB26Es=eYx4ImirtPG/eJLWgJHoMS30voH+tozerMftKbYz27vtYNgsHfAvV4M+oEkNgoibq5qnwsO2Z8nn+ndKxhVqFg==BJ/yaXNlq4FRObyJCBhb5jAz8GVzinK3bBGLjSDfjbJwNfydtgjnlS4EsDmxSRhWyJWq6GIqy5wvnaiARK04uB4=up up and away diff --git a/tests/fixtures/xmldsig/xmldsig11-interop-2012/signature-enveloping-p384_sha384.xml b/tests/fixtures/xmldsig/xmldsig11-interop-2012/signature-enveloping-p384_sha384.xml new file mode 100644 index 0000000..c143ba6 --- /dev/null +++ b/tests/fixtures/xmldsig/xmldsig11-interop-2012/signature-enveloping-p384_sha384.xml @@ -0,0 +1 @@ +QQzWA5o7Aj0x+LglAnSMqaZlUTGiAiWd+wFQwZQixBTly7WkzpFrU3pyPLLOIlB762PoqaZujP1OeuUBaJ4XvBBmMcA1OdZAW+yYP0tllL6oIMcB8Hu11wZtONDDI1XQO0OAFqfVAQ8OEbzUAH15/zydSItqs14IzW9id1dMruVwHzZCSZ0X94e09vLyDrndBO/yd/OZzDfjX4qivDY/vsUIuh6KWAxoxW5P4ukvwd+T6pVljWsX2UBJNNy5MdhTwB8e2YwB8kUbJwdsAS/XGi/fz8unFrs+lVlAgIs6s/xBYFbfUoRiAacD2SpVDe6XBA==up up and away diff --git a/tests/fixtures_smoke.rs b/tests/fixtures_smoke.rs index 0819261..40abc54 100644 --- a/tests/fixtures_smoke.rs +++ b/tests/fixtures_smoke.rs @@ -176,8 +176,8 @@ fn fixture_file_count_matches_expected() { let mut count = 0; count_files_recursive(fixtures_dir(), &mut count); assert_eq!( - count, 79, - "expected 79 fixture files total (23 keys + 41 c14n + 14 donor xmldsig + 1 saml); \ + count, 81, + "expected 81 fixture files total (23 keys + 41 c14n + 16 donor xmldsig + 1 saml); \ if you added/removed files, update this count" ); } From 7828213dc9c7b722de8f340d56874a8222b465d9 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 30 Jun 2026 04:02:05 +0300 Subject: [PATCH 3/7] fix(xmldsig): continue after unusable ECKeyValue --- src/xmldsig/keys.rs | 84 ++++++++++++++++++++++++++++++++++++++++---- src/xmldsig/parse.rs | 4 +-- 2 files changed, 80 insertions(+), 8 deletions(-) diff --git a/src/xmldsig/keys.rs b/src/xmldsig/keys.rs index fa46ab5..1abbcb8 100644 --- a/src/xmldsig/keys.rs +++ b/src/xmldsig/keys.rs @@ -11,8 +11,9 @@ use x509_parser::{ use super::{ DsigError, KeyInfo, KeyInfoSource, KeyResolver, KeyValueInfo, SignatureAlgorithm, VerifyingKey, - X509ChainOptions, X509DataInfo, verify_ecdsa_signature_spki, verify_rsa_signature_spki, - verify_x509_certificate_chain, + X509ChainOptions, X509DataInfo, + parse::{EC_P256_OID, EC_P384_OID}, + verify_ecdsa_signature_spki, verify_rsa_signature_spki, verify_x509_certificate_chain, }; /// A public verification key available to key resolvers. @@ -224,6 +225,7 @@ impl KeyResolver for DefaultKeyResolver { let Some(key_info) = key_info else { return Ok(None); }; + let mut deferred_key_value_error = None; for source in &key_info.sources { let resolved = match source { KeyInfoSource::X509Data(info) => self.resolve_x509(info, algorithm)?, @@ -249,13 +251,23 @@ impl KeyResolver for DefaultKeyResolver { }) .transpose()?, KeyInfoSource::KeyValue(key_value) => { - Self::resolve_key_value(key_value, algorithm)? + match Self::resolve_key_value(key_value, algorithm) { + Ok(resolved) => resolved, + Err(error) if ec_key_value_error_allows_fallback(key_value, &error) => { + deferred_key_value_error.get_or_insert(error); + None + } + Err(error) => return Err(error.into()), + } } }; if let Some(key) = resolved { return Ok(Some(Box::new(key))); } } + if let Some(error) = deferred_key_value_error { + return Err(error.into()); + } Ok(None) } @@ -283,12 +295,12 @@ fn ec_key_value_to_spki_der( public_key: &[u8], ) -> Result, KeyResolutionError> { match curve_oid { - "1.2.840.10045.3.1.7" => p256::PublicKey::from_sec1_bytes(public_key) + EC_P256_OID => p256::PublicKey::from_sec1_bytes(public_key) .map_err(|_| KeyResolutionError::InvalidPublicKey)? .to_public_key_der() .map_err(|_| KeyResolutionError::InvalidPublicKey) .map(|der| der.as_bytes().to_vec()), - "1.3.132.0.34" => p384::PublicKey::from_sec1_bytes(public_key) + EC_P384_OID => p384::PublicKey::from_sec1_bytes(public_key) .map_err(|_| KeyResolutionError::InvalidPublicKey)? .to_public_key_der() .map_err(|_| KeyResolutionError::InvalidPublicKey) @@ -297,6 +309,17 @@ fn ec_key_value_to_spki_der( } } +fn ec_key_value_error_allows_fallback( + key_value: &KeyValueInfo, + error: &KeyResolutionError, +) -> bool { + matches!(key_value, KeyValueInfo::Ec { .. }) + && matches!( + error, + KeyResolutionError::InvalidPublicKey | KeyResolutionError::AlgorithmMismatch + ) +} + fn validate_spki_algorithm( public_key_bytes: &[u8], algorithm: SignatureAlgorithm, @@ -563,7 +586,7 @@ mod tests { let result = super::super::VerifyContext::new() .key_resolver(&resolver) .verify(&xml) - .expect("incompatible ECKeyValue should be ignored"); + .expect("single incompatible ECKeyValue should be ignored"); assert_eq!( result.status, @@ -615,6 +638,55 @@ mod tests { assert_eq!(result.status, super::super::DsigStatus::Valid); } + #[test] + fn invalid_ec_key_value_falls_back_to_later_key_name() { + // Off-curve EC points are typed errors only if no later source can verify. + let key_info = r#"BAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=idp-signing"#; + let xml = replace_key_info(SIGNED_SAML, key_info); + let mut config = KeyResolverConfig::default(); + config.named_keys.insert( + "idp-signing".into(), + VerificationKey { + algorithm: SignatureAlgorithm::EcdsaP256Sha256, + public_key_bytes: public_key_der(SAML_PUBLIC_KEY), + certificate_der: None, + name: Some("idp-signing".into()), + }, + ); + let resolver = DefaultKeyResolver::new(config); + let result = super::super::VerifyContext::new() + .key_resolver(&resolver) + .verify(&xml) + .expect("later KeyName should resolve after invalid ECKeyValue"); + + assert_eq!(result.status, super::super::DsigStatus::Valid); + } + + #[test] + fn mismatched_ec_curve_falls_back_to_later_key_name() { + // A valid P-384 key is unusable for an ECDSA-SHA256 signature but must not + // prevent a later P-256 KeyName from resolving the same document. + let key_info = r#"BO/yd/OZzDfjX4qivDY/vsUIuh6KWAxoxW5P4ukvwd+T6pVljWsX2UBJNNy5MdhTwB8e2YwB8kUbJwdsAS/XGi/fz8unFrs+lVlAgIs6s/xBYFbfUoRiAacD2SpVDe6XBA==idp-signing"#; + let xml = replace_key_info(SIGNED_SAML, key_info); + let mut config = KeyResolverConfig::default(); + config.named_keys.insert( + "idp-signing".into(), + VerificationKey { + algorithm: SignatureAlgorithm::EcdsaP256Sha256, + public_key_bytes: public_key_der(SAML_PUBLIC_KEY), + certificate_der: None, + name: Some("idp-signing".into()), + }, + ); + let resolver = DefaultKeyResolver::new(config); + let result = super::super::VerifyContext::new() + .key_resolver(&resolver) + .verify(&xml) + .expect("later KeyName should resolve after mismatched ECKeyValue"); + + assert_eq!(result.status, super::super::DsigStatus::Valid); + } + #[test] fn chain_verification_rejects_untrusted_embedded_certificate() { // Enabling chain policy must fail closed when no trust anchor is configured. diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index 5a2f4f9..ec343b5 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -39,8 +39,8 @@ const MAX_DER_ENCODED_KEY_VALUE_BASE64_LEN: usize = MAX_DER_ENCODED_KEY_VALUE_LE const MAX_KEY_NAME_TEXT_LEN: usize = 4096; const MAX_RSA_MODULUS_LEN: usize = 1024; const MAX_RSA_EXPONENT_LEN: usize = 8; -const EC_P256_OID: &str = "1.2.840.10045.3.1.7"; -const EC_P384_OID: &str = "1.3.132.0.34"; +pub(crate) const EC_P256_OID: &str = "1.2.840.10045.3.1.7"; +pub(crate) const EC_P384_OID: &str = "1.3.132.0.34"; const MAX_EC_PUBLIC_KEY_LEN: usize = 97; const MAX_X509_BASE64_TEXT_LEN: usize = 262_144; const MAX_X509_BASE64_NORMALIZED_LEN: usize = MAX_X509_BASE64_TEXT_LEN; From cf71e2c748abbbe3672fcac0b37d10bf1b9e7d4c Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 30 Jun 2026 04:21:24 +0300 Subject: [PATCH 4/7] fix(xmldsig): defer malformed ECKeyValue errors --- src/xmldsig/keys.rs | 67 ++++++++++++++++++++++++++++++++++++++++---- src/xmldsig/parse.rs | 31 ++++++++++++++------ 2 files changed, 85 insertions(+), 13 deletions(-) diff --git a/src/xmldsig/keys.rs b/src/xmldsig/keys.rs index 1abbcb8..ac13f79 100644 --- a/src/xmldsig/keys.rs +++ b/src/xmldsig/keys.rs @@ -203,6 +203,7 @@ impl DefaultKeyResolver { } ec_key_value_to_spki_der(curve_oid, public_key)? } + KeyValueInfo::InvalidEcPublicKey => return Err(KeyResolutionError::InvalidPublicKey), KeyValueInfo::Unsupported { .. } => return Ok(None), }; validate_spki_algorithm(&public_key_bytes, algorithm)?; @@ -313,11 +314,13 @@ fn ec_key_value_error_allows_fallback( key_value: &KeyValueInfo, error: &KeyResolutionError, ) -> bool { - matches!(key_value, KeyValueInfo::Ec { .. }) - && matches!( - error, - KeyResolutionError::InvalidPublicKey | KeyResolutionError::AlgorithmMismatch - ) + matches!( + key_value, + KeyValueInfo::Ec { .. } | KeyValueInfo::InvalidEcPublicKey + ) && matches!( + error, + KeyResolutionError::InvalidPublicKey | KeyResolutionError::AlgorithmMismatch + ) } fn validate_spki_algorithm( @@ -662,6 +665,30 @@ mod tests { assert_eq!(result.status, super::super::DsigStatus::Valid); } + #[test] + fn malformed_ec_key_value_falls_back_to_later_key_name() { + // Parse-level EC point errors remain non-fatal while later sources exist. + let key_info = r#"AgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=idp-signing"#; + let xml = replace_key_info(SIGNED_SAML, key_info); + let mut config = KeyResolverConfig::default(); + config.named_keys.insert( + "idp-signing".into(), + VerificationKey { + algorithm: SignatureAlgorithm::EcdsaP256Sha256, + public_key_bytes: public_key_der(SAML_PUBLIC_KEY), + certificate_der: None, + name: Some("idp-signing".into()), + }, + ); + let resolver = DefaultKeyResolver::new(config); + let result = super::super::VerifyContext::new() + .key_resolver(&resolver) + .verify(&xml) + .expect("later KeyName should resolve after malformed ECKeyValue"); + + assert_eq!(result.status, super::super::DsigStatus::Valid); + } + #[test] fn mismatched_ec_curve_falls_back_to_later_key_name() { // A valid P-384 key is unusable for an ECDSA-SHA256 signature but must not @@ -687,6 +714,36 @@ mod tests { assert_eq!(result.status, super::super::DsigStatus::Valid); } + #[test] + fn lone_malformed_ec_key_value_reports_invalid_public_key() { + let key_info = r#"AgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="#; + let xml = replace_key_info(SIGNED_SAML, key_info); + let error = super::super::VerifyContext::new() + .key_resolver(&DefaultKeyResolver::default()) + .verify(&xml) + .expect_err("lone malformed ECKeyValue should surface typed key error"); + + assert!(matches!( + error, + DsigError::KeyResolution(KeyResolutionError::InvalidPublicKey) + )); + } + + #[test] + fn lone_mismatched_ec_curve_reports_algorithm_mismatch() { + let key_info = r#"BO/yd/OZzDfjX4qivDY/vsUIuh6KWAxoxW5P4ukvwd+T6pVljWsX2UBJNNy5MdhTwB8e2YwB8kUbJwdsAS/XGi/fz8unFrs+lVlAgIs6s/xBYFbfUoRiAacD2SpVDe6XBA=="#; + let xml = replace_key_info(SIGNED_SAML, key_info); + let error = super::super::VerifyContext::new() + .key_resolver(&DefaultKeyResolver::default()) + .verify(&xml) + .expect_err("lone mismatched ECKeyValue should surface typed key error"); + + assert!(matches!( + error, + DsigError::KeyResolution(KeyResolutionError::AlgorithmMismatch) + )); + } + #[test] fn chain_verification_rejects_untrusted_embedded_certificate() { // Enabling chain policy must fail closed when no trust anchor is configured. diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index ec343b5..4c2c731 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -177,6 +177,8 @@ pub enum KeyValueInfo { /// Uncompressed SEC1 point (`0x04 || x || y`). public_key: Vec, }, + /// `dsig11:ECKeyValue` with recognized curve metadata but unusable point bytes. + InvalidEcPublicKey, /// Any other `` child not yet supported by this phase. Unsupported { /// Namespace URI of the unsupported child, when present. @@ -622,8 +624,14 @@ fn parse_ec_key_value(node: Node<'_, '_>) -> Result { )); } - let public_key = decode_crypto_binary(public_key_node, "PublicKey", MAX_EC_PUBLIC_KEY_LEN)?; - validate_ec_public_key_point(&public_key, expected_public_key_len)?; + let public_key = match decode_crypto_binary(public_key_node, "PublicKey", MAX_EC_PUBLIC_KEY_LEN) + .and_then(|public_key| { + validate_ec_public_key_point(&public_key, expected_public_key_len)?; + Ok(public_key) + }) { + Ok(public_key) => public_key, + Err(_) => return Ok(KeyValueInfo::InvalidEcPublicKey), + }; Ok(KeyValueInfo::Ec { curve_oid, @@ -2354,6 +2362,9 @@ BA== #[test] fn parse_ec_key_value_accepts_bare_curve_oid() { + use base64::Engine; + + let encoded_public_key = "BO/yd/OZzDfjX4qivDY/vsUIuh6KWAxoxW5P4ukvwd+T6pVljWsX2UBJNNy5MdhTwB8e2YwB8kUbJwdsAS/XGi/fz8unFrs+lVlAgIs6s/xBYFbfUoRiAacD2SpVDe6XBA=="; let xml = r#" @@ -2364,13 +2375,16 @@ BA== "#; let doc = Document::parse(xml).unwrap(); + let expected_public_key = base64::engine::general_purpose::STANDARD + .decode(encoded_public_key) + .unwrap(); let sources = parse_key_info(doc.root_element()).unwrap().sources; assert!(matches!( &sources[0], KeyInfoSource::KeyValue(KeyValueInfo::Ec { curve_oid, public_key }) - if curve_oid == EC_P384_OID && public_key.len() == 97 + if curve_oid == EC_P384_OID && public_key == &expected_public_key )); } @@ -2459,7 +2473,7 @@ BA== } #[test] - fn parse_ec_key_value_rejects_non_uncompressed_point() { + fn parse_ec_key_value_marks_non_uncompressed_point_invalid() { let xml = r#" @@ -2471,10 +2485,11 @@ BA== "#; let doc = Document::parse(xml).unwrap(); - assert!(matches!( - parse_key_info(doc.root_element()), - Err(ParseError::InvalidStructure(_)) - )); + let key_info = parse_key_info(doc.root_element()).unwrap(); + assert_eq!( + key_info.sources, + vec![KeyInfoSource::KeyValue(KeyValueInfo::InvalidEcPublicKey)] + ); } #[test] From 84b9f9f2e4fe1eb9701a0ea5109bc834a29fa848 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 30 Jun 2026 04:30:29 +0300 Subject: [PATCH 5/7] fix(xmldsig): preserve ECKeyValue decode failures --- src/xmldsig/parse.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index 4c2c731..f21eaac 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -624,14 +624,10 @@ fn parse_ec_key_value(node: Node<'_, '_>) -> Result { )); } - let public_key = match decode_crypto_binary(public_key_node, "PublicKey", MAX_EC_PUBLIC_KEY_LEN) - .and_then(|public_key| { - validate_ec_public_key_point(&public_key, expected_public_key_len)?; - Ok(public_key) - }) { - Ok(public_key) => public_key, - Err(_) => return Ok(KeyValueInfo::InvalidEcPublicKey), - }; + let public_key = decode_crypto_binary(public_key_node, "PublicKey", MAX_EC_PUBLIC_KEY_LEN)?; + if validate_ec_public_key_point(&public_key, expected_public_key_len).is_err() { + return Ok(KeyValueInfo::InvalidEcPublicKey); + } Ok(KeyValueInfo::Ec { curve_oid, From 5895062748f9efcb86c38efdb32eb80389de3d78 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 30 Jun 2026 11:30:00 +0300 Subject: [PATCH 6/7] fix(xmldsig): preserve invalid ECKeyValue fallback --- src/xmldsig/keys.rs | 53 ++++++++++++++++++++++++++++++++++++++++++-- src/xmldsig/parse.rs | 31 +++++++++++++++++--------- 2 files changed, 71 insertions(+), 13 deletions(-) diff --git a/src/xmldsig/keys.rs b/src/xmldsig/keys.rs index ac13f79..809737a 100644 --- a/src/xmldsig/keys.rs +++ b/src/xmldsig/keys.rs @@ -203,7 +203,7 @@ impl DefaultKeyResolver { } ec_key_value_to_spki_der(curve_oid, public_key)? } - KeyValueInfo::InvalidEcPublicKey => return Err(KeyResolutionError::InvalidPublicKey), + KeyValueInfo::InvalidEcKeyValue => return Err(KeyResolutionError::InvalidPublicKey), KeyValueInfo::Unsupported { .. } => return Ok(None), }; validate_spki_algorithm(&public_key_bytes, algorithm)?; @@ -316,7 +316,7 @@ fn ec_key_value_error_allows_fallback( ) -> bool { matches!( key_value, - KeyValueInfo::Ec { .. } | KeyValueInfo::InvalidEcPublicKey + KeyValueInfo::Ec { .. } | KeyValueInfo::InvalidEcKeyValue ) && matches!( error, KeyResolutionError::InvalidPublicKey | KeyResolutionError::AlgorithmMismatch @@ -689,6 +689,55 @@ mod tests { assert_eq!(result.status, super::super::DsigStatus::Valid); } + #[test] + fn invalid_base64_ec_key_value_falls_back_to_later_key_name() { + // A bad ECKeyValue payload is an unusable source, not a reason to skip + // later ordered KeyInfo sources that can verify the signature. + let key_info = r#"not base64!idp-signing"#; + let xml = replace_key_info(SIGNED_SAML, key_info); + let mut config = KeyResolverConfig::default(); + config.named_keys.insert( + "idp-signing".into(), + VerificationKey { + algorithm: SignatureAlgorithm::EcdsaP256Sha256, + public_key_bytes: public_key_der(SAML_PUBLIC_KEY), + certificate_der: None, + name: Some("idp-signing".into()), + }, + ); + let resolver = DefaultKeyResolver::new(config); + let result = super::super::VerifyContext::new() + .key_resolver(&resolver) + .verify(&xml) + .expect("later KeyName should resolve after bad ECKeyValue base64"); + + assert_eq!(result.status, super::super::DsigStatus::Valid); + } + + #[test] + fn missing_curve_uri_ec_key_value_falls_back_to_later_key_name() { + // Missing EC curve parameters make only this KeyValue source unusable. + let key_info = r#"BA==idp-signing"#; + let xml = replace_key_info(SIGNED_SAML, key_info); + let mut config = KeyResolverConfig::default(); + config.named_keys.insert( + "idp-signing".into(), + VerificationKey { + algorithm: SignatureAlgorithm::EcdsaP256Sha256, + public_key_bytes: public_key_der(SAML_PUBLIC_KEY), + certificate_der: None, + name: Some("idp-signing".into()), + }, + ); + let resolver = DefaultKeyResolver::new(config); + let result = super::super::VerifyContext::new() + .key_resolver(&resolver) + .verify(&xml) + .expect("later KeyName should resolve after missing EC curve URI"); + + assert_eq!(result.status, super::super::DsigStatus::Valid); + } + #[test] fn mismatched_ec_curve_falls_back_to_later_key_name() { // A valid P-384 key is unusable for an ECDSA-SHA256 signature but must not diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index f21eaac..24ea6c8 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -177,8 +177,8 @@ pub enum KeyValueInfo { /// Uncompressed SEC1 point (`0x04 || x || y`). public_key: Vec, }, - /// `dsig11:ECKeyValue` with recognized curve metadata but unusable point bytes. - InvalidEcPublicKey, + /// `dsig11:ECKeyValue` with unusable curve or point data. + InvalidEcKeyValue, /// Any other `` child not yet supported by this phase. Unsupported { /// Namespace URI of the unsupported child, when present. @@ -605,7 +605,11 @@ fn parse_ec_key_value(node: Node<'_, '_>) -> Result { verify_dsig11_element(named_curve_node, "NamedCurve")?; ensure_no_element_children(named_curve_node, "NamedCurve")?; ensure_no_non_whitespace_text(named_curve_node, "NamedCurve")?; - let Some((curve_oid, expected_public_key_len)) = parse_ec_named_curve_oid(named_curve_node)? + let Some((curve_oid, expected_public_key_len)) = + (match parse_ec_named_curve_oid(named_curve_node) { + Ok(curve) => curve, + Err(_) => return Ok(KeyValueInfo::InvalidEcKeyValue), + }) else { return Ok(KeyValueInfo::Unsupported { namespace: Some(XMLDSIG11_NS.to_string()), @@ -624,9 +628,13 @@ fn parse_ec_key_value(node: Node<'_, '_>) -> Result { )); } - let public_key = decode_crypto_binary(public_key_node, "PublicKey", MAX_EC_PUBLIC_KEY_LEN)?; + let public_key = match decode_crypto_binary(public_key_node, "PublicKey", MAX_EC_PUBLIC_KEY_LEN) + { + Ok(public_key) => public_key, + Err(_) => return Ok(KeyValueInfo::InvalidEcKeyValue), + }; if validate_ec_public_key_point(&public_key, expected_public_key_len).is_err() { - return Ok(KeyValueInfo::InvalidEcPublicKey); + return Ok(KeyValueInfo::InvalidEcKeyValue); } Ok(KeyValueInfo::Ec { @@ -2431,7 +2439,7 @@ BA== } #[test] - fn parse_ec_key_value_rejects_missing_named_curve_uri() { + fn parse_ec_key_value_marks_missing_named_curve_uri_invalid() { let xml = r#" @@ -2443,10 +2451,11 @@ BA== "#; let doc = Document::parse(xml).unwrap(); - assert!(matches!( - parse_key_info(doc.root_element()), - Err(ParseError::InvalidStructure(_)) - )); + let key_info = parse_key_info(doc.root_element()).unwrap(); + assert_eq!( + key_info.sources, + vec![KeyInfoSource::KeyValue(KeyValueInfo::InvalidEcKeyValue)] + ); } #[test] @@ -2484,7 +2493,7 @@ BA== let key_info = parse_key_info(doc.root_element()).unwrap(); assert_eq!( key_info.sources, - vec![KeyInfoSource::KeyValue(KeyValueInfo::InvalidEcPublicKey)] + vec![KeyInfoSource::KeyValue(KeyValueInfo::InvalidEcKeyValue)] ); } From 71b412f51db009dda949066f3141dd518ae78d0f Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 30 Jun 2026 20:52:47 +0300 Subject: [PATCH 7/7] fix(xmldsig): preserve malformed EC fallback --- src/xmldsig/keys.rs | 35 +++++++++++++++++++++++++++++++++++ src/xmldsig/parse.rs | 39 +++++++++++++++++++++++---------------- 2 files changed, 58 insertions(+), 16 deletions(-) diff --git a/src/xmldsig/keys.rs b/src/xmldsig/keys.rs index 809737a..3a0f4d0 100644 --- a/src/xmldsig/keys.rs +++ b/src/xmldsig/keys.rs @@ -738,6 +738,41 @@ mod tests { assert_eq!(result.status, super::super::DsigStatus::Valid); } + #[test] + fn malformed_ec_key_value_children_fall_back_to_later_key_name() { + // An unusable EC source must not prevent later ordered KeyInfo sources + // from resolving, regardless of which required child-shape check fails. + let malformed_ec_key_values = [ + r#""#, + r#""#, + r#"BA==BA=="#, + ]; + + for malformed_children in malformed_ec_key_values { + let key_info = format!( + r#"{malformed_children}idp-signing"# + ); + let xml = replace_key_info(SIGNED_SAML, &key_info); + let mut config = KeyResolverConfig::default(); + config.named_keys.insert( + "idp-signing".into(), + VerificationKey { + algorithm: SignatureAlgorithm::EcdsaP256Sha256, + public_key_bytes: public_key_der(SAML_PUBLIC_KEY), + certificate_der: None, + name: Some("idp-signing".into()), + }, + ); + let resolver = DefaultKeyResolver::new(config); + let result = super::super::VerifyContext::new() + .key_resolver(&resolver) + .verify(&xml) + .expect("later KeyName should resolve after malformed EC child shape"); + + assert_eq!(result.status, super::super::DsigStatus::Valid); + } + } + #[test] fn mismatched_ec_curve_falls_back_to_later_key_name() { // A valid P-384 key is unusable for an ECDSA-SHA256 signature but must not diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index 24ea6c8..51f54e8 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -591,9 +591,9 @@ fn parse_ec_key_value(node: Node<'_, '_>) -> Result { ensure_no_non_whitespace_text(node, "ECKeyValue")?; let mut children = element_children(node); - let named_curve_node = children.next().ok_or_else(|| { - ParseError::InvalidStructure("ECKeyValue requires NamedCurve and PublicKey".into()) - })?; + let Some(named_curve_node) = children.next() else { + return Ok(KeyValueInfo::InvalidEcKeyValue); + }; if named_curve_node.tag_name().namespace() == Some(XMLDSIG11_NS) && named_curve_node.tag_name().name() == "ECParameters" { @@ -602,7 +602,11 @@ fn parse_ec_key_value(node: Node<'_, '_>) -> Result { local_name: "ECKeyValue".into(), }); } - verify_dsig11_element(named_curve_node, "NamedCurve")?; + if named_curve_node.tag_name().namespace() != Some(XMLDSIG11_NS) + || named_curve_node.tag_name().name() != "NamedCurve" + { + return Ok(KeyValueInfo::InvalidEcKeyValue); + } ensure_no_element_children(named_curve_node, "NamedCurve")?; ensure_no_non_whitespace_text(named_curve_node, "NamedCurve")?; let Some((curve_oid, expected_public_key_len)) = @@ -617,15 +621,17 @@ fn parse_ec_key_value(node: Node<'_, '_>) -> Result { }); }; - let public_key_node = children.next().ok_or_else(|| { - ParseError::InvalidStructure("ECKeyValue requires NamedCurve and PublicKey".into()) - })?; - verify_dsig11_element(public_key_node, "PublicKey")?; + let Some(public_key_node) = children.next() else { + return Ok(KeyValueInfo::InvalidEcKeyValue); + }; + if public_key_node.tag_name().namespace() != Some(XMLDSIG11_NS) + || public_key_node.tag_name().name() != "PublicKey" + { + return Ok(KeyValueInfo::InvalidEcKeyValue); + } ensure_no_element_children(public_key_node, "PublicKey")?; if children.next().is_some() { - return Err(ParseError::InvalidStructure( - "ECKeyValue must contain exactly NamedCurve followed by PublicKey".into(), - )); + return Ok(KeyValueInfo::InvalidEcKeyValue); } let public_key = match decode_crypto_binary(public_key_node, "PublicKey", MAX_EC_PUBLIC_KEY_LEN) @@ -2459,7 +2465,7 @@ BA== } #[test] - fn parse_ec_key_value_rejects_reordered_children() { + fn parse_ec_key_value_marks_reordered_children_invalid() { let xml = r#" @@ -2471,10 +2477,11 @@ BA== "#; let doc = Document::parse(xml).unwrap(); - assert!(matches!( - parse_key_info(doc.root_element()), - Err(ParseError::InvalidStructure(_)) - )); + let key_info = parse_key_info(doc.root_element()).unwrap(); + assert_eq!( + key_info.sources, + vec![KeyInfoSource::KeyValue(KeyValueInfo::InvalidEcKeyValue)] + ); } #[test]