From ba5805e60a19ae039db06fa842ac7e76dfea3eb2 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Thu, 5 May 2016 19:58:48 -0700 Subject: [PATCH] bucketPolicy: Do not use regexes, just do prefix matches. (#1497) AWS arn supports wildcards and this is flat namespace, simple prefix matching is fine. Fixes #1481 Fixes #1482 --- bucket-policy-handlers.go | 54 ++++++++++++++++++------------------ bucket-policy-parser.go | 16 ++++++++--- bucket-policy-parser_test.go | 30 +++++++++++++++----- 3 files changed, 62 insertions(+), 38 deletions(-) diff --git a/bucket-policy-handlers.go b/bucket-policy-handlers.go index 6827dd66d..9565e6fed 100644 --- a/bucket-policy-handlers.go +++ b/bucket-policy-handlers.go @@ -75,41 +75,41 @@ func bucketPolicyActionMatch(action string, statement policyStatement) bool { return false } -// Verify if given resource matches with policy statement. -func bucketPolicyResourceMatch(resource string, statement policyStatement) bool { - - match := func(pattern, subj string) bool { - if pattern == "" { - return subj == pattern - } - if pattern == "*" { - return true - } - parts := strings.Split(pattern, "*") - if len(parts) == 1 { - return subj == pattern - } - tGlob := strings.HasSuffix(pattern, "*") - end := len(parts) - 1 - if !strings.HasPrefix(subj, parts[0]) { +// Match function matches wild cards in 'pattern' for resource. +func resourceMatch(pattern, resource string) bool { + if pattern == "" { + return resource == pattern + } + if pattern == "*" { + return true + } + parts := strings.Split(pattern, "*") + if len(parts) == 1 { + return resource == pattern + } + tGlob := strings.HasSuffix(pattern, "*") + end := len(parts) - 1 + if !strings.HasPrefix(resource, parts[0]) { + return false + } + for i := 1; i < end; i++ { + if !strings.Contains(resource, parts[i]) { return false } - for i := 1; i < end; i++ { - if !strings.Contains(subj, parts[i]) { - return false - } - idx := strings.Index(subj, parts[i]) + len(parts[i]) - subj = subj[idx:] - } - return tGlob || strings.HasSuffix(subj, parts[end]) + idx := strings.Index(resource, parts[i]) + len(parts[i]) + resource = resource[idx:] } + return tGlob || strings.HasSuffix(resource, parts[end]) +} - for _, presource := range statement.Resources { +// Verify if given resource matches with policy statement. +func bucketPolicyResourceMatch(resource string, statement policyStatement) bool { + for _, resourcep := range statement.Resources { // the resource rule for object could contain "*" wild card. // the requested object can be given access based on the already set bucket policy if // the match is successful. // More info: http://docs.aws.amazon.com/AmazonS3/latest/dev/s3-arn-format.html . - if matched := match(presource, resource); !matched { + if matched := resourceMatch(resourcep, resource); !matched { return false } } diff --git a/bucket-policy-parser.go b/bucket-policy-parser.go index aa82bb7cf..ebf52dc6f 100644 --- a/bucket-policy-parser.go +++ b/bucket-policy-parser.go @@ -22,7 +22,7 @@ import ( "encoding/json" "errors" "fmt" - "regexp" + "path" "sort" "strings" ) @@ -200,6 +200,14 @@ var invalidPrefixActions = map[string]struct{}{ // Add actions which do not honor prefixes. } +// resourcePrefix - provides the prefix removing any wildcards. +func resourcePrefix(resource string) string { + if strings.HasSuffix(resource, "*") { + resource = strings.TrimSuffix(resource, "*") + } + return path.Clean(resource) +} + // checkBucketPolicyResources validates Resources in unmarshalled bucket policy structure. // First valation of Resources done for given set of Actions. // Later its validated for recursive Resources. @@ -232,7 +240,7 @@ func checkBucketPolicyResources(bucket string, bucketPolicy BucketPolicy) APIErr var resources []string for resource := range resourceMap { - resources = append(resources, resource) + resources = append(resources, resourcePrefix(resource)) } // Sort strings as shorter first. @@ -241,12 +249,12 @@ func checkBucketPolicyResources(bucket string, bucketPolicy BucketPolicy) APIErr for len(resources) > 1 { var resource string resource, resources = resources[0], resources[1:] - resourceRegex := regexp.MustCompile(resource) // Loop through all resources, if one of them matches with // previous shorter one, it means we have detected // nesting. Reject such rules. for _, otherResource := range resources { - if resourceRegex.MatchString(otherResource) { + // Common prefix reject such rules. + if strings.HasPrefix(otherResource, resource) { return ErrMalformedPolicy } } diff --git a/bucket-policy-parser_test.go b/bucket-policy-parser_test.go index e4ca8b96b..55602e1c1 100644 --- a/bucket-policy-parser_test.go +++ b/bucket-policy-parser_test.go @@ -453,31 +453,42 @@ func TestCheckBucketPolicyResources(t *testing.T) { return statements } + // constructing policy statement with lexically close characters. + // should not result in ErrMalformedPolicy + setResourceLexical := func(statements []policyStatement) []policyStatement { + statements[0].Resources = []string{"arn:aws:s3:::minio-bucket/op*", "arn:aws:s3:::minio-bucket/oo*"} + return statements + } + // List of BucketPolicy used for tests. bucketAccessPolicies := []BucketPolicy{ - // BucketPolicy - 0. + // BucketPolicy - 1. // Contains valid read only policy statement. {Version: "1.0", Statements: setReadOnlyStatement("minio-bucket", "")}, - // BucketPolicy - 1. + // BucketPolicy - 2. // Contains valid read-write only policy statement. {Version: "1.0", Statements: setReadWriteStatement("minio-bucket", "Asia/")}, - // BucketPolicy - 2. + // BucketPolicy - 3. // Contains valid write only policy statement. {Version: "1.0", Statements: setWriteOnlyStatement("minio-bucket", "Asia/India/")}, - // BucketPolicy - 3. + // BucketPolicy - 4. // Contains invalidPrefixActions. // Since resourcePrefix is not to the bucket-name, it return ErrMalformedPolicy. {Version: "1.0", Statements: setReadOnlyStatement("minio-bucket-fail", "Asia/India/")}, - // BucketPolicy - 4. + // BucketPolicy - 5. // constructing policy statement without invalidPrefixActions (check bucket-policy-parser.go). // but bucket part of the resource is not equal to the bucket name. // this results in return of ErrMalformedPolicy. {Version: "1.0", Statements: setValidPrefixActions(setWriteOnlyStatement("minio-bucket-fail", "Asia/India/"))}, - // BucketPolicy - 5. - // constructing policy statement without invalidPrefixActions (check bucket-policy-parser.go). + // BucketPolicy - 6. // contructing policy statement with recursive resources. // should result in ErrMalformedPolicy {Version: "1.0", Statements: setRecurseResource(setValidPrefixActions(setWriteOnlyStatement("minio-bucket", "")))}, + // BucketPolciy - 7. + // constructing policy statment with non recursive but + // lexically close resources. + // should result in ErrNone. + {Version: "1.0", Statements: setResourceLexical(setValidPrefixActions(setWriteOnlyStatement("minio-bucket", "oo")))}, } testCases := []struct { @@ -505,6 +516,11 @@ func TestCheckBucketPolicyResources(t *testing.T) { // contructing policy statement with recursive resources. // should result in ErrMalformedPolicy. {bucketAccessPolicies[5], ErrMalformedPolicy, false}, + // Test case - 7. + // constructing policy statement with lexically close + // characters. + // should result in ErrNone. + {bucketAccessPolicies[6], ErrNone, true}, } for i, testCase := range testCases { apiErrCode := checkBucketPolicyResources("minio-bucket", testCase.inputPolicy)