-
Notifications
You must be signed in to change notification settings - Fork 110
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
christopher-henderson
merged 15 commits into
zmap:master
from
digirenpeter:add_code_signing_lints
Jul 21, 2024
Merged
Changes from 9 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
feb35ad
Added lints for code signing
digirenpeter c8a379e
Added test data and code signing util function
digirenpeter bf12818
Removed ev cs oid from isev check, at least 1 ev lint doesn't apply p…
digirenpeter 923489a
Merge branch 'master' into add_code_signing_lints
christopher-henderson 59cb9fa
Update v3/lints/cabf_cs_br/lint_cs_crl_distribution_points.go
digirenpeter 8ae256d
Update v3/lints/cabf_cs_br/lint_cs_eku_required.go
digirenpeter f79e81d
Update v3/lints/cabf_cs_br/lint_cs_key_usage_required.go
digirenpeter 5b0fdea
Split up ku/cdp nil and critical checks, added cs check to base.go
digirenpeter 23c29ba
Merge branch 'master' into add_code_signing_lints
christopher-henderson bd3e6b8
Update v3/lints/cabf_cs_br/lint_cs_crl_distribution_points.go
digirenpeter 083a019
Update v3/lints/cabf_cs_br/lint_cs_eku_required.go
digirenpeter c3e7540
Update v3/lints/cabf_cs_br/lint_cs_key_usage_required.go
digirenpeter 3cc9fc1
Update v3/lints/cabf_cs_br/lint_cs_rsa_key_size.go
digirenpeter 54e5c90
Added conditional prohibited EKU for subca
digirenpeter 7e7288a
Merge branch 'master' into add_code_signing_lints
digirenpeter File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
package cabf_cs_br | ||
|
||
import ( | ||
"strings" | ||
|
||
"github.com/zmap/zcrypto/x509" | ||
"github.com/zmap/zlint/v3/lint" | ||
"github.com/zmap/zlint/v3/util" | ||
) | ||
|
||
/*7.1.2.3 b. cRLDistributionPoints | ||
This extension MUST be present. It MUST NOT be marked critical, and it MUST contain the | ||
HTTP URL of the CA’s CRL service*/ | ||
|
||
func init() { | ||
lint.RegisterCertificateLint(&lint.CertificateLint{ | ||
LintMetadata: lint.LintMetadata{ | ||
Name: "e_cs_crl_distribution_points", | ||
Description: "This extension MUST be present. It MUST NOT be marked critical. It MUST contain the HTTP URL of the CA's CRL service", | ||
Citation: "CABF CS BRs 7.1.2.3.b", | ||
Source: lint.CABFCSBaselineRequirements, | ||
EffectiveDate: util.CABF_CS_BRs_1_2_Date, | ||
}, | ||
Lint: NewCrlDistributionPoints, | ||
}) | ||
} | ||
|
||
type crlDistributionPoints struct{} | ||
|
||
func NewCrlDistributionPoints() lint.LintInterface { | ||
return &crlDistributionPoints{} | ||
} | ||
|
||
func (l *crlDistributionPoints) CheckApplies(c *x509.Certificate) bool { | ||
return util.IsCodeSigning(c.PolicyIdentifiers) && util.IsSubscriberCert(c) | ||
digirenpeter marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
func (l *crlDistributionPoints) Execute(c *x509.Certificate) *lint.LintResult { | ||
cdp := util.GetExtFromCert(c, util.CrlDistOID) | ||
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."} | ||
} | ||
|
||
// MUST contain the HTTP URL of the CA’s CRL service | ||
for _, uri := range c.CRLDistributionPoints { | ||
if !strings.HasPrefix(uri, "http://") { | ||
return &lint.LintResult{Status: lint.Error, Details: "cRLDistributionPoints MUST contain the HTTP URL of the CA's CRL service"} | ||
} | ||
} | ||
|
||
return &lint.LintResult{ | ||
Status: lint.Pass, | ||
} | ||
} |
40 changes: 40 additions & 0 deletions
40
v3/lints/cabf_cs_br/lint_cs_crl_distribution_points_test.go
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
package cabf_cs_br | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/zmap/zlint/v3/lint" | ||
"github.com/zmap/zlint/v3/test" | ||
) | ||
|
||
func TestCsCrlDistributionPoints(t *testing.T) { | ||
testCases := []struct { | ||
Name string | ||
InputFilename string | ||
ExpectedResult lint.LintStatus | ||
}{ | ||
{ | ||
Name: "pass - code signing certificate with CRLDistributionPoints", | ||
InputFilename: "code_signing/validCodeSigningCertificate.pem", | ||
ExpectedResult: lint.Pass, | ||
}, | ||
{ | ||
Name: "fail - code signing certificate without CRLDistributionPoints", | ||
InputFilename: "code_signing/noCrldpIncluded.pem", | ||
ExpectedResult: lint.Error, | ||
}, | ||
{ | ||
Name: "fail - code signing certificate with CRLDistributionPoints without http", | ||
InputFilename: "code_signing/crlDpNoHttp.pem", | ||
ExpectedResult: lint.Error, | ||
}, | ||
} | ||
for _, tc := range testCases { | ||
t.Run(tc.Name, func(t *testing.T) { | ||
result := test.TestLint("e_cs_crl_distribution_points", tc.InputFilename) | ||
if result.Status != tc.ExpectedResult { | ||
t.Errorf("expected result %v was %v - details: %v", tc.ExpectedResult, result.Status, result.Details) | ||
} | ||
}) | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
package cabf_cs_br | ||
|
||
import ( | ||
"fmt" | ||
|
||
"github.com/zmap/zcrypto/x509" | ||
|
||
"github.com/zmap/zlint/v3/lint" | ||
"github.com/zmap/zlint/v3/util" | ||
) | ||
|
||
/* 7.1.2.3 Code signing and Timestamp Certificate | ||
f. extKeyUsage | ||
If the Certificate is a Code Signing Certificate, then id-kp-codeSigning MUST be present | ||
and the following EKUs MAY be present: | ||
• Lifetime Signing OID (1.3.6.1.4.1.311.10.3.13) | ||
• id-kp-emailProtection | ||
• Document Signing (1.3.6.1.4.1.311.3.10.3.12) | ||
|
||
If the Certificate is a Timestamp Certificate, then id-kp-timeStamping MUST be present | ||
and MUST be marked critical. | ||
Additionally, the following EKUs MUST NOT be present: | ||
• anyExtendedKeyUsage | ||
• id-kp-serverAuth | ||
|
||
Other values SHOULD NOT be present. If any other value is present, the CA MUST have a | ||
business agreement with a Platform vendor requiring that EKU in order to issue a | ||
Platform‐specific code signing certificate with that EKU. | ||
*/ | ||
|
||
func init() { | ||
lint.RegisterCertificateLint(&lint.CertificateLint{ | ||
LintMetadata: lint.LintMetadata{ | ||
Name: "e_cs_eku_required", | ||
Description: "If the Certificate is a Code Signing Certificate, then id-kp-codeSigning MUST be present. anyExtendedKeyUsage and id-kp-serverAuth MUST NOT be present.", | ||
Citation: "CABF CS BRs 7.1.2.3.f", | ||
Source: lint.CABFCSBaselineRequirements, | ||
EffectiveDate: util.CABF_CS_BRs_1_2_Date, | ||
}, | ||
Lint: NewCsEKURequired, | ||
}) | ||
} | ||
|
||
type csEKURequired struct{} | ||
|
||
func NewCsEKURequired() lint.LintInterface { | ||
return &csEKURequired{} | ||
} | ||
|
||
func (l *csEKURequired) CheckApplies(c *x509.Certificate) bool { | ||
return util.IsCodeSigning(c.PolicyIdentifiers) && util.IsSubscriberCert(c) | ||
digirenpeter marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
func (l *csEKURequired) Execute(c *x509.Certificate) *lint.LintResult { | ||
prohibitedEKUs := map[x509.ExtKeyUsage]struct{}{ | ||
x509.ExtKeyUsageAny: {}, | ||
x509.ExtKeyUsageServerAuth: {}, | ||
} | ||
|
||
hasCodeSigningEKU := false | ||
|
||
for _, eku := range c.ExtKeyUsage { | ||
if eku == x509.ExtKeyUsageCodeSigning { | ||
hasCodeSigningEKU = true | ||
} | ||
|
||
if _, isProhibited := prohibitedEKUs[eku]; isProhibited { | ||
return &lint.LintResult{ | ||
Status: lint.Error, | ||
Details: fmt.Sprintf("Code Signing certificate includes prohibited EKU: %v", eku), | ||
} | ||
} | ||
} | ||
|
||
if !hasCodeSigningEKU { | ||
return &lint.LintResult{ | ||
Status: lint.Error, | ||
Details: "Code Signing certificate missing required Code Signing EKU", | ||
} | ||
} | ||
|
||
return &lint.LintResult{Status: lint.Pass} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
package cabf_cs_br | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/zmap/zlint/v3/lint" | ||
"github.com/zmap/zlint/v3/test" | ||
) | ||
|
||
func TestCsEKUCheck(t *testing.T) { | ||
testCases := []struct { | ||
Name string | ||
InputFilename string | ||
ExpectedResult lint.LintStatus | ||
}{ | ||
{ | ||
Name: "pass - valid code signing certificate with required EKU", | ||
InputFilename: "code_signing/validCodeSigningCertificate.pem", | ||
ExpectedResult: lint.Pass, | ||
}, | ||
{ | ||
Name: "fail - code signing certificate without required EKU", | ||
InputFilename: "code_signing/noRequiredCodeSigningEKU.pem", | ||
ExpectedResult: lint.Error, | ||
}, | ||
{ | ||
Name: "fail - code signing certificate with prohibited EKU", | ||
InputFilename: "code_signing/containsProhibitedEKU.pem", | ||
ExpectedResult: lint.Error, | ||
}, | ||
} | ||
for _, tc := range testCases { | ||
t.Run(tc.Name, func(t *testing.T) { | ||
result := test.TestLint("e_cs_eku_required", tc.InputFilename) | ||
if result.Status != tc.ExpectedResult { | ||
t.Errorf("expected result %v was %v - details: %v", tc.ExpectedResult, result.Status, result.Details) | ||
} | ||
}) | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
package cabf_cs_br | ||
|
||
import ( | ||
"github.com/zmap/zcrypto/x509" | ||
|
||
"github.com/zmap/zlint/v3/lint" | ||
"github.com/zmap/zlint/v3/util" | ||
) | ||
|
||
/* 7.1.2.3 Code signing and Timestamp Certificate | ||
e. keyUsage | ||
This extension MUST be present and MUST be marked critical. | ||
The bit position for digitalSignature MUST be set. Bit positions for keyCertSign and | ||
cRLSign MUST NOT be set. All other bit positions SHOULD NOT be set. | ||
*/ | ||
|
||
func init() { | ||
lint.RegisterCertificateLint(&lint.CertificateLint{ | ||
LintMetadata: lint.LintMetadata{ | ||
Name: "e_cs_key_usage_required", | ||
Description: "This extension MUST be present and MUST be marked critical. The bit position for digitalSignature MUST be set. The bit positions for keyCertSign and cRLSign MUST NOT be set. All other bit positions SHOULD NOT be set.", | ||
Citation: "CABF CS BRs 7.1.2.3e", | ||
Source: lint.CABFCSBaselineRequirements, | ||
EffectiveDate: util.CABF_CS_BRs_1_2_Date, | ||
}, | ||
Lint: NewCsKeyUsageRequired, | ||
}) | ||
} | ||
|
||
type csKeyUsageRequired struct{} | ||
|
||
func NewCsKeyUsageRequired() lint.LintInterface { | ||
return &csKeyUsageRequired{} | ||
} | ||
|
||
func (l *csKeyUsageRequired) CheckApplies(c *x509.Certificate) bool { | ||
return util.IsCodeSigning(c.PolicyIdentifiers) && util.IsSubscriberCert(c) | ||
digirenpeter marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
func (l *csKeyUsageRequired) Execute(c *x509.Certificate) *lint.LintResult { | ||
ku := util.GetExtFromCert(c, util.KeyUsageOID) | ||
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", | ||
} | ||
} | ||
|
||
if (c.KeyUsage & x509.KeyUsageDigitalSignature) == 0 { | ||
return &lint.LintResult{ | ||
Status: lint.Error, | ||
Details: "Code Signing certificate must have digitalSignature key usage", | ||
} | ||
} | ||
|
||
// keyCertSign and cRLSign bits MUST NOT be set. | ||
if (c.KeyUsage & (x509.KeyUsageCertSign | x509.KeyUsageCRLSign)) != 0 { | ||
return &lint.LintResult{ | ||
Status: lint.Error, | ||
Details: "keyCertSign and cRLSign key usages MUST NOT be set", | ||
} | ||
} | ||
|
||
// All other bit positions SHOULD NOT be set. | ||
if c.KeyUsage & ^x509.KeyUsageDigitalSignature != 0 { | ||
return &lint.LintResult{ | ||
Status: lint.Warn, | ||
Details: "Only digitalSignature key usage is recommended. Other key usages SHOULD NOT be set."} | ||
} | ||
|
||
return &lint.LintResult{Status: lint.Pass} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
package cabf_cs_br | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/zmap/zlint/v3/lint" | ||
"github.com/zmap/zlint/v3/test" | ||
) | ||
|
||
func TestCsKeyUsageCheck(t *testing.T) { | ||
testCases := []struct { | ||
Name string | ||
InputFilename string | ||
ExpectedResult lint.LintStatus | ||
}{ | ||
{ | ||
Name: "pass - valid code signing certificate with digital signature key usage", | ||
InputFilename: "code_signing/validCodeSigningCertificate.pem", | ||
ExpectedResult: lint.Pass, | ||
}, | ||
{ | ||
Name: "fail - code signing certificate without required key usage", | ||
InputFilename: "code_signing/noDigitalSignatureKeyUsage.pem", | ||
ExpectedResult: lint.Error, | ||
}, | ||
{ | ||
Name: "fail - code signing certificate with prohibited key usage", | ||
InputFilename: "code_signing/containsProhibitedKeyUsage.pem", | ||
ExpectedResult: lint.Error, | ||
}, | ||
{ | ||
Name: "warn - code signing certificate with not recommended key usage", | ||
InputFilename: "code_signing/containsNotRecommendedKeyUsage.pem", | ||
ExpectedResult: lint.Warn, | ||
}, | ||
} | ||
for _, tc := range testCases { | ||
t.Run(tc.Name, func(t *testing.T) { | ||
result := test.TestLint("e_cs_key_usage_required", tc.InputFilename) | ||
if result.Status != tc.ExpectedResult { | ||
t.Errorf("expected result %v was %v - details: %v", tc.ExpectedResult, result.Status, result.Details) | ||
} | ||
}) | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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...
...I believe that we can safely add the following assumption to base.go.
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.There was a problem hiding this comment.
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.
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.