fix: prevent CSR extensions from overriding CA-managed key usage in copy_extensions

mitch292 · mitch292 · commit 8994bdd43985 · 2026-04-16T15:16:39.000-04:00
When copy_extensions is enabled, CSR-provided X.509v3 extensions are
copied into the certificate template's ExtraExtensions. Go's
x509.CreateCertificate gives ExtraExtensions precedence over struct
fields for the same OID, which means an attacker-crafted CSR could
inject KeyUsage, ExtKeyUsage, and other security-critical extensions
that silently override the signing profile's intended restrictions.

Add a blocklist of CA-managed extension OIDs (KeyUsage, ExtKeyUsage,
BasicConstraints, SKI, AKI, SAN, Name Constraints, CRL Distribution
Points, Authority Info Access, Certificate Policies, Issuer Alt Name)
that are never copied from CSR extensions. These extensions are
authoritatively set by the CA's signing profile via FillTemplate and
must not be overridable by a CSR submitter.

Non-standard/private-use extensions continue to be copied through
when copy_extensions is enabled.
diff --git a/signer/local/local_test.go b/signer/local/local_test.go
@@ -1636,3 +1636,307 @@ func TestLint(t *testing.T) {
 		})
 	}
 }
+
+// createCSRWithExtensions generates a CSR containing the given raw X.509v3
+// extensions. This simulates an attacker injecting extensions into a CSR.
+func createCSRWithExtensions(t *testing.T, extraExts []pkix.Extension) []byte {
+	t.Helper()
+	key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	if err != nil {
+		t.Fatalf("generating key: %v", err)
+	}
+
+	tmpl := &x509.CertificateRequest{
+		Subject:         pkix.Name{CommonName: "test.example.com"},
+		DNSNames:        []string{"test.example.com"},
+		ExtraExtensions: extraExts,
+	}
+
+	csrDER, err := x509.CreateCertificateRequest(rand.Reader, tmpl, key)
+	if err != nil {
+		t.Fatalf("creating CSR: %v", err)
+	}
+
+	return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE REQUEST", Bytes: csrDER})
+}
+
+// marshalKeyUsage encodes a KeyUsage bitstring into the DER value expected
+// inside an X.509v3 KeyUsage extension (OID 2.5.29.15).
+func marshalKeyUsage(t *testing.T, ku x509.KeyUsage) []byte {
+	t.Helper()
+	// KeyUsage is a bit string where bits are numbered from the MSB.
+	// Go's x509.KeyUsage constants are already bit positions from MSB.
+	var b [2]byte
+	b[0] = byte(ku >> 8) // not used for standard usages below bit 8
+	// Standard key usages fit in one byte for the common ones:
+	// digitalSignature(0), contentCommitment(1), keyEncipherment(2),
+	// dataEncipherment(3), keyAgreement(4), keyCertSign(5), cRLSign(6),
+	// encipherOnly(7)
+	b[0] = 0
+	b[1] = 0
+	if ku&x509.KeyUsageDigitalSignature != 0 {
+		b[0] |= 0x80
+	}
+	if ku&x509.KeyUsageContentCommitment != 0 {
+		b[0] |= 0x40
+	}
+	if ku&x509.KeyUsageKeyEncipherment != 0 {
+		b[0] |= 0x20
+	}
+	if ku&x509.KeyUsageDataEncipherment != 0 {
+		b[0] |= 0x10
+	}
+	if ku&x509.KeyUsageKeyAgreement != 0 {
+		b[0] |= 0x08
+	}
+	if ku&x509.KeyUsageCertSign != 0 {
+		b[0] |= 0x04
+	}
+	if ku&x509.KeyUsageCRLSign != 0 {
+		b[0] |= 0x02
+	}
+	if ku&x509.KeyUsageEncipherOnly != 0 {
+		b[0] |= 0x01
+	}
+	if ku&x509.KeyUsageDecipherOnly != 0 {
+		b[1] |= 0x80
+	}
+
+	// Determine padding bits: find the lowest set bit
+	var usedBytes int
+	var padBits int
+	if b[1] != 0 {
+		usedBytes = 2
+		for i := 0; i < 8; i++ {
+			if b[1]&(1<<uint(i)) != 0 {
+				padBits = i
+				break
+			}
+		}
+	} else {
+		usedBytes = 1
+		for i := 0; i < 8; i++ {
+			if b[0]&(1<<uint(i)) != 0 {
+				padBits = i
+				break
+			}
+		}
+	}
+
+	bs := asn1.BitString{
+		Bytes:     b[:usedBytes],
+		BitLength: usedBytes*8 - padBits,
+	}
+	val, err := asn1.Marshal(bs)
+	if err != nil {
+		t.Fatalf("marshaling KeyUsage: %v", err)
+	}
+	return val
+}
+
+// marshalExtKeyUsage encodes ExtKeyUsage OIDs into the DER value expected
+// inside an X.509v3 ExtKeyUsage extension (OID 2.5.29.37).
+func marshalExtKeyUsage(t *testing.T, ekus []asn1.ObjectIdentifier) []byte {
+	t.Helper()
+	val, err := asn1.Marshal(ekus)
+	if err != nil {
+		t.Fatalf("marshaling ExtKeyUsage: %v", err)
+	}
+	return val
+}
+
+// TestCopyExtensionsDoesNotOverrideKeyUsage verifies that when
+// copy_extensions is enabled, a CSR containing injected KeyUsage and
+// ExtKeyUsage extensions cannot override the signing profile's intended
+// key usage values. This is a regression test for a vulnerability where
+// Go's x509.CreateCertificate gave ExtraExtensions precedence over
+// struct fields, allowing CSR-provided KU/EKU to bypass profile restrictions.
+func TestCopyExtensionsDoesNotOverrideKeyUsage(t *testing.T) {
+	// Profile allows only: digitalSignature + serverAuth
+	profilePolicy := &config.Signing{
+		Default: &config.SigningProfile{
+			Usage:          []string{"digital signature", "server auth"},
+			Expiry:         1 * time.Hour,
+			ExpiryString:   "1h",
+			CopyExtensions: true,
+		},
+	}
+
+	s, err := NewSignerFromFile(testCaFile, testCaKeyFile, profilePolicy)
+	if err != nil {
+		t.Fatalf("creating signer: %v", err)
+	}
+
+	// Attacker CSR injects keyCertSign + cRLSign and codeSigning + emailProtection
+	attackerKU := x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign | x509.KeyUsageCRLSign
+	attackerEKUs := []asn1.ObjectIdentifier{
+		{1, 3, 6, 1, 5, 5, 7, 3, 3}, // codeSigning
+		{1, 3, 6, 1, 5, 5, 7, 3, 4}, // emailProtection
+	}
+
+	csrPEM := createCSRWithExtensions(t, []pkix.Extension{
+		{
+			Id:       asn1.ObjectIdentifier{2, 5, 29, 15}, // KeyUsage
+			Critical: true,
+			Value:    marshalKeyUsage(t, attackerKU),
+		},
+		{
+			Id:       asn1.ObjectIdentifier{2, 5, 29, 37}, // ExtKeyUsage
+			Critical: false,
+			Value:    marshalExtKeyUsage(t, attackerEKUs),
+		},
+	})
+
+	certPEM, err := s.Sign(signer.SignRequest{
+		Hosts:   []string{"test.example.com"},
+		Request: string(csrPEM),
+	})
+	if err != nil {
+		t.Fatalf("signing: %v", err)
+	}
+
+	cert, err := helpers.ParseCertificatePEM(certPEM)
+	if err != nil {
+		t.Fatalf("parsing signed cert: %v", err)
+	}
+
+	// The cert MUST have only the profile's KeyUsage (digitalSignature),
+	// NOT the attacker's keyCertSign or cRLSign.
+	if cert.KeyUsage&x509.KeyUsageCertSign != 0 {
+		t.Errorf("certificate has keyCertSign — attacker's KeyUsage was not filtered")
+	}
+	if cert.KeyUsage&x509.KeyUsageCRLSign != 0 {
+		t.Errorf("certificate has cRLSign — attacker's KeyUsage was not filtered")
+	}
+	if cert.KeyUsage&x509.KeyUsageDigitalSignature == 0 {
+		t.Errorf("certificate missing digitalSignature from profile")
+	}
+
+	// The cert MUST have only the profile's EKU (serverAuth),
+	// NOT the attacker's codeSigning or emailProtection.
+	for _, eku := range cert.ExtKeyUsage {
+		switch eku {
+		case x509.ExtKeyUsageCodeSigning:
+			t.Errorf("certificate has codeSigning EKU — attacker's ExtKeyUsage was not filtered")
+		case x509.ExtKeyUsageEmailProtection:
+			t.Errorf("certificate has emailProtection EKU — attacker's ExtKeyUsage was not filtered")
+		}
+	}
+
+	foundServerAuth := false
+	for _, eku := range cert.ExtKeyUsage {
+		if eku == x509.ExtKeyUsageServerAuth {
+			foundServerAuth = true
+			break
+		}
+	}
+	if !foundServerAuth {
+		t.Errorf("certificate missing serverAuth EKU from profile")
+	}
+}
+
+// TestCopyExtensionsAllowsNonManagedOIDs verifies that CopyExtensions
+// still copies through non-security-critical, non-CA-managed extensions
+// (e.g., private-use OIDs). This ensures the blocklist does not break
+// legitimate use cases for copy_extensions.
+func TestCopyExtensionsAllowsNonManagedOIDs(t *testing.T) {
+	profilePolicy := &config.Signing{
+		Default: &config.SigningProfile{
+			Usage:          []string{"digital signature", "server auth"},
+			Expiry:         1 * time.Hour,
+			ExpiryString:   "1h",
+			CopyExtensions: true,
+		},
+	}
+
+	s, err := NewSignerFromFile(testCaFile, testCaKeyFile, profilePolicy)
+	if err != nil {
+		t.Fatalf("creating signer: %v", err)
+	}
+
+	// A private-use extension that should be copied through
+	customOID := asn1.ObjectIdentifier{1, 2, 3, 4, 5, 6, 7}
+	customValue, _ := asn1.Marshal("custom-extension-value")
+
+	csrPEM := createCSRWithExtensions(t, []pkix.Extension{
+		{
+			Id:    customOID,
+			Value: customValue,
+		},
+	})
+
+	certPEM, err := s.Sign(signer.SignRequest{
+		Hosts:   []string{"test.example.com"},
+		Request: string(csrPEM),
+	})
+	if err != nil {
+		t.Fatalf("signing: %v", err)
+	}
+
+	cert, err := helpers.ParseCertificatePEM(certPEM)
+	if err != nil {
+		t.Fatalf("parsing signed cert: %v", err)
+	}
+
+	found := false
+	for _, ext := range cert.Extensions {
+		if ext.Id.Equal(customOID) {
+			found = true
+			if !bytes.Equal(ext.Value, customValue) {
+				t.Errorf("custom extension value mismatch: got %x, want %x", ext.Value, customValue)
+			}
+			break
+		}
+	}
+	if !found {
+		t.Errorf("custom extension (OID %s) was not copied into the certificate", customOID)
+	}
+}
+
+// TestCopyExtensionsDisabledDoesNotCopy verifies that when copy_extensions
+// is false (the default), CSR extensions are NOT copied — whether they are
+// CA-managed or not.
+func TestCopyExtensionsDisabledDoesNotCopy(t *testing.T) {
+	profilePolicy := &config.Signing{
+		Default: &config.SigningProfile{
+			Usage:          []string{"digital signature", "server auth"},
+			Expiry:         1 * time.Hour,
+			ExpiryString:   "1h",
+			CopyExtensions: false,
+		},
+	}
+
+	s, err := NewSignerFromFile(testCaFile, testCaKeyFile, profilePolicy)
+	if err != nil {
+		t.Fatalf("creating signer: %v", err)
+	}
+
+	customOID := asn1.ObjectIdentifier{1, 2, 3, 4, 5, 6, 7}
+	customValue, _ := asn1.Marshal("should-not-appear")
+
+	csrPEM := createCSRWithExtensions(t, []pkix.Extension{
+		{
+			Id:    customOID,
+			Value: customValue,
+		},
+	})
+
+	certPEM, err := s.Sign(signer.SignRequest{
+		Hosts:   []string{"test.example.com"},
+		Request: string(csrPEM),
+	})
+	if err != nil {
+		t.Fatalf("signing: %v", err)
+	}
+
+	cert, err := helpers.ParseCertificatePEM(certPEM)
+	if err != nil {
+		t.Fatalf("parsing signed cert: %v", err)
+	}
+
+	for _, ext := range cert.Extensions {
+		if ext.Id.Equal(customOID) {
+			t.Errorf("custom extension (OID %s) should NOT be in cert when CopyExtensions is false", customOID)
+		}
+	}
+}
diff --git a/signer/signer.go b/signer/signer.go
@@ -23,6 +23,7 @@ import (
 	cferr "github.com/cloudflare/cfssl/errors"
 	"github.com/cloudflare/cfssl/helpers"
 	"github.com/cloudflare/cfssl/info"
+	"github.com/cloudflare/cfssl/log"
 )
 
 // Subject contains the information that should be used to override the
