From c2ed1347d96649bd80729f93b42d47b9681e8747 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Thu, 13 Dec 2018 20:15:09 -0800 Subject: [PATCH] Do not list objects unless specified in policy (#6970) Currently we use GetObject to check if we are allowed to list, this might be a security problem since there are many users now who actively disable a publicly readable listing, anyone who can guess the browser URL can list the objects. This PR turns off this behavior and provides a more expected way based on the policies. This PR also additionally improves the Download() object implementation to use a more streamlined code. These are precursor changes to facilitate federation and web identity support in browser. --- cmd/web-handlers.go | 134 ++++++++++++++++----------------------- cmd/web-handlers_test.go | 4 +- 2 files changed, 56 insertions(+), 82 deletions(-) diff --git a/cmd/web-handlers.go b/cmd/web-handlers.go index a11664478..8d70f56a0 100644 --- a/cmd/web-handlers.go +++ b/cmd/web-handlers.go @@ -271,9 +271,10 @@ func (web *webAPIHandlers) ListBuckets(r *http.Request, args *WebGenericArgs, re } for _, dnsRecord := range dnsBuckets { bucketName := strings.Trim(dnsRecord.Key, "/") + if globalIAMSys.IsAllowed(iampolicy.Args{ AccountName: claims.Subject, - Action: iampolicy.Action(policy.GetObjectAction), + Action: iampolicy.ListBucketAction, BucketName: bucketName, ConditionValues: getConditionValues(r, ""), IsOwner: owner, @@ -293,7 +294,7 @@ func (web *webAPIHandlers) ListBuckets(r *http.Request, args *WebGenericArgs, re for _, bucket := range buckets { if globalIAMSys.IsAllowed(iampolicy.Args{ AccountName: claims.Subject, - Action: iampolicy.Action(policy.GetObjectAction), + Action: iampolicy.ListBucketAction, BucketName: bucket.Name, ConditionValues: getConditionValues(r, ""), IsOwner: owner, @@ -355,13 +356,17 @@ func (web *webAPIHandlers) ListObjects(r *http.Request, args *ListObjectsArgs, r claims, owner, authErr := webRequestAuthenticate(r) if authErr != nil { if authErr == errNoAuthToken { + // Add this for checking ListObjects conditional. + if args.Prefix != "" { + r.Header.Set("prefix", args.Prefix) + } + // Check if anonymous (non-owner) has access to download objects. readable := globalPolicySys.IsAllowed(policy.Args{ - Action: policy.GetObjectAction, + Action: policy.ListBucketAction, BucketName: args.BucketName, ConditionValues: getConditionValues(r, ""), IsOwner: false, - ObjectName: args.Prefix + "/", }) // Check if anonymous (non-owner) has access to upload objects. @@ -389,18 +394,22 @@ func (web *webAPIHandlers) ListObjects(r *http.Request, args *ListObjectsArgs, r // For authenticated users apply IAM policy. if authErr == nil { + // Add this for checking ListObjects conditional. + if args.Prefix != "" { + r.Header.Set("prefix", args.Prefix) + } + readable := globalIAMSys.IsAllowed(iampolicy.Args{ AccountName: claims.Subject, - Action: iampolicy.Action(policy.GetObjectAction), + Action: iampolicy.ListBucketAction, BucketName: args.BucketName, ConditionValues: getConditionValues(r, ""), IsOwner: owner, - ObjectName: args.Prefix + "/", }) writable := globalIAMSys.IsAllowed(iampolicy.Args{ AccountName: claims.Subject, - Action: iampolicy.Action(policy.PutObjectAction), + Action: iampolicy.PutObjectAction, BucketName: args.BucketName, ConditionValues: getConditionValues(r, ""), IsOwner: owner, @@ -498,7 +507,7 @@ next: if !globalIAMSys.IsAllowed(iampolicy.Args{ AccountName: claims.Subject, - Action: iampolicy.Action(policy.DeleteObjectAction), + Action: iampolicy.DeleteObjectAction, BucketName: args.BucketName, ConditionValues: getConditionValues(r, ""), IsOwner: owner, @@ -515,7 +524,7 @@ next: if !globalIAMSys.IsAllowed(iampolicy.Args{ AccountName: claims.Subject, - Action: iampolicy.Action(policy.DeleteObjectAction), + Action: iampolicy.DeleteObjectAction, BucketName: args.BucketName, ConditionValues: getConditionValues(r, ""), IsOwner: owner, @@ -766,7 +775,7 @@ func (web *webAPIHandlers) Upload(w http.ResponseWriter, r *http.Request) { if authErr == nil { if !globalIAMSys.IsAllowed(iampolicy.Args{ AccountName: claims.Subject, - Action: iampolicy.Action(policy.PutObjectAction), + Action: iampolicy.PutObjectAction, BucketName: bucket, ConditionValues: getConditionValues(r, ""), IsOwner: owner, @@ -866,7 +875,6 @@ func (web *webAPIHandlers) Download(w http.ResponseWriter, r *http.Request) { defer logger.AuditLog(w, r, "WebDownload", mustGetClaimsFromToken(r)) - var wg sync.WaitGroup objectAPI := web.ObjectAPI() if objectAPI == nil { writeWebErrorResponse(w, errServerNotInitialized) @@ -902,7 +910,7 @@ func (web *webAPIHandlers) Download(w http.ResponseWriter, r *http.Request) { if authErr == nil { if !globalIAMSys.IsAllowed(iampolicy.Args{ AccountName: claims.Subject, - Action: iampolicy.Action(policy.GetObjectAction), + Action: iampolicy.GetObjectAction, BucketName: bucket, ConditionValues: getConditionValues(r, ""), IsOwner: owner, @@ -913,101 +921,67 @@ func (web *webAPIHandlers) Download(w http.ResponseWriter, r *http.Request) { } } - opts := ObjectOptions{} - getObjectInfo := objectAPI.GetObjectInfo - getObject := objectAPI.GetObject + getObjectNInfo := objectAPI.GetObjectNInfo if web.CacheAPI() != nil { - getObjectInfo = web.CacheAPI().GetObjectInfo - getObject = web.CacheAPI().GetObject + getObjectNInfo = web.CacheAPI().GetObjectNInfo } - objInfo, err := getObjectInfo(ctx, bucket, object, opts) + + var opts ObjectOptions + gr, err := getObjectNInfo(ctx, bucket, object, nil, r.Header, readLock, opts) if err != nil { writeWebErrorResponse(w, err) return } - length := objInfo.Size - var actualSize int64 - if objInfo.IsCompressed() { - // Read the decompressed size from the meta.json. - actualSize = objInfo.GetActualSize() - if actualSize < 0 { - return - } - } + defer gr.Close() + + objInfo := gr.ObjInfo + if objectAPI.IsEncryptionSupported() { if _, err = DecryptObjectInfo(&objInfo, r.Header); err != nil { writeWebErrorResponse(w, err) return } + } + + // Set encryption response headers + if objectAPI.IsEncryptionSupported() { if crypto.IsEncrypted(objInfo.UserDefined) { - length, _ = objInfo.DecryptedSize() + switch { + case crypto.S3.IsEncrypted(objInfo.UserDefined): + w.Header().Set(crypto.SSEHeader, crypto.SSEAlgorithmAES256) + case crypto.SSEC.IsEncrypted(objInfo.UserDefined): + w.Header().Set(crypto.SSECAlgorithm, r.Header.Get(crypto.SSECAlgorithm)) + w.Header().Set(crypto.SSECKeyMD5, r.Header.Get(crypto.SSECKeyMD5)) + } } } - var startOffset int64 - var writer io.Writer - if objInfo.IsCompressed() { - // The decompress metrics are set. - snappyStartOffset := 0 - snappyLength := actualSize - - // Open a pipe for compression - // Where compressWriter is actually passed to the getObject - decompressReader, compressWriter := io.Pipe() - snappyReader := snappy.NewReader(decompressReader) - - // The limit is set to the actual size. - responseWriter := ioutil.LimitedWriter(w, int64(snappyStartOffset), snappyLength) - wg.Add(1) //For closures. - go func() { - defer wg.Done() - // Finally, writes to the client. - _, perr := io.Copy(responseWriter, snappyReader) - - // Close the compressWriter if the data is read already. - // Closing the pipe, releases the writer passed to the getObject. - compressWriter.CloseWithError(perr) - }() - writer = compressWriter - } else { - writer = w + if err = setObjectHeaders(w, objInfo, nil); err != nil { + writeWebErrorResponse(w, err) + return } - if objectAPI.IsEncryptionSupported() && crypto.S3.IsEncrypted(objInfo.UserDefined) { - // Response writer should be limited early on for decryption upto required length, - // additionally also skipping mod(offset)64KiB boundaries. - writer = ioutil.LimitedWriter(writer, startOffset%(64*1024), length) - writer, startOffset, length, err = DecryptBlocksRequest(writer, r, bucket, object, startOffset, length, objInfo, false) - if err != nil { - writeWebErrorResponse(w, err) - return - } - w.Header().Set(crypto.SSEHeader, crypto.SSEAlgorithmAES256) - } + // Add content disposition. + w.Header().Set("Content-Disposition", fmt.Sprintf("attachment; filename=\"%s\"", path.Base(objInfo.Name))) - httpWriter := ioutil.WriteOnClose(writer) + setHeadGetRespHeaders(w, r.URL.Query()) - // Add content disposition. - w.Header().Set("Content-Disposition", fmt.Sprintf("attachment; filename=\"%s\"", path.Base(object))) + httpWriter := ioutil.WriteOnClose(w) - if err = getObject(ctx, bucket, object, 0, -1, httpWriter, "", opts); err != nil { - httpWriter.Close() - if objInfo.IsCompressed() { - wg.Wait() + // Write object content to response body + if _, err = io.Copy(httpWriter, gr); err != nil { + if !httpWriter.HasWritten() { // write error response only if no data or headers has been written to client yet + writeWebErrorResponse(w, err) } - /// No need to print error, response writer already written to. return } + if err = httpWriter.Close(); err != nil { - if !httpWriter.HasWritten() { // write error response only if no data has been written to client yet + if !httpWriter.HasWritten() { // write error response only if no data or headers has been written to client yet writeWebErrorResponse(w, err) return } } - if objInfo.IsCompressed() { - // Wait for decompression go-routine to retire. - wg.Wait() - } // Get host and port from Request.RemoteAddr. host, port, err := net.SplitHostPort(handlers.GetSourceIP(r)) @@ -1093,7 +1067,7 @@ func (web *webAPIHandlers) DownloadZip(w http.ResponseWriter, r *http.Request) { for _, object := range args.Objects { if !globalIAMSys.IsAllowed(iampolicy.Args{ AccountName: claims.Subject, - Action: iampolicy.Action(policy.GetObjectAction), + Action: iampolicy.GetObjectAction, BucketName: args.BucketName, ConditionValues: getConditionValues(r, ""), IsOwner: owner, diff --git a/cmd/web-handlers_test.go b/cmd/web-handlers_test.go index d97c0573a..7bba46833 100644 --- a/cmd/web-handlers_test.go +++ b/cmd/web-handlers_test.go @@ -539,8 +539,8 @@ func testListObjectsWebHandler(obj ObjectLayer, instanceType string, t TestErrHa Statements: []policy.Statement{policy.NewStatement( policy.Allow, policy.NewPrincipal("*"), - policy.NewActionSet(policy.GetObjectAction), - policy.NewResourceSet(policy.NewResource(bucketName, "*")), + policy.NewActionSet(policy.ListBucketAction), + policy.NewResourceSet(policy.NewResource(bucketName, "")), condition.NewFunctions(), )}, }