From 1f706e067d4aea0fca235fd5d0e77b0482b261aa Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Mon, 18 Jul 2016 21:20:17 -0700 Subject: [PATCH] api: xmlDecoder should honor contentLength. (#2226) This is needed so that we avoid reading large amounts of data from compromised clients. --- bucket-handlers.go | 7 ++--- handler-utils.go | 64 +++++++++++++++++++++++-------------------- handler-utils_test.go | 22 ++++++++++++++- utils.go | 10 +++++-- 4 files changed, 67 insertions(+), 36 deletions(-) diff --git a/bucket-handlers.go b/bucket-handlers.go index 78898af24..9e122a441 100644 --- a/bucket-handlers.go +++ b/bucket-handlers.go @@ -322,10 +322,9 @@ func (api objectAPIHandlers) PutBucketHandler(w http.ResponseWriter, r *http.Req } } - // the location value in the request body should match the Region in serverConfig. - // other values of location are not accepted. - // make bucket fails in such cases. - errCode := isValidLocationContraint(r.Body, serverConfig.GetRegion()) + // Validate if incoming location constraint is valid, reject + // requests which do not follow valid region requirements. + errCode := isValidLocationConstraint(r) if errCode != ErrNone { writeErrorResponse(w, r, errCode, r.URL.Path) return diff --git a/handler-utils.go b/handler-utils.go index 0a3102559..8c43181bf 100644 --- a/handler-utils.go +++ b/handler-utils.go @@ -18,37 +18,43 @@ package main import ( "io" + "net/http" ) -// validates location constraint from the request body. -// the location value in the request body should match the Region in serverConfig. -// other values of location are not accepted. -// make bucket fails in such cases. -func isValidLocationContraint(reqBody io.Reader, serverRegion string) APIErrorCode { - var locationContraint createBucketLocationConfiguration - var errCode APIErrorCode - errCode = ErrNone - e := xmlDecoder(reqBody, &locationContraint) - if e != nil { - if e == io.EOF { - // Do nothing. - // failed due to empty body. The location will be set to default value from the serverConfig. - // this is valid. - errCode = ErrNone - } else { - // Failed due to malformed configuration. - errCode = ErrMalformedXML - //writeErrorResponse(w, r, ErrMalformedXML, r.URL.Path) - } - } else { - // Region obtained from the body. - // It should be equal to Region in serverConfig. - // Else ErrInvalidRegion returned. - // For empty value location will be to set to default value from the serverConfig. - if locationContraint.Location != "" && serverRegion != locationContraint.Location { - //writeErrorResponse(w, r, ErrInvalidRegion, r.URL.Path) - errCode = ErrInvalidRegion +// Validates location constraint in PutBucket request body. +// The location value in the request body should match the +// region configured at serverConfig, otherwise error is returned. +func isValidLocationConstraint(r *http.Request) (s3Error APIErrorCode) { + serverRegion := serverConfig.GetRegion() + // If the request has no body with content-length set to 0, + // we do not have to validate location constraint. Bucket will + // be created at default region. + if r.ContentLength == 0 { + return ErrNone + } + locationConstraint := createBucketLocationConfiguration{} + if err := xmlDecoder(r.Body, &locationConstraint, r.ContentLength); err != nil { + if err == io.EOF && r.ContentLength == -1 { + // EOF is a valid condition here when ContentLength is -1. + return ErrNone } + errorIf(err, "Unable to xml decode location constraint") + // Treat all other failures as XML parsing errors. + return ErrMalformedXML + } // Successfully decoded, proceed to verify the region. + + // Once region has been obtained we proceed to verify it. + incomingRegion := locationConstraint.Location + if incomingRegion == "" { + // Location constraint is empty for region "us-east-1", + // in accordance with protocol. + incomingRegion = "us-east-1" + } + // Return errInvalidRegion if location constraint does not match + // with configured region. + s3Error = ErrNone + if serverRegion != incomingRegion { + s3Error = ErrInvalidRegion } - return errCode + return s3Error } diff --git a/handler-utils_test.go b/handler-utils_test.go index 0b5ac1cb7..d06d7c565 100644 --- a/handler-utils_test.go +++ b/handler-utils_test.go @@ -26,6 +26,24 @@ import ( // Tests validate bucket LocationConstraint. func TestIsValidLocationContraint(t *testing.T) { + savedServerConfig := serverConfig + defer func() { + serverConfig = savedServerConfig + }() + serverConfig = nil + + // Test initialized config file. + path, err := ioutil.TempDir("", "minio-") + if err != nil { + t.Fatalf("Unable to create a temporary directory, %s", err) + } + defer removeAll(path) + + setGlobalConfigPath(path) + if err := initConfig(); err != nil { + t.Fatalf("unable initialize config file, %s", err) + } + // generates the input request with XML bucket configuration set to the request body. createExpectedRequest := func(req *http.Request, location string) (*http.Request, error) { createBucketConfig := createBucketLocationConfiguration{} @@ -37,6 +55,7 @@ func TestIsValidLocationContraint(t *testing.T) { } createBucketConfigBuffer := bytes.NewBuffer(createBucketConfigBytes) req.Body = ioutil.NopCloser(createBucketConfigBuffer) + req.ContentLength = int64(createBucketConfigBuffer.Len()) return req, nil } @@ -58,7 +77,8 @@ func TestIsValidLocationContraint(t *testing.T) { if e != nil { t.Fatalf("Test %d: Failed to Marshal bucket configuration", i+1) } - actualCode := isValidLocationContraint(inputRequest.Body, testCase.serverConfigRegion) + serverConfig.SetRegion(testCase.serverConfigRegion) + actualCode := isValidLocationConstraint(inputRequest) if testCase.expectedCode != actualCode { t.Errorf("Test %d: Expected the APIErrCode to be %d, but instead found %d", i+1, testCase.expectedCode, actualCode) } diff --git a/utils.go b/utils.go index 48cbe2dec..ef60d5bcd 100644 --- a/utils.go +++ b/utils.go @@ -24,8 +24,14 @@ import ( ) // xmlDecoder provide decoded value in xml. -func xmlDecoder(body io.Reader, v interface{}) error { - d := xml.NewDecoder(body) +func xmlDecoder(body io.Reader, v interface{}, size int64) error { + var lbody io.Reader + if size > 0 { + lbody = io.LimitReader(body, size) + } else { + lbody = body + } + d := xml.NewDecoder(lbody) return d.Decode(v) }