From ff06815db1dfd502f0d31f75fb94a7242be316d5 Mon Sep 17 00:00:00 2001
From: edvraa <80588099+edvraa@users.noreply.github.com>
Date: Tue, 27 Apr 2021 20:16:50 +0300
Subject: [PATCH] Code review

---
 ql/src/experimental/CWE-1004/AuthCookie.qll   | 28 +++++--------------
 .../CWE-1004/CookieWithoutHttpOnly.ql         | 27 +++++++++++++-----
 2 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/ql/src/experimental/CWE-1004/AuthCookie.qll b/ql/src/experimental/CWE-1004/AuthCookie.qll
index 34e17ca1cfd..331de790927 100644
--- a/ql/src/experimental/CWE-1004/AuthCookie.qll
+++ b/ql/src/experimental/CWE-1004/AuthCookie.qll
@@ -10,21 +10,14 @@ import go
  * This should cover most typical patterns...
  */
 DataFlow::Node getValueForFieldWrite(StructLit sl, string field) {
-  exists(Write w, DataFlow::Node base, Field f, DataFlow::Node rhs |
+  exists(Write w, DataFlow::Node base, Field f |
     f.getName() = field and
-    w.writesField(base, f, rhs) and
-    result = rhs and
+    w.writesField(base, f, result) and
     (
       sl = base.asExpr()
       or
-      exists(VariableName vn |
-        vn = base.asExpr() and
-        exists(DataFlow::Node creation |
-          DataFlow::localFlow(creation, base) and
-          creation.asExpr() = sl and
-          base.asExpr() = vn
-        )
-      )
+      base.asExpr() instanceof VariableName and
+      base.getAPredecessor*().asExpr() = sl
     )
   )
 }
@@ -44,10 +37,7 @@ class HttpOnlyCookieTrackingConfiguration extends TaintTracking::Configuration {
         or
         exists(DataFlow::Node rhs |
           rhs = getValueForFieldWrite(sl, "HttpOnly") and
-          exists(DataFlow::Node valSrc |
-            DataFlow::localFlow(valSrc, rhs) and
-            valSrc.asExpr().getBoolValue() = false
-          )
+          rhs.getAPredecessor*().asExpr().getBoolValue() = false
         )
       )
     )
@@ -94,10 +84,7 @@ class AuthCookieNameConfiguration extends TaintTracking::Configuration {
       sl.getType().hasQualifiedName("net/http", "Cookie") and
       exists(DataFlow::Node rhs |
         rhs = getValueForFieldWrite(sl, "Name") and
-        exists(DataFlow::Node valSrc |
-          DataFlow::localFlow(valSrc, rhs) and
-          isAuthVariable(valSrc.asExpr())
-        )
+        isAuthVariable(rhs.getAPredecessor*().asExpr())
       )
     )
   }
@@ -157,8 +144,7 @@ class SessionOptionsTrackingConfiguration extends TaintTracking::Configuration {
   override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
     exists(Field f, DataFlow::Write w, DataFlow::Node base |
       f.getQualifiedName() = "github.com/gorilla/sessions.Session.Options" and
-      w.writesField(base, f, _) and
-      pred = w.getRhs() and
+      w.writesField(base, f, pred) and
       succ = base
     )
   }
diff --git a/ql/src/experimental/CWE-1004/CookieWithoutHttpOnly.ql b/ql/src/experimental/CWE-1004/CookieWithoutHttpOnly.ql
index 5f49a361608..4c81779943b 100644
--- a/ql/src/experimental/CWE-1004/CookieWithoutHttpOnly.ql
+++ b/ql/src/experimental/CWE-1004/CookieWithoutHttpOnly.ql
@@ -15,14 +15,18 @@
 import go
 import AuthCookie
 
-from Expr expr
-where
-  exists(SetCookieSink sink, DataFlow::Node source |
-    exists(HttpOnlyCookieTrackingConfiguration httpOnlyCfg | httpOnlyCfg.hasFlow(source, sink)) and
-    exists(AuthCookieNameConfiguration cookieNameCfg | cookieNameCfg.hasFlow(source, sink)) and
+predicate isNetHttpCookieFlow(Expr expr) {
+  exists(
+    HttpOnlyCookieTrackingConfiguration httpOnlyCfg, AuthCookieNameConfiguration cookieNameCfg,
+    SetCookieSink sink, DataFlow::Node source
+  |
+    httpOnlyCfg.hasFlow(source, sink) and
+    cookieNameCfg.hasFlow(source, sink) and
     sink.asExpr() = expr
   )
-  or
+}
+
+predicate isGinContextCookieFlow(Expr expr) {
   exists(CallExpr c |
     c.getTarget().getQualifiedName() = "github.com/gin-gonic/gin.Context.SetCookie" and
     c.getArgument(6) = expr and
@@ -37,7 +41,9 @@ where
       isAuthVariable(nameSrc.asExpr())
     )
   )
-  or
+}
+
+predicate isGorillaSessionsCookieFlow(Expr expr) {
   exists(DataFlow::Node sessionSave |
     sessionSave.asExpr() = expr and
     exists(CookieStoreSaveTrackingConfiguration cfg | cfg.hasFlow(_, sessionSave)) and
@@ -60,4 +66,11 @@ where
       )
     )
   )
+}
+
+from Expr expr
+where
+  isNetHttpCookieFlow(expr) or
+  isGinContextCookieFlow(expr) or
+  isGorillaSessionsCookieFlow(expr)
 select expr, "Cookie attribute 'HttpOnly' is not set to true."