From 9948596e2cf542dd6e5665df95d8b4a7cc64f128 Mon Sep 17 00:00:00 2001 From: Porcupiney Hairs Date: Tue, 28 Apr 2020 23:24:28 +0530 Subject: [PATCH] User-controlled bypass of a comparision --- .../CWE-840/ConditionBypass.qhelp | 35 +++++++++++++++ .../experimental/CWE-840/ConditionBypass.ql | 44 +++++++++++++++++++ .../CWE-840/ConditionBypassBad.go | 12 +++++ .../CWE-840/ConditionBypassGood.go | 11 +++++ .../CWE-840/ConditionBypass.expected | 2 + .../CWE-840/ConditionBypass.qlref | 1 + ql/test/experimental/CWE-840/condition.go | 26 +++++++++++ 7 files changed, 131 insertions(+) create mode 100644 ql/src/experimental/CWE-840/ConditionBypass.qhelp create mode 100644 ql/src/experimental/CWE-840/ConditionBypass.ql create mode 100644 ql/src/experimental/CWE-840/ConditionBypassBad.go create mode 100644 ql/src/experimental/CWE-840/ConditionBypassGood.go create mode 100644 ql/test/experimental/CWE-840/ConditionBypass.expected create mode 100644 ql/test/experimental/CWE-840/ConditionBypass.qlref create mode 100644 ql/test/experimental/CWE-840/condition.go diff --git a/ql/src/experimental/CWE-840/ConditionBypass.qhelp b/ql/src/experimental/CWE-840/ConditionBypass.qhelp new file mode 100644 index 00000000000..42bdc95e0f3 --- /dev/null +++ b/ql/src/experimental/CWE-840/ConditionBypass.qhelp @@ -0,0 +1,35 @@ + + + +

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

+ + +

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

+ + +

+The following example shows a comparision 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: +

+ + + +
  • + MITRE: + + CWE-840. + +
    • +     +     \ No newline at end of file diff --git a/ql/src/experimental/CWE-840/ConditionBypass.ql b/ql/src/experimental/CWE-840/ConditionBypass.ql new file mode 100644 index 00000000000..8fc7fb3d249 --- /dev/null +++ b/ql/src/experimental/CWE-840/ConditionBypass.ql @@ -0,0 +1,44 @@ +/** + * @name Comparision Expression Check Bypass + * @description This query tests for user-controlled bypassing + * of a comparision expression i.e. instances where both the + * lhs and rhs of a comparision are user controlled. + * @id go/condition-bypass + * @kind problem + * @problem.severity medium + * @tags external/cwe/cwe-840 + */ + +import go + +/** + * A data-flow configuration for reasoning about Condition Bypass. + */ +class Configuration extends TaintTracking::Configuration { + Configuration() { this = "Comparision Expression Check Bypass" } + + override predicate isSource(DataFlow::Node source) { + source instanceof UntrustedFlowSource + or + exists(string fieldName | + source.(DataFlow::FieldReadNode).getField().hasQualifiedName("net/http", "Request", fieldName) + | + fieldName = "Host" + ) + } + + override predicate isSink(DataFlow::Node sink) { + exists(ComparisonExpr c | c.getAnOperand() = sink.asExpr()) + } +} + +from + Configuration config, DataFlow::PathNode lhsSource, DataFlow::PathNode lhs, + DataFlow::PathNode rhsSource, DataFlow::PathNode rhs, ComparisonExpr c +where + config.hasFlowPath(rhsSource, rhs) and + rhs.getNode().asExpr() = c.getRightOperand() and + config.hasFlowPath(lhsSource, lhs) and + lhs.getNode().asExpr() = c.getLeftOperand() +select c, "This comparision is between user controlled operands and " ++ "hence may be bypassed." diff --git a/ql/src/experimental/CWE-840/ConditionBypassBad.go b/ql/src/experimental/CWE-840/ConditionBypassBad.go new file mode 100644 index 00000000000..6ec70c694bd --- /dev/null +++ b/ql/src/experimental/CWE-840/ConditionBypassBad.go @@ -0,0 +1,12 @@ +package main + +import ( + "net/http" +) + +// bad the origin and the host headers are user controlled +func ex1(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Origin") != "http://"+r.Host { + //do something + } +} diff --git a/ql/src/experimental/CWE-840/ConditionBypassGood.go b/ql/src/experimental/CWE-840/ConditionBypassGood.go new file mode 100644 index 00000000000..987f97a9cd0 --- /dev/null +++ b/ql/src/experimental/CWE-840/ConditionBypassGood.go @@ -0,0 +1,11 @@ +package main + +import ( + "net/http" +) + +func ex1(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Origin") != config.get("Host") { + //do something + } +} diff --git a/ql/test/experimental/CWE-840/ConditionBypass.expected b/ql/test/experimental/CWE-840/ConditionBypass.expected new file mode 100644 index 00000000000..73f3b39a0cc --- /dev/null +++ b/ql/test/experimental/CWE-840/ConditionBypass.expected @@ -0,0 +1,2 @@ +| condition.go:9:5:9:46 | ...!=... | This comparision is between user controlled operands and hence may be bypassed. | +| condition.go:16:5:16:62 | ...!=... | This comparision is between user controlled operands and hence may be bypassed. | diff --git a/ql/test/experimental/CWE-840/ConditionBypass.qlref b/ql/test/experimental/CWE-840/ConditionBypass.qlref new file mode 100644 index 00000000000..d107d9110d5 --- /dev/null +++ b/ql/test/experimental/CWE-840/ConditionBypass.qlref @@ -0,0 +1 @@ +experimental/CWE-840/ConditionBypass.ql diff --git a/ql/test/experimental/CWE-840/condition.go b/ql/test/experimental/CWE-840/condition.go new file mode 100644 index 00000000000..b61d72b94e1 --- /dev/null +++ b/ql/test/experimental/CWE-840/condition.go @@ -0,0 +1,26 @@ +package main + +import ( + "net/http" +) + +// 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 +func ex2(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Origin") != "http://"+r.Header.Get("Header") { + //do something + } +} + +// good +func ex3(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Origin") != "http://"+"test" { + //do something + } +}