maxmustermann/minio
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

fix routing issue for esoteric characters in gorilla/mux (#8967)

Browse source 
First step is to ensure that Path component is not decoded
by gorilla/mux to avoid routing issues while handling
certain characters while uploading through PutObject()

Delay the decoding and use PathUnescape() to escape
the `object` path component.

Thanks to @buengese and @ncw for neat test cases for us
to test with.

Fixes #8950
Fixes #8647
This commit is contained in:
Harshavardhana 2020-02-12 09:08:02 +05:30 committed by GitHub
parent 7e819d00ea
commit c56c2f5fd3
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
13 changed files with 161 additions and 51 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

9
cmd/acl-handlers.go
View file

@ -19,6 +19,7 @@ package cmd
import (
"encoding/xml"
"net/http"
"net/url"
"github.com/gorilla/mux"
"github.com/minio/minio/cmd/logger"
@ -110,7 +111,11 @@ func (api objectAPIHandlers) GetObjectACLHandler(w http.ResponseWriter, r *http.
vars := mux.Vars(r)
bucket := vars["bucket"]
object := vars["object"]
object, err := url.PathUnescape(vars["object"])
if err != nil {
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
return
}
objAPI := api.ObjectAPI()
if objAPI == nil {
@ -126,7 +131,7 @@ func (api objectAPIHandlers) GetObjectACLHandler(w http.ResponseWriter, r *http.
}
// Before proceeding validate if object exists.
_, err := objAPI.GetObjectInfo(ctx, bucket, object, ObjectOptions{})
_, err = objAPI.GetObjectInfo(ctx, bucket, object, ObjectOptions{})
if err != nil {
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
return

8
cmd/api-errors.go
View file

@ -21,6 +21,7 @@ import (
"encoding/xml"
"fmt"
"net/http"
"net/url"
"strings"
"github.com/Azure/azure-storage-blob-go/azblob"
@ -1802,6 +1803,13 @@ func toAPIError(ctx context.Context, err error) APIError {
// their internal error types. This code is only
// useful with gateway implementations.
switch e := err.(type) {
case url.EscapeError:
apiErr = APIError{
Code: "XMinioInvalidObjectName",
Description: fmt.Sprintf("%s (%s)", errorCodes[ErrInvalidObjectName].Description,
e.Error()),
HTTPStatusCode: http.StatusBadRequest,
}
case lifecycle.Error:
apiErr = APIError{
Code: "InvalidRequest",

2
cmd/fs-v1_test.go
View file

@ -280,7 +280,7 @@ func TestFSDeleteObject(t *testing.T) {
t.Fatal("Unexpected error: ", err)
}
// Test with invalid object name
if err := fs.DeleteObject(context.Background(), bucketName, "\\"); !isSameType(err, ObjectNameInvalid{}) {
if err := fs.DeleteObject(context.Background(), bucketName, "\\"); !isSameType(err, ObjectNotFound{}) {
t.Fatal("Unexpected error: ", err)
}
// Test with object does not exist.

5
cmd/gateway-main.go
View file

@ -156,7 +156,10 @@ func StartGateway(ctx *cli.Context, gw Gateway) {
globalServerConfig = srvCfg
globalServerConfigMu.Unlock()
router := mux.NewRouter().SkipClean(true)
// Initialize router. `SkipClean(true)` stops gorilla/mux from
// normalizing URL path minio/minio#3256
// avoid URL path encoding minio/minio#8950
router := mux.NewRouter().SkipClean(true).UseEncodedPath()
if globalEtcdClient != nil {
// Enable STS router if etcd is enabled.

6
cmd/logger/audit.go
View file

@ -21,6 +21,7 @@ import (
"fmt"
"io"
"net/http"
"net/url"
"time"
"github.com/gorilla/mux"
@ -143,7 +144,10 @@ func AuditLog(w http.ResponseWriter, r *http.Request, api string, reqClaims map[
vars := mux.Vars(r)
bucket := vars["bucket"]
object := vars["object"]
object, err := url.PathUnescape(vars["object"])
if err != nil {
object = vars["object"]
}
// Send audit logs only to http targets.
for _, t := range AuditTargets {

2
cmd/object-api-listobjects_test.go
View file

@ -552,7 +552,7 @@ func testListObjects(obj ObjectLayer, instanceType string, t1 TestErrHandler) {
// Test with prefix and delimiter set to '/'. (60)
{"test-bucket-list-object", SlashSeparator, "", SlashSeparator, 10, resultCases[30], nil, true},
// Test with invalid prefix (61)
{"test-bucket-list-object", "\\", "", SlashSeparator, 10, ListObjectsInfo{}, ObjectNameInvalid{Bucket: "test-bucket-list-object", Object: "\\"}, false},
{"test-bucket-list-object", "\\", "", SlashSeparator, 10, ListObjectsInfo{}, nil, true},
// Test listing an empty directory in recursive mode (62)
{"test-bucket-empty-dir", "", "", "", 10, resultCases[31], nil, true},
// Test listing an empty directory in a non recursive mode (63)

11
cmd/object-api-multipart_test.go
View file

@ -61,17 +61,12 @@ func testObjectNewMultipartUpload(obj ObjectLayer, instanceType string, t TestEr
t.Fatalf("%s : %s", instanceType, err.Error())
}
_, err = obj.NewMultipartUpload(context.Background(), bucket, "\\", opts)
if err == nil {
t.Fatalf("%s: Expected to fail since object name is invalid.", instanceType)
}
uploadID, err := obj.NewMultipartUpload(context.Background(), bucket, object, opts)
uploadID, err := obj.NewMultipartUpload(context.Background(), bucket, "\\", opts)
if err != nil {
t.Fatalf("%s : %s", instanceType, err.Error())
}
err = obj.AbortMultipartUpload(context.Background(), bucket, object, uploadID)
err = obj.AbortMultipartUpload(context.Background(), bucket, "\\", uploadID)
if err != nil {
switch err.(type) {
case InvalidUploadID:
@ -112,9 +107,9 @@ func testObjectAbortMultipartUpload(obj ObjectLayer, instanceType string, t Test
expectedErrType error
}{
{"--", object, uploadID, BucketNotFound{}},
{bucket, "\\", uploadID, ObjectNameInvalid{}},
{"foo", object, uploadID, BucketNotFound{}},
{bucket, object, "foo-foo", InvalidUploadID{}},
{bucket, "\\", uploadID, InvalidUploadID{}},
{bucket, object, uploadID, nil},
}
// Iterating over creatPartCases to generate multipart chunks.

4
cmd/object-api-utils.go
View file

@ -165,10 +165,6 @@ func IsValidObjectPrefix(object string) bool {
if !utf8.ValidString(object) {
return false
}
// Reject unsupported characters in object name.
if strings.ContainsAny(object, `\`) {
return false
}
if strings.Contains(object, `//`) {
return false
}

14
cmd/object-api-utils_test.go
View file

@ -105,13 +105,24 @@ func TestIsValidObjectName(t *testing.T) {
{"f*le", true},
{"contains-^-carret", true},
{"contains-|-pipe", true},
{"contains-\"-quote", true},
{"contains-`-tick", true},
{"..test", true},
{".. test", true},
{". test", true},
{".test", true},
{"There are far too many object names, and far too few bucket names!", true},
{"!\"#$%&'()*+,-.／:;<=>?@[\\]^_`{|}~/!\"#$%&'()*+,-.／:;<=>?@[\\]^_`{|}~)", true},
{"!\"#$%&'()*+,-.／:;<=>?@[\\]^_`{|}~", true},
{"␀␁␂␃␄␅␆␇␈␉␊␋␌␍␎␏␐␑␒␓␔␕␖␗␘␙␚␛␜␝␞␟␡", true},
{"trailing VT␋/trailing VT␋", true},
{"␋leading VT/␋leading VT", true},
{"~leading tilde", true},
{"\rleading CR", true},
{"\nleading LF", true},
{"\tleading HT", true},
{"trailing CR\r", true},
{"trailing LF\n", true},
{"trailing HT\t", true},
// cases for which test should fail.
// passing invalid object names.
{"", false},
@ -122,7 +133,6 @@ func TestIsValidObjectName(t *testing.T) {
{" ../etc", false},
{"./././", false},
{"./etc", false},
{`contains-\-backslash`, false},
{`contains//double/forwardslash`, false},
{`//contains/double-forwardslash-prefix`, false},
{string([]byte{0xff, 0xfe, 0xfd}), false},

125
cmd/object-handlers.go
View file

@ -109,7 +109,11 @@ func (api objectAPIHandlers) SelectObjectContentHandler(w http.ResponseWriter, r
}
vars := mux.Vars(r)
bucket := vars["bucket"]
object := vars["object"]
object, err := url.PathUnescape(vars["object"])
if err != nil {
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
return
}
// get gateway encryption options
opts, err := getOpts(ctx, r, bucket, object)
@ -273,7 +277,11 @@ func (api objectAPIHandlers) GetObjectHandler(w http.ResponseWriter, r *http.Req
}
vars := mux.Vars(r)
bucket := vars["bucket"]
object := vars["object"]
object, err := url.PathUnescape(vars["object"])
if err != nil {
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
return
}
if vid := r.URL.Query().Get("versionId"); vid != "" && vid != "null" {
writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrNoSuchVersion), r.URL, guessIsBrowserReq(r))
@ -451,7 +459,11 @@ func (api objectAPIHandlers) HeadObjectHandler(w http.ResponseWriter, r *http.Re
}
vars := mux.Vars(r)
bucket := vars["bucket"]
object := vars["object"]
object, err := url.PathUnescape(vars["object"])
if err != nil {
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
return
}
if vid := r.URL.Query().Get("versionId"); vid != "" && vid != "null" {
writeErrorResponseHeadersOnly(w, errorCodes.ToAPIErr(ErrNoSuchVersion))
@ -704,7 +716,11 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re
}
vars := mux.Vars(r)
dstBucket := vars["bucket"]
dstObject := vars["object"]
dstObject, err := url.PathUnescape(vars["object"])
if err != nil {
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
return
}
if s3Error := checkRequestAuthType(ctx, r, policy.PutObjectAction, dstBucket, dstObject); s3Error != ErrNone {
writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Error), r.URL, guessIsBrowserReq(r))
@ -774,7 +790,7 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re
}
var srcOpts, dstOpts ObjectOptions
srcOpts, err := copySrcOpts(ctx, r, srcBucket, srcObject)
srcOpts, err = copySrcOpts(ctx, r, srcBucket, srcObject)
if err != nil {
logger.LogIf(ctx, err)
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
@ -1139,7 +1155,11 @@ func (api objectAPIHandlers) PutObjectHandler(w http.ResponseWriter, r *http.Req
}
vars := mux.Vars(r)
bucket := vars["bucket"]
object := vars["object"]
object, err := url.PathUnescape(vars["object"])
if err != nil {
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
return
}
// To detect if the client has disconnected.
r.Body = &detectDisconnect{r.Body, r.Context().Done()}
@ -1428,7 +1448,11 @@ func (api objectAPIHandlers) NewMultipartUploadHandler(w http.ResponseWriter, r
}
vars := mux.Vars(r)
bucket := vars["bucket"]
object := vars["object"]
object, err := url.PathUnescape(vars["object"])
if err != nil {
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
return
}
if s3Error := checkRequestAuthType(ctx, r, policy.PutObjectAction, bucket, object); s3Error != ErrNone {
writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Error), r.URL, guessIsBrowserReq(r))
@ -1443,7 +1467,6 @@ func (api objectAPIHandlers) NewMultipartUploadHandler(w http.ResponseWriter, r
// get gateway encryption options
var opts ObjectOptions
var err error
opts, err = putOpts(ctx, r, bucket, object, nil)
if err != nil {
@ -1550,7 +1573,11 @@ func (api objectAPIHandlers) CopyObjectPartHandler(w http.ResponseWriter, r *htt
vars := mux.Vars(r)
dstBucket := vars["bucket"]
dstObject := vars["object"]
dstObject, err := url.PathUnescape(vars["object"])
if err != nil {
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
return
}
if s3Error := checkRequestAuthType(ctx, r, policy.PutObjectAction, dstBucket, dstObject); s3Error != ErrNone {
writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Error), r.URL, guessIsBrowserReq(r))
@ -1866,7 +1893,11 @@ func (api objectAPIHandlers) PutObjectPartHandler(w http.ResponseWriter, r *http
}
vars := mux.Vars(r)
bucket := vars["bucket"]
object := vars["object"]
object, err := url.PathUnescape(vars["object"])
if err != nil {
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
return
}
// X-Amz-Copy-Source shouldn't be set for this call.
if _, ok := r.Header[xhttp.AmzCopySource]; ok {
@ -2108,7 +2139,11 @@ func (api objectAPIHandlers) AbortMultipartUploadHandler(w http.ResponseWriter,
vars := mux.Vars(r)
bucket := vars["bucket"]
object := vars["object"]
object, err := url.PathUnescape(vars["object"])
if err != nil {
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
return
}
objectAPI := api.ObjectAPI()
if objectAPI == nil {
@ -2143,7 +2178,11 @@ func (api objectAPIHandlers) ListObjectPartsHandler(w http.ResponseWriter, r *ht
vars := mux.Vars(r)
bucket := vars["bucket"]
object := vars["object"]
object, err := url.PathUnescape(vars["object"])
if err != nil {
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
return
}
objectAPI := api.ObjectAPI()
if objectAPI == nil {
@ -2277,7 +2316,11 @@ func (api objectAPIHandlers) CompleteMultipartUploadHandler(w http.ResponseWrite
vars := mux.Vars(r)
bucket := vars["bucket"]
object := vars["object"]
object, err := url.PathUnescape(vars["object"])
if err != nil {
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
return
}
objectAPI := api.ObjectAPI()
if objectAPI == nil {
@ -2474,7 +2517,11 @@ func (api objectAPIHandlers) DeleteObjectHandler(w http.ResponseWriter, r *http.
vars := mux.Vars(r)
bucket := vars["bucket"]
object := vars["object"]
object, err := url.PathUnescape(vars["object"])
if err != nil {
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
return
}
objectAPI := api.ObjectAPI()
if objectAPI == nil {
@ -2532,7 +2579,11 @@ func (api objectAPIHandlers) PutObjectLegalHoldHandler(w http.ResponseWriter, r
vars := mux.Vars(r)
bucket := vars["bucket"]
object := vars["object"]
object, err := url.PathUnescape(vars["object"])
if err != nil {
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
return
}
if vid := r.URL.Query().Get("versionId"); vid != "" && vid != "null" {
writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrNoSuchVersion), r.URL, guessIsBrowserReq(r))
@ -2630,7 +2681,11 @@ func (api objectAPIHandlers) GetObjectLegalHoldHandler(w http.ResponseWriter, r
vars := mux.Vars(r)
bucket := vars["bucket"]
object := vars["object"]
object, err := url.PathUnescape(vars["object"])
if err != nil {
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
return
}
if vid := r.URL.Query().Get("versionId"); vid != "" && vid != "null" {
writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrNoSuchVersion), r.URL, guessIsBrowserReq(r))
@ -2687,7 +2742,11 @@ func (api objectAPIHandlers) PutObjectRetentionHandler(w http.ResponseWriter, r
vars := mux.Vars(r)
bucket := vars["bucket"]
object := vars["object"]
object, err := url.PathUnescape(vars["object"])
if err != nil {
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
return
}
if vid := r.URL.Query().Get("versionId"); vid != "" && vid != "null" {
writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrNoSuchVersion), r.URL, guessIsBrowserReq(r))
@ -2779,7 +2838,11 @@ func (api objectAPIHandlers) GetObjectRetentionHandler(w http.ResponseWriter, r
vars := mux.Vars(r)
bucket := vars["bucket"]
object := vars["object"]
object, err := url.PathUnescape(vars["object"])
if err != nil {
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
return
}
if vid := r.URL.Query().Get("versionId"); vid != "" && vid != "null" {
writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrNoSuchVersion), r.URL, guessIsBrowserReq(r))
@ -2835,7 +2898,11 @@ func (api objectAPIHandlers) GetObjectTaggingHandler(w http.ResponseWriter, r *h
vars := mux.Vars(r)
bucket := vars["bucket"]
object := vars["object"]
object, err := url.PathUnescape(vars["object"])
if err != nil {
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
return
}
objAPI := api.ObjectAPI()
if objAPI == nil {
@ -2866,7 +2933,11 @@ func (api objectAPIHandlers) PutObjectTaggingHandler(w http.ResponseWriter, r *h
vars := mux.Vars(r)
bucket := vars["bucket"]
object := vars["object"]
object, err := url.PathUnescape(vars["object"])
if err != nil {
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
return
}
objAPI := api.ObjectAPI()
if objAPI == nil {
@ -2901,16 +2972,20 @@ func (api objectAPIHandlers) DeleteObjectTaggingHandler(w http.ResponseWriter, r
ctx := newContext(r, w, "DeleteObjectTagging")
defer logger.AuditLog(w, r, "DeleteObjectTagging", mustGetClaimsFromToken(r))
vars := mux.Vars(r)
bucket := vars["bucket"]
object := vars["object"]
objAPI := api.ObjectAPI()
if objAPI == nil {
writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrServerNotInitialized), r.URL, guessIsBrowserReq(r))
return
}
vars := mux.Vars(r)
bucket := vars["bucket"]
object, err := url.PathUnescape(vars["object"])
if err != nil {
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
return
}
// Allow deleteObjectTagging if policy action is set
if s3Error := checkRequestAuthType(ctx, r, policy.DeleteObjectTaggingAction, bucket, object); s3Error != ErrNone {
writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Error), r.URL, guessIsBrowserReq(r))
@ -2918,7 +2993,7 @@ func (api objectAPIHandlers) DeleteObjectTaggingHandler(w http.ResponseWriter, r
}
// Delete object tags
err := objAPI.DeleteObjectTag(ctx, bucket, object)
err = objAPI.DeleteObjectTag(ctx, bucket, object)
if err != nil {
writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
return

2
cmd/routers.go
View file

@ -84,7 +84,7 @@ var globalHandlers = []HandlerFunc{
func configureServerHandler(endpointZones EndpointZones) (http.Handler, error) {
// Initialize router. `SkipClean(true)` stops gorilla/mux from
// normalizing URL path minio/minio#3256
router := mux.NewRouter().SkipClean(true)
router := mux.NewRouter().SkipClean(true).UseEncodedPath()
// Initialize distributed NS lock.
if globalIsDistXL {

12
cmd/utils.go
View file

@ -29,6 +29,7 @@ import (
"io/ioutil"
"net"
"net/http"
"net/url"
"os"
"path/filepath"
"reflect"
@ -506,9 +507,14 @@ func ceilFrac(numerator, denominator int64) (ceil int64) {
func newContext(r *http.Request, w http.ResponseWriter, api string) context.Context {
vars := mux.Vars(r)
bucket := vars["bucket"]
object := vars["object"]
prefix := vars["prefix"]
object, err := url.PathUnescape(vars["object"])
if err != nil {
object = vars["object"]
}
prefix, err := url.QueryUnescape(vars["prefix"])
if err != nil {
prefix = vars["prefix"]
}
if prefix != "" {
object = prefix
}

12
cmd/web-handlers.go
View file

@ -918,7 +918,11 @@ func (web *webAPIHandlers) Upload(w http.ResponseWriter, r *http.Request) {
}
vars := mux.Vars(r)
bucket := vars["bucket"]
object := vars["object"]
object, err := url.PathUnescape(vars["object"])
if err != nil {
writeWebErrorResponse(w, err)
return
}
retPerms := ErrAccessDenied
holdPerms := ErrAccessDenied
@ -1135,7 +1139,11 @@ func (web *webAPIHandlers) Download(w http.ResponseWriter, r *http.Request) {
vars := mux.Vars(r)
bucket := vars["bucket"]
object := vars["object"]
object, err := url.PathUnescape(vars["object"])
if err != nil {
writeWebErrorResponse(w, err)
return
}
token := r.URL.Query().Get("token")
getRetPerms := ErrAccessDenied