From d653525493b1ccb793e9e6f31064184812f4d835 Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Fri, 8 Sep 2023 09:23:09 -0500 Subject: [PATCH] Revert "Reject supplied nonces for non-convergent encryption operations (#22852)" This reverts commit 4ff9d9a6d54f3956e44c46f822285953b7bc62ec. --- 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, 13 insertions(+), 142 deletions(-) delete mode 100644 changelog/22852.txt diff --git a/builtin/logical/transit/backend_test.go b/builtin/logical/transit/backend_test.go index f96409338e067..c4d92a3ddbf57 100644 --- a/builtin/logical/transit/backend_test.go +++ b/builtin/logical/transit/backend_test.go @@ -1111,12 +1111,10 @@ 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" + "plaintext": "emlwIHphcA==", // "zip zap" + "nonce": "b25ldHdvdGhyZWVl", // "onetwothreee" "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) @@ -1147,10 +1145,11 @@ 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" + "plaintext": "emlwIHphcA==", // "zip zap" + "nonce": "dHdvdGhyZWVmb3Vy", // "twothreefour" "context": "pWZ6t/im3AORd0lVYE0zBdKpX6Bl3/SvFtoVTPWbdkzjG788XmMAnOlxandSdd7S", } - if ver == 0 { + if ver < 2 { req.Data["nonce"] = "dHdvdGhyZWVmb3Vy" // "twothreefour" } else { req.Data["context"] = "pWZ6t/im3AORd0lVYE0zBdKpX6Bl3/SvFtoVTPWbdkzjG788XmMAnOldandSdd7S" @@ -1189,12 +1188,10 @@ func testConvergentEncryptionCommon(t *testing.T, ver int, keyType keysutil.KeyT // ...and a different context value req.Data = map[string]interface{}{ - "plaintext": "emlwIHphcA==", // "zip zap" + "plaintext": "emlwIHphcA==", // "zip zap" + "nonce": "dHdvdGhyZWVmb3Vy", // "twothreefour" "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) @@ -1306,11 +1303,9 @@ 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") @@ -1325,11 +1320,9 @@ 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 c17dedce3bc4c..42da161916392 100644 --- a/builtin/logical/transit/path_datakey.go +++ b/builtin/logical/transit/path_datakey.go @@ -160,10 +160,6 @@ 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 4a06b9fb17167..3d8a103f5f49d 100644 --- a/builtin/logical/transit/path_encrypt.go +++ b/builtin/logical/transit/path_encrypt.go @@ -395,13 +395,6 @@ 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 } @@ -472,25 +465,6 @@ 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 3574899aa84d9..734a92524b558 100644 --- a/builtin/logical/transit/path_encrypt_test.go +++ b/builtin/logical/transit/path_encrypt_test.go @@ -3,7 +3,6 @@ package transit import ( "context" "encoding/json" - "net/http" "reflect" "strings" "testing" @@ -646,26 +645,13 @@ func TestTransit_BatchEncryptionCase12(t *testing.T) { } // Case13: Incorrect input for nonce when we aren't in convergent encryption should fail the operation -func TestTransit_EncryptionCase13(t *testing.T) { +func TestTransit_BatchEncryptionCase13(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": "R80hr9eNUIuFV52e"}, + map[string]interface{}{"plaintext": "bXkgc2VjcmV0IGRhdGE=", "nonce": "YmFkbm9uY2U="}, } batchData := map[string]interface{}{ @@ -677,71 +663,10 @@ func TestTransit_EncryptionCase13(t *testing.T) { 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") - } -} - -// 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) + _, 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 fa90630ca1743..1bdf282f7f438 100644 --- a/builtin/logical/transit/path_rewrap.go +++ b/builtin/logical/transit/path_rewrap.go @@ -3,7 +3,6 @@ package transit import ( "context" "encoding/base64" - "errors" "fmt" "github.com/hashicorp/vault/helper/constants" @@ -14,8 +13,6 @@ 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"), @@ -146,11 +143,6 @@ 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 deleted file mode 100644 index 3a667eb23bb0f..0000000000000 --- a/changelog/22852.txt +++ /dev/null @@ -1,3 +0,0 @@ -```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 93be8ce28c890..bbd587feb3554 100644 --- a/sdk/helper/keysutil/policy.go +++ b/sdk/helper/keysutil/policy.go @@ -1873,15 +1873,9 @@ func (p *Policy) EncryptWithFactory(ver int, context []byte, nonce []byte, value encBytes := 32 hmacBytes := 0 - convergentVersion := p.convergentVersion(ver) - if convergentVersion > 2 { + if p.convergentVersion(ver) > 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