Skip to content

Commit e429e72

Browse files
mitch292mitch292
authored
Merge pull request #1436 from mitch292/fix-copy-extensions-security-bypass
fix: prevent CSR extensions from overriding CA-managed key usage in c…
2 parents ed8df49 + 8994bdd commit e429e72

3 files changed

Lines changed: 387 additions & 1 deletion

File tree

‎signer/local/local_test.go‎

Lines changed: 304 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1636,3 +1636,307 @@ func TestLint(t *testing.T) {
16361636
})
16371637
}
16381638
}
1639+
1640+
// createCSRWithExtensions generates a CSR containing the given raw X.509v3
1641+
// extensions. This simulates an attacker injecting extensions into a CSR.
1642+
func createCSRWithExtensions(t *testing.T, extraExts []pkix.Extension) []byte {
1643+
t.Helper()
1644+
key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
1645+
if err != nil {
1646+
t.Fatalf("generating key: %v", err)
1647+
}
1648+
1649+
tmpl := &x509.CertificateRequest{
1650+
Subject: pkix.Name{CommonName: "test.example.com"},
1651+
DNSNames: []string{"test.example.com"},
1652+
ExtraExtensions: extraExts,
1653+
}
1654+
1655+
csrDER, err := x509.CreateCertificateRequest(rand.Reader, tmpl, key)
1656+
if err != nil {
1657+
t.Fatalf("creating CSR: %v", err)
1658+
}
1659+
1660+
return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE REQUEST", Bytes: csrDER})
1661+
}
1662+
1663+
// marshalKeyUsage encodes a KeyUsage bitstring into the DER value expected
1664+
// inside an X.509v3 KeyUsage extension (OID 2.5.29.15).
1665+
func marshalKeyUsage(t *testing.T, ku x509.KeyUsage) []byte {
1666+
t.Helper()
1667+
// KeyUsage is a bit string where bits are numbered from the MSB.
1668+
// Go's x509.KeyUsage constants are already bit positions from MSB.
1669+
var b [2]byte
1670+
b[0] = byte(ku >> 8) // not used for standard usages below bit 8
1671+
// Standard key usages fit in one byte for the common ones:
1672+
// digitalSignature(0), contentCommitment(1), keyEncipherment(2),
1673+
// dataEncipherment(3), keyAgreement(4), keyCertSign(5), cRLSign(6),
1674+
// encipherOnly(7)
1675+
b[0] = 0
1676+
b[1] = 0
1677+
if ku&x509.KeyUsageDigitalSignature != 0 {
1678+
b[0] |= 0x80
1679+
}
1680+
if ku&x509.KeyUsageContentCommitment != 0 {
1681+
b[0] |= 0x40
1682+
}
1683+
if ku&x509.KeyUsageKeyEncipherment != 0 {
1684+
b[0] |= 0x20
1685+
}
1686+
if ku&x509.KeyUsageDataEncipherment != 0 {
1687+
b[0] |= 0x10
1688+
}
1689+
if ku&x509.KeyUsageKeyAgreement != 0 {
1690+
b[0] |= 0x08
1691+
}
1692+
if ku&x509.KeyUsageCertSign != 0 {
1693+
b[0] |= 0x04
1694+
}
1695+
if ku&x509.KeyUsageCRLSign != 0 {
1696+
b[0] |= 0x02
1697+
}
1698+
if ku&x509.KeyUsageEncipherOnly != 0 {
1699+
b[0] |= 0x01
1700+
}
1701+
if ku&x509.KeyUsageDecipherOnly != 0 {
1702+
b[1] |= 0x80
1703+
}
1704+
1705+
// Determine padding bits: find the lowest set bit
1706+
var usedBytes int
1707+
var padBits int
1708+
if b[1] != 0 {
1709+
usedBytes = 2
1710+
for i := 0; i < 8; i++ {
1711+
if b[1]&(1<<uint(i)) != 0 {
1712+
padBits = i
1713+
break
1714+
}
1715+
}
1716+
} else {
1717+
usedBytes = 1
1718+
for i := 0; i < 8; i++ {
1719+
if b[0]&(1<<uint(i)) != 0 {
1720+
padBits = i
1721+
break
1722+
}
1723+
}
1724+
}
1725+
1726+
bs := asn1.BitString{
1727+
Bytes: b[:usedBytes],
1728+
BitLength: usedBytes*8 - padBits,
1729+
}
1730+
val, err := asn1.Marshal(bs)
1731+
if err != nil {
1732+
t.Fatalf("marshaling KeyUsage: %v", err)
1733+
}
1734+
return val
1735+
}
1736+
1737+
// marshalExtKeyUsage encodes ExtKeyUsage OIDs into the DER value expected
1738+
// inside an X.509v3 ExtKeyUsage extension (OID 2.5.29.37).
1739+
func marshalExtKeyUsage(t *testing.T, ekus []asn1.ObjectIdentifier) []byte {
1740+
t.Helper()
1741+
val, err := asn1.Marshal(ekus)
1742+
if err != nil {
1743+
t.Fatalf("marshaling ExtKeyUsage: %v", err)
1744+
}
1745+
return val
1746+
}
1747+
1748+
// TestCopyExtensionsDoesNotOverrideKeyUsage verifies that when
1749+
// copy_extensions is enabled, a CSR containing injected KeyUsage and
1750+
// ExtKeyUsage extensions cannot override the signing profile's intended
1751+
// key usage values. This is a regression test for a vulnerability where
1752+
// Go's x509.CreateCertificate gave ExtraExtensions precedence over
1753+
// struct fields, allowing CSR-provided KU/EKU to bypass profile restrictions.
1754+
func TestCopyExtensionsDoesNotOverrideKeyUsage(t *testing.T) {
1755+
// Profile allows only: digitalSignature + serverAuth
1756+
profilePolicy := &config.Signing{
1757+
Default: &config.SigningProfile{
1758+
Usage: []string{"digital signature", "server auth"},
1759+
Expiry: 1 * time.Hour,
1760+
ExpiryString: "1h",
1761+
CopyExtensions: true,
1762+
},
1763+
}
1764+
1765+
s, err := NewSignerFromFile(testCaFile, testCaKeyFile, profilePolicy)
1766+
if err != nil {
1767+
t.Fatalf("creating signer: %v", err)
1768+
}
1769+
1770+
// Attacker CSR injects keyCertSign + cRLSign and codeSigning + emailProtection
1771+
attackerKU := x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign | x509.KeyUsageCRLSign
1772+
attackerEKUs := []asn1.ObjectIdentifier{
1773+
{1, 3, 6, 1, 5, 5, 7, 3, 3}, // codeSigning
1774+
{1, 3, 6, 1, 5, 5, 7, 3, 4}, // emailProtection
1775+
}
1776+
1777+
csrPEM := createCSRWithExtensions(t, []pkix.Extension{
1778+
{
1779+
Id: asn1.ObjectIdentifier{2, 5, 29, 15}, // KeyUsage
1780+
Critical: true,
1781+
Value: marshalKeyUsage(t, attackerKU),
1782+
},
1783+
{
1784+
Id: asn1.ObjectIdentifier{2, 5, 29, 37}, // ExtKeyUsage
1785+
Critical: false,
1786+
Value: marshalExtKeyUsage(t, attackerEKUs),
1787+
},
1788+
})
1789+
1790+
certPEM, err := s.Sign(signer.SignRequest{
1791+
Hosts: []string{"test.example.com"},
1792+
Request: string(csrPEM),
1793+
})
1794+
if err != nil {
1795+
t.Fatalf("signing: %v", err)
1796+
}
1797+
1798+
cert, err := helpers.ParseCertificatePEM(certPEM)
1799+
if err != nil {
1800+
t.Fatalf("parsing signed cert: %v", err)
1801+
}
1802+
1803+
// The cert MUST have only the profile's KeyUsage (digitalSignature),
1804+
// NOT the attacker's keyCertSign or cRLSign.
1805+
if cert.KeyUsage&x509.KeyUsageCertSign != 0 {
1806+
t.Errorf("certificate has keyCertSign — attacker's KeyUsage was not filtered")
1807+
}
1808+
if cert.KeyUsage&x509.KeyUsageCRLSign != 0 {
1809+
t.Errorf("certificate has cRLSign — attacker's KeyUsage was not filtered")
1810+
}
1811+
if cert.KeyUsage&x509.KeyUsageDigitalSignature == 0 {
1812+
t.Errorf("certificate missing digitalSignature from profile")
1813+
}
1814+
1815+
// The cert MUST have only the profile's EKU (serverAuth),
1816+
// NOT the attacker's codeSigning or emailProtection.
1817+
for _, eku := range cert.ExtKeyUsage {
1818+
switch eku {
1819+
case x509.ExtKeyUsageCodeSigning:
1820+
t.Errorf("certificate has codeSigning EKU — attacker's ExtKeyUsage was not filtered")
1821+
case x509.ExtKeyUsageEmailProtection:
1822+
t.Errorf("certificate has emailProtection EKU — attacker's ExtKeyUsage was not filtered")
1823+
}
1824+
}
1825+
1826+
foundServerAuth := false
1827+
for _, eku := range cert.ExtKeyUsage {
1828+
if eku == x509.ExtKeyUsageServerAuth {
1829+
foundServerAuth = true
1830+
break
1831+
}
1832+
}
1833+
if !foundServerAuth {
1834+
t.Errorf("certificate missing serverAuth EKU from profile")
1835+
}
1836+
}
1837+
1838+
// TestCopyExtensionsAllowsNonManagedOIDs verifies that CopyExtensions
1839+
// still copies through non-security-critical, non-CA-managed extensions
1840+
// (e.g., private-use OIDs). This ensures the blocklist does not break
1841+
// legitimate use cases for copy_extensions.
1842+
func TestCopyExtensionsAllowsNonManagedOIDs(t *testing.T) {
1843+
profilePolicy := &config.Signing{
1844+
Default: &config.SigningProfile{
1845+
Usage: []string{"digital signature", "server auth"},
1846+
Expiry: 1 * time.Hour,
1847+
ExpiryString: "1h",
1848+
CopyExtensions: true,
1849+
},
1850+
}
1851+
1852+
s, err := NewSignerFromFile(testCaFile, testCaKeyFile, profilePolicy)
1853+
if err != nil {
1854+
t.Fatalf("creating signer: %v", err)
1855+
}
1856+
1857+
// A private-use extension that should be copied through
1858+
customOID := asn1.ObjectIdentifier{1, 2, 3, 4, 5, 6, 7}
1859+
customValue, _ := asn1.Marshal("custom-extension-value")
1860+
1861+
csrPEM := createCSRWithExtensions(t, []pkix.Extension{
1862+
{
1863+
Id: customOID,
1864+
Value: customValue,
1865+
},
1866+
})
1867+
1868+
certPEM, err := s.Sign(signer.SignRequest{
1869+
Hosts: []string{"test.example.com"},
1870+
Request: string(csrPEM),
1871+
})
1872+
if err != nil {
1873+
t.Fatalf("signing: %v", err)
1874+
}
1875+
1876+
cert, err := helpers.ParseCertificatePEM(certPEM)
1877+
if err != nil {
1878+
t.Fatalf("parsing signed cert: %v", err)
1879+
}
1880+
1881+
found := false
1882+
for _, ext := range cert.Extensions {
1883+
if ext.Id.Equal(customOID) {
1884+
found = true
1885+
if !bytes.Equal(ext.Value, customValue) {
1886+
t.Errorf("custom extension value mismatch: got %x, want %x", ext.Value, customValue)
1887+
}
1888+
break
1889+
}
1890+
}
1891+
if !found {
1892+
t.Errorf("custom extension (OID %s) was not copied into the certificate", customOID)
1893+
}
1894+
}
1895+
1896+
// TestCopyExtensionsDisabledDoesNotCopy verifies that when copy_extensions
1897+
// is false (the default), CSR extensions are NOT copied — whether they are
1898+
// CA-managed or not.
1899+
func TestCopyExtensionsDisabledDoesNotCopy(t *testing.T) {
1900+
profilePolicy := &config.Signing{
1901+
Default: &config.SigningProfile{
1902+
Usage: []string{"digital signature", "server auth"},
1903+
Expiry: 1 * time.Hour,
1904+
ExpiryString: "1h",
1905+
CopyExtensions: false,
1906+
},
1907+
}
1908+
1909+
s, err := NewSignerFromFile(testCaFile, testCaKeyFile, profilePolicy)
1910+
if err != nil {
1911+
t.Fatalf("creating signer: %v", err)
1912+
}
1913+
1914+
customOID := asn1.ObjectIdentifier{1, 2, 3, 4, 5, 6, 7}
1915+
customValue, _ := asn1.Marshal("should-not-appear")
1916+
1917+
csrPEM := createCSRWithExtensions(t, []pkix.Extension{
1918+
{
1919+
Id: customOID,
1920+
Value: customValue,
1921+
},
1922+
})
1923+
1924+
certPEM, err := s.Sign(signer.SignRequest{
1925+
Hosts: []string{"test.example.com"},
1926+
Request: string(csrPEM),
1927+
})
1928+
if err != nil {
1929+
t.Fatalf("signing: %v", err)
1930+
}
1931+
1932+
cert, err := helpers.ParseCertificatePEM(certPEM)
1933+
if err != nil {
1934+
t.Fatalf("parsing signed cert: %v", err)
1935+
}
1936+
1937+
for _, ext := range cert.Extensions {
1938+
if ext.Id.Equal(customOID) {
1939+
t.Errorf("custom extension (OID %s) should NOT be in cert when CopyExtensions is false", customOID)
1940+
}
1941+
}
1942+
}

