From 4aba80b0bdc1ff30479851ca59cfb3c877dfd4c3 Mon Sep 17 00:00:00 2001
From: Porcupiney Hairs <porcupiney.hairs@protonmail.com>
Date: Mon, 11 May 2020 04:05:41 +0530
Subject: [PATCH] include changes from review

---
 .../experimental/CWE-840/ConditionBypass.qhelp  | 14 +++-----------
 ql/src/experimental/CWE-840/ConditionBypass.ql  | 17 ++++++-----------
 .../experimental/CWE-840/ConditionBypassBad.go  |  2 +-
 3 files changed, 10 insertions(+), 23 deletions(-)

diff --git a/ql/src/experimental/CWE-840/ConditionBypass.qhelp b/ql/src/experimental/CWE-840/ConditionBypass.qhelp
index 42bdc95e0f3..70525e1b0d9 100644
--- a/ql/src/experimental/CWE-840/ConditionBypass.qhelp
+++ b/ql/src/experimental/CWE-840/ConditionBypass.qhelp
@@ -8,15 +8,15 @@ a bypass of the conditional check as the attacker may modify parameters to match
     </overview>
     <recommendation>
         <p>
-To guard against this, it is advisable to avoid framing a comparision 
+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 comparision 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.:
 </p>
         <sample src="ConditionBypassBad.go" />
         <p>
@@ -24,12 +24,4 @@ One way to remedy the problem is to test against a value stored in a configurati
 </p>
         <sample src="ConditionBypassGood.go" />
     </example>
-    <references>
-        <li>
-            MITRE:
-            <a href="https://cwe.mitre.org/data/definitions/840.html">
-                CWE-840.
-            </a>
-        </li>
-    </references>
 </qhelp>
\ No newline at end of file
diff --git a/ql/src/experimental/CWE-840/ConditionBypass.ql b/ql/src/experimental/CWE-840/ConditionBypass.ql
index 8fc7fb3d249..8f4ddcbf5a6 100644
--- a/ql/src/experimental/CWE-840/ConditionBypass.ql
+++ b/ql/src/experimental/CWE-840/ConditionBypass.ql
@@ -1,11 +1,10 @@
 /**
  * @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.
+ * @description Comparing two user controlled inputs may
+ * lead to an effective bypass of the comparison check.
  * @id go/condition-bypass
  * @kind problem
- * @problem.severity medium
+ * @problem.severity warning
  * @tags external/cwe/cwe-840
  */
 
@@ -20,11 +19,7 @@ class Configuration extends TaintTracking::Configuration {
   override predicate isSource(DataFlow::Node source) {
     source instanceof UntrustedFlowSource
     or
-    exists(string fieldName |
-      source.(DataFlow::FieldReadNode).getField().hasQualifiedName("net/http", "Request", fieldName)
-    |
-      fieldName = "Host"
-    )
+    source = any(Field f | f.hasQualifiedName("net/http", "Request", "Host")).getARead()
   }
 
   override predicate isSink(DataFlow::Node sink) {
@@ -40,5 +35,5 @@ where
   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."
+select c, "This comparision is between user controlled operands derived from $@", lhsSource,
+  " and $@", rhsSource, "hence may be bypassed."
diff --git a/ql/src/experimental/CWE-840/ConditionBypassBad.go b/ql/src/experimental/CWE-840/ConditionBypassBad.go
index 6ec70c694bd..3174d8f8012 100644
--- a/ql/src/experimental/CWE-840/ConditionBypassBad.go
+++ b/ql/src/experimental/CWE-840/ConditionBypassBad.go
@@ -4,8 +4,8 @@ import (
 	"net/http"
 )
 
-// bad the origin and the host headers are user controlled
 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
 	}