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 Code Signing lints for EKU, Key Usage, RSA Key Size and CRLDistributionPoints #865

Merged
merged 15 commits into from
Jul 21, 2024

Conversation

digirenpeter
Copy link
Contributor

Adding some Code Signing specific lints, will add more if we think this is worth maintaining. Each lint has the specific entry from the CABF CS BRs as a comment at the top so it's easy to find as well as the reference in the certificate lint register.

@defacto64
Copy link
Contributor

I am not clear why the "4 workflows" are "awaiting approval ": do not they usually run automatically on submitting a PR ?

@mcpherrinm
Copy link

I am not clear why the "4 workflows" are "awaiting approval ": do not they usually run automatically on submitting a PR ?

New contributers to a project on Github need to have somebody approve the workflows to run. This is a measure from Github to prevent using pull requests to run cryptominers or otherwise abuse public github actions. I clicked the button.

Copy link
Member

@christopher-henderson christopher-henderson left a comment

Choose a reason for hiding this comment

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

Thank you for your patience. On top of the 4th of July holiday I have been in the process of moving. So July is going to be slow for me on here.

I think that this a great addition to ZLint! I only had some code nits as well as a question about the general applicability rules that we can expect for this class of lint.

v3/lints/cabf_cs_br/lint_cs_crl_distribution_points.go Outdated Show resolved Hide resolved

func (l *crlDistributionPoints) Execute(c *x509.Certificate) *lint.LintResult {
cdp := util.GetExtFromCert(c, util.CrlDistOID)
if cdp == nil || cdp.Critical {

Choose a reason for hiding this comment

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

Would you mind splitting this branch into two so that the returned error message has no ambiguities? Doing so helps reduce the time an operator has to spend figuring which is actually the issue.

	if cdp == nil {
		return &lint.LintResult{
			Status:  lint.Error,
			Details: "The cRLDistributionPoints extension MUST be present."}
	}
        if cdp.Critical {
		return &lint.LintResult{
			Status:  lint.Error,
			Details: "The cRLDistributionPoints MUST NOT be marked critical."}
        }

v3/lints/cabf_cs_br/lint_cs_eku_required.go Outdated Show resolved Hide resolved

func (l *csKeyUsageRequired) Execute(c *x509.Certificate) *lint.LintResult {
ku := util.GetExtFromCert(c, util.KeyUsageOID)
if ku == nil || !ku.Critical {

Choose a reason for hiding this comment

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

	if ku == nil {
		return &lint.LintResult{
			Status:  lint.Error,
			Details: "Key usage extension MUST be present.",
		}
	}
	if !ku.Critical {
		return &lint.LintResult{
			Status:  lint.Error,
			Details: "Key usage extension MUST be marked critical",
		}
	}

v3/lints/cabf_cs_br/lint_cs_key_usage_required.go Outdated Show resolved Hide resolved
lint.RegisterCertificateLint(&lint.CertificateLint{
LintMetadata: lint.LintMetadata{
Name: "e_cs_rsa_key_size",
Description: "If the Key is RSA, then the modulus MUST be at least 3072 bits in length",

Choose a reason for hiding this comment

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

I agree that each key type is likely deserving of their own lint. Requirements on specific algorithms are usually best left separated.

}

func (l *crlDistributionPoints) CheckApplies(c *x509.Certificate) bool {
return util.IsCodeSigning(c.PolicyIdentifiers) && util.IsSubscriberCert(c)

Choose a reason for hiding this comment

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

Based on the preamble to the document...

The scope of these Requirements includes all "Code Signing Certificates", as defined below, and associated Timestamp Authorities, and all Certification Authorities technically capable of issuing Code Signing Certificates, including any Root CA that is publicly trusted for code signing and all other CAs that might serve to complete the validation path to such Root CA.

...I believe that we can safely add the following assumption to base.go.

	if l.Source == CABFCSBaselineRequirements && !util.IsCodeSigning(cert) {
		return &LintResult{Status: NA}
	}

Additionally, I am somewhat indecisive about the IsSubscriber clause that we have attached to these specific lints. It's unfortunate, but the document seems to have a mix of explicitly stating when a set of requirements is target specifically subscriber certs as well as implicit. That is, lints such as these seem like they would naturally apply to subscriber certs, but it's not explicitly spelled out like I am seeing in some other sections...unless I am simply not seeing it.

Copy link
Contributor Author

@digirenpeter digirenpeter Jul 13, 2024

Choose a reason for hiding this comment

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

Looking at the CABF CS BR 7.1.2, these lints are targeted at the Subscriber Certificate profile, there is some overlap with the Sub CA profile, not so much with the root CA profile.

e_cs_crl_distribution_points
Applies to: Both Subscriber and Subordinate CA certificates. (7.1.2.2b and 7.1.2.3b)
Root CA: Not explicitly required.

e_cs_eku_required
Applies to: Both Subscriber and Subordinate CA certificates.
Root CA: EKUs MUST NOT be present. (7.1.2.1d)
Subordinate CA: Also prohibits id-kp-emailProtection. (7.1.2.2f)

e_cs_key_usage_required
Applies to: Specifically to Subscriber certificates.
Subordinate CA and Root CA: Both require cRLSign and keyCertSign, whereas Code Signing Certificates prohibits both. 

e_cs_rsa_key_size
Subscriber Certificates: RSA key size MUST be at least 3072 bits.
Root and Subordinate CA Certificates: RSA key size MUST be at least 4096 bits.

So we could remove the IsSubscriber check, I would need to update the lints to accommodate the other profiles. But we should keep the IsSubscriber for the key_usage check, since it's basically opposite to the CAs.

v3/lints/cabf_cs_br/lint_cs_crl_distribution_points.go Outdated Show resolved Hide resolved
v3/lints/cabf_cs_br/lint_cs_eku_required.go Outdated Show resolved Hide resolved
v3/lints/cabf_cs_br/lint_cs_key_usage_required.go Outdated Show resolved Hide resolved
v3/lints/cabf_cs_br/lint_cs_rsa_key_size.go Outdated Show resolved Hide resolved
digirenpeter and others added 5 commits July 20, 2024 11:24
Co-authored-by: Christopher Henderson <chris@chenderson.org>
Co-authored-by: Christopher Henderson <chris@chenderson.org>
Co-authored-by: Christopher Henderson <chris@chenderson.org>
Co-authored-by: Christopher Henderson <chris@chenderson.org>
@christopher-henderson christopher-henderson merged commit 33ee62a into zmap:master Jul 21, 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.

4 participants