From 14830e0f059af8f7575b19adaabfe58ce750fc83 Mon Sep 17 00:00:00 2001 From: christopher-henderson Date: Sat, 30 Sep 2023 09:56:18 -0700 Subject: [PATCH 1/2] Lint for CABF SMIME 7.1.2.3.h - subjectAlternativeName SHOULD NOT be marked critical unless the subject field is an empty sequence. --- .../lint_san_should_not_be_critical.go | 78 +++++++++++++++++++ .../lint_san_should_not_be_critical_test.go | 35 +++++++++ .../san_not_critical_with_empty_subject.pem | 41 ++++++++++ .../smime/san_not_critical_with_subject.pem | 41 ++++++++++ 4 files changed, 195 insertions(+) create mode 100644 v3/lints/cabf_smime_br/lint_san_should_not_be_critical.go create mode 100644 v3/lints/cabf_smime_br/lint_san_should_not_be_critical_test.go create mode 100644 v3/testdata/smime/san_not_critical_with_empty_subject.pem create mode 100644 v3/testdata/smime/san_not_critical_with_subject.pem diff --git a/v3/lints/cabf_smime_br/lint_san_should_not_be_critical.go b/v3/lints/cabf_smime_br/lint_san_should_not_be_critical.go new file mode 100644 index 000000000..66aae89f9 --- /dev/null +++ b/v3/lints/cabf_smime_br/lint_san_should_not_be_critical.go @@ -0,0 +1,78 @@ +/* + * ZLint Copyright 2023 Regents of the University of Michigan + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy + * of the License at http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or + * implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package cabf_smime_br + +import ( + "reflect" + + "github.com/zmap/zcrypto/x509" + "github.com/zmap/zcrypto/x509/pkix" + "github.com/zmap/zlint/v3/lint" + "github.com/zmap/zlint/v3/util" +) + +func init() { + lint.RegisterLint(&lint.Lint{ + Name: "e_san_should_not_be_critical", + Description: "subjectAlternativeName SHOULD NOT be marked critical unless the subject field is an empty sequence.", + Citation: "7.1.2.3.h", + Source: lint.CABFSMIMEBaselineRequirements, + EffectiveDate: util.CABF_SMIME_BRs_1_0_0_Date, + Lint: NewSubjectAlternativeNameNotCritical, + }) +} + +type SubjectAlternativeNameNotCritical struct{} + +func NewSubjectAlternativeNameNotCritical() lint.LintInterface { + return &SubjectAlternativeNameNotCritical{} +} + +func (l *SubjectAlternativeNameNotCritical) CheckApplies(c *x509.Certificate) bool { + return util.IsSubscriberCert(c) && util.IsExtInCert(c, util.SubjectAlternateNameOID) +} + +func (l *SubjectAlternativeNameNotCritical) Execute(c *x509.Certificate) *lint.LintResult { + san := util.GetExtFromCert(c, util.SubjectAlternateNameOID) + isCritical := san.Critical + emptySubject := reflect.DeepEqual(c.Subject, pkix.Name{OriginalRDNS: pkix.RDNSequence{}}) + if isCritical && emptySubject { + // "...unless the subject field is an empty sequence" + return &lint.LintResult{Status: lint.Pass} + } else if isCritical && !emptySubject { + // Critical, but there's a non-empty SAN. + return &lint.LintResult{ + Status: lint.Warn, + Details: "subject is not empty, but subjectAlternativeName is marked critical", + } + } else if !isCritical && emptySubject { + // Not critical, but there's an empty SAN. + // + // Is this a warning? That is, does the clause... + // + // ...SHOULD NOT be marked critical unless the subject field is an empty sequence. + // + // ..also imply the inverse? That is... + // + // ...SHOULD be marked critical if the subject field is an empty sequence. + return &lint.LintResult{ + Status: lint.Warn, + Details: "subject is not empty, but subjectAlternativeName is marked critical", + } + } else { + // Not critical, not empty SAN. + return &lint.LintResult{Status: lint.Pass} + } +} diff --git a/v3/lints/cabf_smime_br/lint_san_should_not_be_critical_test.go b/v3/lints/cabf_smime_br/lint_san_should_not_be_critical_test.go new file mode 100644 index 000000000..81c56dadc --- /dev/null +++ b/v3/lints/cabf_smime_br/lint_san_should_not_be_critical_test.go @@ -0,0 +1,35 @@ +package cabf_smime_br + +import ( + "testing" + + "github.com/zmap/zlint/v3/lint" + "github.com/zmap/zlint/v3/test" +) + +func TestSubjectAlternativeNameNotCritical(t *testing.T) { + testCases := []struct { + Name string + InputFilename string + ExpectedResult lint.LintStatus + }{ + { + Name: "warn - san not critical with empty subject", + InputFilename: "smime/san_not_critical_with_empty_subject.pem", + ExpectedResult: lint.Warn, + }, + { + Name: "pass - cert without a CRL distribution point", + InputFilename: "smime/san_not_critical_with_subject.pem", + ExpectedResult: lint.Pass, + }, + } + for _, tc := range testCases { + t.Run(tc.Name, func(t *testing.T) { + result := test.TestLint("e_san_should_not_be_critical", tc.InputFilename) + if result.Status != tc.ExpectedResult { + t.Errorf("expected result %v was %v - details: %v", tc.ExpectedResult, result.Status, result.Details) + } + }) + } +} diff --git a/v3/testdata/smime/san_not_critical_with_empty_subject.pem b/v3/testdata/smime/san_not_critical_with_empty_subject.pem new file mode 100644 index 000000000..550eb8f56 --- /dev/null +++ b/v3/testdata/smime/san_not_critical_with_empty_subject.pem @@ -0,0 +1,41 @@ +Certificate: + Data: + Version: 3 (0x2) + Serial Number: 3 (0x3) + Signature Algorithm: ecdsa-with-SHA256 + Issuer: + Validity + Not Before: Sep 30 16:12:15 2023 GMT + Not After : Nov 30 00:00:00 9998 GMT + Subject: + Subject Public Key Info: + Public Key Algorithm: id-ecPublicKey + Public-Key: (256 bit) + pub: + 04:83:07:4d:66:ba:e8:dd:7c:ed:7a:02:67:9d:80: + 7d:a5:2c:a4:bc:6f:df:aa:9c:3a:f7:6b:50:ae:42: + 36:5c:71:54:88:8e:79:f3:e3:d2:d0:72:26:c1:19: + 9d:28:a9:4f:71:c9:65:ea:40:40:4f:f6:86:d5:fb: + 77:d9:68:dc:21 + ASN1 OID: prime256v1 + NIST CURVE: P-256 + X509v3 extensions: + X509v3 Extended Key Usage: + E-mail Protection + X509v3 Subject Alternative Name: + email:coolguy@coolplace.come + Signature Algorithm: ecdsa-with-SHA256 + Signature Value: + 30:46:02:21:00:c0:a2:c4:c3:f3:b4:27:39:b5:8b:85:c4:5a: + 60:67:27:d1:7c:b4:db:3b:7e:0d:9f:2c:81:0a:5f:f6:8a:e0: + 0a:02:21:00:ec:06:77:d2:09:41:74:e8:8c:9b:64:72:27:fc: + e0:ab:62:fa:2b:bf:94:30:49:24:92:56:d9:e6:65:ba:86:6d +-----BEGIN CERTIFICATE----- +MIIBKzCB0aADAgECAgEDMAoGCCqGSM49BAMCMAAwIBcNMjMwOTMwMTYxMjE1WhgP +OTk5ODExMzAwMDAwMDBaMAAwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAASDB01m +uujdfO16AmedgH2lLKS8b9+qnDr3a1CuQjZccVSIjnnz49LQcibBGZ0oqU9xyWXq +QEBP9obV+3fZaNwhozowODATBgNVHSUEDDAKBggrBgEFBQcDBDAhBgNVHREEGjAY +gRZjb29sZ3V5QGNvb2xwbGFjZS5jb21lMAoGCCqGSM49BAMCA0kAMEYCIQDAosTD +87QnObWLhcRaYGcn0Xy02zt+DZ8sgQpf9orgCgIhAOwGd9IJQXTojJtkcif84Kti ++iu/lDBJJJJW2eZluoZt +-----END CERTIFICATE----- diff --git a/v3/testdata/smime/san_not_critical_with_subject.pem b/v3/testdata/smime/san_not_critical_with_subject.pem new file mode 100644 index 000000000..9f783b255 --- /dev/null +++ b/v3/testdata/smime/san_not_critical_with_subject.pem @@ -0,0 +1,41 @@ +Certificate: + Data: + Version: 3 (0x2) + Serial Number: 3 (0x3) + Signature Algorithm: ecdsa-with-SHA256 + Issuer: + Validity + Not Before: Sep 30 16:52:30 2023 GMT + Not After : Nov 30 00:00:00 9998 GMT + Subject: CN = Bartholomew Kuma + Subject Public Key Info: + Public Key Algorithm: id-ecPublicKey + Public-Key: (256 bit) + pub: + 04:a2:02:fe:ab:17:45:2b:67:21:b9:57:42:17:1d: + bf:e7:11:aa:b0:a7:71:7b:87:b9:4b:79:54:09:9e: + e1:7b:3b:6b:ed:fb:3c:0f:86:4d:94:b3:69:1b:30: + 98:15:69:8d:09:06:09:84:62:6c:02:c7:01:f5:75: + cb:c8:78:31:df + ASN1 OID: prime256v1 + NIST CURVE: P-256 + X509v3 extensions: + X509v3 Extended Key Usage: + E-mail Protection + X509v3 Subject Alternative Name: + email:coolguy@coolplace.come + Signature Algorithm: ecdsa-with-SHA256 + Signature Value: + 30:45:02:21:00:bd:fb:e1:0b:c1:26:2a:85:bf:95:6a:dc:37: + 9a:b9:67:95:ba:43:6d:d1:8d:4d:aa:53:54:70:72:30:f0:24: + ca:02:20:2e:61:95:23:b8:39:a7:48:02:59:d9:a3:9b:36:c6: + c8:95:06:4e:36:63:85:a0:19:88:23:ad:a9:d4:37:6d:44 +-----BEGIN CERTIFICATE----- +MIIBRTCB7KADAgECAgEDMAoGCCqGSM49BAMCMAAwIBcNMjMwOTMwMTY1MjMwWhgP +OTk5ODExMzAwMDAwMDBaMBsxGTAXBgNVBAMTEEJhcnRob2xvbWV3IEt1bWEwWTAT +BgcqhkjOPQIBBggqhkjOPQMBBwNCAASiAv6rF0UrZyG5V0IXHb/nEaqwp3F7h7lL +eVQJnuF7O2vt+zwPhk2Us2kbMJgVaY0JBgmEYmwCxwH1dcvIeDHfozowODATBgNV +HSUEDDAKBggrBgEFBQcDBDAhBgNVHREEGjAYgRZjb29sZ3V5QGNvb2xwbGFjZS5j +b21lMAoGCCqGSM49BAMCA0gAMEUCIQC9++ELwSYqhb+Vatw3mrlnlbpDbdGNTapT +VHByMPAkygIgLmGVI7g5p0gCWdmjmzbGyJUGTjZjhaAZiCOtqdQ3bUQ= +-----END CERTIFICATE----- From 0b7b3c10984597839db46745639a9f6beb8f3a32 Mon Sep 17 00:00:00 2001 From: christopher-henderson Date: Sun, 8 Oct 2023 09:10:53 -0700 Subject: [PATCH 2/2] removing implied warning interpretation --- .../lint_san_should_not_be_critical.go | 16 +--------------- .../lint_san_should_not_be_critical_test.go | 11 +++++------ 2 files changed, 6 insertions(+), 21 deletions(-) diff --git a/v3/lints/cabf_smime_br/lint_san_should_not_be_critical.go b/v3/lints/cabf_smime_br/lint_san_should_not_be_critical.go index 66aae89f9..0500bfe97 100644 --- a/v3/lints/cabf_smime_br/lint_san_should_not_be_critical.go +++ b/v3/lints/cabf_smime_br/lint_san_should_not_be_critical.go @@ -25,7 +25,7 @@ import ( func init() { lint.RegisterLint(&lint.Lint{ - Name: "e_san_should_not_be_critical", + Name: "w_san_should_not_be_critical", Description: "subjectAlternativeName SHOULD NOT be marked critical unless the subject field is an empty sequence.", Citation: "7.1.2.3.h", Source: lint.CABFSMIMEBaselineRequirements, @@ -57,20 +57,6 @@ func (l *SubjectAlternativeNameNotCritical) Execute(c *x509.Certificate) *lint.L Status: lint.Warn, Details: "subject is not empty, but subjectAlternativeName is marked critical", } - } else if !isCritical && emptySubject { - // Not critical, but there's an empty SAN. - // - // Is this a warning? That is, does the clause... - // - // ...SHOULD NOT be marked critical unless the subject field is an empty sequence. - // - // ..also imply the inverse? That is... - // - // ...SHOULD be marked critical if the subject field is an empty sequence. - return &lint.LintResult{ - Status: lint.Warn, - Details: "subject is not empty, but subjectAlternativeName is marked critical", - } } else { // Not critical, not empty SAN. return &lint.LintResult{Status: lint.Pass} diff --git a/v3/lints/cabf_smime_br/lint_san_should_not_be_critical_test.go b/v3/lints/cabf_smime_br/lint_san_should_not_be_critical_test.go index 81c56dadc..b166c1579 100644 --- a/v3/lints/cabf_smime_br/lint_san_should_not_be_critical_test.go +++ b/v3/lints/cabf_smime_br/lint_san_should_not_be_critical_test.go @@ -13,20 +13,19 @@ func TestSubjectAlternativeNameNotCritical(t *testing.T) { InputFilename string ExpectedResult lint.LintStatus }{ - { - Name: "warn - san not critical with empty subject", - InputFilename: "smime/san_not_critical_with_empty_subject.pem", - ExpectedResult: lint.Warn, - }, { Name: "pass - cert without a CRL distribution point", InputFilename: "smime/san_not_critical_with_subject.pem", ExpectedResult: lint.Pass, }, + // I admit that it is very difficult to construct a negative case + // since the Go standard library does the correct thing on your + // behalf at time of signing. Plus no certs came up bad in + // the test corpus, so we don't have any live examples. } for _, tc := range testCases { t.Run(tc.Name, func(t *testing.T) { - result := test.TestLint("e_san_should_not_be_critical", tc.InputFilename) + result := test.TestLint("w_san_should_not_be_critical", tc.InputFilename) if result.Status != tc.ExpectedResult { t.Errorf("expected result %v was %v - details: %v", tc.ExpectedResult, result.Status, result.Details) }