From cd213f5fca29e9850b6339f9cf609358af4d2301 Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Fri, 12 Nov 2021 12:18:38 -0500 Subject: [PATCH] Restrict ECDSA/NIST P-Curve hash function sizes for cert signing (#12872) * Restrict ECDSA signatures with NIST P-Curve hashes When using an ECDSA signature with a NIST P-Curve, we should follow recommendations from BIS (Section 4.2) and Mozilla's root store policy (section 5.1.2) to ensure that arbitrary selection of signature_bits does not exceed what the curve is capable of signing. Related: #11245 Signed-off-by: Alexander Scheel * Switch to certutil.ValidateKeyTypeSignatureLength(...) Replaces previous calls to certutil.ValidateKeyTypeLength(...) and certutil.ValidateSignatureLength(...) with a single call, allowing for curve<->hash validation. Signed-off-by: Alexander Scheel * Switch to autodetection of signature_bits This enables detection of whether the caller manually specified a value for signature_bits or not; when not manually specified, we can provision a value that complies with new NIST P-Curve policy. Signed-off-by: Alexander Scheel * Select hash function length automatically Due to our change in behavior (to default to -1 as the value to signature_bits to allow for automatic hash selection), switch ValidateKeyTypeSignatureLength(...) to accept a pointer to hashBits and provision it with valid default values. Signed-off-by: Alexander Scheel * Prevent invalid Curve size lookups Signed-off-by: Alexander Scheel * Switch from -1 to 0 as default SignatureBits Signed-off-by: Alexander Scheel * Add changelog entry Signed-off-by: Alexander Scheel --- builtin/logical/pki/backend_test.go | 1 - builtin/logical/pki/ca_util.go | 7 +--- builtin/logical/pki/fields.go | 10 ++--- builtin/logical/pki/path_roles.go | 13 +++--- changelog/12872.txt | 3 ++ sdk/helper/certutil/helpers.go | 64 +++++++++++++++++++++++++---- 6 files changed, 69 insertions(+), 29 deletions(-) create mode 100644 changelog/12872.txt diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 2860b5e50..17dd6c11b 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -827,7 +827,6 @@ func generateRoleSteps(t *testing.T, useCSRs bool) []logicaltest.TestStep { KeyType: "rsa", KeyBits: 2048, RequireCN: true, - SignatureBits: 256, } issueVals := certutil.IssueData{} ret := []logicaltest.TestStep{} diff --git a/builtin/logical/pki/ca_util.go b/builtin/logical/pki/ca_util.go index ea6b083c7..e5009386f 100644 --- a/builtin/logical/pki/ca_util.go +++ b/builtin/logical/pki/ca_util.go @@ -55,14 +55,9 @@ func (b *backend) getGenerationParams( return } - if err := certutil.ValidateKeyTypeLength(role.KeyType, role.KeyBits); err != nil { + if err := certutil.ValidateKeyTypeSignatureLength(role.KeyType, role.KeyBits, &role.SignatureBits); err != nil { errorResp = logical.ErrorResponse(err.Error()) } - if err := certutil.ValidateSignatureLength(role.SignatureBits); err != nil { - errorResp = logical.ErrorResponse(err.Error()) - return - } - return } diff --git a/builtin/logical/pki/fields.go b/builtin/logical/pki/fields.go index 9e191f3ad..fcb01d07b 100644 --- a/builtin/logical/pki/fields.go +++ b/builtin/logical/pki/fields.go @@ -261,13 +261,13 @@ the key_type.`, fields["signature_bits"] = &framework.FieldSchema{ Type: framework.TypeInt, - Default: 256, + Default: 0, Description: `The number of bits to use in the signature -algorithm. Defaults to 256 for SHA256. -Set to 384 for SHA384 and 512 for SHA512. -`, +algorithm; accepts 256 for SHA-2-256, 384 for SHA-2-384, and 512 for +SHA-2-512. Defaults to 0 to automatically detect based on key length +(SHA-2-256 for RSA keys, and matching the curve size for NIST P-Curves).`, DisplayAttrs: &framework.DisplayAttributes{ - Value: 256, + Value: 0, }, } diff --git a/builtin/logical/pki/path_roles.go b/builtin/logical/pki/path_roles.go index a240a6a3c..aa77dcf57 100644 --- a/builtin/logical/pki/path_roles.go +++ b/builtin/logical/pki/path_roles.go @@ -207,10 +207,11 @@ the key_type.`, "signature_bits": &framework.FieldSchema{ Type: framework.TypeInt, - Default: 256, + Default: 0, Description: `The number of bits to use in the signature -algorithm. Defaults to 256 for SHA256. -Set to 384 for SHA384 and 512 for SHA512.`, +algorithm; accepts 256 for SHA-2-256, 384 for SHA-2-384, and 512 for +SHA-2-512. Defaults to 0 to automatically detect based on key length +(SHA-2-256 for RSA keys, and matching the curve size for NIST P-Curves).`, }, "key_usage": &framework.FieldSchema{ @@ -624,11 +625,7 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data ), nil } - if err := certutil.ValidateKeyTypeLength(entry.KeyType, entry.KeyBits); err != nil { - return logical.ErrorResponse(err.Error()), nil - } - - if err := certutil.ValidateSignatureLength(entry.SignatureBits); err != nil { + if err := certutil.ValidateKeyTypeSignatureLength(entry.KeyType, entry.KeyBits, &entry.SignatureBits); err != nil { return logical.ErrorResponse(err.Error()), nil } diff --git a/changelog/12872.txt b/changelog/12872.txt new file mode 100644 index 000000000..6a6157ce4 --- /dev/null +++ b/changelog/12872.txt @@ -0,0 +1,3 @@ +```release-note:bug +secrets/pki: Fixes around NIST P-curve signature hash length, default value for signature_bits changed to 0. +``` diff --git a/sdk/helper/certutil/helpers.go b/sdk/helper/certutil/helpers.go index 4ce36b6d6..18012b994 100644 --- a/sdk/helper/certutil/helpers.go +++ b/sdk/helper/certutil/helpers.go @@ -33,6 +33,14 @@ import ( cbasn1 "golang.org/x/crypto/cryptobyte/asn1" ) +// Mapping of NIST P-Curve's key length to expected signature bits. +var expectedNISTPCurveHashBits = map[int]int{ + 224: 256, + 256: 256, + 384: 384, + 521: 512, +} + // GetHexFormatted returns the byte buffer formatted in hex with // the specified separator between bytes. func GetHexFormatted(buf []byte, sep string) string { @@ -525,13 +533,55 @@ func StringToOid(in string) (asn1.ObjectIdentifier, error) { return asn1.ObjectIdentifier(ret), nil } -func ValidateSignatureLength(keyBits int) error { - switch keyBits { +// Validates that the combination of keyType, keyBits, and hashBits are +// valid together; replaces individual calls to ValidateSignatureLength and +// ValidateKeyTypeLength. +func ValidateKeyTypeSignatureLength(keyType string, keyBits int, hashBits *int) error { + if err := ValidateKeyTypeLength(keyType, keyBits); err != nil { + return err + } + + 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 fmt.Errorf("unsupported signature hash algorithm length (%d) for NIST P-%d", *hashBits, keyBits) + } else if *hashBits == 0 { + *hashBits = expectedHashBits + } + } else if keyType == "rsa" && *hashBits == 0 { + // To match previous behavior (and ignoring recommendations of hash + // size to match RSA key sizes), default to SHA-2-256. + *hashBits = 256 + } /* else if keyType == "ed25519" { + // No-op; ed25519 and ed448 internally specify their own hash and + // we do not need to select one. Double hashing isn't supported in + // certificate signing. + } */ + + // Note that this check must come after we've selected a value for + // hashBits above, in the event it was left as the default, but we + // were allowed to update it. + if err := ValidateSignatureLength(*hashBits); err != nil || *hashBits == 0 { + return err + } + + return nil +} + +// Validates that the length of the hash (in bits) used in the signature +// calculation is a known, approved value. +func ValidateSignatureLength(hashBits int) error { + switch hashBits { case 256: case 384: case 512: default: - return fmt.Errorf("unsupported signature algorithm: %d", keyBits) + return fmt.Errorf("unsupported hash signature algorithm: %d", hashBits) } return nil } @@ -548,12 +598,8 @@ func ValidateKeyTypeLength(keyType string, keyBits int) error { return fmt.Errorf("unsupported bit length for RSA key: %d", keyBits) } case "ec": - switch keyBits { - case 224: - case 256: - case 384: - case 521: - default: + _, present := expectedNISTPCurveHashBits[keyBits] + if !present { return fmt.Errorf("unsupported bit length for EC key: %d", keyBits) } case "any", "ed25519":