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

@@ -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.

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()

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

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=