From 2b51fe9f261cd638e1e361a305c0ca0e00572d1d Mon Sep 17 00:00:00 2001 From: Andreas Auernhammer Date: Fri, 20 Sep 2019 23:56:12 +0200 Subject: [PATCH] make SSE request header check comprehensive (#8276) This commit refactors the SSE header check by moving it into the `crypto` package, adds a unit test for it and makes the check comprehensive. --- cmd/bucket-handlers.go | 9 +++-- cmd/crypto/header.go | 7 ++++ cmd/crypto/header_test.go | 23 +++++++++++ cmd/encryption-v1.go | 6 --- cmd/encryption-v1_test.go | 80 --------------------------------------- cmd/object-api-utils.go | 2 +- cmd/object-handlers.go | 26 +++++++------ cmd/web-handlers.go | 2 +- 8 files changed, 53 insertions(+), 102 deletions(-) diff --git a/cmd/bucket-handlers.go b/cmd/bucket-handlers.go index 28197986a..2bb8b9975 100644 --- a/cmd/bucket-handlers.go +++ b/cmd/bucket-handlers.go @@ -557,8 +557,7 @@ func (api objectAPIHandlers) PostPolicyBucketHandler(w http.ResponseWriter, r *h writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrNotImplemented), r.URL, guessIsBrowserReq(r)) return } - - if !api.EncryptionEnabled() && hasServerSideEncryptionHeader(r.Header) { + if !api.EncryptionEnabled() && crypto.IsRequested(r.Header) { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrNotImplemented), r.URL, guessIsBrowserReq(r)) return } @@ -715,7 +714,11 @@ func (api objectAPIHandlers) PostPolicyBucketHandler(w http.ResponseWriter, r *h return } if objectAPI.IsEncryptionSupported() { - if hasServerSideEncryptionHeader(formValues) && !hasSuffix(object, SlashSeparator) { // handle SSE-C and SSE-S3 requests + if crypto.IsRequested(formValues) && !hasSuffix(object, SlashSeparator) { // handle SSE requests + if crypto.SSECopy.IsRequested(r.Header) { + writeErrorResponse(ctx, w, toAPIError(ctx, errInvalidEncryptionParameters), r.URL, guessIsBrowserReq(r)) + return + } var reader io.Reader var key []byte if crypto.SSEC.IsRequested(formValues) { diff --git a/cmd/crypto/header.go b/cmd/crypto/header.go index adf5f8294..b94ef4516 100644 --- a/cmd/crypto/header.go +++ b/cmd/crypto/header.go @@ -83,6 +83,13 @@ func RemoveSensitiveHeaders(h http.Header) { h.Del(SSECopyKey) } +// IsRequested returns true if the HTTP headers indicates +// that any form server-side encryption (SSE-C, SSE-S3 or SSE-KMS) +// is requested. +func IsRequested(h http.Header) bool { + return S3.IsRequested(h) || SSEC.IsRequested(h) || SSECopy.IsRequested(h) || S3KMS.IsRequested(h) +} + // S3 represents AWS SSE-S3. It provides functionality to handle // SSE-S3 requests. var S3 = s3{} diff --git a/cmd/crypto/header_test.go b/cmd/crypto/header_test.go index 5085092fd..3f0a3d725 100644 --- a/cmd/crypto/header_test.go +++ b/cmd/crypto/header_test.go @@ -20,6 +20,29 @@ import ( "testing" ) +func TestIsRequested(t *testing.T) { + for i, test := range kmsIsRequestedTests { + if got := IsRequested(test.Header) && S3KMS.IsRequested(test.Header); got != test.Expected { + t.Errorf("SSE-KMS: Test %d: Wanted %v but got %v", i, test.Expected, got) + } + } + for i, test := range s3IsRequestedTests { + if got := IsRequested(test.Header) && S3.IsRequested(test.Header); got != test.Expected { + t.Errorf("SSE-S3: Test %d: Wanted %v but got %v", i, test.Expected, got) + } + } + for i, test := range ssecIsRequestedTests { + if got := IsRequested(test.Header) && SSEC.IsRequested(test.Header); got != test.Expected { + t.Errorf("SSE-C: Test %d: Wanted %v but got %v", i, test.Expected, got) + } + } + for i, test := range ssecCopyIsRequestedTests { + if got := IsRequested(test.Header) && SSECopy.IsRequested(test.Header); got != test.Expected { + t.Errorf("SSE-C: Test %d: Wanted %v but got %v", i, test.Expected, got) + } + } +} + var kmsIsRequestedTests = []struct { Header http.Header Expected bool diff --git a/cmd/encryption-v1.go b/cmd/encryption-v1.go index 8c7f06601..5b6f473fa 100644 --- a/cmd/encryption-v1.go +++ b/cmd/encryption-v1.go @@ -63,12 +63,6 @@ const ( ) -// hasServerSideEncryptionHeader returns true if the given HTTP header -// contains server-side-encryption. -func hasServerSideEncryptionHeader(header http.Header) bool { - return crypto.S3.IsRequested(header) || crypto.SSEC.IsRequested(header) -} - // isEncryptedMultipart returns true if the current object is // uploaded by the user using multipart mechanism: // initiate new multipart, upload part, complete upload diff --git a/cmd/encryption-v1_test.go b/cmd/encryption-v1_test.go index 1c280e707..0018a89d9 100644 --- a/cmd/encryption-v1_test.go +++ b/cmd/encryption-v1_test.go @@ -28,86 +28,6 @@ import ( "github.com/minio/sio" ) -var hasServerSideEncryptionHeaderTests = []struct { - headers map[string]string - sseRequest bool -}{ - {headers: map[string]string{crypto.SSECAlgorithm: "AES256", crypto.SSECKey: "key", crypto.SSECKeyMD5: "md5"}, sseRequest: true}, // 0 - {headers: map[string]string{crypto.SSECAlgorithm: "AES256"}, sseRequest: true}, // 1 - {headers: map[string]string{crypto.SSECKey: "key"}, sseRequest: true}, // 2 - {headers: map[string]string{crypto.SSECKeyMD5: "md5"}, sseRequest: true}, // 3 - {headers: map[string]string{}, sseRequest: false}, // 4 - {headers: map[string]string{crypto.SSECopyAlgorithm + " ": "AES256", " " + crypto.SSECopyKey: "key", crypto.SSECopyKeyMD5 + " ": "md5"}, sseRequest: false}, // 5 - {headers: map[string]string{crypto.SSECopyAlgorithm: "", crypto.SSECopyKey: "", crypto.SSECopyKeyMD5: ""}, sseRequest: false}, // 6 - {headers: map[string]string{crypto.SSEHeader: ""}, sseRequest: true}, // 7 -} - -func TestHasServerSideEncryptionHeader(t *testing.T) { - for i, test := range hasServerSideEncryptionHeaderTests { - headers := http.Header{} - for k, v := range test.headers { - headers.Set(k, v) - } - if hasServerSideEncryptionHeader(headers) != test.sseRequest { - t.Errorf("Test %d: Expected hasServerSideEncryptionHeader to return %v", i, test.sseRequest) - } - } -} - -var hasSSECopyCustomerHeaderTests = []struct { - headers map[string]string - sseRequest bool -}{ - {headers: map[string]string{crypto.SSECopyAlgorithm: "AES256", crypto.SSECopyKey: "key", crypto.SSECopyKeyMD5: "md5"}, sseRequest: true}, // 0 - {headers: map[string]string{crypto.SSECopyAlgorithm: "AES256"}, sseRequest: true}, // 1 - {headers: map[string]string{crypto.SSECopyKey: "key"}, sseRequest: true}, // 2 - {headers: map[string]string{crypto.SSECopyKeyMD5: "md5"}, sseRequest: true}, // 3 - {headers: map[string]string{}, sseRequest: false}, // 4 - {headers: map[string]string{crypto.SSECopyAlgorithm + " ": "AES256", " " + crypto.SSECopyKey: "key", crypto.SSECopyKeyMD5 + " ": "md5"}, sseRequest: false}, // 5 - {headers: map[string]string{crypto.SSECopyAlgorithm: "", crypto.SSECopyKey: "", crypto.SSECopyKeyMD5: ""}, sseRequest: true}, // 6 - {headers: map[string]string{crypto.SSEHeader: ""}, sseRequest: false}, // 7 - -} - -func TestIsSSECopyCustomerRequest(t *testing.T) { - for i, test := range hasSSECopyCustomerHeaderTests { - headers := http.Header{} - for k, v := range test.headers { - headers.Set(k, v) - } - if crypto.SSECopy.IsRequested(headers) != test.sseRequest { - t.Errorf("Test %d: Expected crypto.SSECopy.IsRequested to return %v", i, test.sseRequest) - } - } -} - -var hasSSECustomerHeaderTests = []struct { - headers map[string]string - sseRequest bool -}{ - {headers: map[string]string{crypto.SSECAlgorithm: "AES256", crypto.SSECKey: "key", crypto.SSECKeyMD5: "md5"}, sseRequest: true}, // 0 - {headers: map[string]string{crypto.SSECAlgorithm: "AES256"}, sseRequest: true}, // 1 - {headers: map[string]string{crypto.SSECKey: "key"}, sseRequest: true}, // 2 - {headers: map[string]string{crypto.SSECKeyMD5: "md5"}, sseRequest: true}, // 3 - {headers: map[string]string{}, sseRequest: false}, // 4 - {headers: map[string]string{crypto.SSECAlgorithm + " ": "AES256", " " + crypto.SSECKey: "key", crypto.SSECKeyMD5 + " ": "md5"}, sseRequest: false}, // 5 - {headers: map[string]string{crypto.SSECAlgorithm: "", crypto.SSECKey: "", crypto.SSECKeyMD5: ""}, sseRequest: true}, // 6 - {headers: map[string]string{crypto.SSEHeader: ""}, sseRequest: false}, // 7 - -} - -func TestHasSSECustomerHeader(t *testing.T) { - for i, test := range hasSSECustomerHeaderTests { - headers := http.Header{} - for k, v := range test.headers { - headers.Set(k, v) - } - if crypto.SSEC.IsRequested(headers) != test.sseRequest { - t.Errorf("Test %d: Expected hasSSECustomerHeader to return %v", i, test.sseRequest) - } - } -} - var encryptRequestTests = []struct { header map[string]string metadata map[string]string diff --git a/cmd/object-api-utils.go b/cmd/object-api-utils.go index dcbb77952..96d616fb2 100644 --- a/cmd/object-api-utils.go +++ b/cmd/object-api-utils.go @@ -354,7 +354,7 @@ func (o ObjectInfo) GetActualSize() int64 { // Using compression and encryption together enables room for side channel attacks. // Eliminate non-compressible objects by extensions/content-types. func isCompressible(header http.Header, object string) bool { - if hasServerSideEncryptionHeader(header) || excludeForCompression(header, object) { + if crypto.IsRequested(header) || excludeForCompression(header, object) { return false } return true diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go index 5776603bd..39f6bfbc0 100644 --- a/cmd/object-handlers.go +++ b/cmd/object-handlers.go @@ -92,7 +92,7 @@ func (api objectAPIHandlers) SelectObjectContentHandler(w http.ResponseWriter, r writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrNotImplemented), r.URL, guessIsBrowserReq(r)) return } - if !api.EncryptionEnabled() && hasServerSideEncryptionHeader(r.Header) { + if !api.EncryptionEnabled() && crypto.IsRequested(r.Header) { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrBadRequest), r.URL, guessIsBrowserReq(r)) return } @@ -251,7 +251,7 @@ func (api objectAPIHandlers) GetObjectHandler(w http.ResponseWriter, r *http.Req writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrBadRequest), r.URL, guessIsBrowserReq(r)) return } - if !api.EncryptionEnabled() && hasServerSideEncryptionHeader(r.Header) { + if !api.EncryptionEnabled() && crypto.IsRequested(r.Header) { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrBadRequest), r.URL, guessIsBrowserReq(r)) return } @@ -422,7 +422,7 @@ func (api objectAPIHandlers) HeadObjectHandler(w http.ResponseWriter, r *http.Re writeErrorResponseHeadersOnly(w, errorCodes.ToAPIErr(ErrBadRequest)) return } - if !api.EncryptionEnabled() && hasServerSideEncryptionHeader(r.Header) { + if !api.EncryptionEnabled() && crypto.IsRequested(r.Header) { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrBadRequest), r.URL, guessIsBrowserReq(r)) return } @@ -646,7 +646,7 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrNotImplemented), r.URL, guessIsBrowserReq(r)) // SSE-KMS is not supported return } - if !api.EncryptionEnabled() && (hasServerSideEncryptionHeader(r.Header) || crypto.SSECopy.IsRequested(r.Header)) { + if !api.EncryptionEnabled() && crypto.IsRequested(r.Header) { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrNotImplemented), r.URL, guessIsBrowserReq(r)) return } @@ -1051,7 +1051,7 @@ func (api objectAPIHandlers) PutObjectHandler(w http.ResponseWriter, r *http.Req writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrNotImplemented), r.URL, guessIsBrowserReq(r)) // SSE-KMS is not supported return } - if !api.EncryptionEnabled() && hasServerSideEncryptionHeader(r.Header) { + if !api.EncryptionEnabled() && crypto.IsRequested(r.Header) { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrNotImplemented), r.URL, guessIsBrowserReq(r)) return } @@ -1232,7 +1232,11 @@ func (api objectAPIHandlers) PutObjectHandler(w http.ResponseWriter, r *http.Req var objectEncryptionKey []byte if objectAPI.IsEncryptionSupported() { - if hasServerSideEncryptionHeader(r.Header) && !hasSuffix(object, SlashSeparator) { // handle SSE requests + if crypto.IsRequested(r.Header) && !hasSuffix(object, SlashSeparator) { // handle SSE requests + if crypto.SSECopy.IsRequested(r.Header) { + writeErrorResponse(ctx, w, toAPIError(ctx, errInvalidEncryptionParameters), r.URL, guessIsBrowserReq(r)) + return + } reader, objectEncryptionKey, err = EncryptRequest(hashReader, r, bucket, object, metadata) if err != nil { writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r)) @@ -1264,7 +1268,7 @@ func (api objectAPIHandlers) PutObjectHandler(w http.ResponseWriter, r *http.Req if !strings.HasSuffix(objInfo.ETag, "-1") { etag = objInfo.ETag + "-1" } - } else if hasServerSideEncryptionHeader(r.Header) { + } else if crypto.IsRequested(r.Header) { etag = getDecryptedETag(r.Header, objInfo, false) } w.Header()[xhttp.ETag] = []string{"\"" + etag + "\""} @@ -1318,7 +1322,7 @@ func (api objectAPIHandlers) NewMultipartUploadHandler(w http.ResponseWriter, r writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrNotImplemented), r.URL, guessIsBrowserReq(r)) // SSE-KMS is not supported return } - if !api.EncryptionEnabled() && hasServerSideEncryptionHeader(r.Header) { + if !api.EncryptionEnabled() && crypto.IsRequested(r.Header) { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrNotImplemented), r.URL, guessIsBrowserReq(r)) return } @@ -1365,7 +1369,7 @@ func (api objectAPIHandlers) NewMultipartUploadHandler(w http.ResponseWriter, r var encMetadata = map[string]string{} if objectAPI.IsEncryptionSupported() { - if hasServerSideEncryptionHeader(r.Header) { + if crypto.IsRequested(r.Header) { if err = setEncryptionMetadata(r, bucket, object, encMetadata); err != nil { writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r)) return @@ -1432,7 +1436,7 @@ func (api objectAPIHandlers) CopyObjectPartHandler(w http.ResponseWriter, r *htt writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrNotImplemented), r.URL, guessIsBrowserReq(r)) // SSE-KMS is not supported return } - if !api.EncryptionEnabled() && (hasServerSideEncryptionHeader(r.Header) || crypto.SSECopy.IsRequested(r.Header)) { + if !api.EncryptionEnabled() && crypto.IsRequested(r.Header) { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrNotImplemented), r.URL, guessIsBrowserReq(r)) return } @@ -1747,7 +1751,7 @@ func (api objectAPIHandlers) PutObjectPartHandler(w http.ResponseWriter, r *http writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrNotImplemented), r.URL, guessIsBrowserReq(r)) // SSE-KMS is not supported return } - if !api.EncryptionEnabled() && hasServerSideEncryptionHeader(r.Header) { + if !api.EncryptionEnabled() && crypto.IsRequested(r.Header) { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrNotImplemented), r.URL, guessIsBrowserReq(r)) return } diff --git a/cmd/web-handlers.go b/cmd/web-handlers.go index 8a0148f9e..898d85b49 100644 --- a/cmd/web-handlers.go +++ b/cmd/web-handlers.go @@ -1022,7 +1022,7 @@ func (web *webAPIHandlers) Upload(w http.ResponseWriter, r *http.Request) { return } if objectAPI.IsEncryptionSupported() { - if hasServerSideEncryptionHeader(r.Header) && !hasSuffix(object, SlashSeparator) { // handle SSE requests + if crypto.IsRequested(r.Header) && !hasSuffix(object, SlashSeparator) { // handle SSE requests rawReader := hashReader var objectEncryptionKey []byte reader, objectEncryptionKey, err = EncryptRequest(hashReader, r, bucket, object, metadata)