From d629ca0a47cb9ea616e1ff36dcba585df4e62da5 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Thu, 25 Mar 2021 13:57:57 -0700 Subject: [PATCH] fix: reject duplicate keys in PostPolicyJSON document (#11902) fixes #11894 --- cmd/bucket-handlers.go | 8 +++++--- cmd/erasure-sets.go | 2 +- cmd/postpolicyform.go | 26 ++++++++++++++++++++------ cmd/postpolicyform_test.go | 20 +++++++++++++++----- 4 files changed, 41 insertions(+), 15 deletions(-) diff --git a/cmd/bucket-handlers.go b/cmd/bucket-handlers.go index 6b23af86d..a744ac1ce 100644 --- a/cmd/bucket-handlers.go +++ b/cmd/bucket-handlers.go @@ -17,6 +17,7 @@ package cmd import ( + "bytes" "encoding/base64" "encoding/xml" "fmt" @@ -758,10 +759,11 @@ func (api objectAPIHandlers) PostPolicyBucketHandler(w http.ResponseWriter, r *h // Handle policy if it is set. if len(policyBytes) > 0 { - - postPolicyForm, err := parsePostPolicyForm(string(policyBytes)) + postPolicyForm, err := parsePostPolicyForm(bytes.NewReader(policyBytes)) if err != nil { - writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrPostPolicyConditionInvalidFormat), r.URL, guessIsBrowserReq(r)) + errAPI := errorCodes.ToAPIErr(ErrPostPolicyConditionInvalidFormat) + errAPI.Description = fmt.Sprintf("%s '(%s)'", errAPI.Description, err) + writeErrorResponse(ctx, w, errAPI, r.URL, guessIsBrowserReq(r)) return } diff --git a/cmd/erasure-sets.go b/cmd/erasure-sets.go index 6c7b428ce..816b1e8b9 100644 --- a/cmd/erasure-sets.go +++ b/cmd/erasure-sets.go @@ -425,7 +425,7 @@ func newErasureSets(ctx context.Context, endpoints Endpoints, storageDisks []Sto } // cleanup ".trash/" folder every 10 minutes with sufficient sleep cycles. - deletedObjectsCleanupInterval, err := time.ParseDuration(env.Get("MINIO_DELETE_CLEANUP_INTERVAL", "10m")) + deletedObjectsCleanupInterval, err := time.ParseDuration(env.Get("MINIO_DELETE_CLEANUP_INTERVAL", "5m")) if err != nil { return nil, err } diff --git a/cmd/postpolicyform.go b/cmd/postpolicyform.go index 6abdc7305..b011f3783 100644 --- a/cmd/postpolicyform.go +++ b/cmd/postpolicyform.go @@ -29,6 +29,7 @@ import ( "time" "github.com/bcicen/jstream" + "github.com/minio/minio-go/v7/pkg/set" ) // startWithConds - map which indicates if a given condition supports starts-with policy operator @@ -121,24 +122,37 @@ type PostPolicyForm struct { // Go stdlib doesn't support parsing JSON with duplicate // keys, so we need to use this technique to merge the // keys. -func sanitizePolicy(policy string) (io.Reader, error) { +func sanitizePolicy(r io.Reader) (io.Reader, error) { var buf bytes.Buffer e := json.NewEncoder(&buf) - d := jstream.NewDecoder(strings.NewReader(policy), 0) + d := jstream.NewDecoder(r, 0).ObjectAsKVS() + sset := set.NewStringSet() for mv := range d.Stream() { - e.Encode(mv.Value) + var kvs jstream.KVS + if mv.ValueType == jstream.Object { + // This is a JSON object type (that preserves key order) + kvs = mv.Value.(jstream.KVS) + for _, kv := range kvs { + if sset.Contains(kv.Key) { + // Reject duplicate conditions or expiration. + return nil, fmt.Errorf("input policy has multiple %s, please fix your client code", kv.Key) + } + sset.Add(kv.Key) + } + e.Encode(kvs) + } } return &buf, d.Err() } // parsePostPolicyForm - Parse JSON policy string into typed PostPolicyForm structure. -func parsePostPolicyForm(policy string) (PostPolicyForm, error) { - preader, err := sanitizePolicy(policy) +func parsePostPolicyForm(r io.Reader) (PostPolicyForm, error) { + reader, err := sanitizePolicy(r) if err != nil { return PostPolicyForm{}, err } - d := json.NewDecoder(preader) + d := json.NewDecoder(reader) // Convert po into interfaces and // perform strict type conversion using reflection. diff --git a/cmd/postpolicyform_test.go b/cmd/postpolicyform_test.go index c1c299248..ad4de4a8e 100644 --- a/cmd/postpolicyform_test.go +++ b/cmd/postpolicyform_test.go @@ -17,9 +17,11 @@ package cmd import ( + "bytes" "encoding/base64" "fmt" "net/http" + "strings" "testing" minio "github.com/minio/minio-go/v7" @@ -40,10 +42,19 @@ func TestParsePostPolicyForm(t *testing.T) { policy: `{"conditions":[["eq","$bucket","asdf"],["eq","$key","hello.txt"]],"conditions":[["eq","$success_action_status","201"],["eq","$Content-Type","plain/text"],["eq","$success_action_status","201"],["eq","$x-amz-algorithm","AWS4-HMAC-SHA256"],["eq","$x-amz-credential","Q3AM3UQ867SPQQA43P2F/20210315/us-east-1/s3/aws4_request"],["eq","$x-amz-date","20210315T091621Z"]]`, success: false, }, - // duplicate conditions, shall be parsed properly + // duplicate 'expiration' reject + { + policy: `{"expiration":"2021-03-22T09:16:21.310Z","expiration":"2021-03-22T09:16:21.310Z","conditions":[["eq","$bucket","evil"],["eq","$key","hello.txt"],["eq","$success_action_status","201"],["eq","$Content-Type","plain/text"],["eq","$success_action_status","201"],["eq","$x-amz-algorithm","AWS4-HMAC-SHA256"],["eq","$x-amz-credential","Q3AM3UQ867SPQQA43P2F/20210315/us-east-1/s3/aws4_request"],["eq","$x-amz-date","20210315T091621Z"]]}`, + }, + // duplicate '$bucket' reject + { + policy: `{"expiration":"2021-03-22T09:16:21.310Z","conditions":[["eq","$bucket","good"],["eq","$key","hello.txt"]],"conditions":[["eq","$bucket","evil"],["eq","$key","hello.txt"],["eq","$success_action_status","201"],["eq","$Content-Type","plain/text"],["eq","$success_action_status","201"],["eq","$x-amz-algorithm","AWS4-HMAC-SHA256"],["eq","$x-amz-credential","Q3AM3UQ867SPQQA43P2F/20210315/us-east-1/s3/aws4_request"],["eq","$x-amz-date","20210315T091621Z"]]}`, + success: false, + }, + // duplicate conditions, reject { policy: `{"expiration":"2021-03-22T09:16:21.310Z","conditions":[["eq","$bucket","asdf"],["eq","$key","hello.txt"]],"conditions":[["eq","$success_action_status","201"],["eq","$Content-Type","plain/text"],["eq","$success_action_status","201"],["eq","$x-amz-algorithm","AWS4-HMAC-SHA256"],["eq","$x-amz-credential","Q3AM3UQ867SPQQA43P2F/20210315/us-east-1/s3/aws4_request"],["eq","$x-amz-date","20210315T091621Z"]]}`, - success: true, + success: false, }, // no duplicates, shall be parsed properly. { @@ -55,8 +66,7 @@ func TestParsePostPolicyForm(t *testing.T) { for _, testCase := range testCases { testCase := testCase t.Run("", func(t *testing.T) { - _, err := parsePostPolicyForm(testCase.policy) - fmt.Println(err) + _, err := parsePostPolicyForm(strings.NewReader(testCase.policy)) if testCase.success && err != nil { t.Errorf("Expected success but failed with %s", err) } @@ -136,7 +146,7 @@ func TestPostPolicyForm(t *testing.T) { t.Fatal(err) } - postPolicyForm, err := parsePostPolicyForm(string(policyBytes)) + postPolicyForm, err := parsePostPolicyForm(bytes.NewReader(policyBytes)) if err != nil { t.Fatal(err) }