Reject supplied nonces for non-convergent encryption operations

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)

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.")
 	}

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

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.

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) {

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.
+```

sdk/helper/keysutil/policy.go

@@ -2013,9 +2013,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