Skip to content

Commit

Permalink
Fix handling of SignatureBits for ECDSA issuers (#14943) (#14994)
Browse files Browse the repository at this point in the history 
When adding SignatureBits control logic, we incorrectly allowed
specification of SignatureBits in the case of an ECDSA issuer. As noted
in the original request, NIST and Mozilla (and others) are fairly
prescriptive in the choice of signatures (matching the size of the
NIST P-curve), and we shouldn't usually use a smaller (or worse, larger
and truncate!) hash.

Ignore the configuration of signature bits and always use autodetection
for ECDSA like ed25519.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
  • Loading branch information
@cipherboy
cipherboy authored Apr 11, 2022
1 parent 5842976 commit dda23ae
Show file tree
Hide file tree
Showing 4 changed files with 104 additions and 70 deletions.
119 changes: 71 additions & 48 deletions builtin/logical/pki/backend_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4245,8 +4245,11 @@ func TestBackend_Roles_IssuanceRegression(t *testing.T) {
}

type KeySizeRegression struct {
RoleKeyType string
RoleKeyBits []int
// Values reused for both Role and CA configuration.
RoleKeyType string
RoleKeyBits []int

// Signature Bits presently is only specified on the role.
RoleSignatureBits []int

// These are tuples; must be of the same length.
Expand All @@ -4257,42 +4260,73 @@ type KeySizeRegression struct {
ExpectError bool
}

func (k KeySizeRegression) KeyTypeValues() []string {
if k.RoleKeyType == "any" {
return []string{"rsa", "ec", "ed25519"}
}

return []string{k.RoleKeyType}
}

func RoleKeySizeRegressionHelper(t *testing.T, client *api.Client, index int, test KeySizeRegression) int {
tested := 0

for _, roleKeyBits := range test.RoleKeyBits {
for _, roleSignatureBits := range test.RoleSignatureBits {
role := fmt.Sprintf("key-size-regression-%d-keytype-%v-keybits-%d-signature-bits-%d", index, test.RoleKeyType, roleKeyBits, roleSignatureBits)
resp, err := client.Logical().Write("pki/roles/"+role, map[string]interface{}{
"key_type": test.RoleKeyType,
"key_bits": roleKeyBits,
"signature_bits": roleSignatureBits,
for _, caKeyType := range test.KeyTypeValues() {
for _, caKeyBits := range test.RoleKeyBits {
// Generate a new CA key.
resp, err := client.Logical().Write("pki/root/generate/exported", map[string]interface{}{
"common_name": "myvault.com",
"ttl": "128h",
"key_type": caKeyType,
"key_bits": caKeyBits,
})
if err != nil {
t.Fatal(err)
}
if resp == nil {
t.Fatal("expected ca info")
}

for index, keyType := range test.TestKeyTypes {
keyBits := test.TestKeyBits[index]
for _, roleKeyBits := range test.RoleKeyBits {
for _, roleSignatureBits := range test.RoleSignatureBits {
role := fmt.Sprintf("key-size-regression-%d-keytype-%v-keybits-%d-signature-bits-%d", index, test.RoleKeyType, roleKeyBits, roleSignatureBits)
resp, err := client.Logical().Write("pki/roles/"+role, map[string]interface{}{
"key_type": test.RoleKeyType,
"key_bits": roleKeyBits,
"signature_bits": roleSignatureBits,
})
if err != nil {
t.Fatal(err)
}

_, _, csrPem := generateCSR(t, &x509.CertificateRequest{
Subject: pkix.Name{
CommonName: "localhost",
},
}, keyType, keyBits)
for index, keyType := range test.TestKeyTypes {
keyBits := test.TestKeyBits[index]

_, _, csrPem := generateCSR(t, &x509.CertificateRequest{
Subject: pkix.Name{
CommonName: "localhost",
},
}, keyType, keyBits)

resp, err = client.Logical().Write("pki/sign/"+role, map[string]interface{}{
"common_name": "localhost",
"csr": csrPem,
})
resp, err = client.Logical().Write("pki/sign/"+role, map[string]interface{}{
"common_name": "localhost",
"csr": csrPem,
})

haveErr := err != nil || resp == nil
haveErr := err != nil || resp == nil

if haveErr != test.ExpectError {
t.Fatalf("key size regression test [%d] failed: haveErr: %v, expectErr: %v, err: %v, resp: %v, test case: %v, role: %v, keyType: %v, keyBits: %v", index, haveErr, test.ExpectError, err, resp, test, role, keyType, keyBits)
if haveErr != test.ExpectError {
t.Fatalf("key size regression test [%d] failed: haveErr: %v, expectErr: %v, err: %v, resp: %v, test case: %v, caKeyType: %v, caKeyBits: %v, role: %v, keyType: %v, keyBits: %v", index, haveErr, test.ExpectError, err, resp, test, caKeyType, caKeyBits, role, keyType, keyBits)
}

tested += 1
}
}
}

tested += 1
_, err = client.Logical().Delete("pki/root")
if err != nil {
t.Fatal(err)
}
}
}
Expand All @@ -4312,26 +4346,29 @@ func TestBackend_Roles_KeySizeRegression(t *testing.T) {
// EC with default parameters should fail to issue smaller EC keys
// and any size RSA/Ed25519 keys.
/* 2 */ {"ec", []int{0}, []int{0}, []string{"rsa", "rsa", "rsa", "ec", "ed25519"}, []int{1024, 2048, 4096, 224, 0}, true},
// But it should work to issue larger EC keys.
/* 3 */ {"ec", []int{0, 256}, []int{0, 256}, []string{"ec"}, []int{256}, false},
/* 4 */ {"ec", []int{384}, []int{0, 384}, []string{"ec"}, []int{384}, false},
/* 5 */ {"ec", []int{521}, []int{0, 512}, []string{"ec"}, []int{521}, false},
// But it should work to issue larger EC keys. Note that we should be
// independent of signature bits as that's computed from the issuer
// type (for EC based issuers).
/* 3 */ {"ec", []int{224}, []int{0, 256, 384, 521}, []string{"ec", "ec", "ec", "ec"}, []int{224, 256, 384, 521}, false},
/* 4 */ {"ec", []int{0, 256}, []int{0, 256, 384, 521}, []string{"ec", "ec", "ec"}, []int{256, 384, 521}, false},
/* 5 */ {"ec", []int{384}, []int{0, 256, 384, 521}, []string{"ec", "ec"}, []int{384, 521}, false},
/* 6 */ {"ec", []int{521}, []int{0, 256, 384, 512}, []string{"ec"}, []int{521}, false},

// Ed25519 should reject RSA and EC keys.
/* 6 */ {"ed25519", []int{0}, []int{0}, []string{"rsa", "rsa", "rsa", "ec", "ec"}, []int{1024, 2048, 4096, 256, 521}, true},
/* 7 */ {"ed25519", []int{0}, []int{0}, []string{"rsa", "rsa", "rsa", "ec", "ec"}, []int{1024, 2048, 4096, 256, 521}, true},
// But it should work to issue Ed25519 keys.
/* 7 */ {"ed25519", []int{0}, []int{0}, []string{"ed25519"}, []int{0}, false},
/* 8 */ {"ed25519", []int{0}, []int{0}, []string{"ed25519"}, []int{0}, false},

// Any key type should reject insecure RSA key sizes.
/* 8 */ {"any", []int{0}, []int{0, 256, 384, 512}, []string{"rsa", "rsa"}, []int{512, 1024}, true},
/* 9 */ {"any", []int{0}, []int{0, 256, 384, 512}, []string{"rsa", "rsa"}, []int{512, 1024}, true},
// But work for everything else.
/* 9 */ {"any", []int{0}, []int{0, 256, 384, 512}, []string{"rsa", "rsa", "rsa", "ec", "ec", "ec", "ec", "ed25519"}, []int{2048, 3072, 4906, 224, 256, 384, 521, 0}, false},
/* 10 */ {"any", []int{0}, []int{0, 256, 384, 512}, []string{"rsa", "rsa", "rsa", "ec", "ec", "ec", "ec", "ed25519"}, []int{2048, 3072, 4906, 224, 256, 384, 521, 0}, false},

// RSA with larger than default key size should reject smaller ones.
/* 10 */ {"rsa", []int{3072}, []int{0, 256, 384, 512}, []string{"rsa", "rsa", "rsa"}, []int{512, 1024, 2048}, true},
/* 11 */ {"rsa", []int{3072}, []int{0, 256, 384, 512}, []string{"rsa", "rsa", "rsa"}, []int{512, 1024, 2048}, true},
}

if len(testCases) != 11 {
if len(testCases) != 12 {
t.Fatalf("misnumbered test case entries will make it hard to find bugs: %v", len(testCases))
}

Expand Down Expand Up @@ -4361,20 +4398,6 @@ func TestBackend_Roles_KeySizeRegression(t *testing.T) {
t.Fatal(err)
}

// We need a RSA key so all signature sizes are valid with it.
resp, err := client.Logical().Write("pki/root/generate/exported", map[string]interface{}{
"common_name": "myvault.com",
"ttl": "128h",
"key_type": "rsa",
"key_bits": 2048,
})
if err != nil {
t.Fatal(err)
}
if resp == nil {
t.Fatal("expected ca info")
}

tested := 0
for index, test := range testCases {
tested += RoleKeySizeRegressionHelper(t, client, index, test)
Expand Down
3 changes: 3 additions & 0 deletions changelog/14943.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
secrets/pki: Fixed bug where larger SHA-2 hashes were truncated with shorter ECDSA CA certificates
```
48 changes: 26 additions & 22 deletions sdk/helper/certutil/helpers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -585,17 +585,8 @@ func DefaultOrValueKeyBits(keyType string, keyBits int) (int, error) {
// certain internal circumstances.
func DefaultOrValueHashBits(keyType string, keyBits int, hashBits int) (int, error) {
if keyType == "ec" {
// To comply with BSI recommendations Section 4.2 and Mozilla root
// store policy section 5.1.2, enforce that NIST P-curves use a hash
// length corresponding to curve length. Note that ed25519 does not
// the "ec" key type.
expectedHashBits := expectedNISTPCurveHashBits[keyBits]

if expectedHashBits != hashBits && hashBits != 0 {
return hashBits, fmt.Errorf("unsupported signature hash algorithm length (%d) for NIST P-%d", hashBits, keyBits)
} else if hashBits == 0 {
hashBits = expectedHashBits
}
// Enforcement of curve moved to selectSignatureAlgorithmForECDSA. See
// note there about why.
} else if keyType == "rsa" && hashBits == 0 {
// To match previous behavior (and ignoring NIST's recommendations for
// hash size to align with RSA key sizes), default to SHA-2-256.
Expand Down Expand Up @@ -643,7 +634,7 @@ func ValidateDefaultOrValueKeyTypeSignatureLength(keyType string, keyBits int, h
// Validates that the length of the hash (in bits) used in the signature
// calculation is a known, approved value.
func ValidateSignatureLength(keyType string, hashBits int) error {
if keyType == "any" || keyType == "ed25519" || keyType == "ed448" {
if keyType == "any" || keyType == "ec" || keyType == "ed25519" || keyType == "ed448" {
// ed25519 and ed448 include built-in hashing and is not externally
// configurable. There are three modes for each of these schemes:
//
Expand All @@ -658,6 +649,10 @@ func ValidateSignatureLength(keyType string, hashBits int) error {
//
// Additionally, when KeyType is any, we can't yet validate the
// signature algorithm size, so it takes the default zero value.
//
// When KeyType is ec, we also can't validate this value as we're
// forcefully ignoring the users' choice and specifying a value based
// on issuer type.
return nil
}

Expand Down Expand Up @@ -863,16 +858,25 @@ func createCertificate(data *CreationBundle, randReader io.Reader, privateKeyGen
}

func selectSignatureAlgorithmForECDSA(pub crypto.PublicKey, signatureBits int) x509.SignatureAlgorithm {
// If signature bits are configured, prefer them to the default choice.
switch signatureBits {
case 256:
return x509.ECDSAWithSHA256
case 384:
return x509.ECDSAWithSHA384
case 512:
return x509.ECDSAWithSHA512
}

// Previously we preferred the user-specified signature bits for ECDSA
// keys. However, this could result in using a longer hash function than
// the underlying NIST P-curve will encode (e.g., a SHA-512 hash with a
// P-256 key). This isn't ideal: the hash is implicitly truncated
// (effectively turning it into SHA-512/256) and we then need to rely
// on the prefix security of the hash. Since both NIST and Mozilla guidance
// suggest instead using the correct hash function, we should prefer that
// over the operator-specified signatureBits.
//
// Lastly, note that pub above needs to be the _signer's_ public key;
// the issue with DefaultOrValueHashBits is that it is called at role
// configuration time, which might _precede_ issuer generation. Thus
// it only has access to the desired key type and not the actual issuer.
// The reference from that function is reproduced below:
//
// > To comply with BSI recommendations Section 4.2 and Mozilla root
// > store policy section 5.1.2, enforce that NIST P-curves use a hash
// > length corresponding to curve length. Note that ed25519 does not
// > implement the "ec" key type.
key, ok := pub.(*ecdsa.PublicKey)
if !ok {
return x509.ECDSAWithSHA256
Expand Down
4 changes: 4 additions & 0 deletions website/content/api-docs/secret/pki.mdx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -971,6 +971,10 @@ request is denied.
on key length (SHA-2-256 for RSA keys, and matching the curve size
for NIST P-Curves).

~> **Note**: ECDSA and Ed25519 issuers do not follow configuration of the
`signature_bits` value; only RSA issuers will change signature types
based on this parameter.

- `key_usage` `(list: ["DigitalSignature", "KeyAgreement", "KeyEncipherment"])` –
Specifies the allowed key usage constraint on issued certificates. Valid
values can be found at https://golang.org/pkg/crypto/x509/#KeyUsage - simply
Expand Down

0 comments on commit dda23ae

Please sign in to comment.