Merge pull request #2217 from BishopFox/fix/cert-revoke

Improve operator cert revocation

Author: moloch--

go-tests.sh

@@ -20,10 +20,15 @@ set -u
 set -o pipefail
 
 SKIP_GENERATE=0
+UNIT_ONLY=0
 for arg in "$@"; do
 	if [ "$arg" = "--skip-generate" ]; then
 		SKIP_GENERATE=1
 	fi
+	if [ "$arg" = "--unit-only" ]; then
+		UNIT_ONLY=1
+		SKIP_GENERATE=1
+	fi
 done
 
 echo "----------------------------------------------------------------"
@@ -219,8 +224,13 @@ done
 
 ## Server c2 (unit + e2e)
 run_test_cmd "./server/c2" go test -tags="server,$TAGS" ./server/c2 || exit 1
-run_test_cmd "./server/c2 (e2e yamux)" go test -tags="server,$TAGS,sliver_e2e" ./server/c2 -run 'Test(MTLS|WG)Yamux_' -count=1 || exit 1
-run_test_cmd "./server/c2 (e2e dns)" go test -tags="server,$TAGS,sliver_e2e" ./server/c2 -run 'TestDNS_' -count=1 || exit 1
+if [ "$UNIT_ONLY" -eq 1 ]; then
+	echo
+	echo "Skipping ./server/c2 e2e tests (--unit-only)"
+else
+	run_test_cmd "./server/c2 (e2e yamux)" go test -tags="server,$TAGS,sliver_e2e" ./server/c2 -run 'Test(MTLS|WG)Yamux_' -count=1 || exit 1
+	run_test_cmd "./server/c2 (e2e dns)" go test -tags="server,$TAGS,sliver_e2e" ./server/c2 -run 'TestDNS_' -count=1 || exit 1
+fi
 
 ## Server generate
 if [ "$SKIP_GENERATE" -eq 0 ]; then
@@ -235,5 +245,9 @@ if [ "$SKIP_GENERATE" -eq 0 ]; then
 		go test -timeout 6h -p "$GENERATE_GO_P" -parallel "$GENERATE_TEST_PARALLEL" -tags="server,$TAGS" ./server/generate || exit 1
 else
 	echo
-	echo "Skipping ./server/generate tests (--skip-generate)"
+	if [ "$UNIT_ONLY" -eq 1 ]; then
+		echo "Skipping ./server/generate tests (--unit-only)"
+	else
+		echo "Skipping ./server/generate tests (--skip-generate)"
+	fi
 fi

server/certs/operators.go

