Tests for KeyUsage and crl bit

erm-g · Feb 7, 2024 · 2b84890 · 2b84890
1 parent a4acb1a
commit 2b84890
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 19 deletions.
diff --git a/security/advancedtls/advancedtls_test.go b/security/advancedtls/advancedtls_test.go
@@ -381,14 +381,14 @@ func (s) TestClientServerHandshake(t *testing.T) {
 		return &GetRootCAsResults{TrustCerts: cs.ServerTrust3}, nil
 	}
 
-	makeStaticCRLProvider := func(crlPath string) *RevocationConfig {
+	makeStaticCRLProvider := func(crlPath string, allowUndetermined bool) *RevocationConfig {
 		rawCRL, err := os.ReadFile(crlPath)
 		if err != nil {
 			t.Fatalf("readFile(%v) failed err = %v", crlPath, err)
 		}
 		cRLProvider := NewStaticCRLProvider([][]byte{rawCRL})
 		return &RevocationConfig{
-			AllowUndetermined: true,
+			AllowUndetermined: allowUndetermined,
 			CRLProvider:       cRLProvider,
 		}
 	}
@@ -735,7 +735,7 @@ func (s) TestClientServerHandshake(t *testing.T) {
 			clientGetRoot:          getRootCAsForClientCRL,
 			clientVerifyFunc:       clientVerifyFuncGood,
 			clientVType:            CertVerification,
-			clientRevocationConfig: makeStaticCRLProvider(testdata.Path("crl/provider_crl_empty.pem")),
+			clientRevocationConfig: makeStaticCRLProvider(testdata.Path("crl/provider_crl_empty.pem"), true),
 			serverMutualTLS:        true,
 			serverCert:             []tls.Certificate{cs.ServerCert3},
 			serverGetRoot:          getRootCAsForServerCRL,
@@ -750,7 +750,7 @@ func (s) TestClientServerHandshake(t *testing.T) {
 			clientGetRoot:          getRootCAsForClientCRL,
 			clientVerifyFunc:       clientVerifyFuncGood,
 			clientVType:            CertVerification,
-			clientRevocationConfig: makeStaticCRLProvider(testdata.Path("crl/provider_crl_server_revoked.pem")),
+			clientRevocationConfig: makeStaticCRLProvider(testdata.Path("crl/provider_crl_server_revoked.pem"), true),
 			serverMutualTLS:        true,
 			serverCert:             []tls.Certificate{cs.ServerCert3},
 			serverGetRoot:          getRootCAsForServerCRL,
@@ -759,18 +759,20 @@ func (s) TestClientServerHandshake(t *testing.T) {
 		},
 		// Client: set valid credentials with the revocation config
 		// Server: set valid credentials with the revocation config
-		// Expected Behavior: fail, because CRL is issued by some malicious CA
+		// Expected Behavior: fail, because CRL is issued by the malicious CA. It
+		// can't be properly processed, and we don't allow RevocationUndetermined.
 		{
 			desc:                   "Client sets peer cert, reload root function with verifyFuncGood; Server sets peer cert, reload root function; Client uses CRL; mutualTLS",
 			clientCert:             []tls.Certificate{cs.ClientCert3},
 			clientGetRoot:          getRootCAsForClientCRL,
 			clientVerifyFunc:       clientVerifyFuncGood,
 			clientVType:            CertVerification,
-			clientRevocationConfig: makeStaticCRLProvider(testdata.Path("crl/provider_crl_empty.pem")),
+			clientRevocationConfig: makeStaticCRLProvider(testdata.Path("crl/provider_malicious_crl_empty.pem"), false),
 			serverMutualTLS:        true,
 			serverCert:             []tls.Certificate{cs.ServerCert3},
 			serverGetRoot:          getRootCAsForServerCRL,
 			serverVType:            CertVerification,
+			serverExpectError:      true,
 		},
 	} {
 		test := test

diff --git a/security/advancedtls/crl.go b/security/advancedtls/crl.go
@@ -552,21 +552,16 @@ func verifyCRL(crl *CRL, rawIssuer []byte, chain []*x509.Certificate) error {
 		// "Conforming CRL issuers MUST use the key identifier method, and MUST
 		// include this extension in all CRLs issued."
 		// So, this is much simpler than RFC4158 and should be compatible.
-		str1 := cryptobyte.String(c.SubjectKeyId)
-		fmt.Println("SubjectKeyId =", str1)
-		str2 := cryptobyte.String(crl.authorityKeyID)
-		fmt.Println("SubjectKeyId =", str2)
-		str3 := cryptobyte.String(c.RawSubject)
-		fmt.Println("SubjectKeyId =", str3)
-		str4 := cryptobyte.String(crl.rawIssuer)
-		fmt.Println("SubjectKeyId =", str4)
-
 		if bytes.Equal(c.SubjectKeyId, crl.authorityKeyID) && bytes.Equal(c.RawSubject, crl.rawIssuer) {
+			// RFC5280, 6.3.3 (f) Key usage and cRLSign bit.
+			if c.KeyUsage != 0 && c.KeyUsage&x509.KeyUsageCRLSign == 0 {
+				return fmt.Errorf("verifyCRL: The trust anchor can't be used for issuing CRLs")
+			}
 			// RFC5280, 6.3.3 (g) Validate signature.
 			return crl.certList.CheckSignatureFrom(c)
 		}
 	}
-	return fmt.Errorf("verifyCRL: No certificates mached CRL issuer (%v)", crl.certList.Issuer)
+	return fmt.Errorf("verifyCRL: No certificates matched CRL issuer (%v)", crl.certList.Issuer)
 }
 
 // pemType is the type of a PEM encoded CRL.

diff --git a/security/advancedtls/crl_test.go b/security/advancedtls/crl_test.go
@@ -471,14 +471,14 @@ func TestVerifyCrl(t *testing.T) {
 			crl:     loadCRL(t, testdata.Path("crl/3.crl")),
 			certs:   makeChain(t, testdata.Path("crl/unrevoked.pem")),
 			cert:    makeChain(t, testdata.Path("crl/revokedInt.pem"))[1],
-			errWant: "No certificates mached",
+			errWant: "No certificates matched",
 		},
 		{
 			desc:    "Fail no certs",
 			crl:     loadCRL(t, testdata.Path("crl/1.crl")),
 			certs:   []*x509.Certificate{},
 			cert:    makeChain(t, testdata.Path("crl/unrevoked.pem"))[1],
-			errWant: "No certificates mached",
+			errWant: "No certificates matched",
 		},
 		{
 			desc:    "Fail Tampered signature",
@@ -501,6 +501,13 @@ func TestVerifyCrl(t *testing.T) {
 			cert:    makeChain(t, testdata.Path("crl/provider_client_trust_cert.pem"))[0],
 			errWant: "verification error",
 		},
+		{
+			desc:    "Fail KeyUsage without cRLSign bit",
+			crl:     loadCRL(t, testdata.Path("crl/provider_malicious_crl_empty.pem")),
+			certs:   makeChain(t, testdata.Path("crl/provider_malicious_client_trust_cert.pem")),
+			cert:    makeChain(t, testdata.Path("crl/provider_malicious_client_trust_cert.pem"))[0],
+			errWant: "trust anchor can't be used",
+		},
 	}
 
 	for _, tt := range verifyTests {
@@ -662,7 +669,6 @@ func setupTLSConn(t *testing.T) (net.Listener, *x509.Certificate, *ecdsa.Private
 }
 
 // TestVerifyConnection will setup a client/server connection and check revocation in the real TLS dialer
-// TODO add CRL provider tests here?
 func TestVerifyConnection(t *testing.T) {
 	lis, cert, key := setupTLSConn(t)
 	defer func() {