‎signer/signer.go‎

Lines changed: 51 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
cferr "github.com/cloudflare/cfssl/errors"
2424
"github.com/cloudflare/cfssl/helpers"
2525
"github.com/cloudflare/cfssl/info"
26+
"github.com/cloudflare/cfssl/log"
2627
)
2728

2829
// Subject contains the information that should be used to override the
@@ -180,6 +181,47 @@ func isCommonAttr(t []int) bool {
180181
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))
181182
}
182183

184+
// caMangedExtensionOIDs is the set of X.509v3 extension OIDs whose values are
185+
// authoritatively determined by the CA's signing profile (via FillTemplate) and
186+
// must never be copied from a CSR. Allowing a CSR to supply these via
187+
// ExtraExtensions would silently override the profile because Go's
188+
// x509.CreateCertificate gives ExtraExtensions precedence over struct fields
189+
// for the same OID.
190+
//
191+
// See: https://pkg.go.dev/crypto/x509#Certificate (ExtraExtensions field).
192+
var caManagedExtensionOIDs = map[string]bool{
193+
// Key Usage (RFC 5280, 4.2.1.3) — set by profile.Usages()
194+
asn1.ObjectIdentifier{2, 5, 29, 15}.String(): true,
195+
// Extended Key Usage (RFC 5280, 4.2.1.12) — set by profile.Usages()
196+
asn1.ObjectIdentifier{2, 5, 29, 37}.String(): true,
197+
// Basic Constraints (RFC 5280, 4.2.1.9) — already handled specially above,
198+
// but included for defense-in-depth
199+
asn1.ObjectIdentifier{2, 5, 29, 19}.String(): true,
200+
// Subject Key Identifier (RFC 5280, 4.2.1.2) — computed by FillTemplate
201+
asn1.ObjectIdentifier{2, 5, 29, 14}.String(): true,
202+
// Authority Key Identifier (RFC 5280, 4.2.1.1) — set by CreateCertificate
203+
// from the issuer
204+
asn1.ObjectIdentifier{2, 5, 29, 35}.String(): true,
205+
// Authority Info Access / OCSP (RFC 5280, 4.2.2.1) — set by profile OCSP URL
206+
asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 1, 1}.String(): true,
207+
// CRL Distribution Points (RFC 5280, 4.2.1.13) — set by profile CRL URL
208+
asn1.ObjectIdentifier{2, 5, 29, 31}.String(): true,
209+
// Certificate Policies (RFC 5280, 4.2.1.4) — set by profile Policies
210+
asn1.ObjectIdentifier{2, 5, 29, 32}.String(): true,
211+
// Name Constraints (RFC 5280, 4.2.1.10) — CA-controlled
212+
asn1.ObjectIdentifier{2, 5, 29, 30}.String(): true,
213+
// Subject Alternative Name (RFC 5280, 4.2.1.6) — handled via Hosts/SANs
214+
asn1.ObjectIdentifier{2, 5, 29, 17}.String(): true,
215+
// Issuer Alternative Name (RFC 5280, 4.2.1.7) — CA-controlled
216+
asn1.ObjectIdentifier{2, 5, 29, 18}.String(): true,
217+
}
218+
219+
// isCAManagedExtension reports whether the given OID is for an extension whose
220+
// value is authoritatively set by the CA and must not be copied from a CSR.
221+
func isCaManagedExtension(oid asn1.ObjectIdentifier) bool {
222+
return caManagedExtensionOIDs[oid.String()]
223+
}
224+
183225
// ParseCertificateRequest takes an incoming certificate request and
184226
// builds a certificate template from it.
185227
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
247289
} else if val.Id.Equal(helpers.DelegationUsage) {
248290
template.ExtraExtensions = append(template.ExtraExtensions, val)
249291
} else {
250-
// If the profile has 'copy_extensions' to true then lets add it
292+
// If the profile has 'copy_extensions' to true then copy the
293+
// extension, but never copy CA-managed extensions (KeyUsage,
294+
// ExtKeyUsage, SKI, AKI, etc.) whose values are set by the
295+
// signing profile. Allowing them through would let a CSR
296+
// override the CA's policy via ExtraExtensions precedence.
251297
if p.CopyExtensions {
298+
if isCaManagedExtension(val.Id) {
299+
log.Warningf("copy_extensions: skipping CA-managed extension OID %s from CSR", val.Id)
300+
continue
301+
}
252302
template.ExtraExtensions = append(template.ExtraExtensions, val)
253303
}
254304
}

0 commit comments

Comments
 (0)