Merge pull request #120 from porcupineyhairs/SensitiveActionBypass

User-controlled bypass of sensitive action

ql/src/experimental/CWE-807/SensitiveConditionBypass.qhelp

@@ -0,0 +1,30 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+    <overview>
+        <p>
+Testing untrusted user input against a fixed constant results in
+a bypass of the conditional check as the attacker may alter the input to match the constant.
+When an incorrect check of this type is used to guard a potentially sensitive block, 
+it results an attacker gaining access to the sensitive block.
+</p>
+    </overview>
+    <recommendation>
+        <p>
+ Never decide whether to authenticate a user based on data that may be controlled by that user. 
+ If necessary, ensure that the data is validated extensively when it is input before any 
+ authentication checks are performed.
+</p>
+<p>
+It is still possible to have a system that "remembers" users, thus not requiring 
+the user to login on every interaction. For example, personalization settings can be applied 
+without authentication because this is not sensitive information. However, users
+should be allowed to take sensitive actions only when they have been fully authenticated. 
+    </recommendation>
+    <example>
+        <p>
+The following example shows a comparison where an user controlled 
+expression is used to guard a sensitive method. This should be avoided.:
+</p>
+        <sample src="SensitiveConditionBypassBad.go" />
+    </example>
+</qhelp>

ql/src/experimental/CWE-807/SensitiveConditionBypass.ql

@@ -0,0 +1,31 @@
+/**
+ * @name User-controlled bypassing of sensitive action
+ * @description This query tests for user-controlled bypassing
+ *  of sensitive actions.
+ * @id go/sensitive-condition-bypass
+ * @kind problem
+ * @problem.severity warning
+ * @tags external/cwe/cwe-807
+ *       external/cwe/cwe-247
+ *       external/cwe/cwe-350
+ */
+
+import go
+import SensitiveConditionBypass
+
+from
+  ControlFlow::ConditionGuardNode guard, DataFlow::Node sensitiveSink,
+  SensitiveExpr::Classification classification, Configuration config, DataFlow::PathNode source,
+  DataFlow::PathNode operand, ComparisonExpr comp
+where
+  // there should be a flow between source and the operand sink
+  config.hasFlowPath(source, operand) and
+  // both the operand should belong to the same comparision expression
+  operand.getNode().asExpr() = comp.getAnOperand() and
+  // get the ConditionGuardNode corresponding to the comparision expr.
+  guard.getCondition() = comp and
+  // the sink `sensitiveSink` should be sensitive,
+  isSensitive(sensitiveSink, classification) and
+  // the guard should control the sink
+  guard.dominates(sensitiveSink.getBasicBlock())
+select comp, "This sensitive comparision check can potentially be bypassed."

ql/src/experimental/CWE-807/SensitiveConditionBypass.qll

@@ -0,0 +1,68 @@
+import go
+import semmle.go.security.SensitiveActions
+
+/**
+ * Holds if `sink` is used in a context that suggests it may hold sensitive data of
+ * the given `type`.
+ */
+predicate isSensitive(DataFlow::Node sink, SensitiveExpr::Classification type) {
+  exists(Write write, string name |
+    write.getRhs() = sink and
+    name = write.getLhs().getName() and
+    // whitelist obvious test password variables
+    not name.regexpMatch(HeuristicNames::notSensitive())
+  |
+    name.regexpMatch(HeuristicNames::maybeSensitive(type))
+  )
+  or
+  exists(SensitiveCall a | sink.asExpr() = a and a.getClassification() = type)
+  or
+  exists(SensitiveExpr a | sink.asExpr() = a and a.getClassification() = type)
+  or
+  exists(SensitiveAction a | a = sink and type = SensitiveExpr::secret())
+}
+
+private class ConstComparisonExpr extends ComparisonExpr {
+  string constString;
+
+  ConstComparisonExpr() {
+    exists(DataFlow::Node n |
+      n.getASuccessor*() = DataFlow::exprNode(this.getAnOperand()) and
+      constString = n.getStringValue()
+    )
+  }
+
+  predicate isPotentialFalsePositive() {
+    // if its an empty string
+    constString.length() = 0 or
+    // // if it is uri path
+    constString.matches("/%") or
+    constString.matches("%/") or
+    constString.matches("%/%")
+  }
+}
+
+/**
+ * A data-flow configuration for reasoning about
+ * user-controlled bypassing of sensitive actions.
+ */
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "Condtional Expression Check Bypass" }
+
+  override predicate isSource(DataFlow::Node source) {
+    source instanceof UntrustedFlowSource
+    or
+    exists(DataFlow::FieldReadNode f |
+      f.getField().hasQualifiedName("net/http", "Request", "Host")
+    |
+      source = f
+    )
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(ConstComparisonExpr c |
+      c.getAnOperand() = sink.asExpr() and
+      not c.isPotentialFalsePositive()
+    )
+  }
+}

