From 7063ba03c02d716493539d5d129e61316edfdbd0 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 4 Apr 2026 23:43:55 +0300 Subject: [PATCH 01/21] feat(xmldsig): add keyinfo dispatch parsing - parse KeyInfo child sources: KeyName, KeyValue, X509Data, DEREncodedKeyValue - wire KeyInfo parsing into signature verification pipeline with explicit ParseKeyInfo error - add parser and pipeline coverage tests for order and malformed input Closes #46 --- src/xmldsig/mod.rs | 3 +- src/xmldsig/parse.rs | 306 ++++++++++++++++++++++++++++++++++++++++++ src/xmldsig/verify.rs | 71 +++++++++- 3 files changed, 378 insertions(+), 2 deletions(-) diff --git a/src/xmldsig/mod.rs b/src/xmldsig/mod.rs index 781b098..cfe8a3b 100644 --- a/src/xmldsig/mod.rs +++ b/src/xmldsig/mod.rs @@ -18,7 +18,8 @@ pub mod verify; pub use digest::{DigestAlgorithm, compute_digest, constant_time_eq}; pub use parse::{ - ParseError, Reference, SignatureAlgorithm, SignedInfo, find_signature_node, parse_signed_info, + KeyInfo, KeyInfoSource, KeyValueInfo, ParseError, Reference, SignatureAlgorithm, SignedInfo, + X509DataInfo, find_signature_node, parse_key_info, parse_signed_info, }; pub use signature::{ SignatureVerificationError, verify_ecdsa_signature_pem, verify_ecdsa_signature_spki, diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index f4ba716..36e8486 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -24,6 +24,8 @@ use crate::c14n::C14nAlgorithm; /// XMLDSig namespace URI. pub(crate) const XMLDSIG_NS: &str = "http://www.w3.org/2000/09/xmldsig#"; +/// XMLDSig 1.1 namespace URI. +pub(crate) const XMLDSIG11_NS: &str = "http://www.w3.org/2009/xmldsig11#"; /// Signature algorithms supported for signing and verification. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] @@ -110,6 +112,54 @@ pub struct Reference { pub digest_value: Vec, } +/// Parsed `` element. +#[derive(Debug, Default, Clone, PartialEq, Eq)] +pub struct KeyInfo { + /// Sources discovered under `` in document order. + pub sources: Vec, +} + +/// Top-level key material source parsed from ``. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum KeyInfoSource { + /// `` source. + KeyName(String), + /// `` source. + KeyValue(KeyValueInfo), + /// `` source. + X509Data(X509DataInfo), + /// `dsig11:DEREncodedKeyValue` source (base64-decoded DER bytes). + DerEncodedKeyValue(Vec), +} + +/// Parsed `` dispatch result. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum KeyValueInfo { + /// ``. + RsaKeyValue, + /// ``. + EcKeyValue, + /// Any other `` child not yet supported by this phase. + Unsupported(String), +} + +/// Parsed `` children (dispatch-only in P2-001). +#[derive(Debug, Default, Clone, PartialEq, Eq)] +pub struct X509DataInfo { + /// Number of `` children. + pub certificate_count: usize, + /// Number of `` children. + pub subject_name_count: usize, + /// Number of `` children. + pub issuer_serial_count: usize, + /// Number of `` children. + pub ski_count: usize, + /// Number of `` children. + pub crl_count: usize, + /// Number of `` children. + pub digest_count: usize, +} + /// Errors during XMLDSig element parsing. #[derive(Debug, thiserror::Error)] #[non_exhaustive] @@ -292,6 +342,49 @@ pub(crate) fn parse_reference(reference_node: Node) -> Result` and dispatch supported child sources. +/// +/// Supported source elements: +/// - `` +/// - `` (dispatch to RSA vs EC by child element name) +/// - `` +/// - `` +/// +/// Unknown elements are ignored (lax processing), matching XMLDSig behavior. +pub fn parse_key_info(key_info_node: Node) -> Result { + verify_ds_element(key_info_node, "KeyInfo")?; + + let mut sources = Vec::new(); + for child in element_children(key_info_node) { + match (child.tag_name().namespace(), child.tag_name().name()) { + (Some(XMLDSIG_NS), "KeyName") => { + let key_name = collect_text_content(child).trim().to_string(); + if key_name.is_empty() { + return Err(ParseError::InvalidStructure( + "KeyName must contain non-empty text".into(), + )); + } + sources.push(KeyInfoSource::KeyName(key_name)); + } + (Some(XMLDSIG_NS), "KeyValue") => { + let key_value = parse_key_value_dispatch(child)?; + sources.push(KeyInfoSource::KeyValue(key_value)); + } + (Some(XMLDSIG_NS), "X509Data") => { + let x509 = parse_x509_data_dispatch(child)?; + sources.push(KeyInfoSource::X509Data(x509)); + } + (Some(XMLDSIG11_NS), "DEREncodedKeyValue") => { + let der = decode_base64_xml_text(&collect_text_content(child))?; + sources.push(KeyInfoSource::DerEncodedKeyValue(der)); + } + _ => {} + } + } + + Ok(KeyInfo { sources }) +} + // ── Helpers ────────────────────────────────────────────────────────────────── /// Iterate only element children (skip text, comments, PIs). @@ -358,6 +451,60 @@ fn parse_inclusive_prefixes(node: Node) -> Result, ParseError> { Ok(None) } +fn parse_key_value_dispatch(node: Node) -> Result { + verify_ds_element(node, "KeyValue")?; + + let mut children = element_children(node); + let Some(first_child) = children.next() else { + return Err(ParseError::InvalidStructure( + "KeyValue must contain exactly one key-value child".into(), + )); + }; + if children.next().is_some() { + return Err(ParseError::InvalidStructure( + "KeyValue must contain exactly one key-value child".into(), + )); + } + + match ( + first_child.tag_name().namespace(), + first_child.tag_name().name(), + ) { + (Some(XMLDSIG_NS), "RSAKeyValue") => Ok(KeyValueInfo::RsaKeyValue), + (Some(XMLDSIG_NS), "ECKeyValue") | (Some(XMLDSIG11_NS), "ECKeyValue") => { + Ok(KeyValueInfo::EcKeyValue) + } + (_, child_name) => Ok(KeyValueInfo::Unsupported(child_name.to_string())), + } +} + +fn parse_x509_data_dispatch(node: Node) -> Result { + verify_ds_element(node, "X509Data")?; + + let mut info = X509DataInfo::default(); + let mut saw_child = false; + for child in element_children(node) { + saw_child = true; + match (child.tag_name().namespace(), child.tag_name().name()) { + (Some(XMLDSIG_NS), "X509Certificate") => info.certificate_count += 1, + (Some(XMLDSIG_NS), "X509SubjectName") => info.subject_name_count += 1, + (Some(XMLDSIG_NS), "X509IssuerSerial") => info.issuer_serial_count += 1, + (Some(XMLDSIG_NS), "X509SKI") => info.ski_count += 1, + (Some(XMLDSIG_NS), "X509CRL") => info.crl_count += 1, + (Some(XMLDSIG11_NS), "X509Digest") => info.digest_count += 1, + _ => {} + } + } + + if !saw_child { + return Err(ParseError::InvalidStructure( + "X509Data must contain at least one child element".into(), + )); + } + + Ok(info) +} + /// Base64-decode a digest value string, stripping whitespace. /// /// XMLDSig allows whitespace within base64 content (line-wrapped encodings). @@ -393,6 +540,35 @@ fn base64_decode_digest(b64: &str, digest_method: DigestAlgorithm) -> Result Result, ParseError> { + use base64::Engine; + use base64::engine::general_purpose::STANDARD; + + let mut cleaned = String::with_capacity(b64.len()); + for ch in b64.chars() { + if matches!(ch, ' ' | '\t' | '\r' | '\n') { + continue; + } + if ch.is_ascii_whitespace() { + return Err(ParseError::Base64(format!( + "invalid XML whitespace U+{:04X} in base64 text", + u32::from(ch) + ))); + } + cleaned.push(ch); + } + + STANDARD + .decode(&cleaned) + .map_err(|e| ParseError::Base64(e.to_string())) +} + +fn collect_text_content(node: Node<'_, '_>) -> String { + node.children() + .filter_map(|child| child.is_text().then(|| child.text()).flatten()) + .collect() +} + #[cfg(test)] #[expect(clippy::unwrap_used, reason = "tests use trusted XML fixtures")] mod tests { @@ -486,6 +662,136 @@ mod tests { assert!(find_signature_node(&doc).is_none()); } + // ── parse_key_info: dispatch parsing ────────────────────────────── + + #[test] + fn parse_key_info_dispatches_supported_children() { + let xml = r#" + idp-signing-key + + + AQAB + AQAB + + + + MIIB + CN=Example + + AQIDBA== + "#; + let doc = Document::parse(xml).unwrap(); + + let key_info = parse_key_info(doc.root_element()).unwrap(); + assert_eq!(key_info.sources.len(), 4); + + assert_eq!( + key_info.sources[0], + KeyInfoSource::KeyName("idp-signing-key".to_string()) + ); + assert_eq!( + key_info.sources[1], + KeyInfoSource::KeyValue(KeyValueInfo::RsaKeyValue) + ); + assert_eq!( + key_info.sources[2], + KeyInfoSource::X509Data(X509DataInfo { + certificate_count: 1, + subject_name_count: 1, + issuer_serial_count: 0, + ski_count: 0, + crl_count: 0, + digest_count: 0, + }) + ); + assert_eq!( + key_info.sources[3], + KeyInfoSource::DerEncodedKeyValue(vec![1, 2, 3, 4]) + ); + } + + #[test] + fn parse_key_info_ignores_unknown_children() { + let xml = r#" + bar + ok + "#; + let doc = Document::parse(xml).unwrap(); + + let key_info = parse_key_info(doc.root_element()).unwrap(); + assert_eq!(key_info.sources, vec![KeyInfoSource::KeyName("ok".into())]); + } + + #[test] + fn parse_key_info_keyvalue_requires_single_child() { + let xml = r#" + + "#; + let doc = Document::parse(xml).unwrap(); + + let err = parse_key_info(doc.root_element()).unwrap_err(); + assert!(matches!(err, ParseError::InvalidStructure(_))); + } + + #[test] + fn parse_key_info_x509data_must_not_be_empty() { + let xml = r#" + + "#; + let doc = Document::parse(xml).unwrap(); + + let err = parse_key_info(doc.root_element()).unwrap_err(); + assert!(matches!(err, ParseError::InvalidStructure(_))); + } + + #[test] + fn parse_key_info_der_encoded_key_value_rejects_invalid_base64() { + let xml = r#" + %%%invalid%%% + "#; + let doc = Document::parse(xml).unwrap(); + + let err = parse_key_info(doc.root_element()).unwrap_err(); + assert!(matches!(err, ParseError::Base64(_))); + } + + #[test] + fn parse_key_info_dispatches_dsig11_ec_keyvalue() { + let xml = r#" + + + + "#; + 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::EcKeyValue)] + ); + } + + #[test] + fn parse_key_info_keeps_unsupported_keyvalue_child_as_marker() { + let xml = r#" + + + + "#; + 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( + "DSAKeyValue".into() + ))] + ); + } + // ── parse_signed_info: happy path ──────────────────────────────── #[test] diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index cd47efb..b396f63 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -18,7 +18,7 @@ use crate::c14n::canonicalize; use super::digest::{DigestAlgorithm, compute_digest, constant_time_eq}; use super::parse::{ParseError, Reference, SignatureAlgorithm, XMLDSIG_NS}; -use super::parse::{parse_reference, parse_signed_info}; +use super::parse::{parse_key_info, parse_reference, parse_signed_info}; use super::signature::{ SignatureVerificationError, verify_ecdsa_signature_pem, verify_rsa_signature_pem, }; @@ -510,6 +510,10 @@ pub enum DsigError { #[error("failed to parse SignedInfo: {0}")] ParseSignedInfo(#[from] super::parse::ParseError), + /// `` parsing failed. + #[error("failed to parse KeyInfo: {0}")] + ParseKeyInfo(#[source] super::parse::ParseError), + /// `//` parsing failed. #[error("failed to parse Manifest reference: {0}")] ParseManifestReference(#[source] ParseError), @@ -617,6 +621,9 @@ fn verify_signature_with_context( let signature_children = parse_signature_children(signature_node)?; let signed_info_node = signature_children.signed_info_node; + if let Some(key_info_node) = signature_children.key_info_node { + parse_key_info(key_info_node).map_err(SignatureVerificationPipelineError::ParseKeyInfo)?; + } let signed_info = parse_signed_info(signed_info_node)?; enforce_reference_policies( @@ -951,6 +958,7 @@ fn transform_uri(transform: &Transform) -> &'static str { struct SignatureChildNodes<'a, 'input> { signed_info_node: Node<'a, 'input>, signature_value_node: Node<'a, 'input>, + key_info_node: Option>, } fn parse_signature_children<'a, 'input>( @@ -958,8 +966,10 @@ fn parse_signature_children<'a, 'input>( ) -> Result, SignatureVerificationPipelineError> { let mut signed_info_node: Option> = None; let mut signature_value_node: Option> = None; + let mut key_info_node: Option> = None; let mut signed_info_index: Option = None; let mut signature_value_index: Option = None; + let mut key_info_index: Option = None; for (zero_based_index, child) in signature_node .children() .filter(|node| node.is_element()) @@ -988,6 +998,15 @@ fn parse_signature_children<'a, 'input>( signature_value_node = Some(child); signature_value_index = Some(element_index); } + "KeyInfo" => { + if key_info_node.is_some() { + return Err(SignatureVerificationPipelineError::InvalidStructure { + reason: "KeyInfo must appear at most once under Signature", + }); + } + key_info_node = Some(child); + key_info_index = Some(element_index); + } _ => {} } } @@ -1010,9 +1029,17 @@ fn parse_signature_children<'a, 'input>( reason: "SignatureValue must be the second element child of Signature", }); } + if let Some(index) = key_info_index + && index != 3 + { + return Err(SignatureVerificationPipelineError::InvalidStructure { + reason: "KeyInfo must be the third element child of Signature when present", + }); + } Ok(SignatureChildNodes { signed_info_node, signature_value_node, + key_info_node, }) } @@ -2444,4 +2471,46 @@ mod tests { } )); } + + #[test] + fn pipeline_reports_keyinfo_parse_error() { + let xml = r#" + + + AA== + + %%%invalid%%% + + +"#; + + let err = verify_signature_with_pem_key(xml, "dummy-key", false) + .expect_err("invalid KeyInfo must map to ParseKeyInfo"); + assert!(matches!( + err, + SignatureVerificationPipelineError::ParseKeyInfo(_) + )); + } + + #[test] + fn pipeline_rejects_keyinfo_out_of_order() { + let xml = r#" + + + AA== + + late + +"#; + + let err = verify_signature_with_pem_key(xml, "dummy-key", false) + .expect_err("KeyInfo after Object must be rejected by Signature child order checks"); + assert!(matches!( + err, + SignatureVerificationPipelineError::InvalidStructure { + reason: "KeyInfo must be the third element child of Signature when present" + } + )); + } } From 2004cf816f9d9dbbf3113455c5138fc7f3b6eb8b Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sun, 5 Apr 2026 00:22:27 +0300 Subject: [PATCH 02/21] fix(xmldsig): harden keyinfo and signature child parsing --- src/xmldsig/parse.rs | 72 +++++++++++++++++++++++++++++++++++++++---- src/xmldsig/verify.rs | 37 +++++++++++++++++++++- 2 files changed, 102 insertions(+), 7 deletions(-) diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index 36e8486..230ecfa 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -114,6 +114,7 @@ pub struct Reference { /// Parsed `` element. #[derive(Debug, Default, Clone, PartialEq, Eq)] +#[non_exhaustive] pub struct KeyInfo { /// Sources discovered under `` in document order. pub sources: Vec, @@ -121,6 +122,7 @@ pub struct KeyInfo { /// Top-level key material source parsed from ``. #[derive(Debug, Clone, PartialEq, Eq)] +#[non_exhaustive] pub enum KeyInfoSource { /// `` source. KeyName(String), @@ -134,6 +136,7 @@ pub enum KeyInfoSource { /// Parsed `` dispatch result. #[derive(Debug, Clone, PartialEq, Eq)] +#[non_exhaustive] pub enum KeyValueInfo { /// ``. RsaKeyValue, @@ -145,6 +148,7 @@ pub enum KeyValueInfo { /// Parsed `` children (dispatch-only in P2-001). #[derive(Debug, Default, Clone, PartialEq, Eq)] +#[non_exhaustive] pub struct X509DataInfo { /// Number of `` children. pub certificate_count: usize, @@ -353,17 +357,14 @@ pub(crate) fn parse_reference(reference_node: Node) -> Result Result { verify_ds_element(key_info_node, "KeyInfo")?; + ensure_no_non_whitespace_text(key_info_node, "KeyInfo")?; let mut sources = Vec::new(); for child in element_children(key_info_node) { match (child.tag_name().namespace(), child.tag_name().name()) { (Some(XMLDSIG_NS), "KeyName") => { - let key_name = collect_text_content(child).trim().to_string(); - if key_name.is_empty() { - return Err(ParseError::InvalidStructure( - "KeyName must contain non-empty text".into(), - )); - } + ensure_no_element_children(child, "KeyName")?; + let key_name = collect_text_content(child); sources.push(KeyInfoSource::KeyName(key_name)); } (Some(XMLDSIG_NS), "KeyValue") => { @@ -375,6 +376,7 @@ pub fn parse_key_info(key_info_node: Node) -> Result { sources.push(KeyInfoSource::X509Data(x509)); } (Some(XMLDSIG11_NS), "DEREncodedKeyValue") => { + ensure_no_element_children(child, "DEREncodedKeyValue")?; let der = decode_base64_xml_text(&collect_text_content(child))?; sources.push(KeyInfoSource::DerEncodedKeyValue(der)); } @@ -453,6 +455,7 @@ fn parse_inclusive_prefixes(node: Node) -> Result, ParseError> { fn parse_key_value_dispatch(node: Node) -> Result { verify_ds_element(node, "KeyValue")?; + ensure_no_non_whitespace_text(node, "KeyValue")?; let mut children = element_children(node); let Some(first_child) = children.next() else { @@ -480,6 +483,7 @@ fn parse_key_value_dispatch(node: Node) -> Result { fn parse_x509_data_dispatch(node: Node) -> Result { verify_ds_element(node, "X509Data")?; + ensure_no_non_whitespace_text(node, "X509Data")?; let mut info = X509DataInfo::default(); let mut saw_child = false; @@ -569,6 +573,28 @@ fn collect_text_content(node: Node<'_, '_>) -> String { .collect() } +fn ensure_no_element_children(node: Node<'_, '_>, element_name: &str) -> Result<(), ParseError> { + if node.children().any(|child| child.is_element()) { + return Err(ParseError::InvalidStructure(format!( + "{element_name} must not contain child elements" + ))); + } + Ok(()) +} + +fn ensure_no_non_whitespace_text(node: Node<'_, '_>, element_name: &str) -> Result<(), ParseError> { + for child in node.children().filter(|child| child.is_text()) { + if let Some(text) = child.text() + && !text.trim().is_empty() + { + return Err(ParseError::InvalidStructure(format!( + "{element_name} must not contain non-whitespace mixed content" + ))); + } + } + Ok(()) +} + #[cfg(test)] #[expect(clippy::unwrap_used, reason = "tests use trusted XML fixtures")] mod tests { @@ -792,6 +818,40 @@ mod tests { ); } + #[test] + fn parse_key_info_rejects_keyname_with_child_elements() { + let xml = r#" + ok + "#; + let doc = Document::parse(xml).unwrap(); + + let err = parse_key_info(doc.root_element()).unwrap_err(); + assert!(matches!(err, ParseError::InvalidStructure(_))); + } + + #[test] + fn parse_key_info_preserves_keyname_text_without_trimming() { + let xml = r#" + signing key + "#; + let doc = Document::parse(xml).unwrap(); + + let key_info = parse_key_info(doc.root_element()).unwrap(); + assert_eq!( + key_info.sources, + vec![KeyInfoSource::KeyName(" signing key ".into())] + ); + } + + #[test] + fn parse_key_info_rejects_non_whitespace_mixed_content() { + let xml = r#"oopsk"#; + let doc = Document::parse(xml).unwrap(); + + let err = parse_key_info(doc.root_element()).unwrap_err(); + assert!(matches!(err, ParseError::InvalidStructure(_))); + } + // ── parse_signed_info: happy path ──────────────────────────────── #[test] diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index b396f63..4dd5bab 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -970,6 +970,9 @@ fn parse_signature_children<'a, 'input>( let mut signed_info_index: Option = None; let mut signature_value_index: Option = None; let mut key_info_index: Option = None; + let mut object_indices: Vec = Vec::new(); + let mut first_unexpected_dsig_index: Option = None; + for (zero_based_index, child) in signature_node .children() .filter(|node| node.is_element()) @@ -1007,7 +1010,14 @@ fn parse_signature_children<'a, 'input>( key_info_node = Some(child); key_info_index = Some(element_index); } - _ => {} + "Object" => { + object_indices.push(element_index); + } + _ => { + if first_unexpected_dsig_index.is_none() { + first_unexpected_dsig_index = Some(element_index); + } + } } } @@ -1036,6 +1046,31 @@ fn parse_signature_children<'a, 'input>( reason: "KeyInfo must be the third element child of Signature when present", }); } + + let allowed_prefix_end = key_info_index.unwrap_or(2); + if let Some(unexpected_index) = first_unexpected_dsig_index { + return Err(SignatureVerificationPipelineError::InvalidStructure { + reason: if unexpected_index > allowed_prefix_end { + "Signature may contain only Object elements after the allowed prefix" + } else { + "unexpected XMLDSIG child element under Signature" + }, + }); + } + + if object_indices + .iter() + .any(|&index| index <= allowed_prefix_end) + { + return Err(SignatureVerificationPipelineError::InvalidStructure { + reason: if allowed_prefix_end == 3 { + "KeyInfo must be the third element child of Signature when present" + } else { + "SignatureValue must be the second element child of Signature" + }, + }); + } + Ok(SignatureChildNodes { signed_info_node, signature_value_node, From 5238c921e097ba28ab3ec03201f41cee2a1f6227 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sun, 5 Apr 2026 00:48:56 +0300 Subject: [PATCH 03/21] fix(xmldsig): cap keyinfo der payload and clarify errors --- src/xmldsig/parse.rs | 56 ++++++++++++++++++++++++++++++++++++++++--- src/xmldsig/verify.rs | 4 ++-- 2 files changed, 55 insertions(+), 5 deletions(-) diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index 230ecfa..b98b810 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -26,6 +26,9 @@ use crate::c14n::C14nAlgorithm; pub(crate) const XMLDSIG_NS: &str = "http://www.w3.org/2000/09/xmldsig#"; /// XMLDSig 1.1 namespace URI. pub(crate) const XMLDSIG11_NS: &str = "http://www.w3.org/2009/xmldsig11#"; +const MAX_DER_ENCODED_KEY_VALUE_LEN: usize = 8192; +const MAX_DER_ENCODED_KEY_VALUE_TEXT_LEN: usize = 65_536; +const MAX_DER_ENCODED_KEY_VALUE_BASE64_LEN: usize = MAX_DER_ENCODED_KEY_VALUE_LEN.div_ceil(3) * 4; /// Signature algorithms supported for signing and verification. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] @@ -549,7 +552,14 @@ fn decode_base64_xml_text(b64: &str) -> Result, ParseError> { use base64::engine::general_purpose::STANDARD; let mut cleaned = String::with_capacity(b64.len()); + let mut raw_text_len = 0usize; for ch in b64.chars() { + if raw_text_len.saturating_add(ch.len_utf8()) > MAX_DER_ENCODED_KEY_VALUE_TEXT_LEN { + return Err(ParseError::InvalidStructure( + "DEREncodedKeyValue exceeds maximum allowed text length".into(), + )); + } + raw_text_len = raw_text_len.saturating_add(ch.len_utf8()); if matches!(ch, ' ' | '\t' | '\r' | '\n') { continue; } @@ -560,11 +570,22 @@ fn decode_base64_xml_text(b64: &str) -> Result, ParseError> { ))); } cleaned.push(ch); + if cleaned.len() > MAX_DER_ENCODED_KEY_VALUE_BASE64_LEN { + return Err(ParseError::InvalidStructure( + "DEREncodedKeyValue exceeds maximum allowed length".into(), + )); + } } - STANDARD + let der = STANDARD .decode(&cleaned) - .map_err(|e| ParseError::Base64(e.to_string())) + .map_err(|e| ParseError::Base64(e.to_string()))?; + if der.len() > MAX_DER_ENCODED_KEY_VALUE_LEN { + return Err(ParseError::InvalidStructure( + "DEREncodedKeyValue exceeds maximum allowed length".into(), + )); + } + Ok(der) } fn collect_text_content(node: Node<'_, '_>) -> String { @@ -585,7 +606,7 @@ fn ensure_no_element_children(node: Node<'_, '_>, element_name: &str) -> Result< fn ensure_no_non_whitespace_text(node: Node<'_, '_>, element_name: &str) -> Result<(), ParseError> { for child in node.children().filter(|child| child.is_text()) { if let Some(text) = child.text() - && !text.trim().is_empty() + && !is_xml_whitespace_only(text) { return Err(ParseError::InvalidStructure(format!( "{element_name} must not contain non-whitespace mixed content" @@ -595,10 +616,16 @@ fn ensure_no_non_whitespace_text(node: Node<'_, '_>, element_name: &str) -> Resu Ok(()) } +fn is_xml_whitespace_only(text: &str) -> bool { + text.chars() + .all(|ch| matches!(ch, ' ' | '\t' | '\r' | '\n')) +} + #[cfg(test)] #[expect(clippy::unwrap_used, reason = "tests use trusted XML fixtures")] mod tests { use super::*; + use base64::Engine; // ── SignatureAlgorithm ─────────────────────────────────────────── @@ -852,6 +879,29 @@ mod tests { assert!(matches!(err, ParseError::InvalidStructure(_))); } + #[test] + fn parse_key_info_rejects_nbsp_as_non_xml_whitespace_mixed_content() { + let xml = "\u{00A0}k"; + let doc = Document::parse(xml).unwrap(); + + let err = parse_key_info(doc.root_element()).unwrap_err(); + assert!(matches!(err, ParseError::InvalidStructure(_))); + } + + #[test] + fn parse_key_info_der_encoded_key_value_rejects_oversized_payload() { + let oversized = + base64::engine::general_purpose::STANDARD + .encode(vec![0u8; MAX_DER_ENCODED_KEY_VALUE_LEN + 1]); + let xml = format!( + "{oversized}" + ); + let doc = Document::parse(&xml).unwrap(); + + let err = parse_key_info(doc.root_element()).unwrap_err(); + assert!(matches!(err, ParseError::InvalidStructure(_))); + } + // ── parse_signed_info: happy path ──────────────────────────────── #[test] diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index 4dd5bab..a10b0e3 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -1051,9 +1051,9 @@ fn parse_signature_children<'a, 'input>( if let Some(unexpected_index) = first_unexpected_dsig_index { return Err(SignatureVerificationPipelineError::InvalidStructure { reason: if unexpected_index > allowed_prefix_end { - "Signature may contain only Object elements after the allowed prefix" + "After SignedInfo, SignatureValue, and optional KeyInfo, Signature may contain only Object elements" } else { - "unexpected XMLDSIG child element under Signature" + "Signature may contain SignedInfo first, SignatureValue second, optional KeyInfo third, and Object elements thereafter" }, }); } From eec05eba2afdf2e877fbd99487cfd2975c52971f Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sun, 5 Apr 2026 02:56:13 +0300 Subject: [PATCH 04/21] fix(xmldsig): harden signature child validation --- src/xmldsig/parse.rs | 54 +++++++++++++++------------ src/xmldsig/verify.rs | 85 +++++++++++++++++++++++++++++++++++++++---- 2 files changed, 107 insertions(+), 32 deletions(-) diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index b98b810..8aeabd1 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -380,7 +380,7 @@ pub fn parse_key_info(key_info_node: Node) -> Result { } (Some(XMLDSIG11_NS), "DEREncodedKeyValue") => { ensure_no_element_children(child, "DEREncodedKeyValue")?; - let der = decode_base64_xml_text(&collect_text_content(child))?; + let der = decode_base64_xml_children(child)?; sources.push(KeyInfoSource::DerEncodedKeyValue(der)); } _ => {} @@ -547,33 +547,39 @@ fn base64_decode_digest(b64: &str, digest_method: DigestAlgorithm) -> Result Result, ParseError> { +fn decode_base64_xml_children(node: Node<'_, '_>) -> Result, ParseError> { use base64::Engine; use base64::engine::general_purpose::STANDARD; - let mut cleaned = String::with_capacity(b64.len()); + let mut cleaned = String::new(); let mut raw_text_len = 0usize; - for ch in b64.chars() { - if raw_text_len.saturating_add(ch.len_utf8()) > MAX_DER_ENCODED_KEY_VALUE_TEXT_LEN { - return Err(ParseError::InvalidStructure( - "DEREncodedKeyValue exceeds maximum allowed text length".into(), - )); - } - raw_text_len = raw_text_len.saturating_add(ch.len_utf8()); - if matches!(ch, ' ' | '\t' | '\r' | '\n') { - continue; - } - if ch.is_ascii_whitespace() { - return Err(ParseError::Base64(format!( - "invalid XML whitespace U+{:04X} in base64 text", - u32::from(ch) - ))); - } - cleaned.push(ch); - if cleaned.len() > MAX_DER_ENCODED_KEY_VALUE_BASE64_LEN { - return Err(ParseError::InvalidStructure( - "DEREncodedKeyValue exceeds maximum allowed length".into(), - )); + for text in node + .children() + .filter(|child| child.is_text()) + .filter_map(|child| child.text()) + { + for ch in text.chars() { + if raw_text_len.saturating_add(ch.len_utf8()) > MAX_DER_ENCODED_KEY_VALUE_TEXT_LEN { + return Err(ParseError::InvalidStructure( + "DEREncodedKeyValue exceeds maximum allowed text length".into(), + )); + } + raw_text_len = raw_text_len.saturating_add(ch.len_utf8()); + if matches!(ch, ' ' | '\t' | '\r' | '\n') { + continue; + } + if ch.is_ascii_whitespace() { + return Err(ParseError::Base64(format!( + "invalid XML whitespace U+{:04X} in base64 text", + u32::from(ch) + ))); + } + cleaned.push(ch); + if cleaned.len() > MAX_DER_ENCODED_KEY_VALUE_BASE64_LEN { + return Err(ParseError::InvalidStructure( + "DEREncodedKeyValue exceeds maximum allowed length".into(), + )); + } } } diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index a10b0e3..47df3b2 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -973,15 +973,31 @@ fn parse_signature_children<'a, 'input>( let mut object_indices: Vec = Vec::new(); let mut first_unexpected_dsig_index: Option = None; - for (zero_based_index, child) in signature_node - .children() - .filter(|node| node.is_element()) - .enumerate() - { - let element_index = zero_based_index + 1; - if child.tag_name().namespace() != Some(XMLDSIG_NS) { + let mut element_index = 0usize; + for child in signature_node.children() { + if child.is_text() { + if child + .text() + .is_some_and(|text| !is_xml_whitespace_only(text)) + { + return Err(SignatureVerificationPipelineError::InvalidStructure { + reason: "Signature must not contain non-whitespace mixed content", + }); + } continue; } + if !child.is_element() { + return Err(SignatureVerificationPipelineError::InvalidStructure { + reason: "Signature must not contain non-element children", + }); + } + + element_index += 1; + if child.tag_name().namespace() != Some(XMLDSIG_NS) { + return Err(SignatureVerificationPipelineError::InvalidStructure { + reason: "Signature must contain only XMLDSIG element children", + }); + } match child.tag_name().name() { "SignedInfo" => { if signed_info_node.is_some() { @@ -1104,6 +1120,11 @@ fn decode_signature_value( Ok(base64::engine::general_purpose::STANDARD.decode(normalized)?) } +fn is_xml_whitespace_only(text: &str) -> bool { + text.chars() + .all(|ch| matches!(ch, ' ' | '\t' | '\n' | '\r')) +} + fn push_normalized_signature_text( text: &str, raw_text_len: &mut usize, @@ -2512,7 +2533,14 @@ mod tests { let xml = r#" - + + + + + + AAAAAAAAAAAAAAAAAAAAAAAAAAA= + + AA== %%%invalid%%% @@ -2528,6 +2556,47 @@ mod tests { )); } + #[test] + fn pipeline_rejects_foreign_element_children_under_signature() { + let xml = r#" + + + + AA== + +"#; + + let err = verify_signature_with_pem_key(xml, "dummy-key", false) + .expect_err("foreign element children under Signature must fail closed"); + assert!(matches!( + err, + SignatureVerificationPipelineError::InvalidStructure { + reason: "Signature must contain only XMLDSIG element children", + } + )); + } + + #[test] + fn pipeline_rejects_non_whitespace_mixed_content_under_signature() { + let xml = r#" + + + oops + AA== + +"#; + + let err = verify_signature_with_pem_key(xml, "dummy-key", false) + .expect_err("non-whitespace mixed content under Signature must fail closed"); + assert!(matches!( + err, + SignatureVerificationPipelineError::InvalidStructure { + reason: "Signature must not contain non-whitespace mixed content", + } + )); + } + #[test] fn pipeline_rejects_keyinfo_out_of_order() { let xml = r#" From 5083410efa92f0e57b1dc2bba365fa52d5b889f7 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sun, 5 Apr 2026 03:20:19 +0300 Subject: [PATCH 05/21] fix(xmldsig): tighten keyinfo and signature child parsing --- src/xmldsig/mod.rs | 1 + src/xmldsig/parse.rs | 25 ++++++++++++++++++----- src/xmldsig/verify.rs | 42 +++++++++++++++++++++++++++++++-------- src/xmldsig/whitespace.rs | 5 +++++ 4 files changed, 60 insertions(+), 13 deletions(-) create mode 100644 src/xmldsig/whitespace.rs diff --git a/src/xmldsig/mod.rs b/src/xmldsig/mod.rs index cfe8a3b..3a913e0 100644 --- a/src/xmldsig/mod.rs +++ b/src/xmldsig/mod.rs @@ -15,6 +15,7 @@ pub mod transforms; pub mod types; pub mod uri; pub mod verify; +pub(crate) mod whitespace; pub use digest::{DigestAlgorithm, compute_digest, constant_time_eq}; pub use parse::{ diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index 8aeabd1..8ee5453 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -20,6 +20,7 @@ use roxmltree::{Document, Node}; use super::digest::DigestAlgorithm; use super::transforms::{self, Transform}; +use super::whitespace::is_xml_whitespace_only; use crate::c14n::C14nAlgorithm; /// XMLDSig namespace URI. @@ -586,6 +587,11 @@ fn decode_base64_xml_children(node: Node<'_, '_>) -> Result, ParseError> let der = STANDARD .decode(&cleaned) .map_err(|e| ParseError::Base64(e.to_string()))?; + if der.is_empty() { + return Err(ParseError::InvalidStructure( + "DEREncodedKeyValue must not be empty".into(), + )); + } if der.len() > MAX_DER_ENCODED_KEY_VALUE_LEN { return Err(ParseError::InvalidStructure( "DEREncodedKeyValue exceeds maximum allowed length".into(), @@ -622,11 +628,6 @@ fn ensure_no_non_whitespace_text(node: Node<'_, '_>, element_name: &str) -> Resu Ok(()) } -fn is_xml_whitespace_only(text: &str) -> bool { - text.chars() - .all(|ch| matches!(ch, ' ' | '\t' | '\r' | '\n')) -} - #[cfg(test)] #[expect(clippy::unwrap_used, reason = "tests use trusted XML fixtures")] mod tests { @@ -908,6 +909,20 @@ mod tests { assert!(matches!(err, ParseError::InvalidStructure(_))); } + #[test] + fn parse_key_info_der_encoded_key_value_rejects_empty_payload() { + let xml = r#" + + + + "#; + let doc = Document::parse(xml).unwrap(); + + let err = parse_key_info(doc.root_element()).unwrap_err(); + assert!(matches!(err, ParseError::InvalidStructure(_))); + } + // ── parse_signed_info: happy path ──────────────────────────────── #[test] diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index 47df3b2..ba16833 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -26,6 +26,7 @@ use super::transforms::{ DEFAULT_IMPLICIT_C14N_URI, Transform, XPATH_TRANSFORM_URI, execute_transforms, }; use super::uri::{UriReferenceResolver, parse_xpointer_id_fragment}; +use super::whitespace::is_xml_whitespace_only; const MAX_SIGNATURE_VALUE_LEN: usize = 8192; const MAX_SIGNATURE_VALUE_TEXT_LEN: usize = 65_536; @@ -987,9 +988,7 @@ fn parse_signature_children<'a, 'input>( continue; } if !child.is_element() { - return Err(SignatureVerificationPipelineError::InvalidStructure { - reason: "Signature must not contain non-element children", - }); + continue; } element_index += 1; @@ -1120,11 +1119,6 @@ fn decode_signature_value( Ok(base64::engine::general_purpose::STANDARD.decode(normalized)?) } -fn is_xml_whitespace_only(text: &str) -> bool { - text.chars() - .all(|ch| matches!(ch, ' ' | '\t' | '\n' | '\r')) -} - fn push_normalized_signature_text( text: &str, raw_text_len: &mut usize, @@ -2617,4 +2611,36 @@ mod tests { } )); } + + #[test] + fn pipeline_accepts_comments_and_processing_instructions_under_signature() { + let xml = r#" + + + + + + + + + AAAAAAAAAAAAAAAAAAAAAAAAAAA= + + + + AA== + +"#; + + let doc = Document::parse(xml).expect("test XML must parse"); + let signature_node = doc.root_element(); + let parsed = parse_signature_children(signature_node) + .expect("comment/PI nodes under Signature must be ignored"); + + assert_eq!(parsed.signed_info_node.tag_name().name(), "SignedInfo"); + assert_eq!( + parsed.signature_value_node.tag_name().name(), + "SignatureValue" + ); + assert!(parsed.key_info_node.is_none()); + } } diff --git a/src/xmldsig/whitespace.rs b/src/xmldsig/whitespace.rs new file mode 100644 index 0000000..55c88ac --- /dev/null +++ b/src/xmldsig/whitespace.rs @@ -0,0 +1,5 @@ +#[inline] +pub(crate) fn is_xml_whitespace_only(text: &str) -> bool { + text.chars() + .all(|ch| matches!(ch, ' ' | '\t' | '\r' | '\n')) +} From 1931cbd91329f92c36a4fbe11e8114da009951c5 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sun, 5 Apr 2026 03:57:15 +0300 Subject: [PATCH 06/21] docs(xmldsig): document whitespace helper and signature checks --- src/xmldsig/verify.rs | 4 ++++ src/xmldsig/whitespace.rs | 3 +++ 2 files changed, 7 insertions(+) diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index ba16833..2372c3d 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -568,6 +568,10 @@ type SignatureVerificationPipelineError = DsigError; /// - The document must contain exactly one XMLDSig `` element. /// - `` must be the first element child of `` and appear once. /// - `` must be the second element child of `` and appear once. +/// - `` is optional and, when present, must be the third element child. +/// - Only XMLDSig namespace element children are allowed under ``. +/// - Non-whitespace mixed text content under `` is rejected. +/// - After ``, ``, and optional ``, only `` elements are allowed. /// - `` must not contain nested element children. pub fn verify_signature_with_pem_key( xml: &str, diff --git a/src/xmldsig/whitespace.rs b/src/xmldsig/whitespace.rs index 55c88ac..3ce5311 100644 --- a/src/xmldsig/whitespace.rs +++ b/src/xmldsig/whitespace.rs @@ -1,3 +1,6 @@ +//! Internal XML whitespace helpers shared across XMLDSig parsing and verification. + +/// Return `true` when the text contains only XML 1.0 whitespace chars. #[inline] pub(crate) fn is_xml_whitespace_only(text: &str) -> bool { text.chars() From 9f9917e835c2e3978af150e0bbde140f7d70a9d1 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sun, 5 Apr 2026 10:20:18 +0300 Subject: [PATCH 07/21] docs(xmldsig): clarify deferred keyinfo consumption --- src/xmldsig/verify.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index 2372c3d..b78f082 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -627,6 +627,7 @@ fn verify_signature_with_context( let signature_children = parse_signature_children(signature_node)?; let signed_info_node = signature_children.signed_info_node; if let Some(key_info_node) = signature_children.key_info_node { + // P2-001: validate KeyInfo structure now; key material consumption is deferred. parse_key_info(key_info_node).map_err(SignatureVerificationPipelineError::ParseKeyInfo)?; } From 52dca518218c58926d2cf4c6ca76146dd163b3b6 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sun, 5 Apr 2026 10:39:19 +0300 Subject: [PATCH 08/21] test(xmldsig): isolate signature-child structure regressions --- src/xmldsig/verify.rs | 55 +++++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 26 deletions(-) diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index b78f082..3f63fc5 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -2557,16 +2557,20 @@ mod tests { #[test] fn pipeline_rejects_foreign_element_children_under_signature() { - let xml = r#" - - - - AA== - -"#; + let base_xml = signature_with_target_reference("AQ=="); + let xml = base_xml + .replace( + r#""#, + r#""#, + ) + .replace( + "\n ", + "\n \n ", + ); - let err = verify_signature_with_pem_key(xml, "dummy-key", false) + let err = VerifyContext::new() + .key(&RejectingKey) + .verify(&xml) .expect_err("foreign element children under Signature must fail closed"); assert!(matches!( err, @@ -2578,15 +2582,15 @@ mod tests { #[test] fn pipeline_rejects_non_whitespace_mixed_content_under_signature() { - let xml = r#" - - - oops - AA== - -"#; + let base_xml = signature_with_target_reference("AQ=="); + let xml = base_xml.replace( + "\n ", + "\n oops\n ", + ); - let err = verify_signature_with_pem_key(xml, "dummy-key", false) + let err = VerifyContext::new() + .key(&RejectingKey) + .verify(&xml) .expect_err("non-whitespace mixed content under Signature must fail closed"); assert!(matches!( err, @@ -2598,16 +2602,15 @@ mod tests { #[test] fn pipeline_rejects_keyinfo_out_of_order() { - let xml = r#" - - - AA== - - late - -"#; + let base_xml = signature_with_target_reference("AQ=="); + let xml = base_xml.replace( + "\n ", + "\n \n late\n ", + ); - let err = verify_signature_with_pem_key(xml, "dummy-key", false) + let err = VerifyContext::new() + .key(&RejectingKey) + .verify(&xml) .expect_err("KeyInfo after Object must be rejected by Signature child order checks"); assert!(matches!( err, From 2b199db364c6c6b52db124741cf891c50dbae0e9 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sun, 5 Apr 2026 11:49:07 +0300 Subject: [PATCH 09/21] docs(xmldsig): align verify pipeline rustdoc --- src/xmldsig/verify.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index 3f63fc5..c51c694 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -555,11 +555,13 @@ type SignatureVerificationPipelineError = DsigError; /// Verify one XMLDSig `` end-to-end with a PEM public key. /// /// Pipeline: -/// 1. Parse `` -/// 2. Validate all `` digests (fail-fast) -/// 3. Canonicalize `` -/// 4. Base64-decode `` -/// 5. Verify signature bytes against canonicalized `` +/// 1. Parse `` children and enforce structural constraints +/// 2. Parse and validate optional `` (when present) +/// 3. Parse `` +/// 4. Validate all `` digests (fail-fast) +/// 5. Canonicalize `` +/// 6. Base64-decode `` +/// 7. Verify signature bytes against canonicalized `` /// /// If any `` digest mismatches, returns `Ok` with /// `status == Invalid(ReferenceDigestMismatch { .. })`. From 196d7fae6cc0fa79363bb0a4b7cdae58fd0a84fe Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sun, 5 Apr 2026 13:33:44 +0300 Subject: [PATCH 10/21] fix(xmldsig): bound keyname text length --- src/xmldsig/parse.rs | 36 ++++++++++++++++++++++++++++++++---- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index 8ee5453..b562b0e 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -30,6 +30,7 @@ pub(crate) const XMLDSIG11_NS: &str = "http://www.w3.org/2009/xmldsig11#"; const MAX_DER_ENCODED_KEY_VALUE_LEN: usize = 8192; const MAX_DER_ENCODED_KEY_VALUE_TEXT_LEN: usize = 65_536; const MAX_DER_ENCODED_KEY_VALUE_BASE64_LEN: usize = MAX_DER_ENCODED_KEY_VALUE_LEN.div_ceil(3) * 4; +const MAX_KEY_NAME_TEXT_LEN: usize = 4096; /// Signature algorithms supported for signing and verification. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] @@ -368,7 +369,8 @@ pub fn parse_key_info(key_info_node: Node) -> Result { match (child.tag_name().namespace(), child.tag_name().name()) { (Some(XMLDSIG_NS), "KeyName") => { ensure_no_element_children(child, "KeyName")?; - let key_name = collect_text_content(child); + let key_name = + collect_text_content_bounded(child, MAX_KEY_NAME_TEXT_LEN, "KeyName")?; sources.push(KeyInfoSource::KeyName(key_name)); } (Some(XMLDSIG_NS), "KeyValue") => { @@ -600,10 +602,24 @@ fn decode_base64_xml_children(node: Node<'_, '_>) -> Result, ParseError> Ok(der) } -fn collect_text_content(node: Node<'_, '_>) -> String { - node.children() +fn collect_text_content_bounded( + node: Node<'_, '_>, + max_len: usize, + element_name: &'static str, +) -> Result { + let mut text = String::new(); + for chunk in node + .children() .filter_map(|child| child.is_text().then(|| child.text()).flatten()) - .collect() + { + if text.len().saturating_add(chunk.len()) > max_len { + return Err(ParseError::InvalidStructure(format!( + "{element_name} exceeds maximum allowed text length" + ))); + } + text.push_str(chunk); + } + Ok(text) } fn ensure_no_element_children(node: Node<'_, '_>, element_name: &str) -> Result<(), ParseError> { @@ -877,6 +893,18 @@ mod tests { ); } + #[test] + fn parse_key_info_rejects_oversized_keyname_text() { + let oversized = "A".repeat(4097); + let xml = format!( + "{oversized}" + ); + let doc = Document::parse(&xml).unwrap(); + + let err = parse_key_info(doc.root_element()).unwrap_err(); + assert!(matches!(err, ParseError::InvalidStructure(_))); + } + #[test] fn parse_key_info_rejects_non_whitespace_mixed_content() { let xml = r#"oopsk"#; From bab5b138a1eb9db2fb4f54583e0ed1c4884e39bc Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Wed, 8 Apr 2026 20:51:52 +0300 Subject: [PATCH 11/21] fix(xmldsig): reject unknown ds children in x509data --- src/xmldsig/parse.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index b562b0e..cf8a38f 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -502,6 +502,11 @@ fn parse_x509_data_dispatch(node: Node) -> Result { (Some(XMLDSIG_NS), "X509SKI") => info.ski_count += 1, (Some(XMLDSIG_NS), "X509CRL") => info.crl_count += 1, (Some(XMLDSIG11_NS), "X509Digest") => info.digest_count += 1, + (Some(XMLDSIG_NS), child_name) => { + return Err(ParseError::InvalidStructure(format!( + "X509Data contains unsupported ds child element <{child_name}>" + ))); + } _ => {} } } @@ -821,6 +826,19 @@ mod tests { assert!(matches!(err, ParseError::InvalidStructure(_))); } + #[test] + fn parse_key_info_rejects_unknown_xmlsig_child_in_x509data() { + let xml = r#" + + + + "#; + let doc = Document::parse(xml).unwrap(); + + let err = parse_key_info(doc.root_element()).unwrap_err(); + assert!(matches!(err, ParseError::InvalidStructure(_))); + } + #[test] fn parse_key_info_der_encoded_key_value_rejects_invalid_base64() { let xml = r#"\u{000C}"; + assert!(Document::parse(xml).is_err()); + } + // ── parse_signed_info: happy path ──────────────────────────────── #[test] From 24684d8ca343a75fd1cb08c5fa48a36cf0d9c19f Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Wed, 8 Apr 2026 20:58:55 +0300 Subject: [PATCH 12/21] docs(xmldsig): clarify keyinfo lax vs strict behavior --- src/xmldsig/parse.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index cf8a38f..4f2e302 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -359,7 +359,8 @@ pub(crate) fn parse_reference(reference_node: Node) -> Result` /// - `` /// -/// Unknown elements are ignored (lax processing), matching XMLDSig behavior. +/// Unknown top-level `` children are ignored (lax processing), while +/// unknown XMLDSig (`ds:*`) children inside `` are rejected fail-closed. pub fn parse_key_info(key_info_node: Node) -> Result { verify_ds_element(key_info_node, "KeyInfo")?; ensure_no_non_whitespace_text(key_info_node, "KeyInfo")?; From 314ae17773953c83440f136051a982f93643d0dd Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Wed, 8 Apr 2026 21:28:17 +0300 Subject: [PATCH 13/21] fix(xmldsig): harden x509data namespace validation --- src/xmldsig/parse.rs | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index 4f2e302..593db16 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -360,7 +360,8 @@ pub(crate) fn parse_reference(reference_node: Node) -> Result` /// /// Unknown top-level `` children are ignored (lax processing), while -/// unknown XMLDSig (`ds:*`) children inside `` are rejected fail-closed. +/// unknown XMLDSig-owned (`ds:*` / `dsig11:*`) children inside `` are +/// rejected fail-closed. pub fn parse_key_info(key_info_node: Node) -> Result { verify_ds_element(key_info_node, "KeyInfo")?; ensure_no_non_whitespace_text(key_info_node, "KeyInfo")?; @@ -384,7 +385,7 @@ pub fn parse_key_info(key_info_node: Node) -> Result { } (Some(XMLDSIG11_NS), "DEREncodedKeyValue") => { ensure_no_element_children(child, "DEREncodedKeyValue")?; - let der = decode_base64_xml_children(child)?; + let der = decode_der_encoded_key_value_base64(child)?; sources.push(KeyInfoSource::DerEncodedKeyValue(der)); } _ => {} @@ -503,9 +504,9 @@ fn parse_x509_data_dispatch(node: Node) -> Result { (Some(XMLDSIG_NS), "X509SKI") => info.ski_count += 1, (Some(XMLDSIG_NS), "X509CRL") => info.crl_count += 1, (Some(XMLDSIG11_NS), "X509Digest") => info.digest_count += 1, - (Some(XMLDSIG_NS), child_name) => { + (Some(XMLDSIG_NS), child_name) | (Some(XMLDSIG11_NS), child_name) => { return Err(ParseError::InvalidStructure(format!( - "X509Data contains unsupported ds child element <{child_name}>" + "X509Data contains unsupported XMLDSig child element <{child_name}>" ))); } _ => {} @@ -556,7 +557,7 @@ fn base64_decode_digest(b64: &str, digest_method: DigestAlgorithm) -> Result) -> Result, ParseError> { +fn decode_der_encoded_key_value_base64(node: Node<'_, '_>) -> Result, ParseError> { use base64::Engine; use base64::engine::general_purpose::STANDARD; @@ -840,6 +841,20 @@ mod tests { assert!(matches!(err, ParseError::InvalidStructure(_))); } + #[test] + fn parse_key_info_rejects_unknown_xmlsig11_child_in_x509data() { + let xml = r#" + + + + "#; + let doc = Document::parse(xml).unwrap(); + + let err = parse_key_info(doc.root_element()).unwrap_err(); + assert!(matches!(err, ParseError::InvalidStructure(_))); + } + #[test] fn parse_key_info_der_encoded_key_value_rejects_invalid_base64() { let xml = r#" Date: Wed, 8 Apr 2026 21:44:40 +0300 Subject: [PATCH 14/21] fix(xmldsig): require supported x509data children --- src/xmldsig/parse.rs | 51 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 41 insertions(+), 10 deletions(-) diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index 593db16..1702033 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -494,16 +494,33 @@ fn parse_x509_data_dispatch(node: Node) -> Result { ensure_no_non_whitespace_text(node, "X509Data")?; let mut info = X509DataInfo::default(); - let mut saw_child = false; + let mut saw_supported_xmlsig_child = false; for child in element_children(node) { - saw_child = true; match (child.tag_name().namespace(), child.tag_name().name()) { - (Some(XMLDSIG_NS), "X509Certificate") => info.certificate_count += 1, - (Some(XMLDSIG_NS), "X509SubjectName") => info.subject_name_count += 1, - (Some(XMLDSIG_NS), "X509IssuerSerial") => info.issuer_serial_count += 1, - (Some(XMLDSIG_NS), "X509SKI") => info.ski_count += 1, - (Some(XMLDSIG_NS), "X509CRL") => info.crl_count += 1, - (Some(XMLDSIG11_NS), "X509Digest") => info.digest_count += 1, + (Some(XMLDSIG_NS), "X509Certificate") => { + saw_supported_xmlsig_child = true; + info.certificate_count += 1; + } + (Some(XMLDSIG_NS), "X509SubjectName") => { + saw_supported_xmlsig_child = true; + info.subject_name_count += 1; + } + (Some(XMLDSIG_NS), "X509IssuerSerial") => { + saw_supported_xmlsig_child = true; + info.issuer_serial_count += 1; + } + (Some(XMLDSIG_NS), "X509SKI") => { + saw_supported_xmlsig_child = true; + info.ski_count += 1; + } + (Some(XMLDSIG_NS), "X509CRL") => { + saw_supported_xmlsig_child = true; + info.crl_count += 1; + } + (Some(XMLDSIG11_NS), "X509Digest") => { + saw_supported_xmlsig_child = true; + info.digest_count += 1; + } (Some(XMLDSIG_NS), child_name) | (Some(XMLDSIG11_NS), child_name) => { return Err(ParseError::InvalidStructure(format!( "X509Data contains unsupported XMLDSig child element <{child_name}>" @@ -513,9 +530,9 @@ fn parse_x509_data_dispatch(node: Node) -> Result { } } - if !saw_child { + if !saw_supported_xmlsig_child { return Err(ParseError::InvalidStructure( - "X509Data must contain at least one child element".into(), + "X509Data must contain at least one supported XMLDSig child element".into(), )); } @@ -855,6 +872,20 @@ mod tests { assert!(matches!(err, ParseError::InvalidStructure(_))); } + #[test] + fn parse_key_info_rejects_x509data_with_only_foreign_namespace_children() { + let xml = r#" + + + + "#; + let doc = Document::parse(xml).unwrap(); + + let err = parse_key_info(doc.root_element()).unwrap_err(); + assert!(matches!(err, ParseError::InvalidStructure(_))); + } + #[test] fn parse_key_info_der_encoded_key_value_rejects_invalid_base64() { let xml = r#" Date: Wed, 8 Apr 2026 21:58:05 +0300 Subject: [PATCH 15/21] fix(xmldsig): allow lax empty x509data parsing --- src/xmldsig/parse.rs | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index 1702033..2a45c1a 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -362,6 +362,7 @@ pub(crate) fn parse_reference(reference_node: Node) -> Result` children are ignored (lax processing), while /// unknown XMLDSig-owned (`ds:*` / `dsig11:*`) children inside `` are /// rejected fail-closed. +/// `` may still be empty or contain only non-XMLDSig extension children. pub fn parse_key_info(key_info_node: Node) -> Result { verify_ds_element(key_info_node, "KeyInfo")?; ensure_no_non_whitespace_text(key_info_node, "KeyInfo")?; @@ -494,31 +495,24 @@ fn parse_x509_data_dispatch(node: Node) -> Result { ensure_no_non_whitespace_text(node, "X509Data")?; let mut info = X509DataInfo::default(); - let mut saw_supported_xmlsig_child = false; for child in element_children(node) { match (child.tag_name().namespace(), child.tag_name().name()) { (Some(XMLDSIG_NS), "X509Certificate") => { - saw_supported_xmlsig_child = true; info.certificate_count += 1; } (Some(XMLDSIG_NS), "X509SubjectName") => { - saw_supported_xmlsig_child = true; info.subject_name_count += 1; } (Some(XMLDSIG_NS), "X509IssuerSerial") => { - saw_supported_xmlsig_child = true; info.issuer_serial_count += 1; } (Some(XMLDSIG_NS), "X509SKI") => { - saw_supported_xmlsig_child = true; info.ski_count += 1; } (Some(XMLDSIG_NS), "X509CRL") => { - saw_supported_xmlsig_child = true; info.crl_count += 1; } (Some(XMLDSIG11_NS), "X509Digest") => { - saw_supported_xmlsig_child = true; info.digest_count += 1; } (Some(XMLDSIG_NS), child_name) | (Some(XMLDSIG11_NS), child_name) => { @@ -530,12 +524,6 @@ fn parse_x509_data_dispatch(node: Node) -> Result { } } - if !saw_supported_xmlsig_child { - return Err(ParseError::InvalidStructure( - "X509Data must contain at least one supported XMLDSig child element".into(), - )); - } - Ok(info) } @@ -835,14 +823,17 @@ mod tests { } #[test] - fn parse_key_info_x509data_must_not_be_empty() { + fn parse_key_info_accepts_empty_x509data() { let xml = r#" "#; let doc = Document::parse(xml).unwrap(); - let err = parse_key_info(doc.root_element()).unwrap_err(); - assert!(matches!(err, ParseError::InvalidStructure(_))); + let key_info = parse_key_info(doc.root_element()).unwrap(); + assert_eq!( + key_info.sources, + vec![KeyInfoSource::X509Data(X509DataInfo::default())] + ); } #[test] @@ -873,7 +864,7 @@ mod tests { } #[test] - fn parse_key_info_rejects_x509data_with_only_foreign_namespace_children() { + fn parse_key_info_accepts_x509data_with_only_foreign_namespace_children() { let xml = r#" @@ -882,8 +873,11 @@ mod tests { "#; let doc = Document::parse(xml).unwrap(); - let err = parse_key_info(doc.root_element()).unwrap_err(); - assert!(matches!(err, ParseError::InvalidStructure(_))); + let key_info = parse_key_info(doc.root_element()).unwrap(); + assert_eq!( + key_info.sources, + vec![KeyInfoSource::X509Data(X509DataInfo::default())] + ); } #[test] From 8f869666ccecc0a03e3855dfd58ec2d3b0daae5a Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Wed, 8 Apr 2026 23:02:26 +0300 Subject: [PATCH 16/21] fix(xmldsig): restrict eckeyvalue dispatch to dsig11 --- src/xmldsig/parse.rs | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index 2a45c1a..a989d00 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -483,9 +483,7 @@ fn parse_key_value_dispatch(node: Node) -> Result { first_child.tag_name().name(), ) { (Some(XMLDSIG_NS), "RSAKeyValue") => Ok(KeyValueInfo::RsaKeyValue), - (Some(XMLDSIG_NS), "ECKeyValue") | (Some(XMLDSIG11_NS), "ECKeyValue") => { - Ok(KeyValueInfo::EcKeyValue) - } + (Some(XMLDSIG11_NS), "ECKeyValue") => Ok(KeyValueInfo::EcKeyValue), (_, child_name) => Ok(KeyValueInfo::Unsupported(child_name.to_string())), } } @@ -909,6 +907,24 @@ mod tests { ); } + #[test] + fn parse_key_info_marks_ds_namespace_ec_keyvalue_as_unsupported() { + let xml = r#" + + + + "#; + 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( + "ECKeyValue".into() + ))] + ); + } + #[test] fn parse_key_info_keeps_unsupported_keyvalue_child_as_marker() { let xml = r#" From 766903210864938617125a300d592171e6ff8f8c Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Wed, 8 Apr 2026 23:18:34 +0300 Subject: [PATCH 17/21] refactor(xmldsig): remove unreachable signature child check --- src/xmldsig/verify.rs | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index c51c694..54ebed6 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -978,7 +978,6 @@ fn parse_signature_children<'a, 'input>( let mut signed_info_index: Option = None; let mut signature_value_index: Option = None; let mut key_info_index: Option = None; - let mut object_indices: Vec = Vec::new(); let mut first_unexpected_dsig_index: Option = None; let mut element_index = 0usize; @@ -1033,7 +1032,8 @@ fn parse_signature_children<'a, 'input>( key_info_index = Some(element_index); } "Object" => { - object_indices.push(element_index); + // Valid Object elements are allowed only after SignedInfo, SignatureValue, + // and optional KeyInfo; this is enforced via first_unexpected_dsig_index. } _ => { if first_unexpected_dsig_index.is_none() { @@ -1080,19 +1080,6 @@ fn parse_signature_children<'a, 'input>( }); } - if object_indices - .iter() - .any(|&index| index <= allowed_prefix_end) - { - return Err(SignatureVerificationPipelineError::InvalidStructure { - reason: if allowed_prefix_end == 3 { - "KeyInfo must be the third element child of Signature when present" - } else { - "SignatureValue must be the second element child of Signature" - }, - }); - } - Ok(SignatureChildNodes { signed_info_node, signature_value_node, From f2d66e81824f3e7738c5aad909b6ebc9eedee39f Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Wed, 8 Apr 2026 23:20:09 +0300 Subject: [PATCH 18/21] fix(xmldsig): preserve keyvalue qname and whitespace coverage --- src/xmldsig/parse.rs | 44 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 8 deletions(-) diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index a989d00..b3d774e 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -148,7 +148,12 @@ pub enum KeyValueInfo { /// ``. EcKeyValue, /// Any other `` child not yet supported by this phase. - Unsupported(String), + Unsupported { + /// Namespace URI of the unsupported child, when present. + namespace: Option, + /// Local name of the unsupported child element. + local_name: String, + }, } /// Parsed `` children (dispatch-only in P2-001). @@ -484,7 +489,10 @@ fn parse_key_value_dispatch(node: Node) -> Result { ) { (Some(XMLDSIG_NS), "RSAKeyValue") => Ok(KeyValueInfo::RsaKeyValue), (Some(XMLDSIG11_NS), "ECKeyValue") => Ok(KeyValueInfo::EcKeyValue), - (_, child_name) => Ok(KeyValueInfo::Unsupported(child_name.to_string())), + (namespace, child_name) => Ok(KeyValueInfo::Unsupported { + namespace: namespace.map(str::to_string), + local_name: child_name.to_string(), + }), } } @@ -890,6 +898,24 @@ mod tests { assert!(matches!(err, ParseError::Base64(_))); } + #[test] + fn parse_key_info_der_encoded_key_value_accepts_xml_whitespace() { + let xml = r#" + + AQID + BA== + + "#; + let doc = Document::parse(xml).unwrap(); + + let key_info = parse_key_info(doc.root_element()).unwrap(); + assert_eq!( + key_info.sources, + vec![KeyInfoSource::DerEncodedKeyValue(vec![1, 2, 3, 4])] + ); + } + #[test] fn parse_key_info_dispatches_dsig11_ec_keyvalue() { let xml = r#" Date: Thu, 9 Apr 2026 00:09:20 +0300 Subject: [PATCH 19/21] fix(xmldsig): address KeyInfo review feedback - gate KeyInfo parsing to resolver/no-explicit-key path and add regression coverage - centralize XML base64 normalization in whitespace helper and reuse in parse/verify - clarify public docs that only dsig11:ECKeyValue is supported --- src/xmldsig/parse.rs | 62 +++++++++++++------------------- src/xmldsig/verify.rs | 75 +++++++++++++++++++++++++-------------- src/xmldsig/whitespace.rs | 31 ++++++++++++++++ 3 files changed, 104 insertions(+), 64 deletions(-) diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index b3d774e..0206155 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -20,7 +20,7 @@ use roxmltree::{Document, Node}; use super::digest::DigestAlgorithm; use super::transforms::{self, Transform}; -use super::whitespace::is_xml_whitespace_only; +use super::whitespace::{is_xml_whitespace_only, normalize_xml_base64_text}; use crate::c14n::C14nAlgorithm; /// XMLDSig namespace URI. @@ -145,7 +145,7 @@ pub enum KeyInfoSource { pub enum KeyValueInfo { /// ``. RsaKeyValue, - /// ``. + /// `dsig11:ECKeyValue` (the XMLDSig 1.1 namespace form). EcKeyValue, /// Any other `` child not yet supported by this phase. Unsupported { @@ -360,7 +360,7 @@ pub(crate) fn parse_reference(reference_node: Node) -> Result` -/// - `` (dispatch to RSA vs EC by child element name) +/// - `` (dispatch by child QName; only `dsig11:ECKeyValue` is treated as supported EC) /// - `` /// - `` /// @@ -541,18 +541,12 @@ fn base64_decode_digest(b64: &str, digest_method: DigestAlgorithm) -> Result) -> Result, Pa .filter(|child| child.is_text()) .filter_map(|child| child.text()) { - for ch in text.chars() { - if raw_text_len.saturating_add(ch.len_utf8()) > MAX_DER_ENCODED_KEY_VALUE_TEXT_LEN { - return Err(ParseError::InvalidStructure( - "DEREncodedKeyValue exceeds maximum allowed text length".into(), - )); - } - raw_text_len = raw_text_len.saturating_add(ch.len_utf8()); - if matches!(ch, ' ' | '\t' | '\r' | '\n') { - continue; - } - if ch.is_ascii_whitespace() { - return Err(ParseError::Base64(format!( - "invalid XML whitespace U+{:04X} in base64 text", - u32::from(ch) - ))); - } - cleaned.push(ch); - if cleaned.len() > MAX_DER_ENCODED_KEY_VALUE_BASE64_LEN { - return Err(ParseError::InvalidStructure( - "DEREncodedKeyValue exceeds maximum allowed length".into(), - )); - } + if raw_text_len.saturating_add(text.len()) > MAX_DER_ENCODED_KEY_VALUE_TEXT_LEN { + return Err(ParseError::InvalidStructure( + "DEREncodedKeyValue exceeds maximum allowed text length".into(), + )); + } + raw_text_len = raw_text_len.saturating_add(text.len()); + normalize_xml_base64_text(text, &mut cleaned).map_err(|err| { + ParseError::Base64(format!( + "invalid XML whitespace U+{:04X} in base64 text", + err.invalid_byte + )) + })?; + if cleaned.len() > MAX_DER_ENCODED_KEY_VALUE_BASE64_LEN { + return Err(ParseError::InvalidStructure( + "DEREncodedKeyValue exceeds maximum allowed length".into(), + )); } } diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index 54ebed6..704e1c3 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -26,7 +26,7 @@ use super::transforms::{ DEFAULT_IMPLICIT_C14N_URI, Transform, XPATH_TRANSFORM_URI, execute_transforms, }; use super::uri::{UriReferenceResolver, parse_xpointer_id_fragment}; -use super::whitespace::is_xml_whitespace_only; +use super::whitespace::{is_xml_whitespace_only, normalize_xml_base64_text}; const MAX_SIGNATURE_VALUE_LEN: usize = 8192; const MAX_SIGNATURE_VALUE_TEXT_LEN: usize = 65_536; @@ -628,7 +628,9 @@ fn verify_signature_with_context( let signature_children = parse_signature_children(signature_node)?; let signed_info_node = signature_children.signed_info_node; - if let Some(key_info_node) = signature_children.key_info_node { + if ctx.key.is_none() + && let Some(key_info_node) = signature_children.key_info_node + { // P2-001: validate KeyInfo structure now; key material consumption is deferred. parse_key_info(key_info_node).map_err(SignatureVerificationPipelineError::ParseKeyInfo)?; } @@ -1118,30 +1120,25 @@ fn push_normalized_signature_text( raw_text_len: &mut usize, normalized: &mut String, ) -> Result<(), SignatureVerificationPipelineError> { - for ch in text.chars() { - if raw_text_len.saturating_add(ch.len_utf8()) > MAX_SIGNATURE_VALUE_TEXT_LEN { - return Err(SignatureVerificationPipelineError::InvalidStructure { - reason: "SignatureValue exceeds maximum allowed text length", - }); - } - *raw_text_len = raw_text_len.saturating_add(ch.len_utf8()); - if matches!(ch, ' ' | '\t' | '\r' | '\n') { - continue; - } - if ch.is_ascii_whitespace() { - let invalid_byte = - u8::try_from(u32::from(ch)).expect("ASCII whitespace always fits into u8"); - return Err(SignatureVerificationPipelineError::SignatureValueBase64( - base64::DecodeError::InvalidByte(normalized.len(), invalid_byte), - )); - } - if normalized.len().saturating_add(ch.len_utf8()) > MAX_SIGNATURE_VALUE_LEN { - return Err(SignatureVerificationPipelineError::InvalidStructure { - reason: "SignatureValue exceeds maximum allowed length", - }); - } - normalized.push(ch); + if raw_text_len.saturating_add(text.len()) > MAX_SIGNATURE_VALUE_TEXT_LEN { + return Err(SignatureVerificationPipelineError::InvalidStructure { + reason: "SignatureValue exceeds maximum allowed text length", + }); } + *raw_text_len = raw_text_len.saturating_add(text.len()); + + normalize_xml_base64_text(text, normalized).map_err(|err| { + SignatureVerificationPipelineError::SignatureValueBase64(base64::DecodeError::InvalidByte( + err.normalized_offset, + err.invalid_byte, + )) + })?; + if normalized.len() > MAX_SIGNATURE_VALUE_LEN { + return Err(SignatureVerificationPipelineError::InvalidStructure { + reason: "SignatureValue exceeds maximum allowed length", + }); + } + Ok(()) } @@ -2536,14 +2533,38 @@ mod tests { "#; - let err = verify_signature_with_pem_key(xml, "dummy-key", false) - .expect_err("invalid KeyInfo must map to ParseKeyInfo"); + let err = VerifyContext::new().verify(xml).expect_err( + "invalid KeyInfo must map to ParseKeyInfo when no explicit key is supplied", + ); assert!(matches!( err, SignatureVerificationPipelineError::ParseKeyInfo(_) )); } + #[test] + fn pipeline_ignores_malformed_keyinfo_when_explicit_key_is_supplied() { + let base_xml = signature_with_target_reference("AQ=="); + let xml = base_xml + .replace( + r#""#, + r#""#, + ) + .replace( + "\n ", + "\n %%%invalid%%%\n ", + ); + + let result = VerifyContext::new() + .key(&RejectingKey) + .verify(&xml) + .expect("explicit key path should not fail on malformed KeyInfo"); + assert!(matches!( + result.status, + DsigStatus::Invalid(FailureReason::SignatureMismatch) + )); + } + #[test] fn pipeline_rejects_foreign_element_children_under_signature() { let base_xml = signature_with_target_reference("AQ=="); diff --git a/src/xmldsig/whitespace.rs b/src/xmldsig/whitespace.rs index 3ce5311..6e55fd9 100644 --- a/src/xmldsig/whitespace.rs +++ b/src/xmldsig/whitespace.rs @@ -6,3 +6,34 @@ pub(crate) fn is_xml_whitespace_only(text: &str) -> bool { text.chars() .all(|ch| matches!(ch, ' ' | '\t' | '\r' | '\n')) } + +/// Error returned when non-XML ASCII whitespace appears in base64 text. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) struct XmlBase64NormalizeError { + /// Offending ASCII byte. + pub invalid_byte: u8, + /// Offset in the normalized output where the byte was encountered. + pub normalized_offset: usize, +} + +/// Normalize base64 text by stripping XML whitespace and rejecting other ASCII whitespace. +pub(crate) fn normalize_xml_base64_text( + text: &str, + normalized: &mut String, +) -> Result<(), XmlBase64NormalizeError> { + for ch in text.chars() { + if matches!(ch, ' ' | '\t' | '\r' | '\n') { + continue; + } + if ch.is_ascii_whitespace() { + let invalid_byte = + u8::try_from(u32::from(ch)).expect("ASCII whitespace always fits into u8"); + return Err(XmlBase64NormalizeError { + invalid_byte, + normalized_offset: normalized.len(), + }); + } + normalized.push(ch); + } + Ok(()) +} From 3abd4e4169399db05f850a919e1c2cc763695fc8 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Thu, 9 Apr 2026 00:55:32 +0300 Subject: [PATCH 20/21] fix(xmldsig): gate keyinfo parse by resolver intent - update PEM verification docs to match explicit-key KeyInfo behavior - add KeyResolver capability flag for document KeyInfo consumption - bound DigestValue base64 length before decode and add regressions --- src/xmldsig/parse.rs | 25 ++++++++++- src/xmldsig/verify.rs | 97 +++++++++++++++++++++++++++++++++++++++---- 2 files changed, 111 insertions(+), 11 deletions(-) diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index 0206155..ca5300d 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -540,17 +540,23 @@ fn base64_decode_digest(b64: &str, digest_method: DigestAlgorithm) -> Result max_base64_len { + return Err(ParseError::Base64( + "DigestValue exceeds maximum allowed base64 length".into(), + )); + } let digest = STANDARD .decode(&cleaned) .map_err(|e| ParseError::Base64(e.to_string()))?; - let expected = digest_method.output_len(); let actual = digest.len(); if actual != expected { return Err(ParseError::DigestLengthMismatch { @@ -1471,6 +1477,21 @@ mod tests { assert!(matches!(err, ParseError::Base64(_))); } + #[test] + fn base64_decode_digest_rejects_oversized_base64_before_decode() { + let err = base64_decode_digest("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", DigestAlgorithm::Sha1) + .expect_err("oversized DigestValue base64 must fail before decode"); + match err { + ParseError::Base64(message) => { + assert!( + message.contains("DigestValue exceeds maximum allowed base64 length"), + "unexpected message: {message}" + ); + } + other => panic!("expected ParseError::Base64, got {other:?}"), + } + } + // ── Real-world SAML structure ──────────────────────────────────── #[test] diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index 704e1c3..2b2d534 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -56,6 +56,16 @@ pub trait KeyResolver { /// maps `Ok(None)` to `DsigStatus::Invalid(FailureReason::KeyNotFound)`; /// reserve `Err(...)` for resolver failures. fn resolve<'a>(&'a self, xml: &str) -> Result>, DsigError>; + + /// Return `true` when this resolver consumes document `` material. + /// + /// The verification pipeline uses this to decide whether malformed + /// `` should raise `DsigError::ParseKeyInfo` before resolver + /// execution. Resolvers that ignore document key material can keep the + /// default `false` to avoid fail-closed parsing on advisory ``. + fn consumes_document_key_info(&self) -> bool { + false + } } /// Allowed URI classes for ``. @@ -556,16 +566,20 @@ type SignatureVerificationPipelineError = DsigError; /// /// Pipeline: /// 1. Parse `` children and enforce structural constraints -/// 2. Parse and validate optional `` (when present) -/// 3. Parse `` -/// 4. Validate all `` digests (fail-fast) -/// 5. Canonicalize `` -/// 6. Base64-decode `` -/// 7. Verify signature bytes against canonicalized `` +/// 2. Parse `` +/// 3. Validate all `` digests (fail-fast) +/// 4. Canonicalize `` +/// 5. Base64-decode `` +/// 6. Verify signature bytes against canonicalized `` using the provided PEM key /// /// If any `` digest mismatches, returns `Ok` with /// `status == Invalid(ReferenceDigestMismatch { .. })`. /// +/// This API uses only the provided PEM key and does not parse embedded +/// `` key material for key selection/validation. Consequently, +/// malformed optional `` does not produce `DsigError::ParseKeyInfo` +/// on this API path. +/// /// Structural constraints enforced by this API: /// - The document must contain exactly one XMLDSig `` element. /// - `` must be the first element child of `` and appear once. @@ -628,9 +642,12 @@ fn verify_signature_with_context( let signature_children = parse_signature_children(signature_node)?; let signed_info_node = signature_children.signed_info_node; - if ctx.key.is_none() - && let Some(key_info_node) = signature_children.key_info_node - { + let should_parse_key_info = match (ctx.key, ctx.key_resolver) { + (Some(_), _) => false, + (None, Some(resolver)) => resolver.consumes_document_key_info(), + (None, None) => true, + }; + if should_parse_key_info && let Some(key_info_node) = signature_children.key_info_node { // P2-001: validate KeyInfo structure now; key material consumption is deferred. parse_key_info(key_info_node).map_err(SignatureVerificationPipelineError::ParseKeyInfo)?; } @@ -1256,6 +1273,22 @@ mod tests { } } + struct ConsumingKeyInfoResolver; + + impl KeyResolver for ConsumingKeyInfoResolver { + fn resolve<'a>( + &'a self, + _xml: &str, + ) -> Result>, SignatureVerificationPipelineError> + { + Ok(None) + } + + fn consumes_document_key_info(&self) -> bool { + true + } + } + fn minimal_signature_xml(reference_uri: &str, transforms_xml: &str) -> String { format!( r#" @@ -1931,6 +1964,52 @@ mod tests { )); } + #[test] + fn verify_context_resolver_can_ignore_malformed_keyinfo_by_default() { + let base_xml = signature_with_target_reference("AQ=="); + let xml = base_xml + .replace( + r#""#, + r#""#, + ) + .replace( + "\n ", + "\n %%%invalid%%%\n ", + ); + + let result = VerifyContext::new() + .key_resolver(&MissingKeyResolver) + .verify(&xml) + .expect("resolver path should not hard-fail on advisory malformed KeyInfo by default"); + assert!(matches!( + result.status, + DsigStatus::Invalid(FailureReason::KeyNotFound) + )); + } + + #[test] + fn verify_context_resolver_can_opt_in_to_keyinfo_parse_failures() { + let base_xml = signature_with_target_reference("AQ=="); + let xml = base_xml + .replace( + r#""#, + r#""#, + ) + .replace( + "\n ", + "\n %%%invalid%%%\n ", + ); + + let err = VerifyContext::new() + .key_resolver(&ConsumingKeyInfoResolver) + .verify(&xml) + .expect_err("resolver opted into KeyInfo parsing, malformed KeyInfo must fail"); + assert!(matches!( + err, + SignatureVerificationPipelineError::ParseKeyInfo(_) + )); + } + #[test] fn verify_context_preserves_signaturevalue_decode_errors_when_resolver_misses() { let xml = signature_with_target_reference("@@@"); From 204cf571635b3659d28711341dfcd2e0210364ae Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Thu, 9 Apr 2026 02:10:10 +0300 Subject: [PATCH 21/21] fix(xmldsig): stream digestvalue normalization - decode DigestValue from node children without building an unbounded intermediate string - enforce normalized base64 cap while streaming DigestValue chunks - remove expect-based ASCII conversion in whitespace normalizer --- src/xmldsig/parse.rs | 46 +++++++++++++++++++++++++++------------ src/xmldsig/whitespace.rs | 5 +++-- 2 files changed, 35 insertions(+), 16 deletions(-) diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index ca5300d..368f9dc 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -323,20 +323,7 @@ pub(crate) fn parse_reference(reference_node: Node) -> Result Result, + digest_method: DigestAlgorithm, +) -> Result, ParseError> { + let max_base64_len = digest_method.output_len().div_ceil(3) * 4; + let mut cleaned = String::with_capacity(max_base64_len); + + for child in digest_value_node.children() { + if child.is_element() { + return Err(ParseError::InvalidStructure( + "DigestValue must not contain element children".into(), + )); + } + if let Some(text) = child.text() { + normalize_xml_base64_text(text, &mut cleaned).map_err(|err| { + ParseError::Base64(format!( + "invalid XML whitespace U+{:04X} in DigestValue", + err.invalid_byte + )) + })?; + if cleaned.len() > max_base64_len { + return Err(ParseError::Base64( + "DigestValue exceeds maximum allowed base64 length".into(), + )); + } + } + } + + base64_decode_digest(&cleaned, digest_method) +} + fn decode_der_encoded_key_value_base64(node: Node<'_, '_>) -> Result, ParseError> { use base64::Engine; use base64::engine::general_purpose::STANDARD; diff --git a/src/xmldsig/whitespace.rs b/src/xmldsig/whitespace.rs index 6e55fd9..2e634a0 100644 --- a/src/xmldsig/whitespace.rs +++ b/src/xmldsig/whitespace.rs @@ -26,8 +26,9 @@ pub(crate) fn normalize_xml_base64_text( continue; } if ch.is_ascii_whitespace() { - let invalid_byte = - u8::try_from(u32::from(ch)).expect("ASCII whitespace always fits into u8"); + let mut utf8 = [0_u8; 4]; + let encoded = ch.encode_utf8(&mut utf8); + let invalid_byte = encoded.as_bytes()[0]; return Err(XmlBase64NormalizeError { invalid_byte, normalized_offset: normalized.len(),