From ed4fcb63f728d620d8e7cbb5f14a1e0478cffc1e Mon Sep 17 00:00:00 2001 From: Anis Elleuch Date: Thu, 2 Feb 2017 19:45:00 +0100 Subject: [PATCH] Require content-length in POST & Upload requests (#3671) Avoid passing size = -1 to PutObject API by requiring content-length header in POST request (as AWS S3 does) and in Upload web handler. Post handler is modified to completely store multipart file to know its size before sending it to PutObject(). --- cmd/bucket-handlers.go | 50 ++++++++++++++++++++-------- cmd/globals.go | 3 ++ cmd/handler-utils.go | 63 ++++++++++++++++++++++-------------- cmd/jwt.go | 4 +-- cmd/post-policy_test.go | 70 ++++++++++++++++++++++++++-------------- cmd/typed-errors.go | 3 ++ cmd/web-handlers.go | 21 +++++++++--- cmd/web-handlers_test.go | 31 ++++++++++++------ 8 files changed, 166 insertions(+), 79 deletions(-) diff --git a/cmd/bucket-handlers.go b/cmd/bucket-handlers.go index db942d755..e6f5c6871 100644 --- a/cmd/bucket-handlers.go +++ b/cmd/bucket-handlers.go @@ -393,6 +393,13 @@ func (api objectAPIHandlers) PostPolicyBucketHandler(w http.ResponseWriter, r *h return } + // Require Content-Length to be set in the request + size := r.ContentLength + if size < 0 { + writeErrorResponse(w, ErrMissingContentLength, r.URL) + return + } + // Here the parameter is the size of the form data that should // be loaded in memory, the remaining being put in temporary files. reader, err := r.MultipartReader() @@ -402,12 +409,28 @@ func (api objectAPIHandlers) PostPolicyBucketHandler(w http.ResponseWriter, r *h return } - fileBody, fileName, formValues, err := extractPostPolicyFormValues(reader) + // Read multipart data and save in memory and in the disk if needed + form, err := reader.ReadForm(maxFormMemory) + if err != nil { + errorIf(err, "Unable to initialize multipart reader.") + writeErrorResponse(w, ErrMalformedPOSTRequest, r.URL) + return + } + + // Remove all tmp files creating during multipart upload + defer form.RemoveAll() + + // Extract all form fields + fileBody, fileName, fileSize, formValues, err := extractPostPolicyFormValues(form) if err != nil { errorIf(err, "Unable to parse form values.") writeErrorResponse(w, ErrMalformedPOSTRequest, r.URL) return } + + // Close multipart file + defer fileBody.Close() + bucket := mux.Vars(r)["bucket"] formValues["Bucket"] = bucket @@ -443,21 +466,20 @@ func (api objectAPIHandlers) PostPolicyBucketHandler(w http.ResponseWriter, r *h return } - // Use rangeReader to ensure that object size is within expected range. + // Ensure that the object size is within expected range, also the file size + // should not exceed the maximum single Put size (5 GiB) lengthRange := postPolicyForm.Conditions.ContentLengthRange if lengthRange.Valid { - // If policy restricted the size of the object. - fileBody = &rangeReader{ - Reader: fileBody, - Min: lengthRange.Min, - Max: lengthRange.Max, + if fileSize < lengthRange.Min { + errorIf(err, "Unable to create object.") + writeErrorResponse(w, toAPIErrorCode(errDataTooSmall), r.URL) + return } - } else { - // Default values of min/max size of the object. - fileBody = &rangeReader{ - Reader: fileBody, - Min: 0, - Max: maxObjectSize, + + if fileSize > lengthRange.Max || fileSize > maxObjectSize { + errorIf(err, "Unable to create object.") + writeErrorResponse(w, toAPIErrorCode(errDataTooLarge), r.URL) + return } } @@ -470,7 +492,7 @@ func (api objectAPIHandlers) PostPolicyBucketHandler(w http.ResponseWriter, r *h objectLock.Lock() defer objectLock.Unlock() - objInfo, err := objectAPI.PutObject(bucket, object, -1, fileBody, metadata, sha256sum) + objInfo, err := objectAPI.PutObject(bucket, object, fileSize, fileBody, metadata, sha256sum) if err != nil { errorIf(err, "Unable to create object.") writeErrorResponse(w, toAPIErrorCode(err), r.URL) diff --git a/cmd/globals.go b/cmd/globals.go index f2ed99dd9..be77a78db 100644 --- a/cmd/globals.go +++ b/cmd/globals.go @@ -59,6 +59,9 @@ const ( // can reach that size according to https://aws.amazon.com/articles/1434 maxFormFieldSize = int64(1 * humanize.MiByte) + // Limit memory allocation to store multipart data + maxFormMemory = int64(5 * humanize.MiByte) + // The maximum allowed difference between the request generation time and the server processing time globalMaxSkewTime = 15 * time.Minute ) diff --git a/cmd/handler-utils.go b/cmd/handler-utils.go index a97f0f192..7b03a7e83 100644 --- a/cmd/handler-utils.go +++ b/cmd/handler-utils.go @@ -18,7 +18,6 @@ package cmd import ( "io" - "io/ioutil" "mime/multipart" "net/http" "strings" @@ -158,33 +157,49 @@ func extractMetadataFromForm(formValues map[string]string) map[string]string { } // Extract form fields and file data from a HTTP POST Policy -func extractPostPolicyFormValues(reader *multipart.Reader) (filePart io.Reader, fileName string, formValues map[string]string, err error) { +func extractPostPolicyFormValues(form *multipart.Form) (filePart io.ReadCloser, fileName string, fileSize int64, formValues map[string]string, err error) { /// HTML Form values formValues = make(map[string]string) fileName = "" - for err == nil { - var part *multipart.Part - part, err = reader.NextPart() - if part != nil { - canonicalFormName := http.CanonicalHeaderKey(part.FormName()) - if canonicalFormName != "File" { - var buffer []byte - limitReader := io.LimitReader(part, maxFormFieldSize+1) - buffer, err = ioutil.ReadAll(limitReader) - if err != nil { - return nil, "", nil, err - } - if int64(len(buffer)) > maxFormFieldSize { - return nil, "", nil, errSizeUnexpected - } - formValues[canonicalFormName] = string(buffer) - } else { - filePart = part - fileName = part.FileName() - // As described in S3 spec, we expect file to be the last form field - break + + // Iterate over form values + for k, v := range form.Value { + canonicalFormName := http.CanonicalHeaderKey(k) + // Check if value's field exceeds S3 limit + if int64(len(v[0])) > maxFormFieldSize { + return nil, "", 0, nil, errSizeUnexpected + } + // Set the form value + formValues[canonicalFormName] = v[0] + } + + // Iterator until we find a valid File field and break + for k, v := range form.File { + canonicalFormName := http.CanonicalHeaderKey(k) + if canonicalFormName == "File" { + if len(v) == 0 { + return nil, "", 0, nil, errInvalidArgument } + // Fetch fileHeader which has the uploaded file information + fileHeader := v[0] + // Set filename + fileName = fileHeader.Filename + // Open the uploaded part + filePart, err = fileHeader.Open() + // Compute file size + fileSize, err = filePart.(io.Seeker).Seek(0, 2) + if err != nil { + return nil, "", 0, nil, err + } + // Reset Seek to the beginning + _, err = filePart.(io.Seeker).Seek(0, 0) + if err != nil { + return nil, "", 0, nil, err + } + // File found and ready for reading + break } } - return filePart, fileName, formValues, nil + + return filePart, fileName, fileSize, formValues, nil } diff --git a/cmd/jwt.go b/cmd/jwt.go index c3d80a08f..f1f0b2373 100644 --- a/cmd/jwt.go +++ b/cmd/jwt.go @@ -106,13 +106,13 @@ func isAuthTokenValid(tokenString string) bool { } func isHTTPRequestValid(req *http.Request) bool { - return webReqestAuthenticate(req) == nil + return webRequestAuthenticate(req) == nil } // Check if the request is authenticated. // Returns nil if the request is authenticated. errNoAuthToken if token missing. // Returns errAuthentication for all other errors. -func webReqestAuthenticate(req *http.Request) error { +func webRequestAuthenticate(req *http.Request) error { jwtToken, err := jwtreq.ParseFromRequest(req, jwtreq.AuthorizationHeaderExtractor, keyFuncCallback) if err != nil { if err == jwtreq.ErrNoTokenInRequest { diff --git a/cmd/post-policy_test.go b/cmd/post-policy_test.go index 60333e3a3..14018979a 100644 --- a/cmd/post-policy_test.go +++ b/cmd/post-policy_test.go @@ -342,46 +342,66 @@ func testPostPolicyBucketHandler(obj ObjectLayer, instanceType string, t TestErr } testCases2 := []struct { - objectName string - data []byte - expectedRespStatus int - accessKey string - secretKey string - malformedBody bool + objectName string + data []byte + expectedRespStatus int + accessKey string + secretKey string + malformedBody bool + ignoreContentLength bool }{ // Success case. { - objectName: "test", - data: bytes.Repeat([]byte("a"), 1025), - expectedRespStatus: http.StatusNoContent, - accessKey: credentials.AccessKey, - secretKey: credentials.SecretKey, - malformedBody: false, + objectName: "test", + data: bytes.Repeat([]byte("a"), 1025), + expectedRespStatus: http.StatusNoContent, + accessKey: credentials.AccessKey, + secretKey: credentials.SecretKey, + malformedBody: false, + ignoreContentLength: false, + }, + // Failed with Content-Length not specified. + { + objectName: "test", + data: bytes.Repeat([]byte("a"), 1025), + expectedRespStatus: http.StatusNoContent, + accessKey: credentials.AccessKey, + secretKey: credentials.SecretKey, + malformedBody: false, + ignoreContentLength: true, }, // Failed with entity too small. { - objectName: "test", - data: bytes.Repeat([]byte("a"), 1023), - expectedRespStatus: http.StatusBadRequest, - accessKey: credentials.AccessKey, - secretKey: credentials.SecretKey, - malformedBody: false, + objectName: "test", + data: bytes.Repeat([]byte("a"), 1023), + expectedRespStatus: http.StatusBadRequest, + accessKey: credentials.AccessKey, + secretKey: credentials.SecretKey, + malformedBody: false, + ignoreContentLength: false, }, // Failed with entity too large. { - objectName: "test", - data: bytes.Repeat([]byte("a"), (1*humanize.MiByte)+1), - expectedRespStatus: http.StatusBadRequest, - accessKey: credentials.AccessKey, - secretKey: credentials.SecretKey, - malformedBody: false, + objectName: "test", + data: bytes.Repeat([]byte("a"), (1*humanize.MiByte)+1), + expectedRespStatus: http.StatusBadRequest, + accessKey: credentials.AccessKey, + secretKey: credentials.SecretKey, + malformedBody: false, + ignoreContentLength: false, }, } for i, testCase := range testCases2 { // initialize HTTP NewRecorder, this records any mutations to response writer inside the handler. rec := httptest.NewRecorder() - req, perr := newPostRequestV4WithContentLength("", bucketName, testCase.objectName, testCase.data, testCase.accessKey, testCase.secretKey) + var req *http.Request + var perr error + if testCase.ignoreContentLength { + req, perr = newPostRequestV4("", bucketName, testCase.objectName, testCase.data, testCase.accessKey, testCase.secretKey) + } else { + req, perr = newPostRequestV4WithContentLength("", bucketName, testCase.objectName, testCase.data, testCase.accessKey, testCase.secretKey) + } if perr != nil { t.Fatalf("Test %d: %s: Failed to create HTTP request for PostPolicyHandler: %v", i+1, instanceType, perr) } diff --git a/cmd/typed-errors.go b/cmd/typed-errors.go index e79ab0dd6..5a7568568 100644 --- a/cmd/typed-errors.go +++ b/cmd/typed-errors.go @@ -33,6 +33,9 @@ var errContentSHA256Mismatch = errors.New("Content checksum SHA256 mismatch") // used when we deal with data larger than expected var errSizeUnexpected = errors.New("Data size larger than expected") +// used when we deal with data with unknown size +var errSizeUnspecified = errors.New("Data size is unspecified") + // When upload object size is greater than 5G in a single PUT/POST operation. var errDataTooLarge = errors.New("Object size larger than allowed limit") diff --git a/cmd/web-handlers.go b/cmd/web-handlers.go index adf9f6db7..4943fa78c 100644 --- a/cmd/web-handlers.go +++ b/cmd/web-handlers.go @@ -151,7 +151,7 @@ func (web *webAPIHandlers) ListBuckets(r *http.Request, args *WebGenericArgs, re if objectAPI == nil { return toJSONError(errServerNotInitialized) } - authErr := webReqestAuthenticate(r) + authErr := webRequestAuthenticate(r) if authErr != nil { return toJSONError(authErr) } @@ -208,7 +208,7 @@ func (web *webAPIHandlers) ListObjects(r *http.Request, args *ListObjectsArgs, r prefix := args.Prefix + "test" // To test if GetObject/PutObject with the specified prefix is allowed. readable := isBucketActionAllowed("s3:GetObject", args.BucketName, prefix) writable := isBucketActionAllowed("s3:PutObject", args.BucketName, prefix) - authErr := webReqestAuthenticate(r) + authErr := webRequestAuthenticate(r) switch { case authErr == errAuthentication: return toJSONError(authErr) @@ -446,7 +446,7 @@ func (web *webAPIHandlers) Upload(w http.ResponseWriter, r *http.Request) { bucket := vars["bucket"] object := vars["object"] - authErr := webReqestAuthenticate(r) + authErr := webRequestAuthenticate(r) if authErr == errAuthentication { writeWebErrorResponse(w, errAuthentication) return @@ -456,6 +456,13 @@ func (web *webAPIHandlers) Upload(w http.ResponseWriter, r *http.Request) { return } + // Require Content-Length to be set in the request + size := r.ContentLength + if size < 0 { + writeWebErrorResponse(w, errSizeUnspecified) + return + } + // Extract incoming metadata if any. metadata := extractMetadataFromHeader(r.Header) @@ -465,7 +472,7 @@ func (web *webAPIHandlers) Upload(w http.ResponseWriter, r *http.Request) { defer objectLock.Unlock() sha256sum := "" - objInfo, err := objectAPI.PutObject(bucket, object, -1, r.Body, metadata, sha256sum) + objInfo, err := objectAPI.PutObject(bucket, object, size, r.Body, metadata, sha256sum) if err != nil { writeWebErrorResponse(w, err) return @@ -816,6 +823,12 @@ func toWebAPIError(err error) APIError { HTTPStatusCode: http.StatusForbidden, Description: err.Error(), } + } else if err == errSizeUnspecified { + return APIError{ + Code: "InvalidRequest", + HTTPStatusCode: http.StatusBadRequest, + Description: err.Error(), + } } // Convert error type to api error code. diff --git a/cmd/web-handlers_test.go b/cmd/web-handlers_test.go index fc72ab9ff..1f8c9e627 100644 --- a/cmd/web-handlers_test.go +++ b/cmd/web-handlers_test.go @@ -738,19 +738,24 @@ func testUploadWebHandler(obj ObjectLayer, instanceType string, t TestErrHandler objectName := "test.file" bucketName := getRandomBucketName() - test := func(token string) int { + test := func(token string, sendContentLength bool) int { rec := httptest.NewRecorder() - var req *http.Request - req, err = http.NewRequest("PUT", "/minio/upload/"+bucketName+"/"+objectName, nil) - if err != nil { - t.Fatalf("Cannot create upload request, %v", err) + req, rErr := http.NewRequest("PUT", "/minio/upload/"+bucketName+"/"+objectName, nil) + if rErr != nil { + t.Fatalf("Cannot create upload request, %v", rErr) } - req.Header.Set("Content-Length", strconv.Itoa(len(content))) req.Header.Set("x-amz-date", "20160814T114029Z") req.Header.Set("Accept", "*/*") + req.Body = ioutil.NopCloser(bytes.NewReader(content)) + if !sendContentLength { + req.ContentLength = -1 + } else { + req.ContentLength = int64(len(content)) + } + if token != "" { req.Header.Set("Authorization", "Bearer "+authorization) } @@ -765,7 +770,7 @@ func testUploadWebHandler(obj ObjectLayer, instanceType string, t TestErrHandler } // Authenticated upload should succeed. - code := test(authorization) + code := test(authorization, true) if code != http.StatusOK { t.Fatalf("Expected the response status to be 200, but instead found `%d`", code) } @@ -780,8 +785,14 @@ func testUploadWebHandler(obj ObjectLayer, instanceType string, t TestErrHandler t.Fatalf("The upload file is different from the download file") } + // Authenticated upload without content-length should fail + code = test(authorization, false) + if code != http.StatusBadRequest { + t.Fatalf("Expected the response status to be 200, but instead found `%d`", code) + } + // Unauthenticated upload should fail. - code = test("") + code = test("", true) if code != http.StatusForbidden { t.Fatalf("Expected the response status to be 403, but instead found `%d`", code) } @@ -794,13 +805,13 @@ func testUploadWebHandler(obj ObjectLayer, instanceType string, t TestErrHandler globalBucketPolicies.SetBucketPolicy(bucketName, policyChange{false, &policy}) // Unauthenticated upload with WRITE policy should succeed. - code = test("") + code = test("", true) if code != http.StatusOK { t.Fatalf("Expected the response status to be 200, but instead found `%d`", code) } } -// Wrapper for calling Upload Handler +// Wrapper for calling Download Handler func TestWebHandlerDownload(t *testing.T) { ExecObjectLayerTest(t, testDownloadWebHandler) }