Merge pull request #121 from porcupineyhairs/conditionBypass

User-controlled bypass of a comparision

ql/src/experimental/CWE-840/ConditionBypass.qhelp

@@ -0,0 +1,27 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+    <overview>
+        <p>
+Testing untrusted user input against untrusted user input results in
+a bypass of the conditional check as the attacker may modify parameters to match.
+</p>
+    </overview>
+    <recommendation>
+        <p>
+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.
+</p>
+    </recommendation>
+    <example>
+        <p>
+The following example shows a comparison where both the sides 
+are from attacker-controlled request headers. This should be avoided.:
+</p>
+        <sample src="ConditionBypassBad.go" />
+        <p>
+One way to remedy the problem is to test against a value stored in a configuration:
+</p>
+        <sample src="ConditionBypassGood.go" />
+    </example>
+</qhelp>

ql/src/experimental/CWE-840/ConditionBypass.ql

@@ -0,0 +1,39 @@
+/**
+ * @name Comparision Expression Check Bypass
+ * @description Comparing two user controlled inputs may
+ * lead to an effective bypass of the comparison check.
+ * @id go/condition-bypass
+ * @kind problem
+ * @problem.severity warning
+ * @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
+    source = any(Field f | f.hasQualifiedName("net/http", "Request", "Host")).getARead()
+  }
+
+  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 derived from $@", lhsSource,
+  " and $@", rhsSource, "hence may be bypassed."

ql/src/experimental/CWE-840/ConditionBypassBad.go

@@ -0,0 +1,12 @@
+package main
+
+import (
+	"net/http"
+)
+
+func ex1(w http.ResponseWriter, r *http.Request) {
+	// bad the origin and the host headers are user controlled
+	if r.Header.Get("Origin") != "http://"+r.Host {
+		//do something
+	}
+}

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
+	}
+}

ql/test/experimental/CWE-840/ConditionBypass.expected

@@ -0,0 +1,2 @@
+| condition.go:9:5:9:46 | ...!=... | This comparision is between user controlled operands derived from $@ | condition.go:9:5:9:12 | selection of Header : Header |  and $@ | condition.go:9:41:9:46 | selection of Host : string | hence may be bypassed. |
+| condition.go:16:5:16:62 | ...!=... | This comparision is between user controlled operands derived from $@ | condition.go:16:5:16:12 | selection of Header : Header |  and $@ | condition.go:16:41:16:48 | selection of Header : Header | hence may be bypassed. |

ql/test/experimental/CWE-840/ConditionBypass.qlref

@@ -0,0 +1 @@
+experimental/CWE-840/ConditionBypass.ql

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
+	}
+}