rustls / webpki-roots

CA certificates for use with webpki
Apache License 2.0
89 stars 47 forks source link

TUBITAK1_NAME_CONSTRAINTS misencoded? #38

Closed cpu closed 1 year ago

cpu commented 1 year ago

Summary

In https://github.com/rustls/webpki-roots/pull/36 the TUBITAK1_NAME_CONSTRAINTS constant was updated:

https://github.com/rustls/webpki-roots/blob/5bc18e3fcf4299be2b88ca83c594e5eb85edcfc2/tests/codegen.rs#L252-L253

There was some back and forth in #36 about the encoded contents, but looking closer today I think we might have arrived at the wrong encoding.

Right now the name_constraint for this root ends up loaded into the TrustAnchor's name_constraints field as: 0xA0 0x0B 0x30 0x09 0xA0 0x07 0x30 0x05 0x82 0x03 0x2E 0x74 0x72

I think the correct encoding is: 0xA0 0x07 0x30 0x05 0x82 0x03 0x2E 0x74 0x72

Notably this drops the first four bytes (0xA0 0x0B 0x30 0x09) of the existing constraint bytes.

(Side note; I really dislike the encoding format that the lib.rs has where it uses a byte string with ascii range characters represented as-is and the rest hex escaped. I think it would be better to express this as Some(&[<hex literals>]))

Discussion

I arrived at this conclusion in two ways:

1. The new test case isn't using the permitted subtrees.

The test case added in ff68fd9b1b7ef055c008ea53a6aa39b80b37410e doesn't seem to be using the name constraint when making a trust decision.

If you step through execution of the test in a debugger, letting execution get down into the webpki/subject_name/verify.rs's check_presented_id_conforms_to_constraints_in_subtree fn that gets called three times (once per cert in the chain) to perform subtree name constraint validation, you'll find that the base GeneralName read from match general_subtree(&mut constraints) is not a GeneralName::DnsName(".tr".as_bytes()) as you'd expect, but GeneralName::Unsupported.

This happens because when GeneralName::from_der is called to parse the base general name it is being given the DER encoded input 0xA0 0x07 0x30 0x05 0x82 0x03 0x2e 0x74 0x72. This breaks down to a tag of 0xA0 (CONTEXT_SPECIFIC | CONSTRUCTED | 0, aka OTHER_NAME_TAG), a length of 0x07, and the value 0x30 0x05 0x82 0x03 0x2E 0x74 0x72 (aka the nonsense string: "0\x05\x82\x03.tr"). The GeneralName::from_der processing of this content matches the OTHER_NAME_TAG into the GeneralName::Unsupported arm, returning an unsupported general name as the base of the permitted subtree constraint.

Back in check_presented_id_conforms_to_constraints_in_subtree execution continues through match (name, base) where we hit the _ arm because we are never comparing GeneralName::Unsupported names from a certificate in the chain against the GeneralName::Unsupported base from the permitted subtrees.

Ultimately check_presented_id_conforms_to_constraints_in_subtree finds has_permitted_subtrees_mismatch == false and has_permitted_subtrees_match == false and returns NameIteration::KeepGoing. The net result is that validation succeeds, but no permitted subtree base was ever truly examined against a certificate subject name.

It seems obvious that the GeneralName base for the permitted subtrees should be a DnsName, but in case there was any doubt that otherName isn't correct, RFC 5280 4.2.1.10 says:

The syntax and semantics for name constraints for otherName, ediPartyName, and registeredID are not defined by this specification, however, syntax and semantics for name constraints for other name forms may be specified in other documents.

2. PyCA Cryptography generates a different encoding for the same constraint

To help double check my reasoning I wrote a quick and dirty Python script to spit out a CA certificate with a name constraint that I think matches the intent for KamuSM:

ca_cert_builder = ca_cert_builder.add_extension(
    x509.NameConstraints(permitted_subtrees=[x509.DNSName(".tr")], excluded_subtrees=None),
    critical=True,
)

Loading the encoded cert as a TrustAnchor we end up with a name_constraints field of Some(&[0xA0, 0x07, 0x30, 0x05, 0x82, 0x03, 0x2E, 0x74, 0x72]). This matches the last 9 bytes of the current name constraint, but is missing the first four bytes (0xA0 0x0B 0x30 0x09).

Running through the same debugging session as described above for the existing name constraint we can see that when it comes time to read the base of the permitted subtree GeneralName::from_der gets the DER input: 0x82, 0x03, 0x2E, 0x74, 0x72. This breaks down as tag 0x82 (CONTEXT_SPECIFIC | CONSTRUCTED | 2, aka DNS_NAME_TAG), a length of 0x03, and the value 0x2E 0x74 0x72, aka ".tr".

Unlike the previous session, the DNS_NAME_TAG is matched into a GeneralName::DnsName(...), not a GeneralName::Unsupported.

Back in check_presented_id_conforms_to_constraints_in_subtree when we come to match (name, base) we can compare a DnsName from the end entity certificate to the DnsName from the base of the permitted subtree. Unlike the previous session, we actually do a comparison against the base of the permitted subtree using dns_name::presented_id_matches_constraint.