privacy: Potential JSON injection (prebid#1304)

flux-dev-team · May 28, 2020 · 864bc55 · 864bc55
1 parent 1aec0db
commit 864bc55
Show file tree

Hide file tree

Showing 4 changed files with 94 additions and 12 deletions.
diff --git a/privacy/ccpa/policy.go b/privacy/ccpa/policy.go
@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 
-	"github.com/buger/jsonparser"
 	"github.com/mxmCherry/openrtb"
 	"github.com/prebid/prebid-server/openrtb_ext"
 )
@@ -41,12 +40,22 @@ func (p Policy) Write(req *openrtb.BidRequest) error {
 	}
 
 	if req.Regs.Ext == nil {
-		req.Regs.Ext = json.RawMessage(`{"us_privacy":"` + p.Value + `"}`)
-		return nil
+		ext, err := json.Marshal(openrtb_ext.ExtRegs{USPrivacy: p.Value})
+		if err == nil {
+			req.Regs.Ext = ext
+		}
+		return err
 	}
 
-	var err error
-	req.Regs.Ext, err = jsonparser.Set(req.Regs.Ext, []byte(`"`+p.Value+`"`), "us_privacy")
+	var extMap map[string]interface{}
+	err := json.Unmarshal(req.Regs.Ext, &extMap)
+	if err == nil {
+		extMap["us_privacy"] = p.Value
+		ext, err := json.Marshal(extMap)
+		if err == nil {
+			req.Regs.Ext = ext
+		}
+	}
 	return err
 }
 

diff --git a/privacy/ccpa/policy_test.go b/privacy/ccpa/policy_test.go
@@ -71,6 +71,17 @@ func TestRead(t *testing.T) {
 			},
 			expectedError: true,
 		},
+		{
+			description: "Injection Attack",
+			request: &openrtb.BidRequest{
+				Regs: &openrtb.Regs{
+					Ext: json.RawMessage(`{"us_privacy":"1YYY\"},\"oops\":\"malicious\",\"p\":{\"p\":\""}`),
+				},
+			},
+			expectedPolicy: Policy{
+				Value: "1YYY\"},\"oops\":\"malicious\",\"p\":{\"p\":\"",
+			},
+		},
 	}
 
 	for _, test := range testCases {
@@ -138,6 +149,32 @@ func TestWrite(t *testing.T) {
 				Ext: json.RawMessage(`malformed`)}},
 			expectedError: true,
 		},
+		{
+			description: "Injection Attack With Nil Request Regs Object",
+			policy:      Policy{Value: "1YYY\"},\"oops\":\"malicious\",\"p\":{\"p\":\""},
+			request:     &openrtb.BidRequest{},
+			expected: &openrtb.BidRequest{Regs: &openrtb.Regs{
+				Ext: json.RawMessage(`{"us_privacy":"1YYY\"},\"oops\":\"malicious\",\"p\":{\"p\":\""}`),
+			}},
+		},
+		{
+			description: "Injection Attack With Nil Request Regs Ext Object",
+			policy:      Policy{Value: "1YYY\"},\"oops\":\"malicious\",\"p\":{\"p\":\""},
+			request:     &openrtb.BidRequest{Regs: &openrtb.Regs{}},
+			expected: &openrtb.BidRequest{Regs: &openrtb.Regs{
+				Ext: json.RawMessage(`{"us_privacy":"1YYY\"},\"oops\":\"malicious\",\"p\":{\"p\":\""}`),
+			}},
+		},
+		{
+			description: "Injection Attack With Existing Request Regs Ext Object",
+			policy:      Policy{Value: "1YYY\"},\"oops\":\"malicious\",\"p\":{\"p\":\""},
+			request: &openrtb.BidRequest{Regs: &openrtb.Regs{
+				Ext: json.RawMessage(`{"existing":"any"}`),
+			}},
+			expected: &openrtb.BidRequest{Regs: &openrtb.Regs{
+				Ext: json.RawMessage(`{"existing":"any","us_privacy":"1YYY\"},\"oops\":\"malicious\",\"p\":{\"p\":\""}`),
+			}},
+		},
 	}
 
 	for _, test := range testCases {

diff --git a/privacy/gdpr/policy.go b/privacy/gdpr/policy.go
@@ -2,8 +2,8 @@ package gdpr
 
 import (
 	"encoding/json"
+	"github.com/prebid/prebid-server/openrtb_ext"
 
-	"github.com/buger/jsonparser"
 	"github.com/mxmCherry/openrtb"
 	"github.com/prebid/go-gdpr/vendorconsent"
 )
@@ -25,12 +25,22 @@ func (p Policy) Write(req *openrtb.BidRequest) error {
 	}
 
 	if req.User.Ext == nil {
-		req.User.Ext = json.RawMessage(`{"consent":"` + p.Consent + `"}`)
-		return nil
+		ext, err := json.Marshal(openrtb_ext.ExtUser{Consent: p.Consent})
+		if err == nil {
+			req.User.Ext = ext
+		}
+		return err
 	}
 
-	var err error
-	req.User.Ext, err = jsonparser.Set(req.User.Ext, []byte(`"`+p.Consent+`"`), "consent")
+	var extMap map[string]interface{}
+	err := json.Unmarshal(req.User.Ext, &extMap)
+	if err == nil {
+		extMap["consent"] = p.Consent
+		ext, err := json.Marshal(extMap)
+		if err == nil {
+			req.User.Ext = ext
+		}
+	}
 	return err
 }
 

