Skip to content

Commit

Permalink
fix vulnerability described in GHSA-vhxv-fg4m-p2w8
Browse files Browse the repository at this point in the history 
As a result of a bug in the construction of radix trees, some CORS middleware
(more specifically those created by specifying two or more origin patterns
whose hosts share a proper suffix) incorrectly allow some untrusted origins,
thereby opening the door to cross-origin attacks from the untrusted origins in
question.

For example, specifying origin patterns "https://foo.com" and
"https://bar.com" (in that order) would yield a middleware that would
incorrectly allow untrusted origin "https://barfoo.com".

See GHSA-vhxv-fg4m-p2w8.
  • Loading branch information
@jub0bs
jub0bs committed May 2, 2024
1 parent a364f2f commit 63900fa
Show file tree
Hide file tree
Showing 3 changed files with 232 additions and 39 deletions.
88 changes: 49 additions & 39 deletions internal/origins/radix/radix.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,73 +23,83 @@ func (t *Tree) Insert(keyPattern string, v int) {
hasLeadingAsterisk = true
keyPattern = rest
}
var parent *node
n := &t.root
// The key pattern is processed from right to left.
search := keyPattern
s := keyPattern
for {
label, ok := lastByte(search)
label, ok := lastByte(s)
if !ok {
n.add(v, hasLeadingAsterisk)
return
}
if n.wSet.Contains(v) {
return
}
parent = n
n = n.edges[label]
if n == nil { // no matching edge found; create one
child := &node{suf: search}
child := n.edges[label]
if child == nil { // No matching edge found; create one.
child = &node{suf: s}
child.add(v, hasLeadingAsterisk)
parent.insertEdge(label, child)
n.insertEdge(label, child)
return
}

// matching edge found
searchPrefix, prefixOfNSuf, suf := splitAtCommonSuffix(search, n.suf)
if len(suf) == len(n.suf) { // n.suf is a suffix of search
search = searchPrefix
sPrefix, prefixOfChildSuf, suf := splitAtCommonSuffix(s, child.suf)
if len(prefixOfChildSuf) == 0 { // child.suf is a suffix of s
s = sPrefix
n = child
continue
}

// n.suf is NOT a suffix of search; split the node
child := &node{suf: suf}
parent.insertEdge(label, child)

// Restore the existing node.
// Because n.suf is not a suffix of search,
// prefixOfNSuf is necessarily non-empty.
byteBeforeSuffix, _ := lastByte(prefixOfNSuf)
child.insertEdge(byteBeforeSuffix, n)
if len(suf) == len(search) { // search is a suffix of n.suf
n.suf = prefixOfNSuf
// child.suf is NOT a suffix of s; we need to split child.
//
// Before splitting:
//
// child
//
// After splitting:
//
// child' -- grandChild1
//
// or perhaps
//
// child' -- grandChild1
// \
// grandChild2

// Create the first grandchild on the basis of the current child.
grandChild1 := child
grandChild1.suf = prefixOfChildSuf

// Replace child in n.
child = &node{suf: suf}
n.insertEdge(label, child)

// Add the first grandchild in child.
label, _ = lastByte(prefixOfChildSuf)
child.insertEdge(label, grandChild1)
if len(sPrefix) == 0 {
child.add(v, hasLeadingAsterisk)
return
}

// search is NOT a suffix of n.suf
n.suf = prefixOfNSuf
// Because search is NOT a suffix of n.suf,
// searchPrefix is necessarily non-empty.
label, _ = lastByte(searchPrefix)
search = searchPrefix
grandChild := &node{suf: search}
grandChild.add(v, hasLeadingAsterisk)
child.insertEdge(label, grandChild)
// Add a second grandchild in child.
label, _ = lastByte(sPrefix)
grandChild2 := &node{suf: sPrefix}
grandChild2.add(v, hasLeadingAsterisk)
child.insertEdge(label, grandChild2)
}
}

// Contains reports whether t contains key-value pair (k,v).
func (t *Tree) Contains(k string, v int) bool {
n := &t.root
search := k
for {
label, ok := lastByte(search)
label, ok := lastByte(k)
if !ok {
return n.set.Contains(v) || n.set.Contains(WildcardElem)
}

// search is not empty; check wildcard edge
// k is not empty; check wildcard edge
if n.wSet.Contains(v) || n.wSet.Contains(WildcardElem) {
return true
}
Expand All @@ -100,12 +110,12 @@ func (t *Tree) Contains(k string, v int) bool {
return false
}

searchPrefix, _, suf := splitAtCommonSuffix(search, n.suf)
if len(suf) != len(n.suf) { // n.suf is NOT a suffix of search
kPrefix, _, suf := splitAtCommonSuffix(k, n.suf)
if len(suf) != len(n.suf) { // n.suf is NOT a suffix of k
return false
}
// n.suf is a suffix of search
search = searchPrefix
// n.suf is a suffix of k
k = kPrefix
}
}

Expand Down
17 changes: 17 additions & 0 deletions internal/origins/radix/radix_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ func TestRadix(t *testing.T) {
{"string_concat", 0},
{"akin", 0},
{"bespin", 0},
// regression tests for GHSA-vhxv-fg4m-p2w8
{"pkin", 0},
{"kpin", 0},
},
}, {
desc: "duplicate patterns",
Expand Down Expand Up @@ -119,6 +122,9 @@ func TestRadix(t *testing.T) {
{"string_concat", 0},
{"akin", 0},
{"bespin", 0},
// regression tests for GHSA-vhxv-fg4m-p2w8
{"pkin", 0},
{"kpin", 0},
},
}, {
desc: "wildcard-free patterns with multiple values",
Expand Down Expand Up @@ -176,6 +182,11 @@ func TestRadix(t *testing.T) {
{"string_concat", 1},
{"akin", 1},
{"bespin", 1},
// regression tests for GHSA-vhxv-fg4m-p2w8
{"pkin", 0},
{"kpin", 0},
{"pkin", 1},
{"kpin", 1},
},
}, {
desc: "wildcard-free patterns in reverse insertion order",
Expand Down Expand Up @@ -213,6 +224,9 @@ func TestRadix(t *testing.T) {
{"string_concat", 0},
{"akin", 0},
{"bespin", 0},
// regression tests for GHSA-vhxv-fg4m-p2w8
{"kpin", 0},
{"pkin", 0},
},
}, {
desc: "some wildcard-full patterns",
Expand Down Expand Up @@ -312,6 +326,9 @@ func TestRadix(t *testing.T) {
{"string_concat", 0},
{"akin", 0},
{"bespin", 0},
// regression tests for GHSA-vhxv-fg4m-p2w8
{"pkin", 0},
{"kpin", 0},
},
}, {
desc: "some wildcard-full patterns and wildcard value",
Expand Down
166 changes: 166 additions & 0 deletions middleware_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1089,6 +1089,172 @@ func TestMiddleware(t *testing.T) {
},
},
},
}, {
desc: "regression tests for GHSA-vhxv-fg4m-p2w8",
newHandler: newSpyHandler(200, Headers{headerVary: "foo"}, "bar"),
cfg: &cors.Config{
Origins: []string{
"https://foo.com",
"https://bar.com",
},
},
cases: []ReqTestCase{
{
desc: "actual GET from disallowed",
reqMethod: "GET",
reqHeaders: Headers{
headerOrigin: "https://barfoo.com",
},
respHeaders: Headers{
headerVary: headerOrigin,
},
}, {
desc: "actual GET from disallowed 2",
reqMethod: "GET",
reqHeaders: Headers{
headerOrigin: "https://foobar.com",
},
respHeaders: Headers{
headerVary: headerOrigin,
},
}, {
desc: "actual OPTIONS from disallowed",
reqMethod: "OPTIONS",
reqHeaders: Headers{
headerOrigin: "https://barfoo.com",
},
respHeaders: Headers{
headerVary: varyPreflightValue,
},
}, {
desc: "actual OPTIONS from disallowed 2",
reqMethod: "OPTIONS",
reqHeaders: Headers{
headerOrigin: "https://foobar.com",
},
respHeaders: Headers{
headerVary: varyPreflightValue,
},
}, {
desc: "preflight with GET from disallowed",
reqMethod: "OPTIONS",
reqHeaders: Headers{
headerOrigin: "https://barfoo.com",
headerACRM: "GET",
},
preflight: true,
respHeaders: Headers{
headerVary: varyPreflightValue,
},
}, {
desc: "preflight with GET from disallowed 2",
reqMethod: "OPTIONS",
reqHeaders: Headers{
headerOrigin: "https://foobar.com",
headerACRM: "GET",
},
preflight: true,
respHeaders: Headers{
headerVary: varyPreflightValue,
},
}, {
desc: "preflight with PUT from disallowed",
reqMethod: "OPTIONS",
reqHeaders: Headers{
headerOrigin: "https://barfoo.com",
headerACRM: "PUT",
},
preflight: true,
respHeaders: Headers{
headerVary: varyPreflightValue,
},
}, {
desc: "preflight with PUT from disallowed 2",
reqMethod: "OPTIONS",
reqHeaders: Headers{
headerOrigin: "https://foobar.com",
headerACRM: "PUT",
},
preflight: true,
respHeaders: Headers{
headerVary: varyPreflightValue,
},
}, {
desc: "preflight with GET and headers from disallowed",
reqMethod: "OPTIONS",
reqHeaders: Headers{
headerOrigin: "https://barfoo.com",
headerACRM: "GET",
headerACRH: "bar,baz,foo",
},
preflight: true,
respHeaders: Headers{
headerVary: varyPreflightValue,
},
}, {
desc: "preflight with GET and headers from disallowed 2",
reqMethod: "OPTIONS",
reqHeaders: Headers{
headerOrigin: "https://foobar.com",
headerACRM: "GET",
headerACRH: "bar,baz,foo",
},
preflight: true,
respHeaders: Headers{
headerVary: varyPreflightValue,
},
}, {
desc: "preflight with GET and ACRPN from disallowed",
reqMethod: "OPTIONS",
reqHeaders: Headers{
headerOrigin: "https://barfoo.com",
headerACRPN: "true",
headerACRM: "GET",
},
preflight: true,
respHeaders: Headers{
headerVary: varyPreflightValue,
},
}, {
desc: "preflight with GET and ACRPN from disallowed 2",
reqMethod: "OPTIONS",
reqHeaders: Headers{
headerOrigin: "https://foobar.com",
headerACRPN: "true",
headerACRM: "GET",
},
preflight: true,
respHeaders: Headers{
headerVary: varyPreflightValue,
},
}, {
desc: "preflight with PUT and ACRPN and headers from disallowed",
reqMethod: "OPTIONS",
reqHeaders: Headers{
headerOrigin: "https://barfoo.com",
headerACRPN: "true",
headerACRM: "PUT",
headerACRH: "bar,baz,foo",
},
preflight: true,
respHeaders: Headers{
headerVary: varyPreflightValue,
},
}, {
desc: "preflight with PUT and ACRPN and headers from disallowed 2",
reqMethod: "OPTIONS",
reqHeaders: Headers{
headerOrigin: "https://foobar.com",
headerACRPN: "true",
headerACRM: "PUT",
headerACRH: "bar,baz,foo",
},
preflight: true,
respHeaders: Headers{
headerVary: varyPreflightValue,
},
},
},
},
}
for _, mwtc := range cases {
Expand Down

0 comments on commit 63900fa

Please sign in to comment.