From c90999df987833790238c702a807a88f85b794c0 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Wed, 3 Apr 2019 23:10:37 -0700 Subject: [PATCH] Valid if bucket names are internal (#7476) This commit fixes a privilege escalation issue against the S3 and web handlers. An authenticated IAM user can: - Read from or write to the internal '.minio.sys' bucket by simply sending a properly signed S3 GET or PUT request. Further, the user can - Read from or write to the internal '.minio.sys' bucket using the 'Upload'/'Download'/'DownloadZIP' API by sending a "browser" request authenticated with its JWT token. --- Makefile | 18 ++++++------ cmd/admin-handlers.go | 4 +-- cmd/api-response.go | 2 +- cmd/generic-handlers.go | 26 ++++++++--------- cmd/generic-handlers_test.go | 9 +++++- cmd/web-handlers.go | 54 ++++++++++++++++++++++++++++++++++++ cmd/web-handlers_test.go | 10 +++---- cmd/xl-v1-healing.go | 3 +- 8 files changed, 92 insertions(+), 34 deletions(-) diff --git a/Makefile b/Makefile index e4a22d61a..0eba81179 100644 --- a/Makefile +++ b/Makefile @@ -33,20 +33,20 @@ fmt: lint: @echo "Running $@" - @${GOPATH}/bin/golint -set_exit_status github.com/minio/minio/cmd/... - @${GOPATH}/bin/golint -set_exit_status github.com/minio/minio/pkg/... + @GO111MODULE=on ${GOPATH}/bin/golint -set_exit_status github.com/minio/minio/cmd/... + @GO111MODULE=on ${GOPATH}/bin/golint -set_exit_status github.com/minio/minio/pkg/... staticcheck: @echo "Running $@" - @${GOPATH}/bin/staticcheck github.com/minio/minio/cmd/... - @${GOPATH}/bin/staticcheck github.com/minio/minio/pkg/... + @GO111MODULE=on ${GOPATH}/bin/staticcheck github.com/minio/minio/cmd/... + @GO111MODULE=on ${GOPATH}/bin/staticcheck github.com/minio/minio/pkg/... spelling: - @${GOPATH}/bin/misspell -locale US -error `find cmd/` - @${GOPATH}/bin/misspell -locale US -error `find pkg/` - @${GOPATH}/bin/misspell -locale US -error `find docs/` - @${GOPATH}/bin/misspell -locale US -error `find buildscripts/` - @${GOPATH}/bin/misspell -locale US -error `find dockerscripts/` + @GO111MODULE=on ${GOPATH}/bin/misspell -locale US -error `find cmd/` + @GO111MODULE=on ${GOPATH}/bin/misspell -locale US -error `find pkg/` + @GO111MODULE=on ${GOPATH}/bin/misspell -locale US -error `find docs/` + @GO111MODULE=on ${GOPATH}/bin/misspell -locale US -error `find buildscripts/` + @GO111MODULE=on ${GOPATH}/bin/misspell -locale US -error `find dockerscripts/` # Builds minio, runs the verifiers then runs the tests. check: test diff --git a/cmd/admin-handlers.go b/cmd/admin-handlers.go index 4c76219a9..7641240be 100644 --- a/cmd/admin-handlers.go +++ b/cmd/admin-handlers.go @@ -39,7 +39,7 @@ import ( "github.com/minio/minio/pkg/cpu" "github.com/minio/minio/pkg/disk" "github.com/minio/minio/pkg/handlers" - "github.com/minio/minio/pkg/iam/policy" + iampolicy "github.com/minio/minio/pkg/iam/policy" "github.com/minio/minio/pkg/madmin" "github.com/minio/minio/pkg/mem" xnet "github.com/minio/minio/pkg/net" @@ -594,7 +594,7 @@ func extractHealInitParams(r *http.Request) (bucket, objPrefix string, err = ErrHealMissingBucket return } - } else if !IsValidBucketName(bucket) { + } else if isReservedOrInvalidBucket(bucket, false) { err = ErrInvalidBucketName return } diff --git a/cmd/api-response.go b/cmd/api-response.go index f77ae5c57..ed42df337 100644 --- a/cmd/api-response.go +++ b/cmd/api-response.go @@ -594,7 +594,7 @@ func writeErrorResponse(ctx context.Context, w http.ResponseWriter, err APIError case "AccessDenied": // The request is from browser and also if browser // is enabled we need to redirect. - if browser && globalIsBrowserEnabled { + if browser { w.Header().Set("Location", minioReservedBucketPath+reqURL.Path) w.WriteHeader(http.StatusTemporaryRedirect) return diff --git a/cmd/generic-handlers.go b/cmd/generic-handlers.go index 28e61e447..f6eb43c18 100644 --- a/cmd/generic-handlers.go +++ b/cmd/generic-handlers.go @@ -196,7 +196,9 @@ func guessIsBrowserReq(req *http.Request) bool { if req == nil { return false } - return strings.Contains(req.Header.Get("User-Agent"), "Mozilla") + aType := getRequestAuthType(req) + return strings.Contains(req.Header.Get("User-Agent"), "Mozilla") && globalIsBrowserEnabled && + (aType == authTypeJWT || aType == authTypeAnonymous) } // guessIsHealthCheckReq - returns true if incoming request looks @@ -232,18 +234,14 @@ func guessIsRPCReq(req *http.Request) bool { } func (h redirectHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { - aType := getRequestAuthType(r) - // Re-direct only for JWT and anonymous requests from browser. - if aType == authTypeJWT || aType == authTypeAnonymous { - // Re-direction is handled specifically for browser requests. - if guessIsBrowserReq(r) && globalIsBrowserEnabled { - // Fetch the redirect location if any. - redirectLocation := getRedirectLocation(r.URL.Path) - if redirectLocation != "" { - // Employ a temporary re-direct. - http.Redirect(w, r, redirectLocation, http.StatusTemporaryRedirect) - return - } + // Re-direction is handled specifically for browser requests. + if guessIsBrowserReq(r) { + // Fetch the redirect location if any. + redirectLocation := getRedirectLocation(r.URL.Path) + if redirectLocation != "" { + // Employ a temporary re-direct. + http.Redirect(w, r, redirectLocation, http.StatusTemporaryRedirect) + return } } h.handler.ServeHTTP(w, r) @@ -259,7 +257,7 @@ func setBrowserCacheControlHandler(h http.Handler) http.Handler { } func (h cacheControlHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { - if r.Method == http.MethodGet && guessIsBrowserReq(r) && globalIsBrowserEnabled { + if r.Method == http.MethodGet && guessIsBrowserReq(r) { // For all browser requests set appropriate Cache-Control policies if hasPrefix(r.URL.Path, minioReservedBucketPath+"/") { if hasSuffix(r.URL.Path, ".js") || r.URL.Path == minioReservedBucketPath+"/favicon.ico" { diff --git a/cmd/generic-handlers_test.go b/cmd/generic-handlers_test.go index 6c0ccf5e7..e544b69e6 100644 --- a/cmd/generic-handlers_test.go +++ b/cmd/generic-handlers_test.go @@ -103,18 +103,25 @@ func TestGuessIsRPC(t *testing.T) { // Tests browser request guess function. func TestGuessIsBrowser(t *testing.T) { + globalIsBrowserEnabled = true if guessIsBrowserReq(nil) { t.Fatal("Unexpected return for nil request") } r := &http.Request{ Header: http.Header{}, + URL: &url.URL{}, } r.Header.Set("User-Agent", "Mozilla") if !guessIsBrowserReq(r) { - t.Fatal("Test shouldn't fail for a possible browser request.") + t.Fatal("Test shouldn't fail for a possible browser request anonymous user") + } + r.Header.Set("Authorization", "Bearer token") + if !guessIsBrowserReq(r) { + t.Fatal("Test shouldn't fail for a possible browser request JWT user") } r = &http.Request{ Header: http.Header{}, + URL: &url.URL{}, } r.Header.Set("User-Agent", "mc") if guessIsBrowserReq(r) { diff --git a/cmd/web-handlers.go b/cmd/web-handlers.go index 6cdd16f2b..ca9b5c12d 100644 --- a/cmd/web-handlers.go +++ b/cmd/web-handlers.go @@ -209,6 +209,11 @@ func (web *webAPIHandlers) DeleteBucket(r *http.Request, args *RemoveBucketArgs, return toJSONError(errAccessDenied) } + // Check if bucket is a reserved bucket name or invalid. + if isReservedOrInvalidBucket(args.BucketName, false) { + return toJSONError(errInvalidBucketName) + } + reply.UIVersion = browser.UIVersion if isRemoteCallRequired(context.Background(), args.BucketName, objectAPI) { @@ -500,6 +505,11 @@ func (web *webAPIHandlers) ListObjects(r *http.Request, args *ListObjectsArgs, r } } + // Check if bucket is a reserved bucket name or invalid. + if isReservedOrInvalidBucket(args.BucketName, false) { + return toJSONError(errInvalidBucketName) + } + lo, err := listObjects(context.Background(), args.BucketName, args.Prefix, args.Marker, slashSeparator, 1000) if err != nil { return &json2.Error{Message: err.Error()} @@ -566,6 +576,11 @@ func (web *webAPIHandlers) RemoveObject(r *http.Request, args *RemoveObjectArgs, return toJSONError(errInvalidArgument) } + // Check if bucket is a reserved bucket name or invalid. + if isReservedOrInvalidBucket(args.BucketName, false) { + return toJSONError(errInvalidBucketName) + } + reply.UIVersion = browser.UIVersion if isRemoteCallRequired(context.Background(), args.BucketName, objectAPI) { sr, err := globalDNSConfig.Get(args.BucketName) @@ -876,6 +891,13 @@ func (web *webAPIHandlers) Upload(w http.ResponseWriter, r *http.Request) { return } } + + // Check if bucket is a reserved bucket name or invalid. + if isReservedOrInvalidBucket(bucket, false) { + writeWebErrorResponse(w, errInvalidBucketName) + return + } + if globalAutoEncryption && !crypto.SSEC.IsRequested(r.Header) { r.Header.Add(crypto.SSEHeader, crypto.SSEAlgorithmAES256) } @@ -1046,6 +1068,12 @@ func (web *webAPIHandlers) Download(w http.ResponseWriter, r *http.Request) { } } + // Check if bucket is a reserved bucket name or invalid. + if isReservedOrInvalidBucket(bucket, false) { + writeWebErrorResponse(w, errInvalidBucketName) + return + } + getObjectNInfo := objectAPI.GetObjectNInfo if web.CacheAPI() != nil { getObjectNInfo = web.CacheAPI().GetObjectNInfo @@ -1193,6 +1221,12 @@ func (web *webAPIHandlers) DownloadZip(w http.ResponseWriter, r *http.Request) { } } + // Check if bucket is a reserved bucket name or invalid. + if isReservedOrInvalidBucket(args.BucketName, false) { + writeWebErrorResponse(w, errInvalidBucketName) + return + } + getObject := objectAPI.GetObject if web.CacheAPI() != nil { getObject = web.CacheAPI().GetObject @@ -1379,6 +1413,11 @@ func (web *webAPIHandlers) GetBucketPolicy(r *http.Request, args *GetBucketPolic return toJSONError(errAccessDenied) } + // Check if bucket is a reserved bucket name or invalid. + if isReservedOrInvalidBucket(args.BucketName, false) { + return toJSONError(errInvalidBucketName) + } + var policyInfo = &miniogopolicy.BucketAccessPolicy{Version: "2012-10-17"} if isRemoteCallRequired(context.Background(), args.BucketName, objectAPI) { sr, err := globalDNSConfig.Get(args.BucketName) @@ -1462,6 +1501,11 @@ func (web *webAPIHandlers) ListAllBucketPolicies(r *http.Request, args *ListAllB return toJSONError(errAccessDenied) } + // Check if bucket is a reserved bucket name or invalid. + if isReservedOrInvalidBucket(args.BucketName, false) { + return toJSONError(errInvalidBucketName) + } + var policyInfo = new(miniogopolicy.BucketAccessPolicy) if isRemoteCallRequired(context.Background(), args.BucketName, objectAPI) { sr, err := globalDNSConfig.Get(args.BucketName) @@ -1538,6 +1582,11 @@ func (web *webAPIHandlers) SetBucketPolicy(r *http.Request, args *SetBucketPolic return toJSONError(errAccessDenied) } + // Check if bucket is a reserved bucket name or invalid. + if isReservedOrInvalidBucket(args.BucketName, false) { + return toJSONError(errInvalidBucketName) + } + policyType := miniogopolicy.BucketPolicy(args.Policy) if !policyType.IsValidBucketPolicy() { return &json2.Error{ @@ -1685,6 +1734,11 @@ func (web *webAPIHandlers) PresignedGet(r *http.Request, args *PresignedGetArgs, } } + // Check if bucket is a reserved bucket name or invalid. + if isReservedOrInvalidBucket(args.BucketName, false) { + return toJSONError(errInvalidBucketName) + } + reply.UIVersion = browser.UIVersion reply.URL = presignedGet(args.HostName, args.BucketName, args.ObjectName, args.Expiry, creds, region) return nil diff --git a/cmd/web-handlers_test.go b/cmd/web-handlers_test.go index 602965f9f..1894c2362 100644 --- a/cmd/web-handlers_test.go +++ b/cmd/web-handlers_test.go @@ -334,13 +334,13 @@ func testDeleteBucketWebHandler(obj ObjectLayer, instanceType string, t TestErrH // Empty string = no error expect string }{ - {"", false, token, "The specified bucket does not exist."}, + {"", false, token, "The specified bucket is not valid"}, {".", false, "auth", "Authentication failed"}, - {".", false, token, "The specified bucket . does not exist."}, - {"..", false, token, "The specified bucket .. does not exist."}, - {"ab", false, token, "The specified bucket ab does not exist."}, + {".", false, token, "The specified bucket is not valid"}, + {"..", false, token, "The specified bucket is not valid"}, + {"ab", false, token, "The specified bucket is not valid"}, {"minio", false, "false token", "Authentication failed"}, - {"minio", false, token, "specified bucket minio does not exist"}, + {"minio", false, token, "The specified bucket is not valid"}, {bucketName, false, token, ""}, {bucketName, true, token, "Bucket not empty"}, {bucketName, false, "", "JWT token missing"}, diff --git a/cmd/xl-v1-healing.go b/cmd/xl-v1-healing.go index 0b925650a..ec77b7a9f 100644 --- a/cmd/xl-v1-healing.go +++ b/cmd/xl-v1-healing.go @@ -169,8 +169,7 @@ func listAllBuckets(storageDisks []StorageAPI) (buckets map[string]VolInfo, // StorageAPI can send volume names which are // incompatible with buckets - these are // skipped, like the meta-bucket. - if !IsValidBucketName(volInfo.Name) || - isMinioMetaBucketName(volInfo.Name) { + if isReservedOrInvalidBucket(volInfo.Name, false) { continue } // Increase counter per bucket name