Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
231 changes: 212 additions & 19 deletions src/xmldsig/parse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
//! ```

use roxmltree::{Document, Node};
use x509_parser::extensions::ParsedExtension;
use x509_parser::prelude::FromDer;
use x509_parser::public_key::PublicKey;

use super::digest::DigestAlgorithm;
use super::transforms::{self, Transform};
Expand Down Expand Up @@ -169,6 +172,8 @@ pub enum KeyValueInfo {
#[non_exhaustive]
pub struct X509DataInfo {
/// DER-encoded certificates from `<X509Certificate>`.
///
/// This vector has a 1:1 index correspondence with `parsed_certificates`.
pub certificates: Vec<Vec<u8>>,
/// Text values from `<X509SubjectName>`.
pub subject_names: Vec<String>,
Expand All @@ -180,6 +185,49 @@ pub struct X509DataInfo {
pub crls: Vec<Vec<u8>>,
/// `(Algorithm URI, digest bytes)` tuples from `dsig11:X509Digest`.
pub digests: Vec<(String, Vec<u8>)>,
/// Parsed metadata for each `<X509Certificate>` entry.
///
/// This vector has a 1:1 index correspondence with `certificates`.
pub parsed_certificates: Vec<ParsedX509Certificate>,
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/// Parsed X.509 certificate details extracted from DER.
#[derive(Debug, Clone, PartialEq, Eq)]
#[non_exhaustive]
pub struct ParsedX509Certificate {
/// Subject distinguished name.
pub subject_dn: String,
/// Issuer distinguished name.
pub issuer_dn: String,
/// Subject Key Identifier extension bytes (if present).
pub subject_key_identifier: Option<Vec<u8>>,
/// Parsed certificate public key material.
pub public_key: X509PublicKeyInfo,
}

/// Public key material extracted from certificate SubjectPublicKeyInfo.
#[derive(Debug, Clone, PartialEq, Eq)]
#[non_exhaustive]
pub enum X509PublicKeyInfo {
/// RSA public key (`modulus`, `exponent`).
Rsa {
/// Unsigned big-endian RSA modulus (`n`), normalized without leading zeroes.
modulus: Vec<u8>,
/// Unsigned big-endian RSA public exponent (`e`), normalized without leading zeroes.
exponent: Vec<u8>,
},
/// EC public key (`curve_oid`, uncompressed point bytes).
Ec {
/// Named-curve OID from SubjectPublicKeyInfo parameters.
curve_oid: String,
/// Raw EC point bytes from SubjectPublicKeyInfo.
public_key: Vec<u8>,
Comment on lines +219 to +224
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

In RFC 5480 and x509-parser 0.18.0, is the EC SubjectPublicKeyInfo point guaranteed to be uncompressed, or can it be encoded in compressed SEC1 form as well?

💡 Result:

In RFC 5480, the EC SubjectPublicKeyInfo point is not guaranteed to be uncompressed. Implementations MUST support the uncompressed form (starting with 0x04) and MAY support the compressed form (starting with 0x02 or 0x03) as defined in SEC1 [1][2][3]. The public key MUST be rejected if the first octet is any other value [1]. In x509-parser 0.18.0, the EC point is parsed as an ECPoint struct containing the raw octet string data from the BIT STRING in SubjectPublicKeyInfo [4][5][6]. The ECPoint constructor takes the raw bytes (ECPoint::from(b) where b is the bit string data) without validation of the format, so it accepts both uncompressed and compressed forms, as well as invalid first octets (though RFC requires rejection of invalid ones, the parser does not enforce this) [4][6]. The library is designed for standards-compliant X.509 parsing per RFC 5280 and references RFC 5480 for EC points [5].

Citations:


Don't promise uncompressed EC point bytes without enforcing it.

X509PublicKeyInfo::Ec documents "uncompressed point bytes," but RFC 5480 permits both uncompressed (0x04) and compressed (0x02/0x03) SEC1 encodings. The x509-parser library accepts both forms without validation. Since the code passes through whatever encoding is present, either validate the 0x04 uncompressed prefix before constructing the struct, or relax the docs to "encoded point bytes" to match the actual behavior.

Suggested doc-only fix
-    /// EC public key (`curve_oid`, uncompressed point bytes).
+    /// EC public key (`curve_oid`, encoded point bytes).
     Ec {
         /// Named-curve OID from SubjectPublicKeyInfo parameters.
         curve_oid: String,
-        /// Raw EC point bytes from SubjectPublicKeyInfo.
+        /// Encoded EC point bytes from SubjectPublicKeyInfo.
         public_key: Vec<u8>,
     },
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// EC public key (`curve_oid`, uncompressed point bytes).
Ec {
/// Named-curve OID from SubjectPublicKeyInfo parameters.
curve_oid: String,
/// Raw EC point bytes from SubjectPublicKeyInfo.
public_key: Vec<u8>,
/// EC public key (`curve_oid`, encoded point bytes).
Ec {
/// Named-curve OID from SubjectPublicKeyInfo parameters.
curve_oid: String,
/// Encoded EC point bytes from SubjectPublicKeyInfo.
public_key: Vec<u8>,
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/xmldsig/parse.rs` around lines 219 - 224, The doc for
X509PublicKeyInfo::Ec promises "uncompressed point bytes" but code (parse.rs,
X509PublicKeyInfo::Ec) currently accepts any SEC1 encoding; either enforce the
uncompressed form by validating the public_key's first byte == 0x04 when
constructing/parsing the Ec variant and return an error if not, or change the
field docstring to "encoded point bytes" to reflect that compressed forms
(0x02/0x03) are allowed; update the constructor/parser that produces
X509PublicKeyInfo::Ec and the doc comment on the Ec variant accordingly so docs
and behavior match.

},
/// Public key algorithm is present but not parsed into a concrete key type.
Unsupported {
/// SubjectPublicKeyInfo algorithm OID.
algorithm_oid: String,
},
}

/// Errors during XMLDSig element parsing.
Expand Down Expand Up @@ -504,6 +552,8 @@ fn parse_x509_data_dispatch(node: Node) -> Result<X509DataInfo, ParseError> {
ensure_x509_data_entry_budget(&info)?;
let cert = decode_x509_base64(child, "X509Certificate")?;
add_x509_data_usage(&mut total_binary_len, cert.len())?;
let parsed_cert = parse_x509_certificate(cert.as_slice())?;
info.parsed_certificates.push(parsed_cert);
info.certificates.push(cert);
}
(Some(XMLDSIG_NS), "X509SubjectName") => {
Expand Down Expand Up @@ -631,6 +681,80 @@ fn decode_x509_base64(
Ok(decoded)
}

fn parse_x509_certificate(cert_der: &[u8]) -> Result<ParsedX509Certificate, ParseError> {
let (rest, cert) =
x509_parser::certificate::X509Certificate::from_der(cert_der).map_err(|err| {
ParseError::InvalidStructure(format!("X509Certificate is not valid DER X.509: {err}"))
})?;
if !rest.is_empty() {
return Err(ParseError::InvalidStructure(
"X509Certificate contains trailing bytes after DER certificate".into(),
));
}

let subject_dn = cert.subject().to_string();
let issuer_dn = cert.issuer().to_string();

let subject_key_identifier = cert.extensions().iter().find_map(|ext| {
if let ParsedExtension::SubjectKeyIdentifier(ski) = ext.parsed_extension() {
Some(ski.0.to_vec())
} else {
None
}
});

let spki = cert.public_key();
let public_key = match spki.parsed().map_err(|err| {
ParseError::InvalidStructure(format!("X509Certificate public key parse error: {err}"))
})? {
PublicKey::RSA(rsa) => {
let modulus = trim_leading_zeroes(rsa.modulus);
let exponent = trim_leading_zeroes(rsa.exponent);
if modulus.is_empty() || exponent.is_empty() {
return Err(ParseError::InvalidStructure(
"X509Certificate RSA key contains empty modulus or exponent".into(),
));
}
X509PublicKeyInfo::Rsa { modulus, exponent }
}
PublicKey::EC(ec_point) => {
let Some(params) = spki.algorithm.parameters.as_ref() else {
return Err(ParseError::InvalidStructure(
"X509Certificate EC key is missing curve parameters".into(),
));
};

match params.as_oid() {
Ok(oid) => X509PublicKeyInfo::Ec {
curve_oid: oid.to_id_string(),
public_key: ec_point.data().to_vec(),
},
Err(_) => X509PublicKeyInfo::Unsupported {
algorithm_oid: spki.algorithm.algorithm.to_id_string(),
},
}
}
_ => X509PublicKeyInfo::Unsupported {
algorithm_oid: spki.algorithm.algorithm.to_id_string(),
},
Comment thread
polaz marked this conversation as resolved.
};

Ok(ParsedX509Certificate {
subject_dn,
issuer_dn,
subject_key_identifier,
public_key,
})
}

fn trim_leading_zeroes(bytes: &[u8]) -> Vec<u8> {
let first_non_zero = bytes
.iter()
.position(|byte| *byte != 0)
.unwrap_or(bytes.len());
bytes[first_non_zero..].to_vec()
}

fn parse_x509_issuer_serial(node: Node<'_, '_>) -> Result<(String, String), ParseError> {
verify_ds_element(node, "X509IssuerSerial")?;
ensure_no_non_whitespace_text(node, "X509IssuerSerial")?;
Expand Down Expand Up @@ -845,6 +969,13 @@ mod tests {
use super::*;
use base64::Engine;

fn fixture_rsa_cert_base64() -> String {
include_str!("../../tests/fixtures/keys/rsa/rsa-2048-cert.pem")
.lines()
.filter(|line| !line.starts_with("-----"))
.collect::<String>()
}

// ── SignatureAlgorithm ───────────────────────────────────────────

#[test]
Expand Down Expand Up @@ -937,7 +1068,9 @@ mod tests {

#[test]
fn parse_key_info_dispatches_supported_children() {
let xml = r#"<KeyInfo xmlns="http://www.w3.org/2000/09/xmldsig#"
let cert_base64 = fixture_rsa_cert_base64();
let xml = format!(
r#"<KeyInfo xmlns="http://www.w3.org/2000/09/xmldsig#"
xmlns:dsig11="http://www.w3.org/2009/xmldsig11#">
<KeyName>idp-signing-key</KeyName>
<KeyValue>
Expand All @@ -947,7 +1080,7 @@ mod tests {
</RSAKeyValue>
</KeyValue>
<X509Data>
<X509Certificate>AQID</X509Certificate>
<X509Certificate>{cert_base64}</X509Certificate>
<X509SubjectName>CN=Example</X509SubjectName>
<X509IssuerSerial>
<X509IssuerName>CN=CA</X509IssuerName>
Expand All @@ -958,8 +1091,9 @@ mod tests {
<dsig11:X509Digest Algorithm="http://www.w3.org/2001/04/xmlenc#sha256">CAkK</dsig11:X509Digest>
</X509Data>
<dsig11:DEREncodedKeyValue>AQIDBA==</dsig11:DEREncodedKeyValue>
</KeyInfo>"#;
let doc = Document::parse(xml).unwrap();
</KeyInfo>"#
);
let doc = Document::parse(&xml).unwrap();

let key_info = parse_key_info(doc.root_element()).unwrap();
assert_eq!(key_info.sources.len(), 4);
Expand All @@ -972,20 +1106,38 @@ mod tests {
key_info.sources[1],
KeyInfoSource::KeyValue(KeyValueInfo::RsaKeyValue)
);
let x509_info = match &key_info.sources[2] {
KeyInfoSource::X509Data(x509) => x509,
other => panic!("expected X509Data source, got {other:?}"),
};
let expected_cert = base64::engine::general_purpose::STANDARD
.decode(&cert_base64)
.expect("fixture PEM must contain valid base64");
assert_eq!(x509_info.certificates, vec![expected_cert]);
assert_eq!(x509_info.subject_names, vec!["CN=Example".to_string()]);
assert_eq!(
key_info.sources[2],
KeyInfoSource::X509Data(X509DataInfo {
certificates: vec![vec![1, 2, 3]],
subject_names: vec!["CN=Example".into()],
issuer_serials: vec![("CN=CA".into(), "42".into())],
skis: vec![vec![1, 2, 3, 4]],
crls: vec![vec![4, 5, 6, 7]],
digests: vec![(
"http://www.w3.org/2001/04/xmlenc#sha256".into(),
vec![8, 9, 10]
)],
})
x509_info.issuer_serials,
vec![("CN=CA".to_string(), "42".to_string())]
);
assert_eq!(x509_info.skis, vec![vec![1, 2, 3, 4]]);
assert_eq!(x509_info.crls, vec![vec![4, 5, 6, 7]]);
assert_eq!(
x509_info.digests,
vec![(
"http://www.w3.org/2001/04/xmlenc#sha256".to_string(),
vec![8, 9, 10]
)]
);
assert_eq!(x509_info.parsed_certificates.len(), 1);
let parsed_cert = &x509_info.parsed_certificates[0];
assert!(!parsed_cert.subject_dn.is_empty());
assert!(!parsed_cert.issuer_dn.is_empty());
assert!(parsed_cert.subject_key_identifier.is_some());
assert!(matches!(
parsed_cert.public_key,
X509PublicKeyInfo::Rsa { .. }
));

assert_eq!(
key_info.sources[3],
KeyInfoSource::DerEncodedKeyValue(vec![1, 2, 3, 4])
Expand Down Expand Up @@ -1202,19 +1354,60 @@ mod tests {
#[test]
fn parse_key_info_rejects_x509_data_exceeding_total_binary_budget() {
let payload = base64::engine::general_purpose::STANDARD.encode(vec![0u8; 190_000]);
let certs = (0..6)
.map(|_| format!("<X509Certificate>{payload}</X509Certificate>"))
let entries = (0..6)
.map(|_| format!("<X509SKI>{payload}</X509SKI>"))
.collect::<Vec<_>>()
.join("");
let xml = format!(
"<KeyInfo xmlns=\"http://www.w3.org/2000/09/xmldsig#\"><X509Data>{certs}</X509Data></KeyInfo>"
"<KeyInfo xmlns=\"http://www.w3.org/2000/09/xmldsig#\"><X509Data>{entries}</X509Data></KeyInfo>"
);
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_x509_certificate_with_invalid_der() {
let xml = r#"<KeyInfo xmlns="http://www.w3.org/2000/09/xmldsig#">
<X509Data>
<X509Certificate>AQID</X509Certificate>
</X509Data>
</KeyInfo>"#;
let doc = Document::parse(xml).unwrap();

let err = parse_key_info(doc.root_element()).unwrap_err();
assert!(matches!(err, ParseError::InvalidStructure(_)));
}
Comment on lines +1370 to +1381
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Add a regression case for valid DER plus trailing bytes.

The new parser explicitly rejects non-empty rest after X509Certificate::from_der, but this test only covers completely invalid DER (AQID). Please add a case that appends a few bytes to a real fixture certificate and asserts ParseError::InvalidStructure, so the branch at Lines 689-692 cannot regress silently.

Example test shape
+    #[test]
+    fn parse_key_info_rejects_x509_certificate_with_trailing_der_bytes() {
+        let mut cert = base64::engine::general_purpose::STANDARD
+            .decode(fixture_rsa_cert_base64())
+            .expect("fixture PEM must contain valid base64");
+        cert.extend_from_slice(&[0x00, 0x01]);
+        let cert_base64 = base64::engine::general_purpose::STANDARD.encode(cert);
+        let xml = format!(
+            r#"<KeyInfo xmlns="http://www.w3.org/2000/09/xmldsig#">
+                <X509Data>
+                    <X509Certificate>{cert_base64}</X509Certificate>
+                </X509Data>
+            </KeyInfo>"#
+        );
+        let doc = Document::parse(&xml).unwrap();
+
+        let err = parse_key_info(doc.root_element()).unwrap_err();
+        assert!(matches!(err, ParseError::InvalidStructure(_)));
+    }

As per coding guidelines, "Corruption/validation tests: tamper the relevant field (e.g., digest value, signature bytes, base64 encoding) and assert the error."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/xmldsig/parse.rs` around lines 1370 - 1381, Add a regression test
alongside parse_key_info_rejects_x509_certificate_with_invalid_der that
constructs a valid DER X.509 fixture, base64-encodes it, appends a few extra
bytes to the DER payload (so X509Certificate::from_der returns Ok((_, rest))
with non-empty rest), embeds that string into the same KeyInfo XML, calls
parse_key_info(doc.root_element()), and asserts it returns
Err(ParseError::InvalidStructure(_)); this ensures the parser branch in
parse_key_info that rejects non-empty rest after X509Certificate::from_der
cannot regress silently.


#[test]
fn parse_key_info_marks_unsupported_spki_algorithm_as_unsupported() {
let xml = include_str!(
"../../tests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-x509-crt.xml"
);
let doc = Document::parse(xml).unwrap();
let key_info_node = doc
.descendants()
.find(|node| {
node.is_element()
&& node.tag_name().namespace() == Some(XMLDSIG_NS)
&& node.tag_name().name() == "KeyInfo"
})
.expect("fixture must contain ds:KeyInfo");

let key_info = parse_key_info(key_info_node).expect("KeyInfo parse should succeed");
let x509_info = match &key_info.sources[0] {
KeyInfoSource::X509Data(x509) => x509,
other => panic!("expected X509Data source, got {other:?}"),
};
assert_eq!(x509_info.certificates.len(), 1);
assert_eq!(x509_info.parsed_certificates.len(), 1);
assert!(matches!(
x509_info.parsed_certificates[0].public_key,
X509PublicKeyInfo::Unsupported { .. }
));
}
Comment on lines +1383 to +1409
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Assert the preserved metadata, not just the unsupported marker.

This regression currently passes even if the parser starts dropping subject_dn, issuer_dn, or SKI while still returning X509PublicKeyInfo::Unsupported. Since the feature here is “preserve parsed metadata for unsupported key algorithms,” add assertions that those fields remain populated as well.

Minimal assertion extension
         assert_eq!(x509_info.certificates.len(), 1);
         assert_eq!(x509_info.parsed_certificates.len(), 1);
+        assert!(!x509_info.parsed_certificates[0].subject_dn.is_empty());
+        assert!(!x509_info.parsed_certificates[0].issuer_dn.is_empty());
         assert!(matches!(
             x509_info.parsed_certificates[0].public_key,
             X509PublicKeyInfo::Unsupported { .. }
         ));
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/xmldsig/parse.rs` around lines 1383 - 1409, Update the test
parse_key_info_marks_unsupported_spki_algorithm_as_unsupported to assert the
preserved metadata on the unsupported public key instead of only checking the
Unsupported variant: after locating parsed_certificates[0] (from parse_key_info
/ KeyInfoSource::X509Data), assert that its X509PublicKeyInfo::Unsupported
contains the preserved fields (subject_dn, issuer_dn, and ski) and that they are
Some/contain the expected values (or non-empty) so the parser must retain those
values when returning Unsupported; adjust the match on
parsed_certificates[0].public_key to bind the Unsupported struct and add
assertions for subject_dn, issuer_dn, and ski.


#[test]
fn parse_key_info_accepts_large_textual_x509_entries_within_entry_budget() {
let issuer_name = "C".repeat(MAX_X509_ISSUER_NAME_TEXT_LEN);
Expand Down