diff --git a/privacy/gdpr/policy_test.go b/privacy/gdpr/policy_test.go
@@ -42,15 +42,15 @@ func TestWrite(t *testing.T) {
 			request: &openrtb.BidRequest{User: &openrtb.User{
 				Ext: json.RawMessage(`{"existing":"any"}`)}},
 			expected: &openrtb.BidRequest{User: &openrtb.User{
-				Ext: json.RawMessage(`{"existing":"any","consent":"anyConsent"}`)}},
+				Ext: json.RawMessage(`{"consent":"anyConsent","existing":"any"}`)}},
 		},
 		{
 			description: "Enabled With Existing Request User Ext Object - Overwrites",
 			policy:      Policy{Consent: "anyConsent"},
 			request: &openrtb.BidRequest{User: &openrtb.User{
 				Ext: json.RawMessage(`{"existing":"any","consent":"toBeOverwritten"}`)}},
 			expected: &openrtb.BidRequest{User: &openrtb.User{
-				Ext: json.RawMessage(`{"existing":"any","consent":"anyConsent"}`)}},
+				Ext: json.RawMessage(`{"consent":"anyConsent","existing":"any"}`)}},
 		},
 		{
 			description: "Enabled With Existing Malformed Request User Ext Object",
@@ -59,6 +59,32 @@ func TestWrite(t *testing.T) {
 				Ext: json.RawMessage(`malformed`)}},
 			expectedError: true,
 		},
+		{
+			description: "Injection Attack With Nil Request User Object",
+			policy:      Policy{Consent: "BONV8oqONXwgmADACHENAO7pqzAAppY\"},\"oops\":\"malicious\",\"p\":{\"p\":\""},
+			request:     &openrtb.BidRequest{},
+			expected: &openrtb.BidRequest{User: &openrtb.User{
+				Ext: json.RawMessage(`{"consent":"BONV8oqONXwgmADACHENAO7pqzAAppY\"},\"oops\":\"malicious\",\"p\":{\"p\":\""}`),
+			}},
+		},
+		{
+			description: "Injection Attack With Nil Request User Ext Object",
+			policy:      Policy{Consent: "BONV8oqONXwgmADACHENAO7pqzAAppY\"},\"oops\":\"malicious\",\"p\":{\"p\":\""},
+			request:     &openrtb.BidRequest{User: &openrtb.User{}},
+			expected: &openrtb.BidRequest{User: &openrtb.User{
+				Ext: json.RawMessage(`{"consent":"BONV8oqONXwgmADACHENAO7pqzAAppY\"},\"oops\":\"malicious\",\"p\":{\"p\":\""}`),
+			}},
+		},
+		{
+			description: "Injection Attack With Existing Request User Ext Object",
+			policy:      Policy{Consent: "BONV8oqONXwgmADACHENAO7pqzAAppY\"},\"oops\":\"malicious\",\"p\":{\"p\":\""},
+			request: &openrtb.BidRequest{User: &openrtb.User{
+				Ext: json.RawMessage(`{"existing":"any"}`),
+			}},
+			expected: &openrtb.BidRequest{User: &openrtb.User{
+				Ext: json.RawMessage(`{"consent":"BONV8oqONXwgmADACHENAO7pqzAAppY\"},\"oops\":\"malicious\",\"p\":{\"p\":\"","existing":"any"}`),
+			}},
+		},
 	}
 
 	for _, test := range testCases {