From 49aa267654cf679c8b8abb73664a1b782a3dedfc Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 4 Apr 2026 10:43:53 +0300 Subject: [PATCH 01/14] feat(xmldsig): process manifest references - parse and process ds:Manifest references under ds:Object when enabled - preserve non-fatal manifest semantics via manifest_references diagnostics - add regression tests for enabled/disabled and mismatch behavior --- src/xmldsig/parse.rs | 2 +- src/xmldsig/verify.rs | 293 +++++++++++++++++++++++++----------------- 2 files changed, 175 insertions(+), 120 deletions(-) diff --git a/src/xmldsig/parse.rs b/src/xmldsig/parse.rs index 1d6c19e..f4ba716 100644 --- a/src/xmldsig/parse.rs +++ b/src/xmldsig/parse.rs @@ -226,7 +226,7 @@ pub fn parse_signed_info(signed_info_node: Node) -> Result` element. /// /// Structure: `?` → `` → `` -fn parse_reference(reference_node: Node) -> Result { +pub(crate) fn parse_reference(reference_node: Node) -> Result { let uri = reference_node.attribute("URI").map(String::from); let id = reference_node.attribute("Id").map(String::from); let ref_type = reference_node.attribute("Type").map(String::from); diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index 8feaa3a..6cf26e1 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -17,8 +17,8 @@ use std::collections::HashSet; use crate::c14n::canonicalize; use super::digest::{DigestAlgorithm, compute_digest, constant_time_eq}; -use super::parse::parse_signed_info; use super::parse::{Reference, SignatureAlgorithm, XMLDSIG_NS}; +use super::parse::{parse_reference, parse_signed_info}; use super::signature::{ SignatureVerificationError, verify_ecdsa_signature_pem, verify_rsa_signature_pem, }; @@ -29,7 +29,6 @@ use super::uri::UriReferenceResolver; const MAX_SIGNATURE_VALUE_LEN: usize = 8192; const MAX_SIGNATURE_VALUE_TEXT_LEN: usize = 65_536; -const MANIFEST_REFERENCE_TYPE_URI: &str = "http://www.w3.org/2000/09/xmldsig#Manifest"; /// Cryptographic verifier used by [`VerifyContext`]. /// /// This trait intentionally has no `Send + Sync` supertraits so lightweight @@ -160,10 +159,11 @@ impl<'a> VerifyContext<'a> { /// Enable or disable `` processing. /// - /// Note: manifest verification is not implemented yet. When enabled, the - /// verifier fails closed with `ManifestProcessingUnsupported` if a - /// `` is present under `` or if a - /// `` is present. + /// When enabled, references under `` are processed + /// and returned in `VerifyResult::manifest_references`. + /// + /// Manifest failures are reported as per-reference statuses and do not + /// alter the final `VerifyResult::status` (non-fatal semantics). pub fn process_manifests(mut self, enabled: bool) -> Self { self.process_manifests = enabled; self @@ -409,7 +409,8 @@ pub struct VerifyResult { /// On fail-fast, this includes references up to and including /// the first digest mismatch only. pub signed_info_references: Vec, - /// `` reference results. Empty until manifest processing is implemented. + /// `` reference results. + /// Populated only when `VerifyContext::process_manifests(true)` is enabled. pub manifest_references: Vec, /// Canonicalized `` bytes when `store_pre_digest` is enabled /// and verification reaches SignedInfo canonicalization. @@ -471,10 +472,6 @@ pub enum DsigError { /// Rejected transform algorithm URI. algorithm: String, }, - - /// Manifest processing was requested but is not implemented in this phase. - #[error("manifest processing is not implemented yet")] - ManifestProcessingUnsupported, } type SignatureVerificationPipelineError = DsigError; @@ -550,14 +547,7 @@ fn verify_signature_with_context( let signature_children = parse_signature_children(signature_node)?; let signed_info_node = signature_children.signed_info_node; - if ctx.process_manifests && has_manifest_children(signature_node) { - return Err(SignatureVerificationPipelineError::ManifestProcessingUnsupported); - } - let signed_info = parse_signed_info(signed_info_node)?; - if ctx.process_manifests && has_manifest_type_references(&signed_info.references) { - return Err(SignatureVerificationPipelineError::ManifestProcessingUnsupported); - } enforce_reference_policies( &signed_info.references, ctx.allowed_uri_types, @@ -581,6 +571,11 @@ fn verify_signature_with_context( canonicalized_signed_info: None, }); } + let manifest_references = if ctx.process_manifests { + process_manifest_references(signature_node, &resolver, ctx)? + } else { + Vec::new() + }; let signed_info_subtree: HashSet<_> = signed_info_node .descendants() @@ -599,7 +594,7 @@ fn verify_signature_with_context( return Ok(VerifyResult { status: DsigStatus::Invalid(FailureReason::KeyNotFound), signed_info_references: references.results, - manifest_references: Vec::new(), + manifest_references, canonicalized_signed_info: if ctx.store_pre_digest { Some(canonical_signed_info) } else { @@ -621,7 +616,7 @@ fn verify_signature_with_context( DsigStatus::Invalid(FailureReason::SignatureMismatch) }, signed_info_references: references.results, - manifest_references: Vec::new(), + manifest_references, canonicalized_signed_info: if ctx.store_pre_digest { Some(canonical_signed_info) } else { @@ -630,23 +625,70 @@ fn verify_signature_with_context( }) } -fn has_manifest_children(signature_node: Node<'_, '_>) -> bool { - signature_node.children().any(|child| { - child.is_element() - && child.tag_name().namespace() == Some(XMLDSIG_NS) - && child.tag_name().name() == "Object" - && child.descendants().any(|inner| { - inner.is_element() - && inner.tag_name().namespace() == Some(XMLDSIG_NS) - && inner.tag_name().name() == "Manifest" - }) - }) +fn process_manifest_references( + signature_node: Node<'_, '_>, + resolver: &UriReferenceResolver<'_>, + ctx: &VerifyContext<'_>, +) -> Result, SignatureVerificationPipelineError> { + let manifest_references = parse_manifest_references(signature_node)?; + if manifest_references.is_empty() { + return Ok(Vec::new()); + } + enforce_reference_policies( + &manifest_references, + ctx.allowed_uri_types, + ctx.allowed_transform_uris(), + )?; + + let mut results = Vec::with_capacity(manifest_references.len()); + for (index, reference) in manifest_references.iter().enumerate() { + results.push(process_reference( + reference, + resolver, + signature_node, + index, + ctx.store_pre_digest, + )?); + } + Ok(results) } -fn has_manifest_type_references(references: &[Reference]) -> bool { - references - .iter() - .any(|reference| reference.ref_type.as_deref() == Some(MANIFEST_REFERENCE_TYPE_URI)) +fn parse_manifest_references( + signature_node: Node<'_, '_>, +) -> Result, SignatureVerificationPipelineError> { + let mut references = Vec::new(); + for object_node in signature_node.children().filter(|node| { + node.is_element() + && node.tag_name().namespace() == Some(XMLDSIG_NS) + && node.tag_name().name() == "Object" + }) { + for manifest_node in object_node.descendants().filter(|node| { + node.is_element() + && node.tag_name().namespace() == Some(XMLDSIG_NS) + && node.tag_name().name() == "Manifest" + }) { + let manifest_children: Vec<_> = manifest_node + .children() + .filter(|node| node.is_element()) + .collect(); + if manifest_children.is_empty() { + return Err(SignatureVerificationPipelineError::MissingElement { + element: "Reference", + }); + } + for child in manifest_children { + if child.tag_name().namespace() != Some(XMLDSIG_NS) + || child.tag_name().name() != "Reference" + { + return Err(SignatureVerificationPipelineError::InvalidStructure { + reason: "Manifest must contain only ds:Reference element children", + }); + } + references.push(parse_reference(child)?); + } + } + } + Ok(references) } enum ResolvedVerifyingKey<'a> { @@ -1077,118 +1119,131 @@ mod tests { )); } - fn signature_with_manifest_xml() -> String { - r#" - - - - - - AAAAAAAAAAAAAAAAAAAAAAAAAAA= - - - AQ== - - - + fn signature_with_manifest_xml(valid_manifest_digest: bool) -> String { + let xml_template = r##" + payload + + + + + + + + - AAAAAAAAAAAAAAAAAAAAAAAAAAA= + SIGNEDINFO_DIGEST_PLACEHOLDER - - -"# - .to_owned() - } - - fn signature_with_nested_manifest_xml() -> String { - r#" - - - - - - AAAAAAAAAAAAAAAAAAAAAAAAAAA= - - - AQ== - - + + AQ== + - + - AAAAAAAAAAAAAAAAAAAAAAAAAAA= + MANIFEST_DIGEST_PLACEHOLDER - - -"# - .to_owned() - } + + +"##; - fn signature_with_manifest_type_reference_xml() -> String { - r#" - - - - - - AAAAAAAAAAAAAAAAAAAAAAAAAAA= - - - AQ== -"# - .to_owned() + let signed_info_digest_b64 = signature_with_target_reference("AQ==") + .split("") + .nth(1) + .and_then(|chunk| chunk.split("").next()) + .expect("signed reference digest should be present") + .to_owned(); + + let seed_xml = xml_template + .replace("SIGNEDINFO_DIGEST_PLACEHOLDER", &signed_info_digest_b64) + .replace( + "MANIFEST_DIGEST_PLACEHOLDER", + "AAAAAAAAAAAAAAAAAAAAAAAAAAA=", + ); + let doc = Document::parse(&seed_xml).unwrap(); + let signature_node = doc + .descendants() + .find(|node| { + node.is_element() + && node.tag_name().namespace() == Some(XMLDSIG_NS) + && node.tag_name().name() == "Signature" + }) + .unwrap(); + let manifest_reference = parse_manifest_references(signature_node) + .unwrap() + .into_iter() + .next() + .expect("manifest reference should be present"); + let resolver = UriReferenceResolver::new(&doc); + let initial_data = resolver + .dereference(manifest_reference.uri.as_deref().unwrap()) + .unwrap(); + let manifest_pre_digest = crate::xmldsig::execute_transforms( + signature_node, + initial_data, + &manifest_reference.transforms, + ) + .unwrap(); + let computed_manifest_digest_b64 = base64::engine::general_purpose::STANDARD.encode( + compute_digest(manifest_reference.digest_method, &manifest_pre_digest), + ); + let final_manifest_digest_b64 = if valid_manifest_digest { + computed_manifest_digest_b64.as_str() + } else { + "AAAAAAAAAAAAAAAAAAAAAAAAAAA=" + }; + + seed_xml.replace("AAAAAAAAAAAAAAAAAAAAAAAAAAA=", final_manifest_digest_b64) } #[test] - fn verify_context_manifest_policy_toggle_is_enforced() { - let xml = signature_with_manifest_xml(); - let err = VerifyContext::new() - .key(&RejectingKey) - .process_manifests(true) - .verify(&xml) - .expect_err("manifest processing must fail closed while unsupported"); - assert!(matches!( - err, - SignatureVerificationPipelineError::ManifestProcessingUnsupported - )); + fn verify_context_processes_manifest_references_when_enabled() { + let xml = signature_with_manifest_xml(true); - let result = VerifyContext::new() + let result_without_manifests = VerifyContext::new() .key(&RejectingKey) - .process_manifests(false) .verify(&xml) - .expect("manifest processing disabled should preserve prior behavior"); + .expect("manifest processing disabled should still verify SignedInfo"); + assert!( + result_without_manifests.manifest_references.is_empty(), + "manifest results must stay empty when manifest processing is disabled", + ); assert!(matches!( - result.status, - DsigStatus::Invalid(FailureReason::ReferenceDigestMismatch { ref_index: 0 }) + result_without_manifests.status, + DsigStatus::Invalid(FailureReason::SignatureMismatch) )); - } - #[test] - fn verify_context_rejects_nested_manifest_when_processing_enabled() { - let xml = signature_with_nested_manifest_xml(); - let err = VerifyContext::new() + let result_with_manifests = VerifyContext::new() .key(&RejectingKey) .process_manifests(true) .verify(&xml) - .expect_err("nested manifests under must also be rejected"); + .expect("manifest references should be processed when enabled"); + assert_eq!(result_with_manifests.manifest_references.len(), 1); assert!(matches!( - err, - SignatureVerificationPipelineError::ManifestProcessingUnsupported + result_with_manifests.manifest_references[0].status, + DsigStatus::Valid + )); + assert!(matches!( + result_with_manifests.status, + DsigStatus::Invalid(FailureReason::SignatureMismatch) )); } #[test] - fn verify_context_rejects_manifest_type_reference_when_processing_enabled() { - let xml = signature_with_manifest_type_reference_xml(); - let err = VerifyContext::new() + fn verify_context_manifest_digest_mismatch_is_non_fatal() { + let xml = signature_with_manifest_xml(false); + let result = VerifyContext::new() .key(&RejectingKey) .process_manifests(true) .verify(&xml) - .expect_err("manifest-typed references must fail closed while unsupported"); + .expect("manifest digest mismatches should be reported as reference status"); + assert_eq!(result.manifest_references.len(), 1); assert!(matches!( - err, - SignatureVerificationPipelineError::ManifestProcessingUnsupported + result.manifest_references[0].status, + DsigStatus::Invalid(FailureReason::ReferenceDigestMismatch { ref_index: 0 }) + )); + assert!(matches!( + result.status, + DsigStatus::Invalid(FailureReason::SignatureMismatch) )); } From 143cce6c5d84c95f50990195e36ab9eef6c07858 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 4 Apr 2026 13:25:12 +0300 Subject: [PATCH 02/14] fix(xmldsig): clarify manifest reference diagnostics - map manifest reference parse failures to explicit DsigError variant - track reference origin/index for SignedInfo vs Manifest processing - process only direct ds:Manifest children under ds:Object Closes #41 --- src/xmldsig/mod.rs | 2 +- src/xmldsig/verify.rs | 215 ++++++++++++++++++++++++++++++--- tests/reference_integration.rs | 3 +- 3 files changed, 199 insertions(+), 21 deletions(-) diff --git a/src/xmldsig/mod.rs b/src/xmldsig/mod.rs index 21ce853..781b098 100644 --- a/src/xmldsig/mod.rs +++ b/src/xmldsig/mod.rs @@ -31,6 +31,6 @@ pub use transforms::{ pub use types::{NodeSet, TransformData, TransformError}; pub use verify::{ DsigError, DsigStatus, FailureReason, KeyResolver, ReferenceProcessingError, ReferenceResult, - ReferencesResult, UriTypeSet, VerifyContext, VerifyResult, VerifyingKey, + ReferenceSet, ReferencesResult, UriTypeSet, VerifyContext, VerifyResult, VerifyingKey, process_all_references, process_reference, verify_signature_with_pem_key, }; diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index 6cf26e1..bfcbcc8 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -17,7 +17,7 @@ use std::collections::HashSet; use crate::c14n::canonicalize; use super::digest::{DigestAlgorithm, compute_digest, constant_time_eq}; -use super::parse::{Reference, SignatureAlgorithm, XMLDSIG_NS}; +use super::parse::{ParseError, Reference, SignatureAlgorithm, XMLDSIG_NS}; use super::parse::{parse_reference, parse_signed_info}; use super::signature::{ SignatureVerificationError, verify_ecdsa_signature_pem, verify_rsa_signature_pem, @@ -225,6 +225,10 @@ impl Default for VerifyContext<'_> { #[non_exhaustive] #[must_use = "inspect status before accepting the reference result"] pub struct ReferenceResult { + /// Whether this reference came from `` or ``. + pub reference_set: ReferenceSet, + /// Zero-based index within `reference_set`. + pub reference_index: usize, /// URI from the `` element (for diagnostics). pub uri: String, /// Digest algorithm used. @@ -235,6 +239,16 @@ pub struct ReferenceResult { pub pre_digest_data: Option>, } +/// Origin of a processed ``. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[non_exhaustive] +pub enum ReferenceSet { + /// `` under ``. + SignedInfo, + /// `` under `/`. + Manifest, +} + /// Verification status. #[derive(Debug, Clone, Copy, PartialEq, Eq)] #[non_exhaustive] @@ -251,7 +265,9 @@ pub enum DsigStatus { pub enum FailureReason { /// `` mismatch for a `` at `ref_index`. ReferenceDigestMismatch { - /// Zero-based index of the failing `` in ``. + /// Zero-based index of the failing `` in its processed set. + /// + /// Use `ReferenceResult::reference_set` to distinguish SignedInfo vs Manifest. ref_index: usize, }, /// `` does not match canonicalized ``. @@ -302,7 +318,8 @@ pub fn process_reference( reference: &Reference, resolver: &UriReferenceResolver<'_>, signature_node: Node<'_, '_>, - ref_index: usize, + reference_set: ReferenceSet, + reference_index: usize, store_pre_digest: bool, ) -> Result { // 1. Dereference URI. Omitted URI is distinct from URI="" in XMLDSig and @@ -326,10 +343,14 @@ pub fn process_reference( let status = if constant_time_eq(&computed_digest, &reference.digest_value) { DsigStatus::Valid } else { - DsigStatus::Invalid(FailureReason::ReferenceDigestMismatch { ref_index }) + DsigStatus::Invalid(FailureReason::ReferenceDigestMismatch { + ref_index: reference_index, + }) }; Ok(ReferenceResult { + reference_set, + reference_index, uri: uri.to_owned(), digest_algorithm: reference.digest_method, status, @@ -361,7 +382,14 @@ pub fn process_all_references( let mut results = Vec::with_capacity(references.len()); for (i, reference) in references.iter().enumerate() { - let result = process_reference(reference, resolver, signature_node, i, store_pre_digest)?; + let result = process_reference( + reference, + resolver, + signature_node, + ReferenceSet::SignedInfo, + i, + store_pre_digest, + )?; let failed = matches!(result.status, DsigStatus::Invalid(_)); results.push(result); @@ -443,6 +471,10 @@ pub enum DsigError { #[error("failed to parse SignedInfo: {0}")] ParseSignedInfo(#[from] super::parse::ParseError), + /// `//` parsing failed. + #[error("failed to parse Manifest reference: {0}")] + ParseManifestReference(#[source] ParseError), + /// Reference processing failed. #[error("reference processing failed: {0}")] Reference(#[from] ReferenceProcessingError), @@ -646,6 +678,7 @@ fn process_manifest_references( reference, resolver, signature_node, + ReferenceSet::Manifest, index, ctx.store_pre_digest, )?); @@ -662,7 +695,7 @@ fn parse_manifest_references( && node.tag_name().namespace() == Some(XMLDSIG_NS) && node.tag_name().name() == "Object" }) { - for manifest_node in object_node.descendants().filter(|node| { + for manifest_node in object_node.children().filter(|node| { node.is_element() && node.tag_name().namespace() == Some(XMLDSIG_NS) && node.tag_name().name() == "Manifest" @@ -684,7 +717,10 @@ fn parse_manifest_references( reason: "Manifest must contain only ds:Reference element children", }); } - references.push(parse_reference(child)?); + references.push( + parse_reference(child) + .map_err(SignatureVerificationPipelineError::ParseManifestReference)?, + ); } } } @@ -1218,6 +1254,14 @@ mod tests { .verify(&xml) .expect("manifest references should be processed when enabled"); assert_eq!(result_with_manifests.manifest_references.len(), 1); + assert_eq!( + result_with_manifests.manifest_references[0].reference_set, + ReferenceSet::Manifest + ); + assert_eq!( + result_with_manifests.manifest_references[0].reference_index, + 0 + ); assert!(matches!( result_with_manifests.manifest_references[0].status, DsigStatus::Valid @@ -1237,6 +1281,11 @@ mod tests { .verify(&xml) .expect("manifest digest mismatches should be reported as reference status"); assert_eq!(result.manifest_references.len(), 1); + assert_eq!( + result.manifest_references[0].reference_set, + ReferenceSet::Manifest + ); + assert_eq!(result.manifest_references[0].reference_index, 0); assert!(matches!( result.manifest_references[0].status, DsigStatus::Invalid(FailureReason::ReferenceDigestMismatch { ref_index: 0 }) @@ -1247,6 +1296,58 @@ mod tests { )); } + #[test] + fn verify_context_ignores_nested_manifests_in_object() { + let xml = signature_with_manifest_xml(true) + .replace("", "") + .replace("", ""); + + let result = VerifyContext::new() + .key(&RejectingKey) + .process_manifests(true) + .verify(&xml) + .expect("nested Manifest nodes are ignored in strict mode"); + assert!( + result.manifest_references.is_empty(), + "only direct ds:Manifest children of ds:Object must be processed" + ); + } + + #[test] + fn verify_context_reports_manifest_reference_parse_errors_explicitly() { + let xml = signature_with_manifest_xml(true); + let (prefix, object_suffix) = xml + .split_once("") + .expect("fixture should contain ds:Object"); + let open = ""; + let close = ""; + let digest_start = object_suffix + .find(open) + .expect("manifest should contain DigestValue"); + let digest_end = object_suffix[digest_start + open.len()..] + .find(close) + .map(|offset| digest_start + open.len() + offset) + .expect("manifest DigestValue must be closed"); + let broken_object_suffix = format!( + "{}{}!!!{}{}", + &object_suffix[..digest_start], + open, + close, + &object_suffix[digest_end + close.len()..], + ); + let broken_xml = format!("{prefix}{broken_object_suffix}"); + + let err = VerifyContext::new() + .key(&RejectingKey) + .process_manifests(true) + .verify(&broken_xml) + .expect_err("invalid Manifest DigestValue must map to ParseManifestReference"); + assert!(matches!( + err, + SignatureVerificationPipelineError::ParseManifestReference(_) + )); + } + #[test] fn verify_context_rejects_implicit_default_c14n_when_not_allowlisted() { let xml = minimal_signature_xml("", ""); @@ -1411,7 +1512,15 @@ mod tests { // Now build a Reference with the correct digest and verify let reference = make_reference("", transforms, DigestAlgorithm::Sha256, expected_digest); - let result = process_reference(&reference, &resolver, sig_node, 0, false).unwrap(); + let result = process_reference( + &reference, + &resolver, + sig_node, + ReferenceSet::SignedInfo, + 0, + false, + ) + .unwrap(); assert!( matches!(result.status, DsigStatus::Valid), "digest should match" @@ -1439,7 +1548,15 @@ mod tests { let wrong_digest = vec![0u8; 32]; let reference = make_reference("", transforms, DigestAlgorithm::Sha256, wrong_digest); - let result = process_reference(&reference, &resolver, sig_node, 0, false).unwrap(); + let result = process_reference( + &reference, + &resolver, + sig_node, + ReferenceSet::SignedInfo, + 0, + false, + ) + .unwrap(); assert!(matches!( result.status, DsigStatus::Invalid(FailureReason::ReferenceDigestMismatch { ref_index: 0 }) @@ -1467,7 +1584,15 @@ mod tests { DigestAlgorithm::Sha256, vec![0u8; 32], ); - let result = process_reference(&reference, &resolver, sig_node, 7, false).unwrap(); + let result = process_reference( + &reference, + &resolver, + sig_node, + ReferenceSet::SignedInfo, + 7, + false, + ) + .unwrap(); assert!(matches!( result.status, DsigStatus::Invalid(FailureReason::ReferenceDigestMismatch { ref_index: 7 }) @@ -1487,7 +1612,15 @@ mod tests { let digest = compute_digest(DigestAlgorithm::Sha256, &pre_digest); let reference = make_reference("", vec![], DigestAlgorithm::Sha256, digest); - let result = process_reference(&reference, &resolver, doc.root_element(), 0, true).unwrap(); + let result = process_reference( + &reference, + &resolver, + doc.root_element(), + ReferenceSet::SignedInfo, + 0, + true, + ) + .unwrap(); assert!(matches!(result.status, DsigStatus::Valid)); assert!(result.pre_digest_data.is_some()); @@ -1527,7 +1660,15 @@ mod tests { DigestAlgorithm::Sha256, expected_digest, ); - let result = process_reference(&reference, &resolver, sig_node, 0, false).unwrap(); + let result = process_reference( + &reference, + &resolver, + sig_node, + ReferenceSet::SignedInfo, + 0, + false, + ) + .unwrap(); assert!(matches!(result.status, DsigStatus::Valid)); } @@ -1539,7 +1680,14 @@ mod tests { let reference = make_reference("#nonexistent", vec![], DigestAlgorithm::Sha256, vec![0; 32]); - let result = process_reference(&reference, &resolver, doc.root_element(), 0, false); + let result = process_reference( + &reference, + &resolver, + doc.root_element(), + ReferenceSet::SignedInfo, + 0, + false, + ); assert!(result.is_err()); } @@ -1558,7 +1706,14 @@ mod tests { digest_value: vec![0; 32], }; - let result = process_reference(&reference, &resolver, doc.root_element(), 0, false); + let result = process_reference( + &reference, + &resolver, + doc.root_element(), + ReferenceSet::SignedInfo, + 0, + false, + ); assert!(matches!(result, Err(ReferenceProcessingError::MissingUri))); } @@ -1666,8 +1821,15 @@ mod tests { let digest = compute_digest(DigestAlgorithm::Sha1, &pre_digest); let reference = make_reference("", vec![], DigestAlgorithm::Sha1, digest); - let result = - process_reference(&reference, &resolver, doc.root_element(), 0, false).unwrap(); + let result = process_reference( + &reference, + &resolver, + doc.root_element(), + ReferenceSet::SignedInfo, + 0, + false, + ) + .unwrap(); assert!(matches!(result.status, DsigStatus::Valid)); assert_eq!(result.digest_algorithm, DigestAlgorithm::Sha1); } @@ -1684,8 +1846,15 @@ mod tests { let digest = compute_digest(DigestAlgorithm::Sha512, &pre_digest); let reference = make_reference("", vec![], DigestAlgorithm::Sha512, digest); - let result = - process_reference(&reference, &resolver, doc.root_element(), 0, false).unwrap(); + let result = process_reference( + &reference, + &resolver, + doc.root_element(), + ReferenceSet::SignedInfo, + 0, + false, + ) + .unwrap(); assert!(matches!(result.status, DsigStatus::Valid)); assert_eq!(result.digest_algorithm, DigestAlgorithm::Sha512); } @@ -1748,7 +1917,15 @@ mod tests { ); // Verify: should pass - let result = process_reference(&corrected_ref, &resolver, sig_node, 0, true).unwrap(); + let result = process_reference( + &corrected_ref, + &resolver, + sig_node, + ReferenceSet::SignedInfo, + 0, + true, + ) + .unwrap(); assert!( matches!(result.status, DsigStatus::Valid), "SAML reference should verify" diff --git a/tests/reference_integration.rs b/tests/reference_integration.rs index 9405aa1..1c7559b 100644 --- a/tests/reference_integration.rs +++ b/tests/reference_integration.rs @@ -16,7 +16,7 @@ use xml_sec::xmldsig::parse::{find_signature_node, parse_signed_info}; use xml_sec::xmldsig::transforms::execute_transforms; use xml_sec::xmldsig::uri::UriReferenceResolver; use xml_sec::xmldsig::verify::{ - DsigStatus, FailureReason, process_all_references, process_reference, + DsigStatus, FailureReason, ReferenceSet, process_all_references, process_reference, }; // ── Helper ─────────────────────────────────────────────────────────────────── @@ -620,6 +620,7 @@ fn process_single_reference_with_pre_digest_valid() { &signed_info.references[0], &resolver, sig_node, + ReferenceSet::SignedInfo, 0, true, // store pre-digest ) From a6f41109d1b6c28ee8d77b797d43bb986a30c787 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 4 Apr 2026 14:22:27 +0300 Subject: [PATCH 03/14] fix(xmldsig): make manifest processing non-fatal - preserve manifest diagnostics even when SignedInfo reference fails - record manifest policy/processing failures as invalid reference statuses - add regression tests for optional manifest failure handling --- src/xmldsig/verify.rs | 130 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 116 insertions(+), 14 deletions(-) diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index bfcbcc8..a04d187 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -270,6 +270,16 @@ pub enum FailureReason { /// Use `ReferenceResult::reference_set` to distinguish SignedInfo vs Manifest. ref_index: usize, }, + /// `` rejected by URI/transform allowlist policy. + ReferencePolicyViolation { + /// Zero-based index of the failing `` in its processed set. + ref_index: usize, + }, + /// `` processing failed (dereference, transform, missing URI). + ReferenceProcessingFailure { + /// Zero-based index of the failing `` in its processed set. + ref_index: usize, + }, /// `` does not match canonicalized ``. SignatureMismatch, /// No verification key was configured or could be resolved. @@ -594,20 +604,21 @@ fn verify_signature_with_context( ctx.store_pre_digest, )?; + let manifest_references = if ctx.process_manifests { + process_manifest_references(signature_node, &resolver, ctx)? + } else { + Vec::new() + }; + if let Some(first_failure) = references.first_failure { let status = references.results[first_failure].status; return Ok(VerifyResult { status, signed_info_references: references.results, - manifest_references: Vec::new(), + manifest_references, canonicalized_signed_info: None, }); } - let manifest_references = if ctx.process_manifests { - process_manifest_references(signature_node, &resolver, ctx)? - } else { - Vec::new() - }; let signed_info_subtree: HashSet<_> = signed_info_node .descendants() @@ -666,26 +677,57 @@ fn process_manifest_references( if manifest_references.is_empty() { return Ok(Vec::new()); } - enforce_reference_policies( - &manifest_references, - ctx.allowed_uri_types, - ctx.allowed_transform_uris(), - )?; - let mut results = Vec::with_capacity(manifest_references.len()); for (index, reference) in manifest_references.iter().enumerate() { - results.push(process_reference( + if enforce_reference_policies( + std::slice::from_ref(reference), + ctx.allowed_uri_types, + ctx.allowed_transform_uris(), + ) + .is_err() + { + results.push(manifest_reference_invalid_result( + reference, + index, + FailureReason::ReferencePolicyViolation { ref_index: index }, + )); + continue; + } + + match process_reference( reference, resolver, signature_node, ReferenceSet::Manifest, index, ctx.store_pre_digest, - )?); + ) { + Ok(result) => results.push(result), + Err(_) => results.push(manifest_reference_invalid_result( + reference, + index, + FailureReason::ReferenceProcessingFailure { ref_index: index }, + )), + } } Ok(results) } +fn manifest_reference_invalid_result( + reference: &Reference, + index: usize, + reason: FailureReason, +) -> ReferenceResult { + ReferenceResult { + reference_set: ReferenceSet::Manifest, + reference_index: index, + uri: reference.uri.clone().unwrap_or_default(), + digest_algorithm: reference.digest_method, + status: DsigStatus::Invalid(reason), + pre_digest_data: None, + } +} + fn parse_manifest_references( signature_node: Node<'_, '_>, ) -> Result, SignatureVerificationPipelineError> { @@ -1296,6 +1338,66 @@ mod tests { )); } + #[test] + fn verify_context_keeps_manifest_results_when_signedinfo_reference_fails() { + let xml = signature_with_manifest_xml(true); + let (signed_info_prefix, object_suffix) = xml + .split_once("") + .expect("fixture should contain ds:Object"); + let open = ""; + let close = ""; + let digest_start = signed_info_prefix + .find(open) + .expect("SignedInfo should contain DigestValue"); + let digest_end = signed_info_prefix[digest_start + open.len()..] + .find(close) + .map(|offset| digest_start + open.len() + offset) + .expect("SignedInfo DigestValue must be closed"); + let broken_signed_info_prefix = format!( + "{}{}AAAAAAAAAAAAAAAAAAAAAAAAAAA={}{}", + &signed_info_prefix[..digest_start], + open, + close, + &signed_info_prefix[digest_end + close.len()..], + ); + let broken_xml = format!("{broken_signed_info_prefix}{object_suffix}"); + let result = VerifyContext::new() + .key(&RejectingKey) + .process_manifests(true) + .verify(&broken_xml) + .expect("manifest references should still be processed on SignedInfo digest failure"); + assert!(matches!( + result.status, + DsigStatus::Invalid(FailureReason::ReferenceDigestMismatch { ref_index: 0 }) + )); + assert_eq!( + result.manifest_references.len(), + 1, + "manifest diagnostics must be preserved even when SignedInfo fails early", + ); + } + + #[test] + fn verify_context_records_manifest_policy_violations_without_aborting() { + let xml = signature_with_manifest_xml(true); + let (prefix, object_suffix) = xml + .split_once("") + .expect("fixture should contain ds:Object"); + let mutated_object_suffix = + object_suffix.replacen("URI=\"#target\"", "URI=\"http://example.com/external\"", 1); + let broken_xml = format!("{prefix}{mutated_object_suffix}"); + let result = VerifyContext::new() + .key(&RejectingKey) + .process_manifests(true) + .verify(&broken_xml) + .expect("manifest policy violations should be recorded, not abort verify()"); + assert_eq!(result.manifest_references.len(), 1); + assert!(matches!( + result.manifest_references[0].status, + DsigStatus::Invalid(FailureReason::ReferencePolicyViolation { ref_index: 0 }) + )); + } + #[test] fn verify_context_ignores_nested_manifests_in_object() { let xml = signature_with_manifest_xml(true) From 7028e5e2df7efa8b24792daec58915df7501e8eb Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 4 Apr 2026 14:23:39 +0300 Subject: [PATCH 04/14] docs(xmldsig): update process_reference rustdoc - document ReferenceSet and reference_index semantics - remove stale ref_index wording tied to SignedInfo only --- src/xmldsig/verify.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index a04d187..427d9d5 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -316,7 +316,8 @@ impl ReferencesResult { /// - `reference`: The parsed `` element. /// - `resolver`: URI resolver for the document. /// - `signature_node`: The `` element (for enveloped-signature transform). -/// - `ref_index`: Zero-based index of this reference in ``. +/// - `reference_set`: Whether this reference belongs to `` or ``. +/// - `reference_index`: Zero-based index of this reference inside `reference_set`. /// - `store_pre_digest`: If true, store the pre-digest bytes in the result. /// /// # Errors From 70f76f4aaebf624e20353dfec60a22b2c5b8703f Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 4 Apr 2026 14:37:56 +0300 Subject: [PATCH 05/14] docs(xmldsig): clarify manifest error semantics - document that structural/parse manifest errors are fatal in verify() - add debug logs for non-fatal manifest policy and processing failures - keep per-reference invalid statuses unchanged for API compatibility --- Cargo.toml | 1 + src/xmldsig/verify.rs | 33 ++++++++++++++++++++++++++------- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 741bd34..9e4c4a2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -35,6 +35,7 @@ base64 = "0.22" # Error handling thiserror = "2" +tracing = "0.1" [features] default = ["xmldsig", "c14n"] diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index 427d9d5..954a22d 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -162,8 +162,10 @@ impl<'a> VerifyContext<'a> { /// When enabled, references under `` are processed /// and returned in `VerifyResult::manifest_references`. /// - /// Manifest failures are reported as per-reference statuses and do not - /// alter the final `VerifyResult::status` (non-fatal semantics). + /// Manifest reference policy violations and processing failures are + /// reported as per-reference statuses and do not alter the final + /// `VerifyResult::status` (non-fatal semantics). Structural/parse errors + /// in Manifest content abort `verify()` and are returned as `Err(...)`. pub fn process_manifests(mut self, enabled: bool) -> Self { self.process_manifests = enabled; self @@ -687,6 +689,13 @@ fn process_manifest_references( ) .is_err() { + tracing::debug!( + reference_set = "manifest", + reference_index = index, + failure_reason = ?FailureReason::ReferencePolicyViolation { ref_index: index }, + uri = ?reference.uri, + "manifest reference policy rejected" + ); results.push(manifest_reference_invalid_result( reference, index, @@ -704,11 +713,21 @@ fn process_manifest_references( ctx.store_pre_digest, ) { Ok(result) => results.push(result), - Err(_) => results.push(manifest_reference_invalid_result( - reference, - index, - FailureReason::ReferenceProcessingFailure { ref_index: index }, - )), + Err(err) => { + tracing::debug!( + reference_set = "manifest", + reference_index = index, + failure_reason = ?FailureReason::ReferenceProcessingFailure { ref_index: index }, + uri = ?reference.uri, + error = %err, + "manifest reference processing failed" + ); + results.push(manifest_reference_invalid_result( + reference, + index, + FailureReason::ReferenceProcessingFailure { ref_index: index }, + )) + } } } Ok(results) From 3dc32a5c2cde718db54f247ca78b73aca029e69a Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 4 Apr 2026 14:44:43 +0300 Subject: [PATCH 06/14] fix(xmldsig): classify manifest precheck failures accurately - map missing URI/non-policy precheck errors to ReferenceProcessingFailure - keep DisallowedUri/DisallowedTransform mapped to ReferencePolicyViolation - preserve omitted URI diagnostics as and add regression coverage --- src/xmldsig/verify.rs | 86 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 68 insertions(+), 18 deletions(-) diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index 954a22d..89cd4b9 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -682,26 +682,47 @@ fn process_manifest_references( } let mut results = Vec::with_capacity(manifest_references.len()); for (index, reference) in manifest_references.iter().enumerate() { - if enforce_reference_policies( + match enforce_reference_policies( std::slice::from_ref(reference), ctx.allowed_uri_types, ctx.allowed_transform_uris(), - ) - .is_err() - { - tracing::debug!( - reference_set = "manifest", - reference_index = index, - failure_reason = ?FailureReason::ReferencePolicyViolation { ref_index: index }, - uri = ?reference.uri, - "manifest reference policy rejected" - ); - results.push(manifest_reference_invalid_result( - reference, - index, - FailureReason::ReferencePolicyViolation { ref_index: index }, - )); - continue; + ) { + Ok(()) => {} + Err( + err @ (SignatureVerificationPipelineError::DisallowedUri { .. } + | SignatureVerificationPipelineError::DisallowedTransform { .. }), + ) => { + tracing::debug!( + reference_set = "manifest", + reference_index = index, + failure_reason = ?FailureReason::ReferencePolicyViolation { ref_index: index }, + uri = ?reference.uri, + error = %err, + "manifest reference policy rejected" + ); + results.push(manifest_reference_invalid_result( + reference, + index, + FailureReason::ReferencePolicyViolation { ref_index: index }, + )); + continue; + } + Err(err) => { + tracing::debug!( + reference_set = "manifest", + reference_index = index, + failure_reason = ?FailureReason::ReferenceProcessingFailure { ref_index: index }, + uri = ?reference.uri, + error = %err, + "manifest reference policy precheck failed as processing error" + ); + results.push(manifest_reference_invalid_result( + reference, + index, + FailureReason::ReferenceProcessingFailure { ref_index: index }, + )); + continue; + } } match process_reference( @@ -741,7 +762,10 @@ fn manifest_reference_invalid_result( ReferenceResult { reference_set: ReferenceSet::Manifest, reference_index: index, - uri: reference.uri.clone().unwrap_or_default(), + uri: reference + .uri + .clone() + .unwrap_or_else(|| "".to_owned()), digest_algorithm: reference.digest_method, status: DsigStatus::Invalid(reason), pre_digest_data: None, @@ -1418,6 +1442,32 @@ mod tests { )); } + #[test] + fn verify_context_records_manifest_missing_uri_as_processing_failure() { + let xml = signature_with_manifest_xml(true); + let (prefix, object_suffix) = xml + .split_once("") + .expect("fixture should contain ds:Object"); + let mutated_object_suffix = object_suffix.replacen( + "", + "", + 1, + ); + let broken_xml = format!("{prefix}{mutated_object_suffix}"); + + let result = VerifyContext::new() + .key(&RejectingKey) + .process_manifests(true) + .verify(&broken_xml) + .expect("manifest missing URI should be recorded as non-fatal processing failure"); + assert_eq!(result.manifest_references.len(), 1); + assert_eq!(result.manifest_references[0].uri, ""); + assert!(matches!( + result.manifest_references[0].status, + DsigStatus::Invalid(FailureReason::ReferenceProcessingFailure { ref_index: 0 }) + )); + } + #[test] fn verify_context_ignores_nested_manifests_in_object() { let xml = signature_with_manifest_xml(true) From 9c3ffa530c90dfe0823c52b781450752af12383e Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 4 Apr 2026 16:21:42 +0300 Subject: [PATCH 07/14] test(xmldsig): decouple manifest fixture from parser - compute manifest digest from known fixture constants (#target, no transforms, SHA-1) - apply rustfmt formatting on manifest test helper blocks --- src/xmldsig/verify.rs | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index 89cd4b9..48ee1e7 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -1290,24 +1290,12 @@ mod tests { && node.tag_name().name() == "Signature" }) .unwrap(); - let manifest_reference = parse_manifest_references(signature_node) - .unwrap() - .into_iter() - .next() - .expect("manifest reference should be present"); let resolver = UriReferenceResolver::new(&doc); - let initial_data = resolver - .dereference(manifest_reference.uri.as_deref().unwrap()) - .unwrap(); - let manifest_pre_digest = crate::xmldsig::execute_transforms( - signature_node, - initial_data, - &manifest_reference.transforms, - ) - .unwrap(); - let computed_manifest_digest_b64 = base64::engine::general_purpose::STANDARD.encode( - compute_digest(manifest_reference.digest_method, &manifest_pre_digest), - ); + let initial_data = resolver.dereference("#target").unwrap(); + let manifest_pre_digest = + crate::xmldsig::execute_transforms(signature_node, initial_data, &[]).unwrap(); + let computed_manifest_digest_b64 = base64::engine::general_purpose::STANDARD + .encode(compute_digest(DigestAlgorithm::Sha1, &manifest_pre_digest)); let final_manifest_digest_b64 = if valid_manifest_digest { computed_manifest_digest_b64.as_str() } else { @@ -1448,11 +1436,8 @@ mod tests { let (prefix, object_suffix) = xml .split_once("") .expect("fixture should contain ds:Object"); - let mutated_object_suffix = object_suffix.replacen( - "", - "", - 1, - ); + let mutated_object_suffix = + object_suffix.replacen("", "", 1); let broken_xml = format!("{prefix}{mutated_object_suffix}"); let result = VerifyContext::new() From 906f5bfd761bea031bd2874057ce43cd8ee0f342 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 4 Apr 2026 16:41:51 +0300 Subject: [PATCH 08/14] test(xmldsig): tighten manifest non-fatal contracts - cover malformed Manifest while process_manifests is disabled - assert top-level SignatureMismatch for manifest policy/processing failures --- src/xmldsig/verify.rs | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index 48ee1e7..42443cd 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -1322,6 +1322,29 @@ mod tests { DsigStatus::Invalid(FailureReason::SignatureMismatch) )); + let xml_with_manifest = signature_with_manifest_xml(true); + let (prefix, object_suffix) = xml_with_manifest + .split_once("") + .expect("fixture should contain ds:Object"); + let malformed_object_suffix = object_suffix + .replacen("", "", 1) + .replacen("", "", 1); + let malformed_manifest_xml = format!("{prefix}{malformed_object_suffix}"); + let malformed_with_manifests_disabled = VerifyContext::new() + .key(&RejectingKey) + .verify(&malformed_manifest_xml) + .expect("malformed Manifest must be ignored when manifest processing is disabled"); + assert!( + malformed_with_manifests_disabled + .manifest_references + .is_empty(), + "manifest parser must not run when process_manifests is disabled", + ); + assert!(matches!( + malformed_with_manifests_disabled.status, + DsigStatus::Invalid(FailureReason::SignatureMismatch) + )); + let result_with_manifests = VerifyContext::new() .key(&RejectingKey) .process_manifests(true) @@ -1428,6 +1451,10 @@ mod tests { result.manifest_references[0].status, DsigStatus::Invalid(FailureReason::ReferencePolicyViolation { ref_index: 0 }) )); + assert!(matches!( + result.status, + DsigStatus::Invalid(FailureReason::SignatureMismatch) + )); } #[test] @@ -1451,6 +1478,10 @@ mod tests { result.manifest_references[0].status, DsigStatus::Invalid(FailureReason::ReferenceProcessingFailure { ref_index: 0 }) )); + assert!(matches!( + result.status, + DsigStatus::Invalid(FailureReason::SignatureMismatch) + )); } #[test] From ddccab5e099c97d17607da598f721c5ef5b366d2 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 4 Apr 2026 16:58:20 +0300 Subject: [PATCH 09/14] fix(xmldsig): harden manifest child validation - reject non-whitespace mixed text in ds:Manifest - return manifest-specific InvalidStructure for empty manifest children - explicitly handle MissingUri in manifest policy precheck - add regression tests for mixed content and empty manifest Closes #41 --- src/xmldsig/verify.rs | 84 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 78 insertions(+), 6 deletions(-) diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index 42443cd..96095c0 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -707,7 +707,26 @@ fn process_manifest_references( )); continue; } + Err(SignatureVerificationPipelineError::Reference( + ReferenceProcessingError::MissingUri, + )) => { + tracing::debug!( + reference_set = "manifest", + reference_index = index, + failure_reason = ?FailureReason::ReferenceProcessingFailure { ref_index: index }, + uri = ?reference.uri, + "manifest reference missing URI during policy precheck" + ); + results.push(manifest_reference_invalid_result( + reference, + index, + FailureReason::ReferenceProcessingFailure { ref_index: index }, + )); + continue; + } Err(err) => { + // Defensive fallback for future enforce_reference_policies variants: + // record as non-fatal per-reference processing failure instead of aborting. tracing::debug!( reference_set = "manifest", reference_index = index, @@ -786,13 +805,24 @@ fn parse_manifest_references( && node.tag_name().namespace() == Some(XMLDSIG_NS) && node.tag_name().name() == "Manifest" }) { - let manifest_children: Vec<_> = manifest_node - .children() - .filter(|node| node.is_element()) - .collect(); + let mut manifest_children = Vec::new(); + for child in manifest_node.children() { + if child.is_text() + && child.text().is_some_and(|text| { + text.chars().any(|c| !matches!(c, ' ' | '\t' | '\n' | '\r')) + }) + { + return Err(SignatureVerificationPipelineError::InvalidStructure { + reason: "Manifest contains non-whitespace mixed content", + }); + } + if child.is_element() { + manifest_children.push(child); + } + } if manifest_children.is_empty() { - return Err(SignatureVerificationPipelineError::MissingElement { - element: "Reference", + return Err(SignatureVerificationPipelineError::InvalidStructure { + reason: "Manifest must contain at least one ds:Reference element child", }); } for child in manifest_children { @@ -1536,6 +1566,48 @@ mod tests { )); } + #[test] + fn verify_context_rejects_manifest_non_whitespace_mixed_content() { + let xml = + signature_with_manifest_xml(true).replacen("", "junk", 1); + + let err = VerifyContext::new() + .key(&RejectingKey) + .process_manifests(true) + .verify(&xml) + .expect_err("Manifest mixed content must fail verification"); + assert!(matches!( + err, + SignatureVerificationPipelineError::InvalidStructure { + reason: "Manifest contains non-whitespace mixed content" + } + )); + } + + #[test] + fn verify_context_rejects_empty_manifest_children() { + let xml = signature_with_manifest_xml(true); + let (prefix, rest) = xml + .split_once("") + .expect("fixture should contain Manifest"); + let (_, suffix) = rest + .split_once("") + .expect("fixture should contain closing Manifest"); + let xml = format!("{prefix}{suffix}"); + + let err = VerifyContext::new() + .key(&RejectingKey) + .process_manifests(true) + .verify(&xml) + .expect_err("empty Manifest must fail verification"); + assert!(matches!( + err, + SignatureVerificationPipelineError::InvalidStructure { + reason: "Manifest must contain at least one ds:Reference element child" + } + )); + } + #[test] fn verify_context_rejects_implicit_default_c14n_when_not_allowlisted() { let xml = minimal_signature_xml("", ""); From 2f874139266b99632d8211a67950bcd9e29a0064 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 4 Apr 2026 18:06:23 +0300 Subject: [PATCH 10/14] fix(xmldsig): refine manifest verdict contract - document non-fatal manifest acceptance semantics in process_manifests docs - add accepting-key regression tests for digest, policy, and missing-uri manifest failures - remove tracing dependency and manifest debug logging to keep dependency surface lean --- Cargo.toml | 1 - src/xmldsig/verify.rs | 136 ++++++++++++++++++++++++++++-------------- 2 files changed, 91 insertions(+), 46 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 9e4c4a2..741bd34 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -35,7 +35,6 @@ base64 = "0.22" # Error handling thiserror = "2" -tracing = "0.1" [features] default = ["xmldsig", "c14n"] diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index 96095c0..d745c83 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -162,10 +162,14 @@ impl<'a> VerifyContext<'a> { /// When enabled, references under `` are processed /// and returned in `VerifyResult::manifest_references`. /// - /// Manifest reference policy violations and processing failures are - /// reported as per-reference statuses and do not alter the final - /// `VerifyResult::status` (non-fatal semantics). Structural/parse errors - /// in Manifest content abort `verify()` and are returned as `Err(...)`. + /// Manifest reference digest mismatches, policy violations, and processing + /// failures are reported in `VerifyResult::manifest_references` and do not + /// alter the final `VerifyResult::status`. + /// Callers that enable `process_manifests(true)` must inspect + /// `VerifyResult::manifest_references` in addition to `VerifyResult::status` + /// when interpreting `verify()` results. + /// Structural/parse errors in Manifest content abort `verify()` and are + /// returned as `Err(...)`. pub fn process_manifests(mut self, enabled: bool) -> Self { self.process_manifests = enabled; self @@ -689,17 +693,9 @@ fn process_manifest_references( ) { Ok(()) => {} Err( - err @ (SignatureVerificationPipelineError::DisallowedUri { .. } - | SignatureVerificationPipelineError::DisallowedTransform { .. }), + SignatureVerificationPipelineError::DisallowedUri { .. } + | SignatureVerificationPipelineError::DisallowedTransform { .. }, ) => { - tracing::debug!( - reference_set = "manifest", - reference_index = index, - failure_reason = ?FailureReason::ReferencePolicyViolation { ref_index: index }, - uri = ?reference.uri, - error = %err, - "manifest reference policy rejected" - ); results.push(manifest_reference_invalid_result( reference, index, @@ -710,13 +706,6 @@ fn process_manifest_references( Err(SignatureVerificationPipelineError::Reference( ReferenceProcessingError::MissingUri, )) => { - tracing::debug!( - reference_set = "manifest", - reference_index = index, - failure_reason = ?FailureReason::ReferenceProcessingFailure { ref_index: index }, - uri = ?reference.uri, - "manifest reference missing URI during policy precheck" - ); results.push(manifest_reference_invalid_result( reference, index, @@ -724,17 +713,9 @@ fn process_manifest_references( )); continue; } - Err(err) => { + Err(_) => { // Defensive fallback for future enforce_reference_policies variants: // record as non-fatal per-reference processing failure instead of aborting. - tracing::debug!( - reference_set = "manifest", - reference_index = index, - failure_reason = ?FailureReason::ReferenceProcessingFailure { ref_index: index }, - uri = ?reference.uri, - error = %err, - "manifest reference policy precheck failed as processing error" - ); results.push(manifest_reference_invalid_result( reference, index, @@ -753,21 +734,11 @@ fn process_manifest_references( ctx.store_pre_digest, ) { Ok(result) => results.push(result), - Err(err) => { - tracing::debug!( - reference_set = "manifest", - reference_index = index, - failure_reason = ?FailureReason::ReferenceProcessingFailure { ref_index: index }, - uri = ?reference.uri, - error = %err, - "manifest reference processing failed" - ); - results.push(manifest_reference_invalid_result( - reference, - index, - FailureReason::ReferenceProcessingFailure { ref_index: index }, - )) - } + Err(_) => results.push(manifest_reference_invalid_result( + reference, + index, + FailureReason::ReferenceProcessingFailure { ref_index: index }, + )), } } Ok(results) @@ -1125,6 +1096,19 @@ mod tests { } } + struct AcceptingKey; + + impl VerifyingKey for AcceptingKey { + fn verify( + &self, + _algorithm: SignatureAlgorithm, + _signed_data: &[u8], + _signature_value: &[u8], + ) -> Result { + Ok(true) + } + } + struct PanicResolver; impl KeyResolver for PanicResolver { @@ -1423,6 +1407,22 @@ mod tests { )); } + #[test] + fn verify_context_manifest_digest_mismatch_is_non_fatal_with_accepting_key() { + let xml = signature_with_manifest_xml(false); + let result = VerifyContext::new() + .key(&AcceptingKey) + .process_manifests(true) + .verify(&xml) + .expect("manifest digest mismatches should be recorded while signature stays valid"); + assert_eq!(result.manifest_references.len(), 1); + assert!(matches!( + result.manifest_references[0].status, + DsigStatus::Invalid(FailureReason::ReferenceDigestMismatch { ref_index: 0 }) + )); + assert!(matches!(result.status, DsigStatus::Valid)); + } + #[test] fn verify_context_keeps_manifest_results_when_signedinfo_reference_fails() { let xml = signature_with_manifest_xml(true); @@ -1487,6 +1487,28 @@ mod tests { )); } + #[test] + fn verify_context_records_manifest_policy_violations_with_accepting_key() { + let xml = signature_with_manifest_xml(true); + let (prefix, object_suffix) = xml + .split_once("") + .expect("fixture should contain ds:Object"); + let mutated_object_suffix = + object_suffix.replacen("URI=\"#target\"", "URI=\"http://example.com/external\"", 1); + let broken_xml = format!("{prefix}{mutated_object_suffix}"); + let result = VerifyContext::new() + .key(&AcceptingKey) + .process_manifests(true) + .verify(&broken_xml) + .expect("manifest policy violations should be recorded while signature stays valid"); + assert_eq!(result.manifest_references.len(), 1); + assert!(matches!( + result.manifest_references[0].status, + DsigStatus::Invalid(FailureReason::ReferencePolicyViolation { ref_index: 0 }) + )); + assert!(matches!(result.status, DsigStatus::Valid)); + } + #[test] fn verify_context_records_manifest_missing_uri_as_processing_failure() { let xml = signature_with_manifest_xml(true); @@ -1514,6 +1536,30 @@ mod tests { )); } + #[test] + fn verify_context_records_manifest_missing_uri_with_accepting_key() { + let xml = signature_with_manifest_xml(true); + let (prefix, object_suffix) = xml + .split_once("") + .expect("fixture should contain ds:Object"); + let mutated_object_suffix = + object_suffix.replacen("", "", 1); + let broken_xml = format!("{prefix}{mutated_object_suffix}"); + + let result = VerifyContext::new() + .key(&AcceptingKey) + .process_manifests(true) + .verify(&broken_xml) + .expect("manifest missing URI should be recorded while signature stays valid"); + assert_eq!(result.manifest_references.len(), 1); + assert_eq!(result.manifest_references[0].uri, ""); + assert!(matches!( + result.manifest_references[0].status, + DsigStatus::Invalid(FailureReason::ReferenceProcessingFailure { ref_index: 0 }) + )); + assert!(matches!(result.status, DsigStatus::Valid)); + } + #[test] fn verify_context_ignores_nested_manifests_in_object() { let xml = signature_with_manifest_xml(true) From fac1310961d20a2763098ede2640d8852e340280 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 4 Apr 2026 19:10:39 +0300 Subject: [PATCH 11/14] fix(xmldsig): gate manifest processing to signed refs --- src/xmldsig/verify.rs | 197 +++++++++++++++++++++++++++++++++--------- 1 file changed, 155 insertions(+), 42 deletions(-) diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index d745c83..f5dc181 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -159,8 +159,11 @@ impl<'a> VerifyContext<'a> { /// Enable or disable `` processing. /// - /// When enabled, references under `` are processed - /// and returned in `VerifyResult::manifest_references`. + /// When enabled, references in `` elements that are direct + /// element children of `` are processed and returned in + /// `VerifyResult::manifest_references`. + /// Nested `` descendants under `` are not + /// processed. /// /// Manifest reference digest mismatches, policy violations, and processing /// failures are reported in `VerifyResult::manifest_references` and do not @@ -273,7 +276,13 @@ pub enum FailureReason { ReferenceDigestMismatch { /// Zero-based index of the failing `` in its processed set. /// - /// Use `ReferenceResult::reference_set` to distinguish SignedInfo vs Manifest. + /// On per-reference verification entries, use + /// `ReferenceResult::reference_set` to distinguish the `` + /// and `` reference sets. + /// + /// When this reason appears in `VerifyResult::status` without an + /// accompanying `ReferenceResult`, `ref_index` always refers to the + /// `` reference set. ref_index: usize, }, /// `` rejected by URI/transform allowlist policy. @@ -612,7 +621,8 @@ fn verify_signature_with_context( )?; let manifest_references = if ctx.process_manifests { - process_manifest_references(signature_node, &resolver, ctx)? + let signed_info_reference_ids = collect_signed_info_reference_ids(&signed_info.references); + process_manifest_references(signature_node, &resolver, ctx, &signed_info_reference_ids)? } else { Vec::new() }; @@ -679,8 +689,9 @@ fn process_manifest_references( signature_node: Node<'_, '_>, resolver: &UriReferenceResolver<'_>, ctx: &VerifyContext<'_>, + signed_info_reference_ids: &HashSet, ) -> Result, SignatureVerificationPipelineError> { - let manifest_references = parse_manifest_references(signature_node)?; + let manifest_references = parse_manifest_references(signature_node, signed_info_reference_ids)?; if manifest_references.is_empty() { return Ok(Vec::new()); } @@ -764,6 +775,7 @@ fn manifest_reference_invalid_result( fn parse_manifest_references( signature_node: Node<'_, '_>, + signed_info_reference_ids: &HashSet, ) -> Result, SignatureVerificationPipelineError> { let mut references = Vec::new(); for object_node in signature_node.children().filter(|node| { @@ -771,11 +783,18 @@ fn parse_manifest_references( && node.tag_name().namespace() == Some(XMLDSIG_NS) && node.tag_name().name() == "Object" }) { + let object_is_signed = + node_id_value(object_node).is_some_and(|id| signed_info_reference_ids.contains(id)); for manifest_node in object_node.children().filter(|node| { node.is_element() && node.tag_name().namespace() == Some(XMLDSIG_NS) && node.tag_name().name() == "Manifest" }) { + let manifest_is_signed = node_id_value(manifest_node) + .is_some_and(|id| signed_info_reference_ids.contains(id)); + if !object_is_signed && !manifest_is_signed { + continue; + } let mut manifest_children = Vec::new(); for child in manifest_node.children() { if child.is_text() @@ -814,6 +833,43 @@ fn parse_manifest_references( Ok(references) } +fn collect_signed_info_reference_ids(references: &[Reference]) -> HashSet { + references + .iter() + .filter_map(|reference| reference.uri.as_deref()) + .filter_map(signed_info_reference_id_from_uri) + .map(str::to_owned) + .collect() +} + +fn signed_info_reference_id_from_uri(uri: &str) -> Option<&str> { + let fragment = uri.strip_prefix('#')?; + if fragment.is_empty() || fragment == "xpointer(/)" { + return None; + } + if let Some(id) = parse_xpointer_id_fragment(fragment) { + return (!id.is_empty()).then_some(id); + } + (!fragment.starts_with("xpointer(")).then_some(fragment) +} + +fn parse_xpointer_id_fragment(fragment: &str) -> Option<&str> { + let inner = fragment.strip_prefix("xpointer(id(")?.strip_suffix("))")?; + if let Some(stripped) = inner.strip_prefix('\'').and_then(|s| s.strip_suffix('\'')) { + Some(stripped) + } else if let Some(stripped) = inner.strip_prefix('"').and_then(|s| s.strip_suffix('"')) { + Some(stripped) + } else { + None + } +} + +fn node_id_value<'a, 'input>(node: Node<'a, 'input>) -> Option<&'a str> { + node.attribute("ID") + .or_else(|| node.attribute("Id")) + .or_else(|| node.attribute("id")) +} + enum ResolvedVerifyingKey<'a> { Borrowed(&'a dyn VerifyingKey), Owned(Box), @@ -1256,23 +1312,25 @@ mod tests { } fn signature_with_manifest_xml(valid_manifest_digest: bool) -> String { + const TMP_SIGNED_INFO_DIGEST: &str = "AAAAAAAAAAAAAAAAAAAAAAAAAAA="; + const INVALID_MANIFEST_DIGEST: &str = "//////////////////////////8="; let xml_template = r##" payload - + - SIGNEDINFO_DIGEST_PLACEHOLDER + SIGNEDINFO_OBJECT_DIGEST_PLACEHOLDER AQ== - + MANIFEST_DIGEST_PLACEHOLDER @@ -1281,20 +1339,10 @@ mod tests { "##; - - let signed_info_digest_b64 = signature_with_target_reference("AQ==") - .split("") - .nth(1) - .and_then(|chunk| chunk.split("").next()) - .expect("signed reference digest should be present") - .to_owned(); - - let seed_xml = xml_template - .replace("SIGNEDINFO_DIGEST_PLACEHOLDER", &signed_info_digest_b64) - .replace( - "MANIFEST_DIGEST_PLACEHOLDER", - "AAAAAAAAAAAAAAAAAAAAAAAAAAA=", - ); + let seed_xml = xml_template.replace( + "SIGNEDINFO_OBJECT_DIGEST_PLACEHOLDER", + TMP_SIGNED_INFO_DIGEST, + ); let doc = Document::parse(&seed_xml).unwrap(); let signature_node = doc .descendants() @@ -1313,10 +1361,45 @@ mod tests { let final_manifest_digest_b64 = if valid_manifest_digest { computed_manifest_digest_b64.as_str() } else { - "AAAAAAAAAAAAAAAAAAAAAAAAAAA=" + INVALID_MANIFEST_DIGEST }; + let xml_with_manifest_digest = + seed_xml.replace("MANIFEST_DIGEST_PLACEHOLDER", final_manifest_digest_b64); + let signed_doc = Document::parse(&xml_with_manifest_digest).unwrap(); + let signed_signature_node = signed_doc + .descendants() + .find(|node| { + node.is_element() + && node.tag_name().namespace() == Some(XMLDSIG_NS) + && node.tag_name().name() == "Signature" + }) + .unwrap(); + let signed_info_node = signed_signature_node + .children() + .find(|node| { + node.is_element() + && node.tag_name().namespace() == Some(XMLDSIG_NS) + && node.tag_name().name() == "SignedInfo" + }) + .unwrap(); + let signed_info = parse_signed_info(signed_info_node).unwrap(); + let object_reference = &signed_info.references[0]; + let signed_resolver = UriReferenceResolver::new(&signed_doc); + let signed_initial_data = signed_resolver + .dereference(object_reference.uri.as_deref().unwrap()) + .unwrap(); + let signed_pre_digest = crate::xmldsig::execute_transforms( + signed_signature_node, + signed_initial_data, + &object_reference.transforms, + ) + .unwrap(); + let signed_digest_b64 = base64::engine::general_purpose::STANDARD.encode(compute_digest( + object_reference.digest_method, + &signed_pre_digest, + )); - seed_xml.replace("AAAAAAAAAAAAAAAAAAAAAAAAAAA=", final_manifest_digest_b64) + xml_with_manifest_digest.replacen(TMP_SIGNED_INFO_DIGEST, &signed_digest_b64, 1) } #[test] @@ -1336,14 +1419,11 @@ mod tests { DsigStatus::Invalid(FailureReason::SignatureMismatch) )); - let xml_with_manifest = signature_with_manifest_xml(true); - let (prefix, object_suffix) = xml_with_manifest - .split_once("") - .expect("fixture should contain ds:Object"); - let malformed_object_suffix = object_suffix - .replacen("", "", 1) - .replacen("", "", 1); - let malformed_manifest_xml = format!("{prefix}{malformed_object_suffix}"); + let malformed_manifest_xml = signature_with_manifest_xml(true).replacen( + "", + "", + 1, + ); let malformed_with_manifests_disabled = VerifyContext::new() .key(&RejectingKey) .verify(&malformed_manifest_xml) @@ -1483,7 +1563,7 @@ mod tests { )); assert!(matches!( result.status, - DsigStatus::Invalid(FailureReason::SignatureMismatch) + DsigStatus::Invalid(FailureReason::ReferenceDigestMismatch { ref_index: 0 }) )); } @@ -1506,7 +1586,10 @@ mod tests { result.manifest_references[0].status, DsigStatus::Invalid(FailureReason::ReferencePolicyViolation { ref_index: 0 }) )); - assert!(matches!(result.status, DsigStatus::Valid)); + assert!(matches!( + result.status, + DsigStatus::Invalid(FailureReason::ReferenceDigestMismatch { ref_index: 0 }) + )); } #[test] @@ -1532,7 +1615,7 @@ mod tests { )); assert!(matches!( result.status, - DsigStatus::Invalid(FailureReason::SignatureMismatch) + DsigStatus::Invalid(FailureReason::ReferenceDigestMismatch { ref_index: 0 }) )); } @@ -1557,14 +1640,21 @@ mod tests { result.manifest_references[0].status, DsigStatus::Invalid(FailureReason::ReferenceProcessingFailure { ref_index: 0 }) )); - assert!(matches!(result.status, DsigStatus::Valid)); + assert!(matches!( + result.status, + DsigStatus::Invalid(FailureReason::ReferenceDigestMismatch { ref_index: 0 }) + )); } #[test] fn verify_context_ignores_nested_manifests_in_object() { let xml = signature_with_manifest_xml(true) - .replace("", "") - .replace("", ""); + .replacen( + "", + "", + 1, + ) + .replacen("", "", 1); let result = VerifyContext::new() .key(&RejectingKey) @@ -1614,8 +1704,11 @@ mod tests { #[test] fn verify_context_rejects_manifest_non_whitespace_mixed_content() { - let xml = - signature_with_manifest_xml(true).replacen("", "junk", 1); + let xml = signature_with_manifest_xml(true).replacen( + "", + "junk", + 1, + ); let err = VerifyContext::new() .key(&RejectingKey) @@ -1634,12 +1727,12 @@ mod tests { fn verify_context_rejects_empty_manifest_children() { let xml = signature_with_manifest_xml(true); let (prefix, rest) = xml - .split_once("") + .split_once("") .expect("fixture should contain Manifest"); let (_, suffix) = rest .split_once("") .expect("fixture should contain closing Manifest"); - let xml = format!("{prefix}{suffix}"); + let xml = format!("{prefix}{suffix}"); let err = VerifyContext::new() .key(&RejectingKey) @@ -1654,6 +1747,26 @@ mod tests { )); } + #[test] + fn verify_context_ignores_unsigned_malformed_manifest_blocks() { + let xml = signature_with_manifest_xml(true).replacen( + "", + "junk", + 1, + ); + let result = VerifyContext::new() + .key(&AcceptingKey) + .process_manifests(true) + .verify(&xml) + .expect("unsigned malformed Manifest must be ignored"); + assert_eq!( + result.manifest_references.len(), + 1, + "only signed Manifest references must be reported", + ); + assert!(matches!(result.status, DsigStatus::Valid)); + } + #[test] fn verify_context_rejects_implicit_default_c14n_when_not_allowlisted() { let xml = minimal_signature_xml("", ""); From 31cb76073a5798e10ff65b30f38db74354f263e9 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 4 Apr 2026 19:25:11 +0300 Subject: [PATCH 12/14] test(xmldsig): keep accepting-key manifest tests valid --- src/xmldsig/verify.rs | 55 +++++++++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 25 deletions(-) diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index f5dc181..b10057e 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -160,10 +160,14 @@ impl<'a> VerifyContext<'a> { /// Enable or disable `` processing. /// /// When enabled, references in `` elements that are direct - /// element children of `` are processed and returned in + /// element children of `` are processed only when the direct-child + /// `` or `` itself is referenced from ``. + /// Only those signed Manifest references are returned in /// `VerifyResult::manifest_references`. /// Nested `` descendants under `` are not /// processed. + /// Direct-child unsigned/unreferenced Manifests are skipped and do not + /// appear in `VerifyResult::manifest_references`. /// /// Manifest reference digest mismatches, policy violations, and processing /// failures are reported in `VerifyResult::manifest_references` and do not @@ -465,6 +469,10 @@ pub struct VerifyResult { pub signed_info_references: Vec, /// `` reference results. /// Populated only when `VerifyContext::process_manifests(true)` is enabled. + /// Includes only references from signed direct-child `/` + /// blocks that are referenced from ``. + /// Unsigned/unreferenced direct-child Manifest blocks are skipped, so an + /// empty list does not imply that no Manifest elements existed in `verify()` input. pub manifest_references: Vec, /// Canonicalized `` bytes when `store_pre_digest` is enabled /// and verification reaches SignedInfo canonicalization. @@ -1312,6 +1320,16 @@ mod tests { } fn signature_with_manifest_xml(valid_manifest_digest: bool) -> String { + signature_with_manifest_xml_with_manifest_mutation(valid_manifest_digest, |xml| xml) + } + + fn signature_with_manifest_xml_with_manifest_mutation( + valid_manifest_digest: bool, + mutate_manifest: F, + ) -> String + where + F: FnOnce(String) -> String, + { const TMP_SIGNED_INFO_DIGEST: &str = "AAAAAAAAAAAAAAAAAAAAAAAAAAA="; const INVALID_MANIFEST_DIGEST: &str = "//////////////////////////8="; let xml_template = r##" @@ -1363,8 +1381,9 @@ mod tests { } else { INVALID_MANIFEST_DIGEST }; - let xml_with_manifest_digest = - seed_xml.replace("MANIFEST_DIGEST_PLACEHOLDER", final_manifest_digest_b64); + let xml_with_manifest_digest = mutate_manifest( + seed_xml.replace("MANIFEST_DIGEST_PLACEHOLDER", final_manifest_digest_b64), + ); let signed_doc = Document::parse(&xml_with_manifest_digest).unwrap(); let signed_signature_node = signed_doc .descendants() @@ -1569,13 +1588,9 @@ mod tests { #[test] fn verify_context_records_manifest_policy_violations_with_accepting_key() { - let xml = signature_with_manifest_xml(true); - let (prefix, object_suffix) = xml - .split_once("") - .expect("fixture should contain ds:Object"); - let mutated_object_suffix = - object_suffix.replacen("URI=\"#target\"", "URI=\"http://example.com/external\"", 1); - let broken_xml = format!("{prefix}{mutated_object_suffix}"); + let broken_xml = signature_with_manifest_xml_with_manifest_mutation(true, |xml| { + xml.replacen("URI=\"#target\"", "URI=\"http://example.com/external\"", 1) + }); let result = VerifyContext::new() .key(&AcceptingKey) .process_manifests(true) @@ -1586,10 +1601,7 @@ mod tests { result.manifest_references[0].status, DsigStatus::Invalid(FailureReason::ReferencePolicyViolation { ref_index: 0 }) )); - assert!(matches!( - result.status, - DsigStatus::Invalid(FailureReason::ReferenceDigestMismatch { ref_index: 0 }) - )); + assert!(matches!(result.status, DsigStatus::Valid)); } #[test] @@ -1621,13 +1633,9 @@ mod tests { #[test] fn verify_context_records_manifest_missing_uri_with_accepting_key() { - let xml = signature_with_manifest_xml(true); - let (prefix, object_suffix) = xml - .split_once("") - .expect("fixture should contain ds:Object"); - let mutated_object_suffix = - object_suffix.replacen("", "", 1); - let broken_xml = format!("{prefix}{mutated_object_suffix}"); + let broken_xml = signature_with_manifest_xml_with_manifest_mutation(true, |xml| { + xml.replacen("", "", 1) + }); let result = VerifyContext::new() .key(&AcceptingKey) @@ -1640,10 +1648,7 @@ mod tests { result.manifest_references[0].status, DsigStatus::Invalid(FailureReason::ReferenceProcessingFailure { ref_index: 0 }) )); - assert!(matches!( - result.status, - DsigStatus::Invalid(FailureReason::ReferenceDigestMismatch { ref_index: 0 }) - )); + assert!(matches!(result.status, DsigStatus::Valid)); } #[test] From 93ee5cd6aeb9403cc8a1586fde8b43e0d293dc97 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 4 Apr 2026 19:47:26 +0300 Subject: [PATCH 13/14] fix(xmldsig): match signed manifests by node identity --- src/xmldsig/uri.rs | 36 ++++++++++++++++------- src/xmldsig/verify.rs | 68 ++++++++++++++++++++++++------------------- 2 files changed, 63 insertions(+), 41 deletions(-) diff --git a/src/xmldsig/uri.rs b/src/xmldsig/uri.rs index 539cf83..e5911a7 100644 --- a/src/xmldsig/uri.rs +++ b/src/xmldsig/uri.rs @@ -14,7 +14,7 @@ use std::collections::hash_map::Entry; use std::collections::{HashMap, HashSet}; -use roxmltree::{Document, Node}; +use roxmltree::{Document, Node, NodeId}; use super::types::{NodeSet, TransformData, TransformError}; @@ -169,7 +169,7 @@ impl<'a> UriReferenceResolver<'a> { Ok(TransformData::NodeSet( NodeSet::entire_document_with_comments(self.doc), )) - } else if let Some(id) = parse_xpointer_id(fragment) { + } else if let Some(id) = parse_xpointer_id_fragment(fragment) { // xpointer(id('foo')) → same as bare-name #foo // Reject empty parsed ID (e.g., xpointer(id(''))) — not a valid XML Name if id.is_empty() { @@ -198,6 +198,14 @@ impl<'a> UriReferenceResolver<'a> { self.id_map.contains_key(id) } + /// Resolve a same-document ID token to a stable node identity. + /// + /// Returns `None` when the ID is absent or ambiguous (duplicate ID collision), + /// matching the resolver behavior used by `dereference()`. + pub(crate) fn node_id_for_id(&self, id: &str) -> Option { + self.id_map.get(id).map(|node| node.id()) + } + /// Get the number of registered IDs. pub fn id_count(&self) -> usize { self.id_map.len() @@ -206,7 +214,7 @@ impl<'a> UriReferenceResolver<'a> { /// Parse `xpointer(id('value'))` or `xpointer(id("value"))` and return the ID value. /// Returns `None` if the fragment doesn't match this pattern. -fn parse_xpointer_id(fragment: &str) -> Option<&str> { +pub(crate) fn parse_xpointer_id_fragment(fragment: &str) -> Option<&str> { let inner = fragment.strip_prefix("xpointer(id(")?.strip_suffix("))")?; // Strip single or double quotes using safe helpers to avoid panics @@ -622,21 +630,27 @@ mod tests { #[test] fn parse_xpointer_id_variants() { // Valid forms - assert_eq!(super::parse_xpointer_id("xpointer(id('foo'))"), Some("foo")); assert_eq!( - super::parse_xpointer_id(r#"xpointer(id("bar"))"#), + super::parse_xpointer_id_fragment("xpointer(id('foo'))"), + Some("foo") + ); + assert_eq!( + super::parse_xpointer_id_fragment(r#"xpointer(id("bar"))"#), Some("bar") ); // Invalid forms - assert_eq!(super::parse_xpointer_id("xpointer(/)"), None); - assert_eq!(super::parse_xpointer_id("xpointer(id(foo))"), None); // no quotes - assert_eq!(super::parse_xpointer_id("not-xpointer"), None); - assert_eq!(super::parse_xpointer_id(""), None); + assert_eq!(super::parse_xpointer_id_fragment("xpointer(/)"), None); + assert_eq!(super::parse_xpointer_id_fragment("xpointer(id(foo))"), None); // no quotes + assert_eq!(super::parse_xpointer_id_fragment("not-xpointer"), None); + assert_eq!(super::parse_xpointer_id_fragment(""), None); // Malformed: single quote char — must not panic (was slicing bug) - assert_eq!(super::parse_xpointer_id("xpointer(id('))"), None); - assert_eq!(super::parse_xpointer_id(r#"xpointer(id("))"#), None); + assert_eq!(super::parse_xpointer_id_fragment("xpointer(id('))"), None); + assert_eq!( + super::parse_xpointer_id_fragment(r#"xpointer(id("))"#), + None + ); } #[test] diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index b10057e..325b7bb 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -11,7 +11,7 @@ //! - [`verify_signature_with_pem_key`] for full pipeline validation (`SignedInfo` + `SignatureValue`) use base64::Engine; -use roxmltree::{Document, Node}; +use roxmltree::{Document, Node, NodeId}; use std::collections::HashSet; use crate::c14n::canonicalize; @@ -25,7 +25,7 @@ use super::signature::{ use super::transforms::{ DEFAULT_IMPLICIT_C14N_URI, Transform, XPATH_TRANSFORM_URI, execute_transforms, }; -use super::uri::UriReferenceResolver; +use super::uri::{UriReferenceResolver, parse_xpointer_id_fragment}; const MAX_SIGNATURE_VALUE_LEN: usize = 8192; const MAX_SIGNATURE_VALUE_TEXT_LEN: usize = 65_536; @@ -629,8 +629,9 @@ fn verify_signature_with_context( )?; let manifest_references = if ctx.process_manifests { - let signed_info_reference_ids = collect_signed_info_reference_ids(&signed_info.references); - process_manifest_references(signature_node, &resolver, ctx, &signed_info_reference_ids)? + let signed_info_reference_nodes = + collect_signed_info_reference_nodes(&signed_info.references, &resolver); + process_manifest_references(signature_node, &resolver, ctx, &signed_info_reference_nodes)? } else { Vec::new() }; @@ -697,9 +698,10 @@ fn process_manifest_references( signature_node: Node<'_, '_>, resolver: &UriReferenceResolver<'_>, ctx: &VerifyContext<'_>, - signed_info_reference_ids: &HashSet, + signed_info_reference_nodes: &HashSet, ) -> Result, SignatureVerificationPipelineError> { - let manifest_references = parse_manifest_references(signature_node, signed_info_reference_ids)?; + let manifest_references = + parse_manifest_references(signature_node, signed_info_reference_nodes)?; if manifest_references.is_empty() { return Ok(Vec::new()); } @@ -783,7 +785,7 @@ fn manifest_reference_invalid_result( fn parse_manifest_references( signature_node: Node<'_, '_>, - signed_info_reference_ids: &HashSet, + signed_info_reference_nodes: &HashSet, ) -> Result, SignatureVerificationPipelineError> { let mut references = Vec::new(); for object_node in signature_node.children().filter(|node| { @@ -791,15 +793,13 @@ fn parse_manifest_references( && node.tag_name().namespace() == Some(XMLDSIG_NS) && node.tag_name().name() == "Object" }) { - let object_is_signed = - node_id_value(object_node).is_some_and(|id| signed_info_reference_ids.contains(id)); + let object_is_signed = signed_info_reference_nodes.contains(&object_node.id()); for manifest_node in object_node.children().filter(|node| { node.is_element() && node.tag_name().namespace() == Some(XMLDSIG_NS) && node.tag_name().name() == "Manifest" }) { - let manifest_is_signed = node_id_value(manifest_node) - .is_some_and(|id| signed_info_reference_ids.contains(id)); + let manifest_is_signed = signed_info_reference_nodes.contains(&manifest_node.id()); if !object_is_signed && !manifest_is_signed { continue; } @@ -841,12 +841,15 @@ fn parse_manifest_references( Ok(references) } -fn collect_signed_info_reference_ids(references: &[Reference]) -> HashSet { +fn collect_signed_info_reference_nodes( + references: &[Reference], + resolver: &UriReferenceResolver<'_>, +) -> HashSet { references .iter() .filter_map(|reference| reference.uri.as_deref()) .filter_map(signed_info_reference_id_from_uri) - .map(str::to_owned) + .filter_map(|id| resolver.node_id_for_id(id)) .collect() } @@ -861,23 +864,6 @@ fn signed_info_reference_id_from_uri(uri: &str) -> Option<&str> { (!fragment.starts_with("xpointer(")).then_some(fragment) } -fn parse_xpointer_id_fragment(fragment: &str) -> Option<&str> { - let inner = fragment.strip_prefix("xpointer(id(")?.strip_suffix("))")?; - if let Some(stripped) = inner.strip_prefix('\'').and_then(|s| s.strip_suffix('\'')) { - Some(stripped) - } else if let Some(stripped) = inner.strip_prefix('"').and_then(|s| s.strip_suffix('"')) { - Some(stripped) - } else { - None - } -} - -fn node_id_value<'a, 'input>(node: Node<'a, 'input>) -> Option<&'a str> { - node.attribute("ID") - .or_else(|| node.attribute("Id")) - .or_else(|| node.attribute("id")) -} - enum ResolvedVerifyingKey<'a> { Borrowed(&'a dyn VerifyingKey), Owned(Box), @@ -1772,6 +1758,28 @@ mod tests { assert!(matches!(result.status, DsigStatus::Valid)); } + #[test] + fn verify_context_skips_ambiguous_manifest_id_blocks() { + let xml = signature_with_manifest_xml(true).replacen( + "", + "junk", + 1, + ); + let err = VerifyContext::new() + .key(&RejectingKey) + .process_manifests(true) + .verify(&xml) + .expect_err("ambiguous manifest IDs should make SignedInfo #manifest dereference fail"); + assert!(matches!( + err, + SignatureVerificationPipelineError::Reference( + ReferenceProcessingError::UriDereference( + crate::xmldsig::types::TransformError::ElementNotFound(id) + ) + ) if id == "manifest" + )); + } + #[test] fn verify_context_rejects_implicit_default_c14n_when_not_allowlisted() { let xml = minimal_signature_xml("", ""); From 0f3ceac277cd259e36d39d790eabe69f7be6f377 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 4 Apr 2026 21:33:08 +0300 Subject: [PATCH 14/14] test(xmldsig): cover signed-object manifest path - clarify process_manifests docs to require ID-based same-document references - document whole-document URI references as non-signing for direct child selection - add regression test for SignedInfo reference to ds:Object with nested Manifest without ID Refs #41 --- src/xmldsig/verify.rs | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index 325b7bb..cd47efb 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -161,13 +161,18 @@ impl<'a> VerifyContext<'a> { /// /// When enabled, references in `` elements that are direct /// element children of `` are processed only when the direct-child - /// `` or `` itself is referenced from ``. + /// `` or `` itself is referenced from `` + /// by an ID-based same-document fragment URI such as `#id` or + /// `#xpointer(id('id'))`. /// Only those signed Manifest references are returned in /// `VerifyResult::manifest_references`. /// Nested `` descendants under `` are not /// processed. /// Direct-child unsigned/unreferenced Manifests are skipped and do not /// appear in `VerifyResult::manifest_references`. + /// Whole-document same-document references such as `URI=""` or + /// `URI="#xpointer(/)"` do not mark a specific direct-child + /// ``/`` as signed for this option. /// /// Manifest reference digest mismatches, policy violations, and processing /// failures are reported in `VerifyResult::manifest_references` and do not @@ -1468,6 +1473,35 @@ mod tests { )); } + #[test] + fn verify_context_processes_manifest_when_signedinfo_references_object() { + let xml = signature_with_manifest_xml_with_manifest_mutation(true, |xml| { + xml.replacen("URI=\"#manifest\"", "URI=\"#object-id\"", 1) + .replacen("", "", 1) + .replacen("", "", 1) + }); + + let result = VerifyContext::new() + .key(&RejectingKey) + .process_manifests(true) + .verify(&xml) + .expect("manifest references should be processed when SignedInfo references ds:Object"); + assert_eq!( + result.manifest_references.len(), + 1, + "signed ds:Object should enable processing of its direct-child ds:Manifest", + ); + assert_eq!( + result.manifest_references[0].reference_set, + ReferenceSet::Manifest + ); + assert_eq!(result.manifest_references[0].reference_index, 0); + assert!(matches!( + result.manifest_references[0].status, + DsigStatus::Valid + )); + } + #[test] fn verify_context_manifest_digest_mismatch_is_non_fatal() { let xml = signature_with_manifest_xml(false);