From 726d80dbb75f6e3631b7f4ad70c1b43c0fc9a7d5 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Sat, 20 Mar 2021 22:16:30 -0700 Subject: [PATCH] fix: merge duplicate keys in post policy (#11843) some SDKs might incorrectly send duplicate entries for keys such as "conditions", Go stdlib unmarshal for JSON does not support duplicate keys - instead skips the first duplicate and only preserves the last entry. This can lead to issues where a policy JSON while being valid might not properly apply the required conditions, allowing situations where POST policy JSON would end up allowing uploads to unauthorized buckets and paths. This PR fixes this properly. --- cmd/postpolicyform.go | 38 +++++++++++++++++++++++++++++----- cmd/postpolicyform_test.go | 42 ++++++++++++++++++++++++++++++++++++++ go.mod | 2 +- go.sum | 4 ++-- 4 files changed, 78 insertions(+), 8 deletions(-) diff --git a/cmd/postpolicyform.go b/cmd/postpolicyform.go index 69889b4e1..6abdc7305 100644 --- a/cmd/postpolicyform.go +++ b/cmd/postpolicyform.go @@ -17,14 +17,18 @@ package cmd import ( + "bytes" "encoding/json" "errors" "fmt" + "io" "net/http" "reflect" "strconv" "strings" "time" + + "github.com/bcicen/jstream" ) // startWithConds - map which indicates if a given condition supports starts-with policy operator @@ -110,8 +114,32 @@ type PostPolicyForm struct { } } +// implemented to ensure that duplicate keys in JSON +// are merged together into a single JSON key, also +// to remove any extraneous JSON bodies. +// +// 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) { + var buf bytes.Buffer + e := json.NewEncoder(&buf) + d := jstream.NewDecoder(strings.NewReader(policy), 0) + for mv := range d.Stream() { + e.Encode(mv.Value) + } + return &buf, d.Err() +} + // parsePostPolicyForm - Parse JSON policy string into typed PostPolicyForm structure. -func parsePostPolicyForm(policy string) (ppf PostPolicyForm, e error) { +func parsePostPolicyForm(policy string) (PostPolicyForm, error) { + preader, err := sanitizePolicy(policy) + if err != nil { + return PostPolicyForm{}, err + } + + d := json.NewDecoder(preader) + // Convert po into interfaces and // perform strict type conversion using reflection. var rawPolicy struct { @@ -119,9 +147,9 @@ func parsePostPolicyForm(policy string) (ppf PostPolicyForm, e error) { Conditions []interface{} `json:"conditions"` } - err := json.Unmarshal([]byte(policy), &rawPolicy) - if err != nil { - return ppf, err + d.DisallowUnknownFields() + if err := d.Decode(&rawPolicy); err != nil { + return PostPolicyForm{}, err } parsedPolicy := PostPolicyForm{} @@ -129,7 +157,7 @@ func parsePostPolicyForm(policy string) (ppf PostPolicyForm, e error) { // Parse expiry time. parsedPolicy.Expiration, err = time.Parse(time.RFC3339Nano, rawPolicy.Expiration) if err != nil { - return ppf, err + return PostPolicyForm{}, err } // Parse conditions. diff --git a/cmd/postpolicyform_test.go b/cmd/postpolicyform_test.go index 5ac451140..c1c299248 100644 --- a/cmd/postpolicyform_test.go +++ b/cmd/postpolicyform_test.go @@ -25,6 +25,48 @@ import ( minio "github.com/minio/minio-go/v7" ) +func TestParsePostPolicyForm(t *testing.T) { + testCases := []struct { + policy string + success bool + }{ + // missing expiration, will fail. + { + 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, + }, + // invalid json. + { + 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 + { + 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, + }, + // no duplicates, shall be parsed properly. + { + policy: `{"expiration":"2021-03-27T20:35:28.458Z","conditions":[["eq","$bucket","testbucket"],["eq","$key","wtf.txt"],["eq","$x-amz-date","20210320T203528Z"],["eq","$x-amz-algorithm","AWS4-HMAC-SHA256"],["eq","$x-amz-credential","Q3AM3UQ867SPQQA43P2F/20210320/us-east-1/s3/aws4_request"]]}`, + success: true, + }, + } + + for _, testCase := range testCases { + testCase := testCase + t.Run("", func(t *testing.T) { + _, err := parsePostPolicyForm(testCase.policy) + fmt.Println(err) + if testCase.success && err != nil { + t.Errorf("Expected success but failed with %s", err) + } + if !testCase.success && err == nil { + t.Errorf("Expected failed but succeeded") + } + }) + } +} + // Test Post Policy parsing and checking conditions func TestPostPolicyForm(t *testing.T) { pp := minio.NewPostPolicy() diff --git a/go.mod b/go.mod index 8d313eb7e..3490df1d2 100644 --- a/go.mod +++ b/go.mod @@ -74,7 +74,7 @@ require ( github.com/shirou/gopsutil/v3 v3.21.1 github.com/spaolacci/murmur3 v1.1.0 // indirect github.com/streadway/amqp v1.0.0 - github.com/tidwall/gjson v1.6.7 + github.com/tidwall/gjson v1.6.8 github.com/tidwall/sjson v1.0.4 github.com/tinylib/msgp v1.1.3 github.com/valyala/tcplisten v0.0.0-20161114210144-ceec8f93295a diff --git a/go.sum b/go.sum index 7edc8448c..f7a4d9661 100644 --- a/go.sum +++ b/go.sum @@ -584,8 +584,8 @@ github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81P github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA= github.com/stretchr/testify v1.6.1 h1:hDPOHmpOpP40lSULcqw7IrRb/u7w6RpDC9399XyoNd0= github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= -github.com/tidwall/gjson v1.6.7 h1:Mb1M9HZCRWEcXQ8ieJo7auYyyiSux6w9XN3AdTpxJrE= -github.com/tidwall/gjson v1.6.7/go.mod h1:zeFuBCIqD4sN/gmqBzZ4j7Jd6UcA2Fc56x7QFsv+8fI= +github.com/tidwall/gjson v1.6.8 h1:CTmXMClGYPAmln7652e69B7OLXfTi5ABcPPwjIWUv7w= +github.com/tidwall/gjson v1.6.8/go.mod h1:zeFuBCIqD4sN/gmqBzZ4j7Jd6UcA2Fc56x7QFsv+8fI= github.com/tidwall/match v1.0.3 h1:FQUVvBImDutD8wJLN6c5eMzWtjgONK9MwIBCOrUJKeE= github.com/tidwall/match v1.0.3/go.mod h1:eRSPERbgtNPcGhD8UCthc6PmLEQXEWd3PRB5JTxsfmM= github.com/tidwall/pretty v1.0.2 h1:Z7S3cePv9Jwm1KwS0513MRaoUe3S01WPbLNV40pwWZU=