ql/src/experimental/CWE-807/SensitiveConditionBypassBad.go

@@ -0,0 +1,6 @@
+func ex3(w http.ResponseWriter, r *http.Request) {
+	test2 := "test"
+	if r.Header.Get("X-Password") != test2 {
+		login()
+	}
+}

ql/test/experimental/CWE-807/SensitiveConditionBypass.expected

@@ -0,0 +1,4 @@
+| SensitiveConditionBypassBad.go:7:5:7:39 | ...!=... | This sensitive comparision check can potentially be bypassed. |
+| condition.go:16:5:16:34 | ...!=... | This sensitive comparision check can potentially be bypassed. |
+| condition.go:25:5:25:35 | ...!=... | This sensitive comparision check can potentially be bypassed. |
+| condition.go:34:5:34:35 | ...!=... | This sensitive comparision check can potentially be bypassed. |

ql/test/experimental/CWE-807/SensitiveConditionBypass.qlref

@@ -0,0 +1 @@
+experimental/CWE-807/SensitiveConditionBypass.ql

ql/test/experimental/CWE-807/SensitiveConditionBypassBad.go

@@ -0,0 +1,10 @@
+package main
+
+import "net/http"
+
+func example(w http.ResponseWriter, r *http.Request) {
+	test2 := "test"
+	if r.Header.Get("X-Password") != test2 {
+		login()
+	}
+}

ql/test/experimental/CWE-807/condition.go

@@ -0,0 +1,89 @@
+package main
+
+import (
+	"io"
+	"net/http"
+)
+
+func use(xs ...interface{})      {}
+func t(xs ...interface{}) string { return "sadsad" }
+func login(xs ...interface{})    {}
+
+const test = "localhost"
+
+// Should alert as authkey is sensitive
+func ex1(w http.ResponseWriter, r *http.Request) {
+	if r.Header.Get("Origin") != test {
+		authkey := "randomDatta"
+		io.WriteString(w, authkey)
+	}
+}
+
+// Should alert as authkey is sensitive
+func ex2(w http.ResponseWriter, r *http.Request) {
+	test2 := "test"
+	if r.Header.Get("Origin") != test2 {
+		authkey := "randomDatta2"
+		io.WriteString(w, authkey)
+	}
+}
+
+// Should alert as login() is sensitive
+func ex3(w http.ResponseWriter, r *http.Request) {
+	test2 := "test"
+	if r.Header.Get("Origin") != test2 {
+		login()
+	}
+}
+
+// no alert as we can't say if the rhs resolves to a fixed pattern everytime.
+func ex4(w http.ResponseWriter, r *http.Request) {
+	if r.Header.Get("Origin") != t()+r.Header.Get("Origin") {
+		login()
+	}
+}
+
+// No alert as use is not sensitive
+func ex5(w http.ResponseWriter, r *http.Request) {
+	test2 := "test"
+	if r.Header.Get("Origin") != test2 {
+		use()
+	}
+}
+
+// Should not alert as test is against empty string
+func ex6(w http.ResponseWriter, r *http.Request) {
+	if r.Header.Get("Origin") != "" {
+		login()
+	}
+}
+
+// Should not alert as test is against uri path
+func ex7(w http.ResponseWriter, r *http.Request) {
+	if r.Header.Get("Origin") != "/asd/" {
+		login()
+	}
+}
+
+// Should not alert as test is against uri path
+func ex8(w http.ResponseWriter, r *http.Request) {
+	if r.Header.Get("Origin") != "/asd/a" {
+		login()
+	}
+}
+
+// Should not alert as test is against uri path
+func ex9(w http.ResponseWriter, r *http.Request) {
+	if r.Header.Get("Origin") != "/asd" {
+		login()
+	}
+}
+
+// Should not alert as test is against uri path
+func ex10(w http.ResponseWriter, r *http.Request) {
+	if r.Header.Get("Origin") != "asd/" {
+		login()
+	}
+}
+
+func main() {}