security/advancedtls: CRL checks improvement (#6968)

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 {
+	makeStaticCRLRevocationConfig := 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,
 		}
 	}
@@ -731,13 +731,13 @@ func (s) TestClientServerHandshake(t *testing.T) {
 		// Expected Behavior: success, because none of the certificate chains sent in the connection are revoked
 		{
 			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},
+			clientCert:             []tls.Certificate{cs.ClientCertForCRL},
 			clientGetRoot:          getRootCAsForClientCRL,
 			clientVerifyFunc:       clientVerifyFuncGood,
 			clientVType:            CertVerification,
-			clientRevocationConfig: makeStaticCRLProvider(testdata.Path("crl/provider_crl_empty.pem")),
+			clientRevocationConfig: makeStaticCRLRevocationConfig(testdata.Path("crl/provider_crl_empty.pem"), true),
 			serverMutualTLS:        true,
-			serverCert:             []tls.Certificate{cs.ServerCert3},
+			serverCert:             []tls.Certificate{cs.ServerCertForCRL},
 			serverGetRoot:          getRootCAsForServerCRL,
 			serverVType:            CertVerification,
 		},
@@ -746,13 +746,30 @@ func (s) TestClientServerHandshake(t *testing.T) {
 		// Expected Behavior: fail, server creds are revoked
 		{
 			desc:                   "Client sets peer cert, reload root function with verifyFuncGood; Server sets revoked cert; Client uses CRL; mutualTLS",
-			clientCert:             []tls.Certificate{cs.ClientCert3},
+			clientCert:             []tls.Certificate{cs.ClientCertForCRL},
 			clientGetRoot:          getRootCAsForClientCRL,
 			clientVerifyFunc:       clientVerifyFuncGood,
 			clientVType:            CertVerification,
-			clientRevocationConfig: makeStaticCRLProvider(testdata.Path("crl/provider_crl_server_revoked.pem")),
+			clientRevocationConfig: makeStaticCRLRevocationConfig(testdata.Path("crl/provider_crl_server_revoked.pem"), true),
 			serverMutualTLS:        true,
-			serverCert:             []tls.Certificate{cs.ServerCert3},
+			serverCert:             []tls.Certificate{cs.ServerCertForCRL},
+			serverGetRoot:          getRootCAsForServerCRL,
+			serverVType:            CertVerification,
+			serverExpectError:      true,
+		},
+		// Client: set valid credentials with the revocation config
+		// Server: set valid credentials with the revocation config
+		// 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.ClientCertForCRL},
+			clientGetRoot:          getRootCAsForClientCRL,
+			clientVerifyFunc:       clientVerifyFuncGood,
+			clientVType:            CertVerification,
+			clientRevocationConfig: makeStaticCRLRevocationConfig(testdata.Path("crl/provider_malicious_crl_empty.pem"), false),
+			serverMutualTLS:        true,
+			serverCert:             []tls.Certificate{cs.ServerCertForCRL},
 			serverGetRoot:          getRootCAsForServerCRL,
 			serverVType:            CertVerification,
 			serverExpectError:      true,

security/advancedtls/crl.go

@@ -285,7 +285,7 @@ func fetchIssuerCRL(rawIssuer []byte, crlVerifyCrt []*x509.Certificate, cfg Revo
 		return nil, fmt.Errorf("fetchCRL() failed: %v", err)
 	}
 
-	if err := verifyCRL(crl, rawIssuer, crlVerifyCrt); err != nil {
+	if err := verifyCRL(crl, crlVerifyCrt); err != nil {
 		return nil, fmt.Errorf("verifyCRL() failed: %v", err)
 	}
 	if cfg.Cache != nil {
@@ -303,24 +303,31 @@ func fetchCRL(c *x509.Certificate, crlVerifyCrt []*x509.Certificate, cfg Revocat
 		if crl == nil {
 			return nil, fmt.Errorf("no CRL found for certificate's issuer")
 		}
+		if err := verifyCRL(crl, crlVerifyCrt); err != nil {
+			return nil, fmt.Errorf("verifyCRL() failed: %v", err)
+		}
 		return crl, nil
 	}
 	return fetchIssuerCRL(c.RawIssuer, crlVerifyCrt, cfg)
 }
 
-// checkCert checks a single certificate against the CRL defined in the certificate.
-// It will fetch and verify the CRL(s) defined in the root directory specified by cfg.
-// If we can't load any authoritative CRL files, the status is RevocationUndetermined.
+// checkCert checks a single certificate against the CRL defined in the
+// certificate. It will fetch and verify the CRL(s) defined in the root
+// directory (or a CRLProvider) specified by cfg. If we can't load (and verify -
+// see verifyCRL) any valid authoritative CRL files, the status is
+// RevocationUndetermined.
 // c is the certificate to check.
 // crlVerifyCrt is the group of possible certificates to verify the crl.
 func checkCert(c *x509.Certificate, crlVerifyCrt []*x509.Certificate, cfg RevocationConfig) RevocationStatus {
 	crl, err := fetchCRL(c, crlVerifyCrt, cfg)
 	if err != nil {
-		// We couldn't load any CRL files for the certificate, so we don't know
-		// if it's RevocationUnrevoked or not.  This is not necessarily a
+		// We couldn't load any valid CRL files for the certificate, so we don't
+		// know if it's RevocationUnrevoked or not. This is not necessarily a
 		// problem - it's not invalid to have no CRLs if you don't have any
-		// revocations for an issuer. We just return RevocationUndetermined and
-		// there is a setting for the user to control the handling of that.
+		// revocations for an issuer. It also might be an indication that the CRL
+		// file is invalid.
+		// We just return RevocationUndetermined and there is a setting for the user
+		// to control the handling of that.
 		grpclogLogger.Warningf("fetchCRL() err = %v", err)
 		return RevocationUndetermined
 	}
@@ -534,8 +541,8 @@ func fetchCRLOpenSSLHashDir(rawIssuer []byte, cfg RevocationConfig) (*CRL, error
 	return parsedCRL, nil
 }
 
-func verifyCRL(crl *CRL, rawIssuer []byte, chain []*x509.Certificate) error {
-	// RFC5280, 6.3.3 (f) Obtain and validateate the certification path for the issuer of the complete CRL
+func verifyCRL(crl *CRL, chain []*x509.Certificate) error {
+	// RFC5280, 6.3.3 (f) Obtain and validate the certification path for the issuer of the complete CRL
 	// We intentionally limit our CRLs to be signed with the same certificate path as the certificate
 	// so we can use the chain from the connection.
 
@@ -547,11 +554,15 @@ func verifyCRL(crl *CRL, rawIssuer []byte, chain []*x509.Certificate) error {
 		// include this extension in all CRLs issued."
 		// So, this is much simpler than RFC4158 and should be compatible.
 		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 certificate 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.

security/advancedtls/crl_provider_test.go

@@ -142,7 +142,7 @@ func (s) TestFileWatcherCRLProviderConfig(t *testing.T) {
 // that it’s correctly processed. Additionally, we also check if number of
 // invocations of custom callback is correct.
 func (s) TestFileWatcherCRLProvider(t *testing.T) {
-	const nonCRLFilesUnderCRLDirectory = 15
+	const nonCRLFilesUnderCRLDirectory = 17
 	nonCRLFilesSet := make(map[string]struct{})
 	customCallback := func(err error) {
 		if strings.Contains(err.Error(), "BUILD") {

security/advancedtls/crl_test.go

@@ -441,9 +441,12 @@ func TestGetIssuerCRLCache(t *testing.T) {
 }
 
 func TestVerifyCrl(t *testing.T) {
-	tampered := loadCRL(t, testdata.Path("crl/1.crl"))
+	tamperedSignature := loadCRL(t, testdata.Path("crl/1.crl"))
 	// Change the signature so it won't verify
-	tampered.certList.Signature[0]++
+	tamperedSignature.certList.Signature[0]++
+	tamperedContent := loadCRL(t, testdata.Path("crl/provider_crl_empty.pem"))
+	// Change the content so it won't find a match
+	tamperedContent.rawIssuer[0]++
 
 	verifyTests := []struct {
 		desc    string
@@ -471,27 +474,48 @@ 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",
-			crl:     tampered,
+			crl:     tamperedSignature,
 			certs:   makeChain(t, testdata.Path("crl/unrevoked.pem")),
 			cert:    makeChain(t, testdata.Path("crl/unrevoked.pem"))[1],
 			errWant: "verification failure",
 		},
+		{
+			desc:    "Fail Tampered content",
+			crl:     tamperedContent,
+			certs:   makeChain(t, testdata.Path("crl/provider_client_trust_cert.pem")),
+			cert:    makeChain(t, testdata.Path("crl/provider_client_trust_cert.pem"))[0],
+			errWant: "No certificates",
+		},
+		{
+			desc:    "Fail CRL by malicious CA",
+			crl:     loadCRL(t, testdata.Path("crl/provider_malicious_crl_empty.pem")),
+			certs:   makeChain(t, testdata.Path("crl/provider_client_trust_cert.pem")),
+			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: "certificate can't be used",
+		},
 	}
 
 	for _, tt := range verifyTests {
 		t.Run(tt.desc, func(t *testing.T) {
-			err := verifyCRL(tt.crl, tt.cert.RawIssuer, tt.certs)
+			err := verifyCRL(tt.crl, tt.certs)
 			switch {
 			case tt.errWant == "" && err != nil:
 				t.Errorf("Valid CRL did not verify err = %v", err)
@@ -648,7 +672,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() {

security/advancedtls/internal/testutils/testutils.go

@@ -35,18 +35,18 @@ type CertStore struct {
 	// ClientCert2 is the certificate sent by client to prove its identity.
 	// It is trusted by ServerTrust2.
 	ClientCert2 tls.Certificate
-	// ClientCert3 is the certificate sent by client to prove its identity.
+	// ClientCertForCRL is the certificate sent by client to prove its identity.
 	// It is trusted by ServerTrust3. Used in CRL tests
-	ClientCert3 tls.Certificate
+	ClientCertForCRL tls.Certificate
 	// ServerCert1 is the certificate sent by server to prove its identity.
 	// It is trusted by ClientTrust1.
 	ServerCert1 tls.Certificate
 	// ServerCert2 is the certificate sent by server to prove its identity.
 	// It is trusted by ClientTrust2.
 	ServerCert2 tls.Certificate
-	// ServerCert3 is a revoked certificate
-	// (this info is stored in crl_server_revoked.pem).
-	ServerCert3 tls.Certificate
+	// ServerCertForCRL is a revoked certificate
+	// (this info is stored in provider_crl_server_revoked.pem).
+	ServerCertForCRL tls.Certificate
 	// ServerPeer3 is the certificate sent by server to prove its identity.
 	ServerPeer3 tls.Certificate
 	// ServerPeerLocalhost1 is the certificate sent by server to prove its
@@ -89,7 +89,7 @@ func (cs *CertStore) LoadCerts() error {
 	if cs.ClientCert2, err = tls.LoadX509KeyPair(testdata.Path("client_cert_2.pem"), testdata.Path("client_key_2.pem")); err != nil {
 		return err
 	}
-	if cs.ClientCert3, err = tls.LoadX509KeyPair(testdata.Path("crl/provider_client_cert.pem"), testdata.Path("crl/provider_client_cert.key")); err != nil {
+	if cs.ClientCertForCRL, err = tls.LoadX509KeyPair(testdata.Path("crl/provider_client_cert.pem"), testdata.Path("crl/provider_client_cert.key")); err != nil {
 		return err
 	}
 	if cs.ServerCert1, err = tls.LoadX509KeyPair(testdata.Path("server_cert_1.pem"), testdata.Path("server_key_1.pem")); err != nil {
@@ -98,7 +98,7 @@ func (cs *CertStore) LoadCerts() error {
 	if cs.ServerCert2, err = tls.LoadX509KeyPair(testdata.Path("server_cert_2.pem"), testdata.Path("server_key_2.pem")); err != nil {
 		return err
 	}
-	if cs.ServerCert3, err = tls.LoadX509KeyPair(testdata.Path("crl/provider_server_cert.pem"), testdata.Path("crl/provider_server_cert.key")); err != nil {
+	if cs.ServerCertForCRL, err = tls.LoadX509KeyPair(testdata.Path("crl/provider_server_cert.pem"), testdata.Path("crl/provider_server_cert.key")); err != nil {
 		return err
 	}
 	if cs.ServerPeer3, err = tls.LoadX509KeyPair(testdata.Path("server_cert_3.pem"), testdata.Path("server_key_3.pem")); err != nil {

security/advancedtls/testdata/crl/README.md

@@ -49,8 +49,8 @@ Certificate chain where the leaf is revoked
 
 ## Test Data for testing CRL providers functionality
 
-To generate test data please follow the steps below or run provider_create.sh 
-script. All the files have `provider_` prefix.
+To generate test data please run provider_create.sh script. All the files have 
+`provider_` prefix.
 
 We need to generate the following artifacts for testing CRL provider:
 * server self signed CA cert
@@ -59,61 +59,34 @@ We need to generate the following artifacts for testing CRL provider:
 * client cert signed by server CA
 * empty crl file
 * crl file containing information about revoked server cert
+* crl file by 'malicious' CA which contains the same issuer with original CA 
 
-Please find the related commands below.
 
-* Generate self signed CAs
-```
-$ openssl req -x509 -newkey rsa:4096 -keyout provider_server_trust_key.pem -out provider_server_trust_cert.pem -days 365 -subj "/C=US/ST=VA/O=Internet Widgits Pty Ltd/CN=foo.bar.hoo.ca.com" -nodes
-$ openssl req -x509 -newkey rsa:4096 -keyout provider_client_trust_key.pem -out provider_client_trust_cert.pem -days 365 -subj "/C=US/ST=CA/L=SVL/O=Internet Widgits Pty Ltd" -nodes
-```
+All the commands are provided in provider_create.sh script. Please find the 
+description below.
 
-* Generate client and server certs signed by CAs
-```
-$ openssl req -newkey rsa:4096 -keyout provider_server_cert.key -out provider_new_cert.csr -nodes -subj "/C=US/ST=CA/L=DUMMYCITY/O=Internet Widgits Pty Ltd/CN=foo.bar.com" -sha256
-$ openssl x509 -req -in provider_new_cert.csr -out provider_server_cert.pem -CA provider_client_trust_cert.pem -CAkey provider_client_trust_key.pem -CAcreateserial -days 3650 -sha256 -extfile provider_extensions.conf
+1. The first two commands generate self signed CAs for client and server:
+   - provider_server_trust_key.pem 
+   - provider_server_trust_cert.pem 
+   - provider_client_trust_key.pem 
+   - provider_client_trust_cert.pem 
 
-$ openssl req -newkey rsa:4096 -keyout provider_client_cert.key -out provider_new_cert.csr -nodes -subj "/C=US/ST=CA/O=Internet Widgits Pty Ltd/CN=foo.bar.hoo.com" -sha256
-$ openssl x509 -req -in provider_new_cert.csr -out provider_client_cert.pem -CA provider_server_trust_cert.pem -CAkey provider_server_trust_key.pem -CAcreateserial -days 3650 -sha256 -extfile provider_extensions.conf
-```
+2. Generate client and server certs signed by the CAs above:
+   - provider_server_cert.pem 
+   - provider_client_cert.pem
 
-Here is the content of `provider_extensions.conf` -
-```
-subjectKeyIdentifier = hash
-authorityKeyIdentifier = keyid,issuer
-basicConstraints = CA:FALSE
-keyUsage = digitalSignature, keyEncipherment
-```
+3. The next 2 commands create 2 files needed for CRL issuing:
+   - provider_crlnumber.txt
+   - provider_index.txt
 
-* Generate CRLs
-  For CRL generation we need 2 more files called `index.txt` and `crlnumber.txt`:
-```
-$ echo "1000" > provider_crlnumber.txt
-$ touch provider_index.txt
-```
-Also we need another config `provider_crl.cnf` -
-```
-[ ca ]
-default_ca = my_ca
-
-[ my_ca ]
-crl = crl.pem
-default_md = sha256
-database = provider_index.txt
-crlnumber = provider_crlnumber.txt
-default_crl_days = 30
-default_crl_hours = 1
-crl_extensions = crl_ext
-
-[crl_ext]
-# Authority Key Identifier extension
-authorityKeyIdentifier=keyid:always,issuer:always
-```
+4. The next 3 commands generate an empty CRL file and a CRL file containing 
+revoked server cert:
+   - provider_crl_empty.pem 
+   - provider_crl_server_revoked.pem 
 
-The commands to generate empty CRL file and CRL file containing revoked server
-cert are below.
-```
-$ openssl ca -gencrl -keyfile provider_client_trust_key.pem -cert provider_client_trust_cert.pem -out provider_crl_empty.pem -config provider_crl.cnf
-$ openssl ca -revoke provider_server_cert.pem -keyfile provider_client_trust_key.pem -cert provider_client_trust_cert.pem -config provider_crl.cnf
-$ openssl ca -gencrl -keyfile provider_client_trust_key.pem -cert provider_client_trust_cert.pem -out provider_crl_server_revoked.pem -config provider_crl.cnf
-```
+5. The final section contains commands to generate CRL file by 'malicious' CA. 
+Note that we use Subject Key Identifier from previously created 
+provider_client_trust_cert.pem to generate malicious certs / CRL.
+   - provider_malicious_client_trust_key.pem
+   - provider_malicious_client_trust_cert.pem 
+   - provider_malicious_crl_empty.pem