Skip to content
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.

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

Add lint to check that SubCA certificates do not have illegal values in their EKU extension #840

Merged
merged 37 commits into from
May 26, 2024

Conversation

defacto64
Copy link
Contributor

This lint verifies that Subordinate CA certificates do not have illegal values in their EKU extension according to the CABF BRs, and generates an error in case they do. This kind of problem has happened a few times in the past, as can be seen on https://bugzilla.mozilla.org, but apparently not in recent times; at any rate, it's better to prevent it.

This lint does not discriminate between normal SubCA certificates and cross-certificates, this being impossible in the absence of an extra input (which could be passed through a specific lint-configuration, but I find it preferable to do without it). However, this is not necessary in order to do a few essential checks that were missing until now.

The lint's logic is as follows: in the CABF BR context, if the anyExtendedKeyUsage is present in a SubCA cert, then it can be inferred that it's actually an unrestricted cross certificate; if instead anyExtendedKeyUsage is not present, then it must be either a restricted cross certificate or a normal SubCA (non cross), and in both these cases the same rules apply to the EKU extension.

defacto64 and others added 30 commits March 8, 2024 16:07
Added //nolint:all to comment block to avoid golangci-lint to complain about duplicate words in comment
Fine to me.

Co-authored-by: Christopher Henderson <chris@chenderson.org>
As per Chris Henderson's suggestion, to "improve readability".
As per Chris Henderson's suggestion.
Added CABFEV_Sec9_2_8_Date
if eku == x509.ExtKeyUsageEmailProtection ||
eku == x509.ExtKeyUsageCodeSigning ||
eku == x509.ExtKeyUsageTimeStamping ||
eku == x509.ExtKeyUsageOcspSigning {
Copy link
Member

Choose a reason for hiding this comment

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

I believe that we are referring to sub-section g?

For Subordinate CA Certificates that will be used to issue TLS certificates, the value
id-kp-serverAuth [RFC5280] MUST be present. The value id-kp-clientAuth
[RFC5280] MAY be present. The values id-kp-emailProtection [RFC5280], id-kpcodeSigning [RFC5280], id-kp-timeStamping [RFC5280], and anyExtendedKeyUsage
[RFC5280] MUST NOT be present. Other values SHOULD NOT be present.

For Subordinate CA Certificates that are not used to issue TLS certificates, then the
value id-kp-serverAuth [RFC5280] MUST NOT be present. Other values MAY be
present, but SHOULD NOT combine multiple independent usages (e.g. including idkp-timeStamping [RFC5280] with id-kp-codeSigning [RFC5280]).

So unless my eyes are missing it, I think that since OCSP Signing is not explicitly enumerated that it, so would fall under the Other values SHOULD NOT be present clause, thus making it a warning?

Copy link
Contributor

Choose a reason for hiding this comment

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

So unless my eyes are missing it, I think that since OCSP Signing is not explicitly enumerated that it, so would fall under the Other values SHOULD NOT be present clause, thus making it a warning?

This list matches 7.1.2.10.6 CA Certificate Extended Key Usage.

Choose a reason for hiding this comment

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

@mathewhodson you are correct, it is enumerated in latest. This is where it gets mildly tricky since this lint is marked as being effective as of August 2020 via CABF/BR 1.7.1 where I don't think I see it marked as a MUST NOT.

@defacto64 did you have a particular reason for choosing 1.7.1?

v3/lints/cabf_br/lint_ca_invalid_eku.go Show resolved Hide resolved
@christopher-henderson christopher-henderson merged commit c8164d8 into zmap:master May 26, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants