From 88714e7c8ee601db5d68c545381373a2e72ffc0e Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Tue, 15 Mar 2016 10:38:04 -0700 Subject: [PATCH] bucketpolicy: Improve bucket policy validation, avoid nested rules. Bucket policy validation is more stricter now, to avoid nested rules. The reason to do this is keep the rules simpler and more meaningful avoiding conflicts. This patch implements stricter checks. Example policy to be generally avoided. ``` { "Version": "2012-10-17", "Statement": [ { "Action": [ "s3:GetObject", "s3:DeleteObject" ], "Effect": "Allow", "Principal": { "AWS": [ "*" ] }, "Resource": [ "arn:aws:s3:::jarjarbing/*" ] }, { "Action": [ "s3:GetObject", "s3:DeleteObject" ], "Effect": "Deny", "Principal": { "AWS": [ "*" ] }, "Resource": [ "arn:aws:s3:::jarjarbing/restic/key/*" ] } ] } ``` --- pkg/s3/access/README.md => Bucket-Policy.md | 8 +- bucket-handlers.go | 19 +--- bucket-policy-handlers.go | 40 ++++--- .../policy.go => bucket-policy-parser.go | 102 +++++++++++++++--- bucket-policy.go | 5 + server_fs_test.go | 70 ++++++++++++ 6 files changed, 187 insertions(+), 57 deletions(-) rename pkg/s3/access/README.md => Bucket-Policy.md (91%) rename pkg/s3/access/policy.go => bucket-policy-parser.go (68%) diff --git a/pkg/s3/access/README.md b/Bucket-Policy.md similarity index 91% rename from pkg/s3/access/README.md rename to Bucket-Policy.md index b1ff17075..b642de294 100644 --- a/pkg/s3/access/README.md +++ b/Bucket-Policy.md @@ -9,14 +9,10 @@ This package implements parsing and validating bucket access policies based on A ### Supports following set of operations. - * - s3:* s3:GetObject s3:ListBucket s3:PutObject - s3:CreateBucket s3:GetBucketLocation - s3:DeleteBucket s3:DeleteObject s3:AbortMultipartUpload s3:ListBucketMultipartUploads @@ -31,3 +27,7 @@ Supported applicable condition keys for each conditions. s3:prefix s3:max-keys + +### Nested policy support. + +Nested policies are not allowed. diff --git a/bucket-handlers.go b/bucket-handlers.go index 3d2c6d4e1..210c6f60c 100644 --- a/bucket-handlers.go +++ b/bucket-handlers.go @@ -33,7 +33,6 @@ import ( "github.com/minio/minio/pkg/crypto/sha256" "github.com/minio/minio/pkg/fs" "github.com/minio/minio/pkg/probe" - "github.com/minio/minio/pkg/s3/access" "github.com/minio/minio/pkg/s3/signature4" ) @@ -54,14 +53,14 @@ func enforceBucketPolicy(action string, bucket string, reqURL *url.URL) (s3Error } } // Parse the saved policy. - accessPolicy, e := accesspolicy.Validate(policy) + bucketPolicy, e := parseBucketPolicy(policy) if e != nil { errorIf(probe.NewError(e), "Parse policy failed.", nil) return ErrAccessDenied } // Construct resource in 'arn:aws:s3:::examplebucket' format. - resource := accesspolicy.AWSResourcePrefix + strings.TrimPrefix(reqURL.Path, "/") + resource := AWSResourcePrefix + strings.TrimPrefix(reqURL.Path, "/") // Get conditions for policy verification. conditions := make(map[string]string) @@ -70,7 +69,7 @@ func enforceBucketPolicy(action string, bucket string, reqURL *url.URL) (s3Error } // Validate action, resource and conditions with current policy statements. - if !bucketPolicyEvalStatements(action, resource, conditions, accessPolicy.Statements) { + if !bucketPolicyEvalStatements(action, resource, conditions, bucketPolicy.Statements) { return ErrAccessDenied } return ErrNone @@ -436,12 +435,6 @@ func (api storageAPI) PutBucketHandler(w http.ResponseWriter, r *http.Request) { // For all unknown auth types return error. writeErrorResponse(w, r, ErrAccessDenied, r.URL.Path) return - case authTypeAnonymous: - // http://docs.aws.amazon.com/AmazonS3/latest/dev/mpuAndPermissions.html - if s3Error := enforceBucketPolicy("s3:CreateBucket", bucket, r.URL); s3Error != ErrNone { - writeErrorResponse(w, r, s3Error, r.URL.Path) - return - } case authTypePresigned: ok, err := auth.DoesPresignedSignatureMatch() if err != nil { @@ -639,12 +632,6 @@ func (api storageAPI) DeleteBucketHandler(w http.ResponseWriter, r *http.Request // For all unknown auth types return error. writeErrorResponse(w, r, ErrAccessDenied, r.URL.Path) return - case authTypeAnonymous: - // http://docs.aws.amazon.com/AmazonS3/latest/dev/mpuAndPermissions.html - if s3Error := enforceBucketPolicy("s3:DeleteBucket", bucket, r.URL); s3Error != ErrNone { - writeErrorResponse(w, r, s3Error, r.URL.Path) - return - } case authTypePresigned, authTypeSigned: if s3Error := isReqAuthenticated(api.Signature, r); s3Error != ErrNone { writeErrorResponse(w, r, s3Error, r.URL.Path) diff --git a/bucket-policy-handlers.go b/bucket-policy-handlers.go index d75968831..f59244d20 100644 --- a/bucket-policy-handlers.go +++ b/bucket-policy-handlers.go @@ -29,7 +29,6 @@ import ( mux "github.com/gorilla/mux" "github.com/minio/minio/pkg/fs" "github.com/minio/minio/pkg/probe" - "github.com/minio/minio/pkg/s3/access" ) // maximum supported access policy size. @@ -37,12 +36,13 @@ const maxAccessPolicySize = 20 * 1024 * 1024 // 20KiB. // Verify if a given action is valid for the url path based on the // existing bucket access policy. -func bucketPolicyEvalStatements(action string, resource string, conditions map[string]string, statements []accesspolicy.Statement) bool { +func bucketPolicyEvalStatements(action string, resource string, conditions map[string]string, statements []policyStatement) bool { for _, statement := range statements { if bucketPolicyMatchStatement(action, resource, conditions, statement) { if statement.Effect == "Allow" { return true } + // Do not uncomment kept here for readability. // else statement.Effect == "Deny" return false } @@ -52,7 +52,7 @@ func bucketPolicyEvalStatements(action string, resource string, conditions map[s } // Verify if action, resource and conditions match input policy statement. -func bucketPolicyMatchStatement(action string, resource string, conditions map[string]string, statement accesspolicy.Statement) bool { +func bucketPolicyMatchStatement(action string, resource string, conditions map[string]string, statement policyStatement) bool { // Verify if action matches. if bucketPolicyActionMatch(action, statement) { // Verify if resource matches. @@ -67,7 +67,7 @@ func bucketPolicyMatchStatement(action string, resource string, conditions map[s } // Verify if given action matches with policy statement. -func bucketPolicyActionMatch(action string, statement accesspolicy.Statement) bool { +func bucketPolicyActionMatch(action string, statement policyStatement) bool { for _, policyAction := range statement.Actions { // Policy action can be a regex, validate the action with matching string. matched, e := regexp.MatchString(policyAction, action) @@ -80,7 +80,7 @@ func bucketPolicyActionMatch(action string, statement accesspolicy.Statement) bo } // Verify if given resource matches with policy statement. -func bucketPolicyResourceMatch(resource string, statement accesspolicy.Statement) bool { +func bucketPolicyResourceMatch(resource string, statement policyStatement) bool { for _, presource := range statement.Resources { matched, e := regexp.MatchString(presource, strings.TrimPrefix(resource, "/")) fatalIf(probe.NewError(e), "Invalid pattern, please verify the pattern string.", nil) @@ -93,7 +93,7 @@ func bucketPolicyResourceMatch(resource string, statement accesspolicy.Statement } // Verify if given condition matches with policy statement. -func bucketPolicyConditionMatch(conditions map[string]string, statement accesspolicy.Statement) bool { +func bucketPolicyConditionMatch(conditions map[string]string, statement policyStatement) bool { // Supports following conditions. // - StringEquals // - StringNotEquals @@ -152,29 +152,25 @@ func (api storageAPI) PutBucketPolicyHandler(w http.ResponseWriter, r *http.Requ // Read access policy up to maxAccessPolicySize. // http://docs.aws.amazon.com/AmazonS3/latest/dev/access-policy-language-overview.html // bucket policies are limited to 20KB in size, using a limit reader. - accessPolicyBytes, e := ioutil.ReadAll(io.LimitReader(r.Body, maxAccessPolicySize)) + bucketPolicyBuf, e := ioutil.ReadAll(io.LimitReader(r.Body, maxAccessPolicySize)) if e != nil { errorIf(probe.NewError(e).Trace(bucket), "Reading policy failed.", nil) writeErrorResponse(w, r, ErrInternalError, r.URL.Path) return } - // Parse access access. - accessPolicy, e := accesspolicy.Validate(accessPolicyBytes) + // Parse bucket policy. + bucketPolicy, e := parseBucketPolicy(bucketPolicyBuf) if e != nil { + errorIf(probe.NewError(e), "Unable to parse bucket policy.", nil) writeErrorResponse(w, r, ErrInvalidPolicyDocument, r.URL.Path) return } - // If the policy resource has different bucket name, reject it. - for _, statement := range accessPolicy.Statements { - for _, resource := range statement.Resources { - resourcePrefix := strings.SplitAfter(resource, accesspolicy.AWSResourcePrefix)[1] - if !strings.HasPrefix(resourcePrefix, bucket) { - writeErrorResponse(w, r, ErrMalformedPolicy, r.URL.Path) - return - } - } + // Parse check bucket policy. + if s3Error := checkBucketPolicy(bucket, bucketPolicy); s3Error != ErrNone { + writeErrorResponse(w, r, s3Error, r.URL.Path) + return } // Set http request for signature verification. @@ -192,10 +188,10 @@ func (api storageAPI) PutBucketPolicyHandler(w http.ResponseWriter, r *http.Requ } } else if isRequestSignatureV4(r) { sh := sha256.New() - sh.Write(accessPolicyBytes) + sh.Write(bucketPolicyBuf) ok, err := api.Signature.DoesSignatureMatch(hex.EncodeToString(sh.Sum(nil))) if err != nil { - errorIf(err.Trace(string(accessPolicyBytes)), "SaveBucketPolicy failed.", nil) + errorIf(err.Trace(string(bucketPolicyBuf)), "SaveBucketPolicy failed.", nil) writeErrorResponse(w, r, ErrSignatureDoesNotMatch, r.URL.Path) return } @@ -206,9 +202,9 @@ func (api storageAPI) PutBucketPolicyHandler(w http.ResponseWriter, r *http.Requ } // Save bucket policy. - err := writeBucketPolicy(bucket, accessPolicyBytes) + err := writeBucketPolicy(bucket, bucketPolicyBuf) if err != nil { - errorIf(err.Trace(bucket), "SaveBucketPolicy failed.", nil) + errorIf(err.Trace(bucket, string(bucketPolicyBuf)), "SaveBucketPolicy failed.", nil) switch err.ToGoError().(type) { case fs.BucketNameInvalid: writeErrorResponse(w, r, ErrInvalidBucketName, r.URL.Path) diff --git a/pkg/s3/access/policy.go b/bucket-policy-parser.go similarity index 68% rename from pkg/s3/access/policy.go rename to bucket-policy-parser.go index a659720ee..695cd97f7 100644 --- a/pkg/s3/access/policy.go +++ b/bucket-policy-parser.go @@ -14,14 +14,16 @@ * limitations under the License. */ -// Package accesspolicy implements AWS Access Policy Language parser in +// This file implements AWS Access Policy Language parser in // accordance with http://docs.aws.amazon.com/AmazonS3/latest/dev/access-policy-language-overview.html -package accesspolicy +package main import ( "encoding/json" "errors" "fmt" + "regexp" + "sort" "strings" ) @@ -32,14 +34,10 @@ const ( // supportedActionMap - lists all the actions supported by minio. var supportedActionMap = map[string]struct{}{ - "*": {}, - "s3:*": {}, "s3:GetObject": {}, "s3:ListBucket": {}, "s3:PutObject": {}, - "s3:CreateBucket": {}, "s3:GetBucketLocation": {}, - "s3:DeleteBucket": {}, "s3:DeleteObject": {}, "s3:AbortMultipartUpload": {}, "s3:ListBucketMultipartUploads": {}, @@ -47,15 +45,15 @@ var supportedActionMap = map[string]struct{}{ } // User - canonical users list. -type User struct { +type policyUser struct { AWS []string } // Statement - minio policy statement -type Statement struct { +type policyStatement struct { Sid string Effect string - Principal User + Principal policyUser `json:"Principal"` Actions []string `json:"Action"` Resources []string `json:"Resource"` Conditions map[string]map[string]string `json:"Condition"` @@ -63,8 +61,8 @@ type Statement struct { // BucketPolicy - minio policy collection type BucketPolicy struct { - Version string // date in 0000-00-00 format - Statements []Statement `json:"Statement"` + Version string // date in 0000-00-00 format + Statements []policyStatement `json:"Statement"` } // supportedEffectMap - supported effects. @@ -183,9 +181,66 @@ func isValidConditions(conditions map[string]map[string]string) (err error) { return nil } -// Validate - validate if request body is of proper JSON and in -// accordance with policy standards. -func Validate(bucketPolicyBuf []byte) (policy BucketPolicy, err error) { +// List of actions for which prefixes are not allowed. +var invalidPrefixActions = map[string]struct{}{ + "s3:GetBucketLocation": {}, + "s3:ListBucket": {}, + "s3:ListBucketMultipartUploads": {}, + // Add actions which do not honor prefixes. +} + +// checkBucketPolicy validates unmarshalled bucket policy structure. +func checkBucketPolicy(bucket string, bucketPolicy BucketPolicy) APIErrorCode { + // Validate statements for special actions and collect resources + // for others to validate nesting. + var resources []string + for _, statement := range bucketPolicy.Statements { + for _, action := range statement.Actions { + for _, resource := range statement.Resources { + resourcePrefix := strings.SplitAfter(resource, AWSResourcePrefix)[1] + if _, ok := invalidPrefixActions[action]; ok { + // Resource prefix is not equal to bucket for + // prefix invalid actions, reject them. + if resourcePrefix != bucket { + return ErrMalformedPolicy + } + } else { + // For all other actions validate if prefix begins + // with bucket, if not reject them. + if !strings.HasPrefix(resourcePrefix, bucket) { + return ErrMalformedPolicy + } + // All valid resources collect them separately to verify nesting. + resources = append(resources, resourcePrefix) + } + } + } + } + + // Sort strings as shorter first. + sort.Strings(resources) + + 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) { + return ErrMalformedPolicy + } + } + } + + // No errors found. + return ErrNone +} + +// parseBucketPolicy - parses and validates if bucket policy is of +// proper JSON and follows allowed restrictions with policy standards. +func parseBucketPolicy(bucketPolicyBuf []byte) (policy BucketPolicy, err error) { if err = json.Unmarshal(bucketPolicyBuf, &policy); err != nil { return BucketPolicy{}, err } @@ -216,7 +271,7 @@ func Validate(bucketPolicyBuf []byte) (policy BucketPolicy, err error) { if err := isValidActions(statement.Actions); err != nil { return BucketPolicy{}, err } - // Statment resources should be valid. + // Statement resources should be valid. if err := isValidResources(statement.Resources); err != nil { return BucketPolicy{}, err } @@ -225,6 +280,23 @@ func Validate(bucketPolicyBuf []byte) (policy BucketPolicy, err error) { return BucketPolicy{}, err } } + + // Separate deny and allow statements, so that we can apply deny + // statements in the beginning followed by Allow statements. + var denyStatements []policyStatement + var allowStatements []policyStatement + for _, statement := range policy.Statements { + if statement.Effect == "Deny" { + denyStatements = append(denyStatements, statement) + continue + } + // else if statement.Effect == "Allow" + allowStatements = append(allowStatements, statement) + } + + // Deny statements are enforced first once matched. + policy.Statements = append(denyStatements, allowStatements...) + // Return successfully parsed policy structure. return policy, nil } diff --git a/bucket-policy.go b/bucket-policy.go index fb995bfd9..14f8c0ae0 100644 --- a/bucket-policy.go +++ b/bucket-policy.go @@ -138,6 +138,11 @@ func writeBucketPolicy(bucket string, accessPolicyBytes []byte) *probe.Error { } } + // Create top level directory. + if e := os.MkdirAll(filepath.Dir(bucketPolicyFile), 0700); e != nil { + return probe.NewError(e) + } + // Write bucket policy. if e := ioutil.WriteFile(bucketPolicyFile, accessPolicyBytes, 0600); e != nil { return probe.NewError(e) diff --git a/server_fs_test.go b/server_fs_test.go index 3c6aff822..2ba7c8e68 100644 --- a/server_fs_test.go +++ b/server_fs_test.go @@ -279,6 +279,76 @@ func (s *MyAPIFSCacheSuite) TestAuth(c *C) { c.Assert(len(accessID), Equals, minioAccessID) } +func (s *MyAPIFSCacheSuite) TestBucketPolicy(c *C) { + // Sample bucket policy. + bucketPolicyBuf := `{ + "Version": "2012-10-17", + "Statement": [ + { + "Action": [ + "s3:GetBucketLocation", + "s3:ListBucket" + ], + "Effect": "Allow", + "Principal": { + "AWS": [ + "*" + ] + }, + "Resource": [ + "arn:aws:s3:::policybucket" + ] + }, + { + "Action": [ + "s3:GetObject" + ], + "Effect": "Allow", + "Principal": { + "AWS": [ + "*" + ] + }, + "Resource": [ + "arn:aws:s3:::policybucket/this*" + ] + } + ] +}` + + // Put a new bucket policy. + request, err := s.newRequest("PUT", testAPIFSCacheServer.URL+"/policybucket?policy", int64(len(bucketPolicyBuf)), bytes.NewReader([]byte(bucketPolicyBuf))) + c.Assert(err, IsNil) + + client := http.Client{} + response, err := client.Do(request) + c.Assert(err, IsNil) + c.Assert(response.StatusCode, Equals, http.StatusNoContent) + + // Fetch the uploaded policy. + request, err = s.newRequest("GET", testAPIFSCacheServer.URL+"/policybucket?policy", 0, nil) + c.Assert(err, IsNil) + + client = http.Client{} + response, err = client.Do(request) + c.Assert(err, IsNil) + c.Assert(response.StatusCode, Equals, http.StatusOK) + + bucketPolicyReadBuf, err := ioutil.ReadAll(response.Body) + c.Assert(err, IsNil) + // Verify if downloaded policy matches with previousy uploaded. + c.Assert(bytes.Equal([]byte(bucketPolicyBuf), bucketPolicyReadBuf), Equals, true) + + // Delete policy. + request, err = s.newRequest("DELETE", testAPIFSCacheServer.URL+"/policybucket?policy", 0, nil) + c.Assert(err, IsNil) + + client = http.Client{} + response, err = client.Do(request) + c.Assert(err, IsNil) + c.Assert(response.StatusCode, Equals, http.StatusNoContent) +} + func (s *MyAPIFSCacheSuite) TestDeleteBucket(c *C) { request, err := s.newRequest("PUT", testAPIFSCacheServer.URL+"/deletebucket", 0, nil) c.Assert(err, IsNil)