diff --git a/ql/src/experimental/CWE-840/ConditionalBypass.qhelp b/ql/src/experimental/CWE-840/ConditionalBypass.qhelp index 5151c402cef..3a1e5503de2 100644 --- a/ql/src/experimental/CWE-840/ConditionalBypass.qhelp +++ b/ql/src/experimental/CWE-840/ConditionalBypass.qhelp @@ -2,26 +2,25 @@

-Testing untrusted user input against untrusted user input results in -a bypass of the conditional check as the attacker may modify parameters to match. -

+Conditional checks that compare two values that are both controlled by an untrusted user against +each other are easy to bypass and should not be used in security-critical contexts. +

-To guard against this, it is advisable to avoid framing a comparison -where both sides are untrusted user inputs. -Instead, use a configuration to store and access the values required. -

+To guard against bypass, it is advisable to avoid framing a comparison where both sides are +untrusted user inputs. Instead, use a configuration to store and access the values required. +

-The following example shows a comparison where both the sides -are from attacker-controlled request headers. This should be avoided.: -

+The following example shows a comparison where both the sides are from attacker-controlled request +headers. This should be avoided: +

One way to remedy the problem is to test against a value stored in a configuration: -

+

\ No newline at end of file diff --git a/ql/src/experimental/CWE-840/ConditionalBypassBad.go b/ql/src/experimental/CWE-840/ConditionalBypassBad.go index 3174d8f8012..b788dee2009 100644 --- a/ql/src/experimental/CWE-840/ConditionalBypassBad.go +++ b/ql/src/experimental/CWE-840/ConditionalBypassBad.go @@ -4,8 +4,8 @@ import ( "net/http" ) -func ex1(w http.ResponseWriter, r *http.Request) { - // bad the origin and the host headers are user controlled +func exampleHandlerBad(w http.ResponseWriter, r *http.Request) { + // BAD: the Origin and Host headers are user controlled if r.Header.Get("Origin") != "http://"+r.Host { //do something } diff --git a/ql/src/experimental/CWE-840/ConditionalBypassGood.go b/ql/src/experimental/CWE-840/ConditionalBypassGood.go index 987f97a9cd0..635d16d1f8f 100644 --- a/ql/src/experimental/CWE-840/ConditionalBypassGood.go +++ b/ql/src/experimental/CWE-840/ConditionalBypassGood.go @@ -4,7 +4,8 @@ import ( "net/http" ) -func ex1(w http.ResponseWriter, r *http.Request) { +func exampleHandlerGood(w http.ResponseWriter, r *http.Request) { + // GOOD: the configuration is not user controlled if r.Header.Get("Origin") != config.get("Host") { //do something } diff --git a/ql/test/experimental/CWE-840/ConditionalBypass.expected b/ql/test/experimental/CWE-840/ConditionalBypass.expected index 931da422399..c57a25bddff 100644 --- a/ql/test/experimental/CWE-840/ConditionalBypass.expected +++ b/ql/test/experimental/CWE-840/ConditionalBypass.expected @@ -1,2 +1,3 @@ +| ConditionalBypassBad.go:9:5:9:46 | ...!=... | This comparison compares user-controlled values from $@ and $@, and hence can be bypassed. | ConditionalBypassBad.go:9:5:9:12 | selection of Header : Header | here | ConditionalBypassBad.go:9:41:9:46 | selection of Host : string | here | | condition.go:9:5:9:46 | ...!=... | This comparison compares user-controlled values from $@ and $@, and hence can be bypassed. | condition.go:9:5:9:12 | selection of Header : Header | here | condition.go:9:41:9:46 | selection of Host : string | here | | condition.go:16:5:16:62 | ...!=... | This comparison compares user-controlled values from $@ and $@, and hence can be bypassed. | condition.go:16:5:16:12 | selection of Header : Header | here | condition.go:16:41:16:48 | selection of Header : Header | here | diff --git a/ql/test/experimental/CWE-840/ConditionalBypassBad.go b/ql/test/experimental/CWE-840/ConditionalBypassBad.go new file mode 100644 index 00000000000..b788dee2009 --- /dev/null +++ b/ql/test/experimental/CWE-840/ConditionalBypassBad.go @@ -0,0 +1,12 @@ +package main + +import ( + "net/http" +) + +func exampleHandlerBad(w http.ResponseWriter, r *http.Request) { + // BAD: the Origin and Host headers are user controlled + if r.Header.Get("Origin") != "http://"+r.Host { + //do something + } +} diff --git a/ql/test/experimental/CWE-840/ConditionalBypassGood.go b/ql/test/experimental/CWE-840/ConditionalBypassGood.go new file mode 100644 index 00000000000..635d16d1f8f --- /dev/null +++ b/ql/test/experimental/CWE-840/ConditionalBypassGood.go @@ -0,0 +1,12 @@ +package main + +import ( + "net/http" +) + +func exampleHandlerGood(w http.ResponseWriter, r *http.Request) { + // GOOD: the configuration is not user controlled + if r.Header.Get("Origin") != config.get("Host") { + //do something + } +} diff --git a/ql/test/experimental/CWE-840/condition.go b/ql/test/experimental/CWE-840/condition.go index b61d72b94e1..7b7b7480c10 100644 --- a/ql/test/experimental/CWE-840/condition.go +++ b/ql/test/experimental/CWE-840/condition.go @@ -4,21 +4,21 @@ import ( "net/http" ) -// bad : taken from https://www.gorillatoolkit.org/pkg/websocket +// BAD: taken from https://www.gorillatoolkit.org/pkg/websocket func ex1(w http.ResponseWriter, r *http.Request) { if r.Header.Get("Origin") != "http://"+r.Host { //do something } } -// bad both are from remote sources +// BAD: both operands are from remote sources func ex2(w http.ResponseWriter, r *http.Request) { if r.Header.Get("Origin") != "http://"+r.Header.Get("Header") { //do something } } -// good +// GOOD func ex3(w http.ResponseWriter, r *http.Request) { if r.Header.Get("Origin") != "http://"+"test" { //do something diff --git a/ql/test/experimental/CWE-840/util.go b/ql/test/experimental/CWE-840/util.go new file mode 100644 index 00000000000..9e7a9a27f20 --- /dev/null +++ b/ql/test/experimental/CWE-840/util.go @@ -0,0 +1,9 @@ +package main + +type Config struct{} + +func (_ Config) get(s string) string { + return "" +} + +var config = Config{}