From 0f726ea0e7256f9056da372f822bd534f9d1dc5f Mon Sep 17 00:00:00 2001 From: Alistair Hey <40488132+Waterdrips@users.noreply.github.com> Date: Sat, 29 May 2021 02:45:11 +0100 Subject: [PATCH] Fix issue with MapClaims VerifyAudience []string (#12) * Fix issue with MapClaims VerifyAudience []string There was an issue in MapClaims's VerifyAudiance where a []string (which is valid in the spec) would return true (claim is found, or nil) when required was not set. It now checks interface types correctly and has tests written Signed-off-by: Alistair Hey * Keep aud validation constant time compare Keep aud validation using constant time compare by not instantly returning on a true comparison, keep comparing all options and store result in a variable Signed-off-by: Alistair Hey Co-authored-by: Banse, Christian --- .gitignore | 2 +- claims.go | 26 +++++++++++++----- map_claims.go | 18 ++++++++++-- map_claims_test.go | 68 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 104 insertions(+), 10 deletions(-) create mode 100644 map_claims_test.go diff --git a/.gitignore b/.gitignore index 80bed65..09573e0 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,4 @@ .DS_Store bin - +.idea/ diff --git a/claims.go b/claims.go index 1133fd4..f1dba3c 100644 --- a/claims.go +++ b/claims.go @@ -61,7 +61,7 @@ func (c StandardClaims) Valid() error { // Compares the aud claim against cmp. // If required is false, this method will return true if the value matches or is unset func (c *StandardClaims) VerifyAudience(cmp string, req bool) bool { - return verifyAud(c.Audience, cmp, req) + return verifyAud([]string{c.Audience}, cmp, req) } // Compares the exp claim against cmp. @@ -90,15 +90,27 @@ func (c *StandardClaims) VerifyNotBefore(cmp int64, req bool) bool { // ----- helpers -func verifyAud(aud string, cmp string, required bool) bool { - if aud == "" { +func verifyAud(aud []string, cmp string, required bool) bool { + if len(aud) == 0 { return !required } - if subtle.ConstantTimeCompare([]byte(aud), []byte(cmp)) != 0 { - return true - } else { - return false + // use a var here to keep constant time compare when looping over a number of claims + result := false + + var stringClaims string + for _, a := range aud { + if subtle.ConstantTimeCompare([]byte(a), []byte(cmp)) != 0 { + result = true + } + stringClaims = stringClaims + a } + + // case where "" is sent in one or many aud claims + if len(stringClaims) == 0 { + return !required + } + + return result } func verifyExp(exp int64, now int64, required bool) bool { diff --git a/map_claims.go b/map_claims.go index e1d6f3f..ba290f4 100644 --- a/map_claims.go +++ b/map_claims.go @@ -10,10 +10,24 @@ import ( // This is the default claims type if you don't supply one type MapClaims map[string]interface{} -// Compares the aud claim against cmp. +// VerifyAudience Compares the aud claim against cmp. // If required is false, this method will return true if the value matches or is unset func (m MapClaims) VerifyAudience(cmp string, req bool) bool { - aud, _ := m["aud"].(string) + var aud []string + switch v := m["aud"].(type) { + case string: + aud = append(aud, v) + case []string: + aud = v + case []interface{}: + for _, a := range v { + vs, ok := a.(string) + if !ok { + return false + } + aud = append(aud, vs) + } + } return verifyAud(aud, cmp, req) } diff --git a/map_claims_test.go b/map_claims_test.go new file mode 100644 index 0000000..ff65913 --- /dev/null +++ b/map_claims_test.go @@ -0,0 +1,68 @@ +package jwt + +import ( + "testing" +) + +func TestVerifyAud(t *testing.T) { + var nilInterface interface{} + var nilListInterface []interface{} + var intListInterface interface{} = []int{1,2,3} + type test struct{ + Name string + MapClaims MapClaims + Expected bool + Comparison string + Required bool + } + tests := []test{ + // Matching Claim in aud + // Required = true + { Name: "String Aud matching required", MapClaims: MapClaims{"aud": "example.com"}, Expected: true , Required: true, Comparison: "example.com"}, + { Name: "[]String Aud with match required", MapClaims: MapClaims{"aud": []string{"example.com", "example.example.com"}}, Expected: true, Required: true, Comparison: "example.com"}, + + // Required = false + { Name: "String Aud with match not required", MapClaims: MapClaims{"aud": "example.com"}, Expected: true , Required: false, Comparison: "example.com"}, + { Name: "Empty String Aud with match not required", MapClaims: MapClaims{}, Expected: true , Required: false, Comparison: "example.com"}, + { Name: "Empty String Aud with match not required", MapClaims: MapClaims{"aud": ""}, Expected: true , Required: false, Comparison: "example.com"}, + { Name: "Nil String Aud with match not required", MapClaims: MapClaims{"aud": nil}, Expected: true , Required: false, Comparison: "example.com"}, + + { Name: "[]String Aud with match not required", MapClaims: MapClaims{"aud": []string{"example.com", "example.example.com"}}, Expected: true, Required: false, Comparison: "example.com"}, + { Name: "Empty []String Aud with match not required", MapClaims: MapClaims{"aud": []string{}}, Expected: true, Required: false, Comparison: "example.com"}, + + // Non-Matching Claim in aud + // Required = true + { Name: "String Aud without match required", MapClaims: MapClaims{"aud": "not.example.com"}, Expected: false, Required: true, Comparison: "example.com"}, + { Name: "Empty String Aud without match required", MapClaims: MapClaims{"aud": ""}, Expected: false, Required: true, Comparison: "example.com"}, + { Name: "[]String Aud without match required", MapClaims: MapClaims{"aud": []string{"not.example.com", "example.example.com"}}, Expected: false, Required: true, Comparison: "example.com"}, + { Name: "Empty []String Aud without match required", MapClaims: MapClaims{"aud": []string{""}}, Expected: false, Required: true, Comparison: "example.com"}, + { Name: "String Aud without match not required", MapClaims: MapClaims{"aud": "not.example.com"}, Expected: false, Required: true, Comparison: "example.com"}, + { Name: "Empty String Aud without match not required", MapClaims: MapClaims{"aud": ""}, Expected: false, Required: true, Comparison: "example.com"}, + { Name: "[]String Aud without match not required", MapClaims: MapClaims{"aud": []string{"not.example.com", "example.example.com"}}, Expected: false, Required: true, Comparison: "example.com"}, + + // Required = false + { Name: "Empty []String Aud without match required", MapClaims: MapClaims{"aud": []string{""}}, Expected: false, Required: true, Comparison: "example.com"}, + + // []interface{} + { Name: "Empty []interface{} Aud without match required", MapClaims: MapClaims{"aud": nilListInterface}, Expected: true, Required: false, Comparison: "example.com"}, + { Name: "[]interface{} Aud wit match required", MapClaims: MapClaims{"aud": []interface{}{"a", "foo", "example.com"}}, Expected: true, Required: true, Comparison: "example.com"}, + { Name: "[]interface{} Aud wit match but invalid types", MapClaims: MapClaims{"aud": []interface{}{"a", 5, "example.com"}}, Expected: false, Required: true, Comparison: "example.com"}, + { Name: "[]interface{} Aud int wit match required", MapClaims: MapClaims{"aud": intListInterface}, Expected: false, Required: true, Comparison: "example.com"}, + + + // interface{} + { Name: "Empty interface{} Aud without match not required", MapClaims: MapClaims{"aud": nilInterface}, Expected: true, Required: false, Comparison: "example.com"}, + + } + + + for _, test := range tests { + t.Run(test.Name, func(t *testing.T) { + got := test.MapClaims.VerifyAudience(test.Comparison, test.Required) + + if got != test.Expected { + t.Errorf("Expected %v, got %v", test.Expected, got) + } + }) + } +}