Skip to content

fix(cert): NotBefore=0 collides with CHIP no-expiry sentinel #454

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
glennswest wants to merge 1 commit into from
+96 −12
Closed

fix(cert): NotBefore=0 collides with CHIP no-expiry sentinel #454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
57 changes: 51 additions & 6 deletions rs-matter/src/cert/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,20 +263,23 @@ impl<'a> CertBuilderCore<'a> {
subject_key_id: &KeyId,
authority_key_id: &KeyId,
) -> Result<(), Error> {
// 1. Basic Constraints
// 1. Basic Constraints — per Matter Spec §6.5.11.1:
// RCAC: cA = TRUE, pathLenConstraint SHALL NOT be present
// ICAC: cA = TRUE, pathLenConstraint = 0
// NOC: cA = FALSE, pathLenConstraint SHALL NOT be present
tw.start_struct(&TLVTag::Context(1))?;
match cert_type {
CertType::Rcac => {
tw.bool(&TLVTag::Context(1), true)?; // is_ca = true
tw.u8(&TLVTag::Context(2), 1)?; // path_len = 1
tw.bool(&TLVTag::Context(1), true)?;
// No path_len for RCAC (was incorrectly set to 1 prior;
// CHIP would accept it but it's a spec violation).
}
CertType::Icac => {
tw.bool(&TLVTag::Context(1), true)?; // is_ca = true
tw.bool(&TLVTag::Context(1), true)?;
tw.u8(&TLVTag::Context(2), 0)?; // path_len = 0
}
CertType::Noc => {
tw.bool(&TLVTag::Context(1), false)?; // is_ca = false
// No path_len for end entity
tw.bool(&TLVTag::Context(1), false)?;
}
}
tw.end_container()?;
Expand Down Expand Up @@ -659,6 +662,48 @@ mod tests {
assert!(len < MAX_CERT_TLV_LEN);
}

/// Round-trip: build an RCAC, then re-parse and self-verify the
/// signature using rs-matter's own `CertVerifier`. If this passes,
/// the TLV→ASN.1→hash→verify pipeline is internally consistent.
/// If it fails, our signing flow is broken regardless of whether
/// CHIP can parse it.
#[test]
fn test_rcac_self_verify() {
let crypto = test_only_crypto();
let rcac_secret_key = unwrap!(crypto.generate_secret_key());
let rcac_pubkey = rcac_secret_key.pub_key().unwrap();

let subject = SubjectDN {
node_id: None,
fabric_id: Some(0x0000_0000_0000_0001),
cat_ids: &[],
ca_id: Some(0x1122_3344_5566_7788),
};
let validity = Validity {
not_before: 0,
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 18, 2026

Choose a reason for hiding this comment

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

medium

To align with the main fix in this PR and avoid using a value known to cause interoperability issues, it's better to use 1 for not_before here instead of 0. This ensures the test validates the corrected behavior and serves as a better example for future use.

Suggested change
not_before: 0,
not_before: 1,

not_after: 0,
};
let mut cert_buf = [0u8; MAX_CERT_TLV_AND_ASN1_LEN];
let mut builder = RcacBuilder::new(&mut cert_buf);
let len = unwrap!(builder.build(
&crypto,
subject,
validity,
&rcac_pubkey,
&rcac_secret_key,
&[0x01],
));

// Re-parse the just-built RCAC and self-verify.
let cert = CertRef::new(crate::tlv::TLVElement::new(&cert_buf[..len]));
let mut scratch = [0u8; MAX_CERT_TLV_AND_ASN1_LEN];
let res = cert.verify_chain_start(&crypto).finalise(&mut scratch);
assert!(
res.is_ok(),
"RCAC built by RcacBuilder failed self-verification: {res:?}"
);
}

/// Test building an ICAC signed by RCAC
#[test]
fn test_build_icac() {
Expand Down
51 changes: 45 additions & 6 deletions rs-matter/src/commissioner/noc_generator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,22 @@ impl NocGenerator {
ca_id: Some(rcac_id),
};

// NotBefore MUST NOT be 0 (Matter epoch start, 2000-01-01).
// CHIP's ChipEpochToASN1Time treats epoch=0 as the "no
// well-defined expiration date" sentinel and re-emits it as
// GeneralizedTime "99991231235959Z" regardless of which field
// it appears in (see CHIPCert.cpp:1076-1106 and the
// explanatory comment about CHIP epoch 0 NotBefore producing
// an invalid TBS signature on round-trip).
//
// We sign over UTCTime "000101000000Z" (Matter epoch); CHIP
// would reconstruct GeneralizedTime "99991231235959Z" and the
// hash would mismatch. Using 1 second past the Matter epoch
// avoids the sentinel collision while keeping the cert
// effectively unbounded on the lower end.
let validity = crate::cert::builder::Validity {
not_before: 0, // epoch start
not_after: 0, // no expiry
not_before: 1, // 2000-01-01 00:00:01 — past CHIP's epoch=0 sentinel
not_after: 0, // no expiry (NotAfter sentinel is legitimate)
};
Comment on lines +126 to 142
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 18, 2026

Choose a reason for hiding this comment

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

medium

This block of code (comment and validity initialization) is duplicated in three places within this file (new, generate_icac, and generate_noc). To improve maintainability and reduce duplication, consider defining a module-level constant for the Validity struct.

For example, you could add this before impl NocGenerator:

// NotBefore MUST NOT be 0 (Matter epoch start, 2000-01-01).
// CHIP's ChipEpochToASN1Time treats epoch=0 as the "no
// well-defined expiration date" sentinel and re-emits it as
// GeneralizedTime "99991231235959Z" regardless of which field
// it appears in (see CHIPCert.cpp:1076-1106 and the
// explanatory comment about CHIP epoch 0 NotBefore producing
// an invalid TBS signature on round-trip).
//
// We sign over UTCTime "000101000000Z" (Matter epoch); CHIP
// would reconstruct GeneralizedTime "99991231235959Z" and the
// hash would mismatch.  Using 1 second past the Matter epoch
// avoids the sentinel collision while keeping the cert
// effectively unbounded on the lower end.
const DEFAULT_VALIDITY: crate::cert::builder::Validity = crate::cert::builder::Validity {
    not_before: 1, // 2000-01-01 00:00:01 — past CHIP's epoch=0 sentinel
    not_after: 0,  // no expiry (NotAfter sentinel is legitimate)
};

Then, you can replace this block and the other two occurrences with a single line.

Suggested change
// NotBefore MUST NOT be 0 (Matter epoch start, 2000-01-01).
// CHIP's ChipEpochToASN1Time treats epoch=0 as the "no
// well-defined expiration date" sentinel and re-emits it as
// GeneralizedTime "99991231235959Z" regardless of which field
// it appears in (see CHIPCert.cpp:1076-1106 and the
// explanatory comment about CHIP epoch 0 NotBefore producing
// an invalid TBS signature on round-trip).
//
// We sign over UTCTime "000101000000Z" (Matter epoch); CHIP
// would reconstruct GeneralizedTime "99991231235959Z" and the
// hash would mismatch. Using 1 second past the Matter epoch
// avoids the sentinel collision while keeping the cert
// effectively unbounded on the lower end.
let validity = crate::cert::builder::Validity {
not_before: 0, // epoch start
not_after: 0, // no expiry
not_before: 1, // 2000-01-01 00:00:01 — past CHIP's epoch=0 sentinel
not_after: 0, // no expiry (NotAfter sentinel is legitimate)
};
let validity = DEFAULT_VALIDITY;


let cert_len = RcacBuilder::new(&mut cert_buf).build(
Expand Down Expand Up @@ -234,9 +247,22 @@ impl NocGenerator {
ca_id: Some(icac_id),
};

// NotBefore MUST NOT be 0 (Matter epoch start, 2000-01-01).
// CHIP's ChipEpochToASN1Time treats epoch=0 as the "no
// well-defined expiration date" sentinel and re-emits it as
// GeneralizedTime "99991231235959Z" regardless of which field
// it appears in (see CHIPCert.cpp:1076-1106 and the
// explanatory comment about CHIP epoch 0 NotBefore producing
// an invalid TBS signature on round-trip).
//
// We sign over UTCTime "000101000000Z" (Matter epoch); CHIP
// would reconstruct GeneralizedTime "99991231235959Z" and the
// hash would mismatch. Using 1 second past the Matter epoch
// avoids the sentinel collision while keeping the cert
// effectively unbounded on the lower end.
let validity = crate::cert::builder::Validity {
not_before: 0, // epoch start
not_after: 0, // no expiry
not_before: 1, // 2000-01-01 00:00:01 — past CHIP's epoch=0 sentinel
not_after: 0, // no expiry (NotAfter sentinel is legitimate)
};

let issuer = crate::cert::builder::IssuerDN {
Expand Down Expand Up @@ -330,9 +356,22 @@ impl NocGenerator {
ca_id: None,
};

// NotBefore MUST NOT be 0 (Matter epoch start, 2000-01-01).
// CHIP's ChipEpochToASN1Time treats epoch=0 as the "no
// well-defined expiration date" sentinel and re-emits it as
// GeneralizedTime "99991231235959Z" regardless of which field
// it appears in (see CHIPCert.cpp:1076-1106 and the
// explanatory comment about CHIP epoch 0 NotBefore producing
// an invalid TBS signature on round-trip).
//
// We sign over UTCTime "000101000000Z" (Matter epoch); CHIP
// would reconstruct GeneralizedTime "99991231235959Z" and the
// hash would mismatch. Using 1 second past the Matter epoch
// avoids the sentinel collision while keeping the cert
// effectively unbounded on the lower end.
let validity = crate::cert::builder::Validity {
not_before: 0, // epoch start
not_after: 0, // no expiry
not_before: 1, // 2000-01-01 00:00:01 — past CHIP's epoch=0 sentinel
not_after: 0, // no expiry (NotAfter sentinel is legitimate)
};

let cert_len = NocBuilder::new(&mut cert_buf).build(
Expand Down
Loading