@@ -21,6 +21,7 @@ package certs
 import (
 	"crypto/x509"
 	"encoding/pem"
+	"errors"
 	"fmt"
 
 	"github.com/bishopfox/sliver/server/db"
@@ -35,6 +36,17 @@ const (
 	serverNamespace = "server" // Operator servers
 )
 
+var (
+	// ErrOperatorClientCertificateNotFound indicates that the presented operator
+	// client certificate is no longer trusted because it is not present in the
+	// certificate store.
+	ErrOperatorClientCertificateNotFound = errors.New("operator client certificate not found in database")
+
+	// ErrInvalidOperatorClientCertificate indicates that the presented
+	// certificate is not shaped like an operator client leaf certificate.
+	ErrInvalidOperatorClientCertificate = errors.New("invalid operator client certificate")
+)
+
 // OperatorClientGenerateCertificate - Generate a certificate signed with a given CA
 func OperatorClientGenerateCertificate(operator string) ([]byte, []byte, error) {
 	cert, key := GenerateECCCertificate(OperatorCA, operator, false, true, true)
@@ -52,6 +64,52 @@ func OperatorClientRemoveCertificate(operator string) error {
 	return RemoveCertificate(OperatorCA, ECCKey, fmt.Sprintf("%s.%s", clientNamespace, operator))
 }
 
+// ValidateOperatorClientCertificate ensures that the presented operator client
+// certificate is still present in the database. A valid chain alone is not
+// enough; the exact leaf certificate must still exist in storage.
+func ValidateOperatorClientCertificate(peerCertificates []*x509.Certificate) error {
+	if len(peerCertificates) == 0 || peerCertificates[0] == nil {
+		return ErrInvalidOperatorClientCertificate
+	}
+
+	leaf := peerCertificates[0]
+	if leaf.IsCA || leaf.Subject.CommonName == "" || !hasExtKeyUsage(leaf, x509.ExtKeyUsageClientAuth) {
+		return ErrInvalidOperatorClientCertificate
+	}
+
+	pemBytes := pem.EncodeToMemory(&pem.Block{
+		Type:  "CERTIFICATE",
+		Bytes: leaf.Raw,
+	})
+	if len(pemBytes) == 0 {
+		return ErrInvalidOperatorClientCertificate
+	}
+
+	record := &models.Certificate{}
+	result := db.Session().Select("id").Where(&models.Certificate{
+		CommonName:     fmt.Sprintf("%s.%s", clientNamespace, leaf.Subject.CommonName),
+		CAType:         OperatorCA,
+		KeyType:        ECCKey,
+		CertificatePEM: string(pemBytes),
+	}).First(record)
+	if result.Error == nil {
+		return nil
+	}
+	if errors.Is(result.Error, db.ErrRecordNotFound) {
+		return ErrOperatorClientCertificateNotFound
+	}
+	return result.Error
+}
+
+func hasExtKeyUsage(cert *x509.Certificate, usage x509.ExtKeyUsage) bool {
+	for _, extKeyUsage := range cert.ExtKeyUsage {
+		if extKeyUsage == usage {
+			return true
+		}
+	}
+	return false
+}
+
 // OperatorServerGetCertificate - Helper function to fetch a server cert
 func OperatorServerGetCertificate(hostname string) ([]byte, []byte, error) {
 	return GetECCCertificate(OperatorCA, fmt.Sprintf("%s.%s", serverNamespace, hostname))

server/certs/operators_validation_test.go

@@ -0,0 +1,117 @@
+package certs
+
+import (
+	"crypto/x509"
+	"encoding/pem"
+	"errors"
+	"fmt"
+	"testing"
+	"time"
+
+	"github.com/bishopfox/sliver/server/db"
+	"github.com/bishopfox/sliver/server/db/models"
+)
+
+func TestValidateOperatorClientCertificateAdversarial(t *testing.T) {
+	SetupCAs()
+
+	t.Run("accepts stored operator client leaf", func(t *testing.T) {
+		operatorName := uniqueOperatorCertificateName(t, "stored")
+		certPEM, _, err := OperatorClientGenerateCertificate(operatorName)
+		if err != nil {
+			t.Fatalf("generate operator client certificate: %v", err)
+		}
+
+		err = ValidateOperatorClientCertificate([]*x509.Certificate{mustParseCertificate(t, certPEM)})
+		if err != nil {
+			t.Fatalf("expected stored operator client certificate to be accepted, got %v", err)
+		}
+	})
+
+	t.Run("rejects valid stored certificate hidden behind an unstored first certificate", func(t *testing.T) {
+		operatorName := uniqueOperatorCertificateName(t, "hidden")
+		storedCertPEM, _, err := OperatorClientGenerateCertificate(operatorName)
+		if err != nil {
+			t.Fatalf("generate stored operator certificate: %v", err)
+		}
+		rogueCertPEM, _ := GenerateECCCertificate(OperatorCA, operatorName, false, true, true)
+
+		err = ValidateOperatorClientCertificate([]*x509.Certificate{
+			mustParseCertificate(t, rogueCertPEM),
+			mustParseCertificate(t, storedCertPEM),
+		})
+		if !errors.Is(err, ErrOperatorClientCertificateNotFound) {
+			t.Fatalf("expected first unstored certificate to be rejected, got %v", err)
+		}
+	})
+
+	t.Run("rejects same-common-name different-leaf certificate", func(t *testing.T) {
+		operatorName := uniqueOperatorCertificateName(t, "same-cn")
+		_, _, err := OperatorClientGenerateCertificate(operatorName)
+		if err != nil {
+			t.Fatalf("generate stored operator certificate: %v", err)
+		}
+		rogueCertPEM, _ := GenerateECCCertificate(OperatorCA, operatorName, false, true, true)
+
+		err = ValidateOperatorClientCertificate([]*x509.Certificate{mustParseCertificate(t, rogueCertPEM)})
+		if !errors.Is(err, ErrOperatorClientCertificateNotFound) {
+			t.Fatalf("expected rotated or forged same-CN leaf to be rejected, got %v", err)
+		}
+	})
+
+	t.Run("rejects stored operator server certificate", func(t *testing.T) {
+		hostName := uniqueOperatorCertificateName(t, "server")
+		serverCertPEM, _, err := OperatorServerGenerateCertificate(hostName)
+		if err != nil {
+			t.Fatalf("generate operator server certificate: %v", err)
+		}
+
+		err = ValidateOperatorClientCertificate([]*x509.Certificate{mustParseCertificate(t, serverCertPEM)})
+		if !errors.Is(err, ErrInvalidOperatorClientCertificate) {
+			t.Fatalf("expected stored operator server certificate to be rejected, got %v", err)
+		}
+	})
+
+	t.Run("rejects operator CA certificate even if inserted into the certificates table", func(t *testing.T) {
+		caCertPEM, caKeyPEM, err := GetCertificateAuthorityPEM(OperatorCA)
+		if err != nil {
+			t.Fatalf("get operator CA: %v", err)
+		}
+		caCert := mustParseCertificate(t, caCertPEM)
+
+		record := &models.Certificate{
+			CommonName:     fmt.Sprintf("%s.%s", clientNamespace, caCert.Subject.CommonName),
+			CAType:         OperatorCA,
+			KeyType:        ECCKey,
+			CertificatePEM: string(caCertPEM),
+			PrivateKeyPEM:  string(caKeyPEM),
+		}
+		if err := db.Session().Create(record).Error; err != nil {
+			t.Fatalf("insert CA cert into certificates table: %v", err)
+		}
+
+		err = ValidateOperatorClientCertificate([]*x509.Certificate{caCert})
+		if !errors.Is(err, ErrInvalidOperatorClientCertificate) {
+			t.Fatalf("expected CA certificate to be rejected, got %v", err)
+		}
+	})
+}
+
+func mustParseCertificate(t *testing.T, certPEM []byte) *x509.Certificate {
+	t.Helper()
+
+	block, _ := pem.Decode(certPEM)
+	if block == nil {
+		t.Fatal("failed to decode certificate PEM")
+	}
+	cert, err := x509.ParseCertificate(block.Bytes)
+	if err != nil {
+		t.Fatalf("parse x509 certificate: %v", err)
+	}
+	return cert
+}
+
+func uniqueOperatorCertificateName(t *testing.T, prefix string) string {
+	t.Helper()
+	return fmt.Sprintf("%s-%d", prefix, time.Now().UnixNano())
+}

server/console/console-admin.go

@@ -218,18 +218,9 @@ func kickOperatorCmd(cmd *cobra.Command, args []string) {
 	}
 
 	fmt.Printf(Info+"Removing auth token(s) for %s, please wait ... \n", operator)
-	err = db.Session().Where(&models.Operator{
-		Name: operator,
-	}).Delete(&models.Operator{}).Error
-	if err != nil {
-		fmt.Printf(Warn+"Failed to remove operator %s: %v\n", operator, err)
-		return
-	}
-	transport.ClearTokenCache()
-	fmt.Printf(Info+"Removing client certificate(s) for %s, please wait ... \n", operator)
-	err = certs.OperatorClientRemoveCertificate(operator)
+	err = kickOperator(operator)
 	if err != nil {
-		fmt.Printf(Warn+"Failed to remove the operator certificate: %v \n", err)
+		fmt.Printf(Warn+"Failed to kick operator %s: %v\n", operator, err)
 		return
 	}
 	fmt.Printf(Info+"Operator %s has been kicked out.\n", operator)
@@ -242,6 +233,33 @@ func shouldPromptKickOperator(cmd *cobra.Command, args []string) bool {
 	return cmd.Flags().NFlag() == 0
 }
 
+func removeOperator(operator string) error {
+	err := db.Session().Where(&models.Operator{
+		Name: operator,
+	}).Delete(&models.Operator{}).Error
+	if err != nil {
+		return err
+	}
+	transport.ClearTokenCache()
+	return nil
+}
+
+func revokeOperatorClientCertificate(operator string) error {
+	return certs.OperatorClientRemoveCertificate(operator)
+}
+
+func closeOperatorStreams(operator string) {
+	transport.CloseOperatorStreams(operator)
+}
+
+func kickOperator(operator string) error {
+	if err := removeOperator(operator); err != nil {
+		return err
+	}
+	defer closeOperatorStreams(operator)
+	return revokeOperatorClientCertificate(operator)
+}
+
 func operatorNames() ([]string, error) {
 	operators, err := db.OperatorAll()
 	if err != nil {