From d3eb5815d9951fdb82d0134b7651368bab71d72e Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Mon, 25 Sep 2017 14:47:58 -0700 Subject: [PATCH] Avoid DDOS in PutObject() when objectName is '/' and size '0' (#4962) It can happen that an incoming PutObject() request might have inputs of following form eg:- - bucketName is 'testbucket' - objectName is '/' bucketName exists and was previously created but there are no other objects in this bucket. In a situation like this parentDirIsObject() goes into an infinite loop. Verifying that if '/' is an object fails on both backends but the resulting `path.Dir('/')` returns `'/'` this causes the closure to loop onto itself. Fixes #4940 --- cmd/fs-v1.go | 5 +- cmd/fs-v1_test.go | 71 ++++++++++++++++++++++++++++ cmd/xl-v1-common.go | 8 ++-- cmd/xl-v1-common_test.go | 99 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 178 insertions(+), 5 deletions(-) create mode 100644 cmd/xl-v1-common_test.go diff --git a/cmd/fs-v1.go b/cmd/fs-v1.go index 922c2aaee..a672791c1 100644 --- a/cmd/fs-v1.go +++ b/cmd/fs-v1.go @@ -491,13 +491,14 @@ func (fs fsObjects) GetObjectInfo(bucket, object string) (oi ObjectInfo, e error func (fs fsObjects) parentDirIsObject(bucket, parent string) bool { var isParentDirObject func(string) bool isParentDirObject = func(p string) bool { - if p == "." { + if p == "." || p == "/" { return false } if _, err := fsStatFile(pathJoin(fs.fsPath, bucket, p)); err == nil { - // If there is already a file at prefix "p" return error. + // If there is already a file at prefix "p", return true. return true } + // Check if there is a file as one of the parent paths. return isParentDirObject(path.Dir(p)) } diff --git a/cmd/fs-v1_test.go b/cmd/fs-v1_test.go index 9dd1bebaf..5f38d2ee1 100644 --- a/cmd/fs-v1_test.go +++ b/cmd/fs-v1_test.go @@ -24,6 +24,77 @@ import ( "testing" ) +// Tests for if parent directory is object +func TestFSParentDirIsObject(t *testing.T) { + rootPath, err := newTestConfig(globalMinioDefaultRegion) + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(rootPath) + + obj, disk, err := prepareFS() + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(disk) + + bucketName := "testbucket" + objectName := "object" + + if err = obj.MakeBucketWithLocation(bucketName, ""); err != nil { + t.Fatal(err) + } + objectContent := "12345" + objInfo, err := obj.PutObject(bucketName, objectName, + NewHashReader(bytes.NewReader([]byte(objectContent)), int64(len(objectContent)), "", ""), nil) + if err != nil { + t.Fatal(err) + } + if objInfo.Name != objectName { + t.Fatalf("Unexpected object name returned got %s, expected %s", objInfo.Name, objectName) + } + + fs := obj.(*fsObjects) + testCases := []struct { + parentIsObject bool + objectName string + }{ + // parentIsObject is true if object is available. + { + parentIsObject: true, + objectName: objectName, + }, + { + parentIsObject: false, + objectName: "", + }, + { + parentIsObject: false, + objectName: ".", + }, + // Should not cause infinite loop. + { + parentIsObject: false, + objectName: "/", + }, + { + parentIsObject: false, + objectName: "\\", + }, + // Should not cause infinite loop with double forward slash. + { + parentIsObject: false, + objectName: "//", + }, + } + for i, testCase := range testCases { + gotValue := fs.parentDirIsObject(bucketName, testCase.objectName) + if testCase.parentIsObject != gotValue { + t.Errorf("Test %d: Unexpected value returned got %t, expected %t", i+1, gotValue, testCase.parentIsObject) + } + } +} + // TestNewFS - tests initialization of all input disks // and constructs a valid `FS` object layer. func TestNewFS(t *testing.T) { diff --git a/cmd/xl-v1-common.go b/cmd/xl-v1-common.go index a3ab28ee6..098eace47 100644 --- a/cmd/xl-v1-common.go +++ b/cmd/xl-v1-common.go @@ -16,7 +16,9 @@ package cmd -import "path" +import ( + "path" +) // getLoadBalancedDisks - fetches load balanced (sufficiently randomized) disk slice. func (xl xlObjects) getLoadBalancedDisks() (disks []StorageAPI) { @@ -33,11 +35,11 @@ func (xl xlObjects) getLoadBalancedDisks() (disks []StorageAPI) { func (xl xlObjects) parentDirIsObject(bucket, parent string) bool { var isParentDirObject func(string) bool isParentDirObject = func(p string) bool { - if p == "." { + if p == "." || p == "/" { return false } if xl.isObject(bucket, p) { - // If there is already a file at prefix "p" return error. + // If there is already a file at prefix "p", return true. return true } // Check if there is a file as one of the parent paths. diff --git a/cmd/xl-v1-common_test.go b/cmd/xl-v1-common_test.go new file mode 100644 index 000000000..a5fa0e06d --- /dev/null +++ b/cmd/xl-v1-common_test.go @@ -0,0 +1,99 @@ +/* + * Minio Cloud Storage, (C) 2017 Minio, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package cmd + +import ( + "bytes" + "os" + "testing" +) + +// Tests for if parent directory is object +func TestXLParentDirIsObject(t *testing.T) { + rootPath, err := newTestConfig(globalMinioDefaultRegion) + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(rootPath) + + obj, fsDisks, err := prepareXL() + if err != nil { + t.Fatalf("Unable to initialize 'XL' object layer.") + } + + // Remove all disks. + for _, disk := range fsDisks { + defer os.RemoveAll(disk) + } + + bucketName := "testbucket" + objectName := "object" + + if err = obj.MakeBucketWithLocation(bucketName, ""); err != nil { + t.Fatal(err) + } + objectContent := "12345" + objInfo, err := obj.PutObject(bucketName, objectName, + NewHashReader(bytes.NewReader([]byte(objectContent)), int64(len(objectContent)), "", ""), nil) + if err != nil { + t.Fatal(err) + } + if objInfo.Name != objectName { + t.Fatalf("Unexpected object name returned got %s, expected %s", objInfo.Name, objectName) + } + + fs := obj.(*xlObjects) + testCases := []struct { + parentIsObject bool + objectName string + }{ + // parentIsObject is true if object is available. + { + parentIsObject: true, + objectName: objectName, + }, + { + parentIsObject: false, + objectName: "", + }, + { + parentIsObject: false, + objectName: ".", + }, + // Should not cause infinite loop. + { + parentIsObject: false, + objectName: "/", + }, + { + parentIsObject: false, + objectName: "\\", + }, + // Should not cause infinite loop with double forward slash. + { + parentIsObject: false, + objectName: "//", + }, + } + + for i, testCase := range testCases { + gotValue := fs.parentDirIsObject(bucketName, testCase.objectName) + if testCase.parentIsObject != gotValue { + t.Errorf("Test %d: Unexpected value returned got %t, expected %t", i+1, gotValue, testCase.parentIsObject) + } + } +}