From bdee24128acd5a7201c53a135cf3afbdc15ad938 Mon Sep 17 00:00:00 2001 From: hc-github-team-secure-vault-core <82990506+hc-github-team-secure-vault-core@users.noreply.github.com> Date: Fri, 8 Sep 2023 10:48:26 -0400 Subject: [PATCH] backport of commit 7d4d8cb708de62167340fa84770f8237c7bfdd1e (#22900) Co-authored-by: Scott Miller --- builtin/logical/transit/backend_test.go | 25 +++--- builtin/logical/transit/path_datakey.go | 4 + builtin/logical/transit/path_encrypt.go | 26 +++++++ builtin/logical/transit/path_encrypt_test.go | 81 +++++++++++++++++++- builtin/logical/transit/path_rewrap.go | 8 ++ changelog/22852.txt | 3 + sdk/helper/keysutil/policy.go | 8 +- 7 files changed, 142 insertions(+), 13 deletions(-) create mode 100644 changelog/22852.txt diff --git a/builtin/logical/transit/backend_test.go b/builtin/logical/transit/backend_test.go index d23c19465..2ee9e9b6b 100644 --- a/builtin/logical/transit/backend_test.go +++ b/builtin/logical/transit/backend_test.go @@ -1153,10 +1153,12 @@ func testConvergentEncryptionCommon(t *testing.T, ver int, keyType keysutil.KeyT // Now test encrypting the same value twice req.Data = map[string]interface{}{ - "plaintext": "emlwIHphcA==", // "zip zap" - "nonce": "b25ldHdvdGhyZWVl", // "onetwothreee" + "plaintext": "emlwIHphcA==", // "zip zap" "context": "pWZ6t/im3AORd0lVYE0zBdKpX6Bl3/SvFtoVTPWbdkzjG788XmMAnOlxandSdd7S", } + if ver == 0 { + req.Data["nonce"] = "b25ldHdvdGhyZWVl" // "onetwothreee" + } resp, err = b.HandleRequest(context.Background(), req) if err != nil { t.Fatal(err) @@ -1187,11 +1189,10 @@ func testConvergentEncryptionCommon(t *testing.T, ver int, keyType keysutil.KeyT // For sanity, also check a different nonce value... req.Data = map[string]interface{}{ - "plaintext": "emlwIHphcA==", // "zip zap" - "nonce": "dHdvdGhyZWVmb3Vy", // "twothreefour" + "plaintext": "emlwIHphcA==", // "zip zap" "context": "pWZ6t/im3AORd0lVYE0zBdKpX6Bl3/SvFtoVTPWbdkzjG788XmMAnOlxandSdd7S", } - if ver < 2 { + if ver == 0 { req.Data["nonce"] = "dHdvdGhyZWVmb3Vy" // "twothreefour" } else { req.Data["context"] = "pWZ6t/im3AORd0lVYE0zBdKpX6Bl3/SvFtoVTPWbdkzjG788XmMAnOldandSdd7S" @@ -1230,10 +1231,12 @@ func testConvergentEncryptionCommon(t *testing.T, ver int, keyType keysutil.KeyT // ...and a different context value req.Data = map[string]interface{}{ - "plaintext": "emlwIHphcA==", // "zip zap" - "nonce": "dHdvdGhyZWVmb3Vy", // "twothreefour" + "plaintext": "emlwIHphcA==", // "zip zap" "context": "qV4h9iQyvn+raODOer4JNAsOhkXBwdT4HZ677Ql4KLqXSU+Jk4C/fXBWbv6xkSYT", } + if ver == 0 { + req.Data["nonce"] = "dHdvdGhyZWVmb3Vy" // "twothreefour" + } resp, err = b.HandleRequest(context.Background(), req) if err != nil { t.Fatal(err) @@ -1345,9 +1348,11 @@ func testConvergentEncryptionCommon(t *testing.T, ver int, keyType keysutil.KeyT // Finally, check operations on empty values // First, check without setting a plaintext at all req.Data = map[string]interface{}{ - "nonce": "b25ldHdvdGhyZWVl", // "onetwothreee" "context": "pWZ6t/im3AORd0lVYE0zBdKpX6Bl3/SvFtoVTPWbdkzjG788XmMAnOlxandSdd7S", } + if ver == 0 { + req.Data["nonce"] = "dHdvdGhyZWVmb3Vy" // "twothreefour" + } resp, err = b.HandleRequest(context.Background(), req) if err == nil { t.Fatal("expected error, got nil") @@ -1362,9 +1367,11 @@ func testConvergentEncryptionCommon(t *testing.T, ver int, keyType keysutil.KeyT // Now set plaintext to empty req.Data = map[string]interface{}{ "plaintext": "", - "nonce": "b25ldHdvdGhyZWVl", // "onetwothreee" "context": "pWZ6t/im3AORd0lVYE0zBdKpX6Bl3/SvFtoVTPWbdkzjG788XmMAnOlxandSdd7S", } + if ver == 0 { + req.Data["nonce"] = "dHdvdGhyZWVmb3Vy" // "twothreefour" + } resp, err = b.HandleRequest(context.Background(), req) if err != nil { t.Fatal(err) diff --git a/builtin/logical/transit/path_datakey.go b/builtin/logical/transit/path_datakey.go index 1c607d0e2..774ffd480 100644 --- a/builtin/logical/transit/path_datakey.go +++ b/builtin/logical/transit/path_datakey.go @@ -170,6 +170,10 @@ func (b *backend) pathDatakeyWrite(ctx context.Context, req *logical.Request, d }, } + if len(nonce) > 0 && !nonceAllowed(p) { + return nil, ErrNonceNotAllowed + } + if constants.IsFIPS() && shouldWarnAboutNonceUsage(p, nonce) { resp.AddWarning("A provided nonce value was used within FIPS mode, this violates FIPS 140 compliance.") } diff --git a/builtin/logical/transit/path_encrypt.go b/builtin/logical/transit/path_encrypt.go index 78b43a64c..f7bdf9c68 100644 --- a/builtin/logical/transit/path_encrypt.go +++ b/builtin/logical/transit/path_encrypt.go @@ -468,6 +468,13 @@ func (b *backend) pathEncryptWrite(ctx context.Context, req *logical.Request, d successesInBatch := false for i, item := range batchInputItems { if batchResponseItems[i].Error != "" { + userErrorInBatch = true + continue + } + + if item.Nonce != "" && !nonceAllowed(p) { + userErrorInBatch = true + batchResponseItems[i].Error = ErrNonceNotAllowed.Error() continue } @@ -568,6 +575,25 @@ func (b *backend) pathEncryptWrite(ctx context.Context, req *logical.Request, d return batchRequestResponse(d, resp, req, successesInBatch, userErrorInBatch, internalErrorInBatch) } +func nonceAllowed(p *keysutil.Policy) bool { + var supportedKeyType bool + switch p.Type { + case keysutil.KeyType_MANAGED_KEY: + return true + case keysutil.KeyType_AES128_GCM96, keysutil.KeyType_AES256_GCM96, keysutil.KeyType_ChaCha20_Poly1305: + supportedKeyType = true + default: + supportedKeyType = false + } + + if supportedKeyType && p.ConvergentEncryption && p.ConvergentVersion == 1 { + // We only use the user supplied nonce for v1 convergent encryption keys + return true + } + + return false +} + // Depending on the errors in the batch, different status codes should be returned. User errors // will return a 400 and precede internal errors which return a 500. The reasoning behind this is // that user errors are non-retryable without making changes to the request, and should be surfaced diff --git a/builtin/logical/transit/path_encrypt_test.go b/builtin/logical/transit/path_encrypt_test.go index b886d4fef..c0e0f9c01 100644 --- a/builtin/logical/transit/path_encrypt_test.go +++ b/builtin/logical/transit/path_encrypt_test.go @@ -7,6 +7,7 @@ import ( "context" "encoding/json" "fmt" + "net/http" "reflect" "strings" "testing" @@ -654,13 +655,26 @@ func TestTransit_BatchEncryptionCase12(t *testing.T) { } // Case13: Incorrect input for nonce when we aren't in convergent encryption should fail the operation -func TestTransit_BatchEncryptionCase13(t *testing.T) { +func TestTransit_EncryptionCase13(t *testing.T) { var err error b, s := createBackendWithStorage(t) + // Non-batch first + data := map[string]interface{}{"plaintext": "bXkgc2VjcmV0IGRhdGE=", "nonce": "R80hr9eNUIuFV52e"} + req := &logical.Request{ + Operation: logical.CreateOperation, + Path: "encrypt/my-key", + Storage: s, + Data: data, + } + resp, err := b.HandleRequest(context.Background(), req) + if err == nil { + t.Fatal("expected invalid request") + } + batchInput := []interface{}{ - map[string]interface{}{"plaintext": "bXkgc2VjcmV0IGRhdGE=", "nonce": "YmFkbm9uY2U="}, + map[string]interface{}{"plaintext": "bXkgc2VjcmV0IGRhdGE=", "nonce": "R80hr9eNUIuFV52e"}, } batchData := map[string]interface{}{ @@ -672,10 +686,71 @@ func TestTransit_BatchEncryptionCase13(t *testing.T) { Storage: s, Data: batchData, } - _, err = b.HandleRequest(context.Background(), batchReq) + resp, err = b.HandleRequest(context.Background(), batchReq) if err != nil { t.Fatal(err) } + + if v, ok := resp.Data["http_status_code"]; !ok || v.(int) != http.StatusBadRequest { + t.Fatal("expected request error") + } +} + +// Case14: Incorrect input for nonce when we are in convergent version 3 should fail +func TestTransit_EncryptionCase14(t *testing.T) { + var err error + + b, s := createBackendWithStorage(t) + + cReq := &logical.Request{ + Operation: logical.UpdateOperation, + Path: "keys/my-key", + Storage: s, + Data: map[string]interface{}{ + "convergent_encryption": "true", + "derived": "true", + }, + } + resp, err := b.HandleRequest(context.Background(), cReq) + if err != nil { + t.Fatal(err) + } + + // Non-batch first + data := map[string]interface{}{"plaintext": "bXkgc2VjcmV0IGRhdGE=", "context": "SGVsbG8sIFdvcmxkCg==", "nonce": "R80hr9eNUIuFV52e"} + req := &logical.Request{ + Operation: logical.CreateOperation, + Path: "encrypt/my-key", + Storage: s, + Data: data, + } + + resp, err = b.HandleRequest(context.Background(), req) + if err == nil { + t.Fatal("expected invalid request") + } + + batchInput := []interface{}{ + data, + } + + batchData := map[string]interface{}{ + "batch_input": batchInput, + } + batchReq := &logical.Request{ + Operation: logical.CreateOperation, + Path: "encrypt/my-key", + Storage: s, + Data: batchData, + } + resp, err = b.HandleRequest(context.Background(), batchReq) + if err != nil { + t.Fatal(err) + } + + if v, ok := resp.Data["http_status_code"]; !ok || v.(int) != http.StatusBadRequest { + t.Fatal("expected request error") + } } // Test that the fast path function decodeBatchRequestItems behave like mapstructure.Decode() to decode []BatchRequestItem. diff --git a/builtin/logical/transit/path_rewrap.go b/builtin/logical/transit/path_rewrap.go index 35d5c68a3..ced28d3cf 100644 --- a/builtin/logical/transit/path_rewrap.go +++ b/builtin/logical/transit/path_rewrap.go @@ -6,6 +6,7 @@ package transit import ( "context" "encoding/base64" + "errors" "fmt" "github.com/hashicorp/vault/helper/constants" @@ -16,6 +17,8 @@ import ( "github.com/mitchellh/mapstructure" ) +var ErrNonceNotAllowed = errors.New("provided nonce not allowed for this key") + func (b *backend) pathRewrap() *framework.Path { return &framework.Path{ Pattern: "rewrap/" + framework.GenericNameRegex("name"), @@ -152,6 +155,11 @@ func (b *backend) pathRewrapWrite(ctx context.Context, req *logical.Request, d * continue } + if item.Nonce != "" && !nonceAllowed(p) { + batchResponseItems[i].Error = ErrNonceNotAllowed.Error() + continue + } + plaintext, err := p.Decrypt(item.DecodedContext, item.DecodedNonce, item.Ciphertext) if err != nil { switch err.(type) { diff --git a/changelog/22852.txt b/changelog/22852.txt new file mode 100644 index 000000000..3a667eb23 --- /dev/null +++ b/changelog/22852.txt @@ -0,0 +1,3 @@ +```release-note:security +secrets/transit: fix a regression that was honoring nonces provided in non-convergent modes during encryption. +``` diff --git a/sdk/helper/keysutil/policy.go b/sdk/helper/keysutil/policy.go index d3f8bc2e9..34a28dfcd 100644 --- a/sdk/helper/keysutil/policy.go +++ b/sdk/helper/keysutil/policy.go @@ -2009,9 +2009,15 @@ func (p *Policy) EncryptWithFactory(ver int, context []byte, nonce []byte, value encBytes := 32 hmacBytes := 0 - if p.convergentVersion(ver) > 2 { + convergentVersion := p.convergentVersion(ver) + if convergentVersion > 2 { deriveHMAC = true hmacBytes = 32 + if len(nonce) > 0 { + return "", errutil.UserError{Err: "nonce provided when not allowed"} + } + } else if len(nonce) > 0 && (!p.ConvergentEncryption || convergentVersion != 1) { + return "", errutil.UserError{Err: "nonce provided when not allowed"} } if p.Type == KeyType_AES128_GCM96 { encBytes = 16