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

@@ -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

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
 	}

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

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" {

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) {

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

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"},

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