diff --git a/weed/s3api/s3_sse_kms.go b/weed/s3api/s3_sse_kms.go index b87e0bf1a..d671c1638 100644 --- a/weed/s3api/s3_sse_kms.go +++ b/weed/s3api/s3_sse_kms.go @@ -38,6 +38,7 @@ type SSEKMSKey struct { BucketKeyEnabled bool // Whether S3 Bucket Keys are enabled IV []byte // The initialization vector for encryption ChunkOffset int64 // Offset of this chunk within the original part (for IV calculation) + KeyCommitment []byte // HMAC-SHA256 commitment binding key to IV+algorithm } // SSEKMSMetadata represents the metadata stored with SSE-KMS objects @@ -49,6 +50,7 @@ type SSEKMSMetadata struct { BucketKeyEnabled bool `json:"bucketKeyEnabled"` // S3 Bucket Key optimization IV string `json:"iv"` // Base64-encoded initialization vector PartOffset int64 `json:"partOffset"` // Offset within original multipart part (for IV calculation) + KeyCommitment string `json:"keyCommitment,omitempty"` // Base64-encoded HMAC key commitment } const ( @@ -93,16 +95,15 @@ func CreateSSEKMSEncryptedReaderWithBucketKey(r io.Reader, keyID string, encrypt // Create CTR mode cipher stream stream := cipher.NewCTR(dataKeyResult.Block, iv) - // Create the SSE-KMS metadata using utility function + // Create the SSE-KMS metadata using utility function. createSSEKMSKey + // computes the key commitment too, so all encryption paths produce + // commitment-bound metadata uniformly. sseKey := createSSEKMSKey(dataKeyResult, encryptionContext, bucketKeyEnabled, iv, 0) // The IV is stored in SSE key metadata, so the encrypted stream does not need to prepend the IV // This ensures correct Content-Length for clients encryptedReader := &cipher.StreamReader{S: stream, R: r} - // Store IV in the SSE key for metadata storage - sseKey.IV = iv - return encryptedReader, sseKey, nil } @@ -122,15 +123,20 @@ func CreateSSEKMSEncryptedReaderWithBaseIVAndOffset(r io.Reader, keyID string, e // Ensure we clear the plaintext data key from memory when done defer clearKMSDataKey(dataKeyResult) - // Calculate unique IV using base IV and offset to prevent IV reuse in multipart uploads - // Skip is not used here because we're encrypting from the start (not reading a range) + // Calculate unique IV using base IV and offset to prevent IV reuse in multipart uploads. + // Skip is not used here because we're encrypting from the start (not reading a range). iv, _ := calculateIVWithOffset(baseIV, offset) // Create CTR mode cipher stream stream := cipher.NewCTR(dataKeyResult.Block, iv) - // Create the SSE-KMS metadata using utility function - sseKey := createSSEKMSKey(dataKeyResult, encryptionContext, bucketKeyEnabled, iv, offset) + // Store the BASE IV (not the offset-derived IV) in metadata. The decrypt + // path applies calculateIVWithOffset to sseKey.IV when ChunkOffset > 0; + // storing the derived IV here would cause it to offset twice and produce + // the wrong CTR keystream. The key commitment, computed inside + // createSSEKMSKey, therefore binds the base IV — exactly the value the + // verify call at decrypt time hashes. + sseKey := createSSEKMSKey(dataKeyResult, encryptionContext, bucketKeyEnabled, baseIV, offset) // The IV is stored in SSE key metadata, so the encrypted stream does not need to prepend the IV // This ensures correct Content-Length for clients @@ -274,13 +280,19 @@ func (s3a *S3ApiServer) CreateSSEKMSEncryptedReaderForBucket(r io.Reader, bucket // Create CTR mode cipher stream stream := cipher.NewCTR(block, iv) - // Create the encrypting reader + // Create the encrypting reader. Compute the HMAC commitment alongside + // every other field so this bucket-scoped path is on the same downgrade- + // resistant footing as the helper-driven paths above. Store the KMS + // response's KeyID rather than the request's; CreateSSEKMSDecryptedReader + // compares against decryptResp.KeyID, and a request alias would mismatch + // the resolved ARN the response carries. sseKey := &SSEKMSKey{ - KeyID: keyID, + KeyID: dataKeyResp.KeyID, EncryptedDataKey: dataKeyResp.CiphertextBlob, EncryptionContext: encryptionContext, BucketKeyEnabled: bucketKeyEnabled, IV: iv, + KeyCommitment: ComputeKeyCommitment(dataKeyResp.Plaintext, iv, s3_constants.SSEAlgorithmKMS), } return &cipher.StreamReader{S: stream, R: r}, sseKey, nil @@ -374,6 +386,11 @@ func CreateSSEKMSDecryptedReader(r io.Reader, sseKey *SSEKMSKey) (io.Reader, err return nil, fmt.Errorf("KMS key ID mismatch: expected %s, got %s", sseKey.KeyID, decryptResp.KeyID) } + // Verify key commitment before decryption if one exists in metadata + if err := VerifyKeyCommitment(decryptResp.Plaintext, sseKey.IV, s3_constants.SSEAlgorithmKMS, sseKey.KeyCommitment); err != nil { + return nil, err + } + // Use the IV from the SSE key metadata, calculating offset if this is a chunked part if err := ValidateIV(sseKey.IV, "SSE key IV"); err != nil { return nil, fmt.Errorf("invalid IV in SSE key: %w", err) @@ -465,6 +482,11 @@ func SerializeSSEKMSMetadata(sseKey *SSEKMSKey) ([]byte, error) { PartOffset: sseKey.ChunkOffset, // Store within-part offset } + // Include key commitment if present + if len(sseKey.KeyCommitment) > 0 { + metadata.KeyCommitment = base64.StdEncoding.EncodeToString(sseKey.KeyCommitment) + } + data, err := json.Marshal(metadata) if err != nil { return nil, fmt.Errorf("failed to marshal SSE-KMS metadata: %w", err) @@ -510,6 +532,15 @@ func DeserializeSSEKMSMetadata(data []byte) (*SSEKMSKey, error) { } } + // Decode key commitment if present + var keyCommitment []byte + if metadata.KeyCommitment != "" { + keyCommitment, err = base64.StdEncoding.DecodeString(metadata.KeyCommitment) + if err != nil { + return nil, fmt.Errorf("failed to decode key commitment: %w", err) + } + } + sseKey := &SSEKMSKey{ KeyID: metadata.KeyID, EncryptedDataKey: encryptedDataKey, @@ -517,6 +548,7 @@ func DeserializeSSEKMSMetadata(data []byte) (*SSEKMSKey, error) { BucketKeyEnabled: metadata.BucketKeyEnabled, IV: iv, // Restore IV for decryption ChunkOffset: metadata.PartOffset, // Use stored within-part offset + KeyCommitment: keyCommitment, } glog.V(4).Infof("Deserialized SSE-KMS metadata: keyID=%s, bucketKey=%t", sseKey.KeyID, sseKey.BucketKeyEnabled) diff --git a/weed/s3api/s3_sse_kms_utils.go b/weed/s3api/s3_sse_kms_utils.go index be6d72626..41c9a4562 100644 --- a/weed/s3api/s3_sse_kms_utils.go +++ b/weed/s3api/s3_sse_kms_utils.go @@ -86,7 +86,12 @@ func clearKMSDataKey(result *KMSDataKeyResult) { } } -// createSSEKMSKey creates an SSEKMSKey struct from data key result and parameters +// createSSEKMSKey creates an SSEKMSKey struct from data key result and parameters. +// The HMAC key commitment is computed here (rather than at each call site) so +// every SSE-KMS encryption path produces metadata that can later be verified +// against tampering — a missing commitment was an attacker-controlled silent +// downgrade vector. plaintext must still be live; deferred clearKMSDataKey +// runs after this function returns. func createSSEKMSKey(result *KMSDataKeyResult, encryptionContext map[string]string, bucketKeyEnabled bool, iv []byte, chunkOffset int64) *SSEKMSKey { return &SSEKMSKey{ KeyID: result.Response.KeyID, @@ -95,5 +100,6 @@ func createSSEKMSKey(result *KMSDataKeyResult, encryptionContext map[string]stri BucketKeyEnabled: bucketKeyEnabled, IV: iv, ChunkOffset: chunkOffset, + KeyCommitment: ComputeKeyCommitment(result.Response.Plaintext, iv, s3_constants.SSEAlgorithmKMS), } } diff --git a/weed/s3api/s3_sse_s3.go b/weed/s3api/s3_sse_s3.go index 801221ed3..54367413e 100644 --- a/weed/s3api/s3_sse_s3.go +++ b/weed/s3api/s3_sse_s3.go @@ -36,10 +36,11 @@ const ( // SSES3Key represents a server-managed encryption key for SSE-S3 type SSES3Key struct { - Key []byte - KeyID string - Algorithm string - IV []byte // Initialization Vector for this key + Key []byte + KeyID string + Algorithm string + IV []byte // Initialization Vector for this key + KeyCommitment []byte // HMAC-SHA256 commitment binding key to IV+algorithm } // IsSSES3RequestInternal checks if the request specifies SSE-S3 encryption @@ -116,6 +117,18 @@ func CreateSSES3EncryptedReader(reader io.Reader, key *SSES3Key) (io.Reader, []b // CreateSSES3DecryptedReader creates a decrypted reader for SSE-S3 using IV from metadata func CreateSSES3DecryptedReader(reader io.Reader, key *SSES3Key, iv []byte) (io.Reader, error) { + // IV comes from object metadata, which is mutable. Validate before passing + // to cipher.NewCTR so a tampered length produces an error rather than the + // crypto/cipher panic the documentation specifies. + if err := ValidateIV(iv, "SSE-S3 IV"); err != nil { + return nil, err + } + + // Verify key commitment before decryption if one exists in metadata + if err := VerifyKeyCommitment(key.Key, iv, key.Algorithm, key.KeyCommitment); err != nil { + return nil, err + } + // Create AES cipher block, err := aes.NewCipher(key.Key) if err != nil { @@ -160,6 +173,9 @@ func SerializeSSES3Metadata(key *SSES3Key) ([]byte, error) { // Include IV if present (needed for chunk-level decryption) if key.IV != nil { metadata["iv"] = base64.StdEncoding.EncodeToString(key.IV) + // Compute and store key commitment binding key ↔ IV + algorithm + commitment := ComputeKeyCommitment(key.Key, key.IV, key.Algorithm) + metadata["keyCommitment"] = base64.StdEncoding.EncodeToString(commitment) } // Use JSON for proper serialization @@ -238,6 +254,15 @@ func DeserializeSSES3Metadata(data []byte, keyManager *SSES3KeyManager) (*SSES3K key.IV = iv } + // Restore key commitment if present (for tamper detection) + if commitStr, exists := metadata["keyCommitment"]; exists { + commitment, err := base64.StdEncoding.DecodeString(commitStr) + if err != nil { + return nil, fmt.Errorf("failed to decode key commitment: %w", err) + } + key.KeyCommitment = commitment + } + return key, nil } diff --git a/weed/s3api/s3_validation_utils.go b/weed/s3api/s3_validation_utils.go index 16e63595c..8c5852059 100644 --- a/weed/s3api/s3_validation_utils.go +++ b/weed/s3api/s3_validation_utils.go @@ -1,12 +1,83 @@ package s3api import ( + "crypto/hmac" + "crypto/sha256" "fmt" + "os" "strings" + "sync/atomic" + "github.com/seaweedfs/seaweedfs/weed/glog" "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" ) +// RequireKeyCommitmentEnv is the environment variable that flips the +// commitment check from "skip when missing" (the AWS-compatible default, +// needed for objects written before commitments shipped) to "reject when +// missing". Operators who have either re-encrypted all legacy objects or +// who never wrote any objects under the pre-commitment code path can opt +// in via this env var to close the silent downgrade vector that an +// attacker with write access to object metadata could otherwise exploit +// by stripping the commitment field. +const RequireKeyCommitmentEnv = "WEED_S3_REQUIRE_KEY_COMMITMENT" + +// requireKeyCommitment is the runtime mirror of the env var, kept as an +// atomic so config-reload paths can flip it without a global mutex. +var requireKeyCommitment atomic.Bool + +func init() { + if v := os.Getenv(RequireKeyCommitmentEnv); v == "1" || strings.EqualFold(v, "true") { + requireKeyCommitment.Store(true) + glog.V(1).Infof("SSE: %s=true; SSE objects without a key commitment will be rejected", RequireKeyCommitmentEnv) + } +} + +// SetRequireKeyCommitment toggles strict-commitment enforcement at runtime. +// Used by tests and by future config-reload code paths. +func SetRequireKeyCommitment(require bool) { + requireKeyCommitment.Store(require) +} + +// ComputeKeyCommitment computes an HMAC-SHA256 key commitment over the +// encryption parameters (IV + algorithm). This binds the ciphertext to the +// exact key material and IV that were used, preventing key-confusion and +// IV-manipulation attacks against unauthenticated AES-CTR. +// +// The commitment is stored alongside the IV in object metadata. On decrypt +// the commitment is re-derived and compared; a mismatch means the key or IV +// was tampered with. +func ComputeKeyCommitment(key []byte, iv []byte, algorithm string) []byte { + mac := hmac.New(sha256.New, key) + mac.Write(iv) + mac.Write([]byte(algorithm)) + return mac.Sum(nil) +} + +// VerifyKeyCommitment checks a previously stored commitment against the +// current key, IV, and algorithm. Returns nil on success. +// +// When the commitment is empty (legacy object written before commitments +// shipped), the default behaviour is to accept the object — this is the +// AWS-compatible path. Setting WEED_S3_REQUIRE_KEY_COMMITMENT=true (via +// env at startup or via SetRequireKeyCommitment at runtime) flips that +// to reject, closing the silent-downgrade vector at the cost of locking +// out un-migrated legacy objects. +func VerifyKeyCommitment(key []byte, iv []byte, algorithm string, commitment []byte) error { + if len(commitment) == 0 { + if requireKeyCommitment.Load() { + return fmt.Errorf("key commitment is required but missing from object metadata: %s set; legacy objects must be re-encrypted before this flag can be enabled", RequireKeyCommitmentEnv) + } + // Legacy data written before key commitments were added; skip. + return nil + } + expected := ComputeKeyCommitment(key, iv, algorithm) + if !hmac.Equal(expected, commitment) { + return fmt.Errorf("key commitment verification failed: encryption parameters may have been tampered with") + } + return nil +} + // isValidKMSKeyID performs basic validation of KMS key identifiers. // Following Minio's approach: be permissive and accept any reasonable key format. // Only reject keys with leading/trailing spaces or other obvious issues. diff --git a/weed/s3api/s3_validation_utils_require_test.go b/weed/s3api/s3_validation_utils_require_test.go new file mode 100644 index 000000000..78e3b7422 --- /dev/null +++ b/weed/s3api/s3_validation_utils_require_test.go @@ -0,0 +1,102 @@ +package s3api + +import "testing" + +func TestVerifyKeyCommitment_DefaultAcceptsMissing(t *testing.T) { + // Default behaviour mirrors AWS: a missing commitment field is treated + // as a legacy object and accepted. This is the cushion that lets + // operators upgrade without breaking pre-commitment uploads. + prev := requireKeyCommitment.Load() + t.Cleanup(func() { requireKeyCommitment.Store(prev) }) + requireKeyCommitment.Store(false) + + if err := VerifyKeyCommitment([]byte("k"), []byte("iv"), "AES256", nil); err != nil { + t.Fatalf("default path should accept missing commitment, got: %v", err) + } +} + +func TestVerifyKeyCommitment_StrictRejectsMissing(t *testing.T) { + // Strict mode: any object whose metadata lacks the commitment field is + // rejected. Closes the silent-downgrade vector — an attacker who can + // strip the commitment from metadata can no longer bypass verification. + prev := requireKeyCommitment.Load() + t.Cleanup(func() { requireKeyCommitment.Store(prev) }) + requireKeyCommitment.Store(true) + + err := VerifyKeyCommitment([]byte("k"), []byte("iv"), "AES256", nil) + if err == nil { + t.Fatal("strict mode should reject missing commitment; got nil error") + } +} + +func TestVerifyKeyCommitment_StrictAcceptsValidCommitment(t *testing.T) { + // Strict mode does not change the verification outcome for objects that + // do carry a commitment — the only behavioural delta is the missing + // case. + prev := requireKeyCommitment.Load() + t.Cleanup(func() { requireKeyCommitment.Store(prev) }) + requireKeyCommitment.Store(true) + + key := []byte("strict-mode-test-key") + // IV here is just an opaque input to the HMAC commitment; the test + // doesn't pass it into AES-CTR so it doesn't have to be the AES block + // size. The 16 bytes match the AES block size to keep the literal + // realistic. + iv := []byte("strict-mode-iv16") + commit := ComputeKeyCommitment(key, iv, "AES256") + if err := VerifyKeyCommitment(key, iv, "AES256", commit); err != nil { + t.Fatalf("strict mode should accept valid commitment, got: %v", err) + } +} + +func TestVerifyKeyCommitment_RejectsTamperedKey(t *testing.T) { + // Real attack shape: an attacker who can mutate object metadata cannot + // craft a valid commitment without the original key. The verify path + // must catch it whether they tamper with the key, the IV, or the + // algorithm — all three are bound by the HMAC. + prev := requireKeyCommitment.Load() + t.Cleanup(func() { requireKeyCommitment.Store(prev) }) + requireKeyCommitment.Store(false) + + originalKey := []byte("legit-key-for-commitment") + iv := []byte("commitment-iv-16") + algo := "AES256" + commit := ComputeKeyCommitment(originalKey, iv, algo) + + t.Run("tampered key", func(t *testing.T) { + if err := VerifyKeyCommitment([]byte("attacker-substituted-key"), iv, algo, commit); err == nil { + t.Fatal("verify must reject when the key changed but commitment did not") + } + }) + t.Run("tampered IV", func(t *testing.T) { + if err := VerifyKeyCommitment(originalKey, []byte("attacker-iv-16!!"), algo, commit); err == nil { + t.Fatal("verify must reject when the IV changed but commitment did not") + } + }) + t.Run("tampered algorithm", func(t *testing.T) { + if err := VerifyKeyCommitment(originalKey, iv, "AES128", commit); err == nil { + t.Fatal("verify must reject when the algorithm changed but commitment did not") + } + }) + t.Run("tampered commitment", func(t *testing.T) { + bad := append([]byte{}, commit...) + bad[0] ^= 0x01 + if err := VerifyKeyCommitment(originalKey, iv, algo, bad); err == nil { + t.Fatal("verify must reject when the commitment itself was flipped") + } + }) +} + +func TestSetRequireKeyCommitment(t *testing.T) { + prev := requireKeyCommitment.Load() + t.Cleanup(func() { requireKeyCommitment.Store(prev) }) + + SetRequireKeyCommitment(true) + if !requireKeyCommitment.Load() { + t.Fatal("SetRequireKeyCommitment(true) did not propagate to the atomic") + } + SetRequireKeyCommitment(false) + if requireKeyCommitment.Load() { + t.Fatal("SetRequireKeyCommitment(false) did not propagate to the atomic") + } +}