From 8833875b1071fcb8c2f16d82dc1a5919ff6534eb Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Tue, 26 Oct 2021 09:30:09 -0400 Subject: [PATCH] Fix PKI Weak Cryptographic Key Lenghths Warning (#12886) * Modernize SSH key lengths No default change was made in this commit; note that the code already enforced a default of 2048 bits. ssh-keygen and Go's RSA key generation allows for key sizes including 3072, 4096, 8192; update the values of SSH key generation to match PKI's allowed RSA key sizes (from certutil.ValidateKeyTypeLength(...)). We still allow the legacy SSH key size of 1024; in the near future we should likely remove it. Signed-off-by: Alexander Scheel * Ensure minimum of 2048-bit PKI RSA keys While the stated path is a false-positive, verifying all paths is non-trivial. We largely validate API call lengths using certutil.ValidateKeyTypeLength(...), but ensuring no other path calls certutil.generatePrivateKey(...) --- directly or indirectly --- is non-trivial. Thus enforcing a minimum in this method sounds like a sane compromise. Resolves: https://github.com/hashicorp/vault/security/code-scanning/55 Signed-off-by: Alexander Scheel --- builtin/logical/ssh/path_roles.go | 11 +++++------ sdk/helper/certutil/helpers.go | 11 +++++++++++ 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/builtin/logical/ssh/path_roles.go b/builtin/logical/ssh/path_roles.go index 265581ec05..021c787dba 100644 --- a/builtin/logical/ssh/path_roles.go +++ b/builtin/logical/ssh/path_roles.go @@ -461,16 +461,15 @@ func (b *backend) pathRoleWrite(ctx context.Context, req *logical.Request, d *fr return logical.ErrorResponse("missing admin username"), nil } - // This defaults to 1024 and it can also be 2048 and 4096. + // This defaults to 2048, but it can also be 1024, 3072, 4096, or 8192. + // In the near future, we should disallow 1024-bit SSH keys. keyBits := d.Get("key_bits").(int) - if keyBits != 0 && keyBits != 1024 && keyBits != 2048 && keyBits != 4096 { - return logical.ErrorResponse("invalid key_bits field"), nil - } - - // If user has not set this field, default it to 2048 if keyBits == 0 { keyBits = 2048 } + if keyBits != 1024 && keyBits != 2048 && keyBits != 3072 && keyBits != 4096 && keyBits != 8192 { + return logical.ErrorResponse("invalid key_bits field"), nil + } // Store all the fields required by dynamic key type roleEntry = sshRole{ diff --git a/sdk/helper/certutil/helpers.go b/sdk/helper/certutil/helpers.go index 9b7bb3cc3a..e8edcfd291 100644 --- a/sdk/helper/certutil/helpers.go +++ b/sdk/helper/certutil/helpers.go @@ -222,6 +222,17 @@ func generatePrivateKey(keyType string, keyBits int, container ParsedPrivateKeyC switch keyType { case "rsa": + // XXX: there is a false-positive CodeQL path here around keyBits; + // because of a default zero value in the TypeDurationSecond and + // TypeSignedDurationSecond cases of schema.DefaultOrZero(), it + // thinks it is possible to end up with < 2048 bit RSA Key here. + // While this is true for SSH keys, it isn't true for PKI keys + // due to ValidateKeyTypeLength(...) below. While we could close + // the report as a false-positive, enforcing a minimum keyBits size + // here of 2048 would ensure no other paths exist. + if keyBits < 2048 { + return errutil.InternalError{Err: fmt.Sprintf("insecure bit length for RSA private key: %d", keyBits)} + } privateKeyType = RSAPrivateKey privateKey, err = rsa.GenerateKey(randReader, keyBits) if err != nil {