Repository navigation
reject empty subjectAltName and issuerAltName sequences - #15472
Conversation
| // RFC 5280, so an empty sequence is malformed. The path validation engine | ||
| // represents a certificate with no subjectAltName using its own empty | ||
| // sequence and does not call this. | ||
| if gn_seq.clone().next().is_none() { |
There was a problem hiding this comment.
SequenceOf takes a MINIMUM_LEN, is there a reason not to just set that to 1?
There was a problem hiding this comment.
i looked at that first, but the SubjectAlternativeName and IssuerAlternativeName aliases are shared with the path validation engine, which stands in for a certificate with no SAN by parsing an empty sequence into SubjectAlternativeName (the \x30\x00 parse in cryptography-x509-verification/src/lib.rs). setting MINIMUM_LEN to 1 on the alias would make that parse fail. keeping the check in parse_general_names also means the other GeneralNames users that go through it, authorityCertIssuer and the CRL distribution point names, pick up the same 1..MAX constraint, since 5280 defines them all the same way. happy to move it to a type-level constraint if you'd rather give verification its own zero-minimum type.
There was a problem hiding this comment.
Oh, I think we should probably fix the \x30\x00 callsites to just store an Option<SequenceOf<>>
There was a problem hiding this comment.
done. set MINIMUM_LEN to 1 on both aliases and made the verification NameChain store an Option instead of parsing the empty \x30\x00 sequence, so the empty rejection now comes from the type. parse_general_names is generic over the minimum length now so the len-0 GeneralNames callers still go through unchanged. rust builds clean.
|
@mhils it looks like mitmproxy's tests rely on SANs that are 0 length, even though they're not permitted by the RFC. Is that intentional, or inadveratnat? |
|
makes sense to settle that before this lands. if the mitmproxy breakage means a hard reject at parse time is too disruptive, i'm happy to adjust the approach once you and mhils have a view on it. |
|
Apologies for missing this initially. This looks inadveratnat, I'll fix it. |
|
CI should be fixed! |
|
Awesome, thanks @mhils ! |
a0ecb80 to
bbf0748
Compare
|
thanks for sorting the mitmproxy side. i've rebased this on main to clear the changelog conflict, so it should now run clean against the fixed downstream tests. no code changes from the earlier review, just the rebase. |
|
Looks like there's some spelling issues in the docs :-) |
|
good catch, that was the spelling job tripping on the bare camelCase names. wrapped both in literals so the checker skips them, same as the changelog entry already does. |
|
Thanks. |
3 participants
RFC 5280 defines subjectAltName and issuerAltName as a GeneralNames value, which is a SEQUENCE of one or more GeneralName, but the parser accepted an empty sequence, so a malformed certificate or CRL carrying a zero-entry SAN or IAN parsed to an empty extension rather than being rejected. The check sits in parse_general_names rather than on the GeneralNames type alias because the path validation engine reuses that alias and relies on an empty sequence to stand in for a certificate with no SAN. This rejects an empty sequence when the extension is parsed, matching the constraint and the behaviour already in place for an empty extendedKeyUsage.