@@ -180,6 +181,47 @@ func isCommonAttr(t []int) bool {
 	return (len(t) == 4 && t[0] == 2 && t[1] == 5 && t[2] == 4 && (t[3] == 3 || (t[3] >= 5 && t[3] <= 11) || t[3] == 17))
 }
 
+// caMangedExtensionOIDs is the set of X.509v3 extension OIDs whose values are
+// authoritatively determined by the CA's signing profile (via FillTemplate) and
+// must never be copied from a CSR. Allowing a CSR to supply these via
+// ExtraExtensions would silently override the profile because Go's
+// x509.CreateCertificate gives ExtraExtensions precedence over struct fields
+// for the same OID.
+//
+// See: https://pkg.go.dev/crypto/x509#Certificate (ExtraExtensions field).
+var caManagedExtensionOIDs = map[string]bool{
+	// Key Usage (RFC 5280, 4.2.1.3) — set by profile.Usages()
+	asn1.ObjectIdentifier{2, 5, 29, 15}.String(): true,
+	// Extended Key Usage (RFC 5280, 4.2.1.12) — set by profile.Usages()
+	asn1.ObjectIdentifier{2, 5, 29, 37}.String(): true,
+	// Basic Constraints (RFC 5280, 4.2.1.9) — already handled specially above,
+	// but included for defense-in-depth
+	asn1.ObjectIdentifier{2, 5, 29, 19}.String(): true,
+	// Subject Key Identifier (RFC 5280, 4.2.1.2) — computed by FillTemplate
+	asn1.ObjectIdentifier{2, 5, 29, 14}.String(): true,
+	// Authority Key Identifier (RFC 5280, 4.2.1.1) — set by CreateCertificate
+	// from the issuer
+	asn1.ObjectIdentifier{2, 5, 29, 35}.String(): true,
+	// Authority Info Access / OCSP (RFC 5280, 4.2.2.1) — set by profile OCSP URL
+	asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 1, 1}.String(): true,
+	// CRL Distribution Points (RFC 5280, 4.2.1.13) — set by profile CRL URL
+	asn1.ObjectIdentifier{2, 5, 29, 31}.String(): true,
+	// Certificate Policies (RFC 5280, 4.2.1.4) — set by profile Policies
+	asn1.ObjectIdentifier{2, 5, 29, 32}.String(): true,
+	// Name Constraints (RFC 5280, 4.2.1.10) — CA-controlled
+	asn1.ObjectIdentifier{2, 5, 29, 30}.String(): true,
+	// Subject Alternative Name (RFC 5280, 4.2.1.6) — handled via Hosts/SANs
+	asn1.ObjectIdentifier{2, 5, 29, 17}.String(): true,
+	// Issuer Alternative Name (RFC 5280, 4.2.1.7) — CA-controlled
+	asn1.ObjectIdentifier{2, 5, 29, 18}.String(): true,
+}
+
+// isCAManagedExtension reports whether the given OID is for an extension whose
+// value is authoritatively set by the CA and must not be copied from a CSR.
+func isCaManagedExtension(oid asn1.ObjectIdentifier) bool {
+	return caManagedExtensionOIDs[oid.String()]
+}
+
 // ParseCertificateRequest takes an incoming certificate request and
 // builds a certificate template from it.
 func ParseCertificateRequest(s Signer, p *config.SigningProfile, csrBytes []byte) (template *x509.Certificate, err error) {
@@ -247,8 +289,16 @@ func ParseCertificateRequest(s Signer, p *config.SigningProfile, csrBytes []byte
 		} else if val.Id.Equal(helpers.DelegationUsage) {
 			template.ExtraExtensions = append(template.ExtraExtensions, val)
 		} else {
-			// If the profile has 'copy_extensions' to true then lets add it
+			// If the profile has 'copy_extensions' to true then copy the
+			// extension, but never copy CA-managed extensions (KeyUsage,
+			// ExtKeyUsage, SKI, AKI, etc.) whose values are set by the
+			// signing profile. Allowing them through would let a CSR
+			// override the CA's policy via ExtraExtensions precedence.
 			if p.CopyExtensions {
+				if isCaManagedExtension(val.Id) {
+					log.Warningf("copy_extensions: skipping CA-managed extension OID %s from CSR", val.Id)
+					continue
+				}
 				template.ExtraExtensions = append(template.ExtraExtensions, val)
 			}
 		}
diff --git a/signer/signer_test.go b/signer/signer_test.go
