rusticata / x509-parser

X.509 parser written in pure Rust. Fast, zero-copy, safe.
Other
206 stars 67 forks source link

cert: UniqueIdentifier explicit -> implicit parse. #145

Closed cpu closed 8 months ago

cpu commented 1 year ago

Description

The issuerUniqueId and subjectUniqueId fields of a TBSCertificate are being parsed as an explicitly tagged bit string when RFC 5280 describes it as implicit:

issuerUniqueID  [1]  IMPLICIT UniqueIdentifier OPTIONAL,
                     -- If present, version MUST be v2 or v3
subjectUniqueID [2]  IMPLICIT UniqueIdentifier OPTIONAL,
                     -- If present, version MUST be v2 or v3

Because of this mismatch the real world examples I've found of certificates with these fields present fail to parse using x509-parser, with the error InvalidIssuerUID. I've included one that I found using Censys as a test asset.

I suspect this bug hasn't bubbled up before because RFC5280 forbids CAs from issuing certs with these fields when adhering to the defined profile, making their use rare.

Fix

This commit fixes parse to expect implicit tagging and adds a unit test parsing the exemplar certificate to ensure a certificate with subject/issuer UIDs can be parsed successfully.

Testing

Running the unit test without the fix fails: ``` running 1 test test test_tbscert_unique_identifiers ... FAILED failures: ---- test_tbscert_unique_identifiers stdout ---- thread 'test_tbscert_unique_identifiers' panicked at 'parsing failed: Error(InvalidIssuerUID)', tests/readcert.rs:366:50 stack backtrace: 0: rust_begin_unwind 1: core::panicking::panic_fmt 2: core::result::unwrap_failed 3: core::result::Result::expect at /build/rustc-1.70.0-src/library/core/src/result.rs:1046:23 4: readcert::test_tbscert_unique_identifiers at ./tests/readcert.rs:366:21 5: readcert::test_tbscert_unique_identifiers::{{closure}} at ./tests/readcert.rs:364:38 6: core::ops::function::FnOnce::call_once at /build/rustc-1.70.0-src/library/core/src/ops/function.rs:250:5 ```
With the fix, it passes: ``` test test_tbscert_unique_identifiers ... ok ``` ``` ❯ openssl x509 -inform der -in assets/unique_ids.der -noout -text | grep "Unique ID" -A1 Issuer Unique ID: 30:16:80:14:c5:78:84:b8:0c:6e:8c:4c:ce:b9:94:6f:98:fc: f3:8a:54:b1:80:e0 Subject Unique ID: 04:14:df:13:ac:69:14:90:62:db:3d:e9:b4:56:e6:a6:90:26: bf:2c:ef:81 ```
cpu commented 1 year ago

@chifflier Gentle bump - would love to see this small fix land in-tree.

chifflier commented 8 months ago

Hi, Good catch, thanks! Indeed, these fields are IMPLICIT. Since they are only seldomly encountered, it's not really surprising the bug was not triggered before. Thank you for the detailed report and the fix.