Make CookieWithoutHttpOnly use new API

owen-mc · owen-mc · commit 00cc78dfe6e8 · 2023-08-10T15:49:00.000+01:00
The extra nodes in .expected files are due to the changes from #13717, which are not applied to configuration classes extending DataFlow::Configuration or TaintTracking::Configuration.
diff --git a/go/ql/src/experimental/CWE-1004/AuthCookie.qll b/go/ql/src/experimental/CWE-1004/AuthCookie.qll
@@ -65,10 +65,12 @@ private class SetCookieSink extends DataFlow::Node {
 }
 
 /**
+ * DEPRECATED: Use `NameToNetHttpCookieTrackingFlow` instead.
+ *
  * A taint-tracking configuration for tracking flow from sensitive names to
  * `net/http.SetCookie`.
  */
-class NameToNetHttpCookieTrackingConfiguration extends TaintTracking::Configuration {
+deprecated class NameToNetHttpCookieTrackingConfiguration extends TaintTracking::Configuration {
   NameToNetHttpCookieTrackingConfiguration() { this = "NameToNetHttpCookieTrackingConfiguration" }
 
   override predicate isSource(DataFlow::Node source) { isAuthVariable(source.asExpr()) }
@@ -84,11 +86,29 @@ class NameToNetHttpCookieTrackingConfiguration extends TaintTracking::Configurat
   }
 }
 
+private module NameToNetHttpCookieTrackingConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { isAuthVariable(source.asExpr()) }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof SetCookieSink }
+
+  predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(StructLit sl |
+      sl.getType() instanceof NetHttpCookieType and
+      getValueForFieldWrite(sl, "Name") = pred and
+      sl = succ.asExpr()
+    )
+  }
+}
+
+module NameToNetHttpCookieTrackingFlow = TaintTracking::Global<NameToNetHttpCookieTrackingConfig>;
+
 /**
+ * DEPRECATED: Use `BoolToNetHttpCookieTrackingFlow` instead.
+ *
  * A taint-tracking configuration for tracking flow from `bool` assigned to
  * `HttpOnly` that flows into `net/http.SetCookie`.
  */
-class BoolToNetHttpCookieTrackingConfiguration extends TaintTracking::Configuration {
+deprecated class BoolToNetHttpCookieTrackingConfiguration extends TaintTracking::Configuration {
   BoolToNetHttpCookieTrackingConfiguration() { this = "BoolToNetHttpCookieTrackingConfiguration" }
 
   override predicate isSource(DataFlow::Node source) {
@@ -106,11 +126,31 @@ class BoolToNetHttpCookieTrackingConfiguration extends TaintTracking::Configurat
   }
 }
 
+private module BoolToNetHttpCookieTrackingConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    source.getType().getUnderlyingType() instanceof BoolType
+  }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof SetCookieSink }
+
+  predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(StructLit sl |
+      sl.getType() instanceof NetHttpCookieType and
+      getValueForFieldWrite(sl, "HttpOnly") = pred and
+      sl = succ.asExpr()
+    )
+  }
+}
+
+module BoolToNetHttpCookieTrackingFlow = TaintTracking::Global<BoolToNetHttpCookieTrackingConfig>;
+
 /**
+ * DEPRECATED: Use `BoolToGinSetCookieTrackingFlow` instead.
+ *
  * A taint-tracking configuration for tracking flow from `HttpOnly` set to
  * `false` to `gin-gonic/gin.Context.SetCookie`.
  */
-class BoolToGinSetCookieTrackingConfiguration extends DataFlow::Configuration {
+deprecated class BoolToGinSetCookieTrackingConfiguration extends DataFlow::Configuration {
   BoolToGinSetCookieTrackingConfiguration() { this = "BoolToGinSetCookieTrackingConfiguration" }
 
   override predicate isSource(DataFlow::Node source) { source.getBoolValue() = false }
@@ -119,31 +159,48 @@ class BoolToGinSetCookieTrackingConfiguration extends DataFlow::Configuration {
     exists(DataFlow::MethodCallNode mcn |
       mcn.getTarget() instanceof GinContextSetCookieMethod and
       mcn.getArgument(6) = sink and
-      exists(NameToGinSetCookieTrackingConfiguration cfg, DataFlow::Node nameArg |
-        cfg.hasFlowTo(nameArg) and
+      exists(DataFlow::Node nameArg |
+        NameToGinSetCookieTrackingFlow::flowTo(nameArg) and
+        mcn.getArgument(0) = nameArg
+      )
+    )
+  }
+}
+
+private module BoolToGinSetCookieTrackingConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source.getBoolValue() = false }
+
+  predicate isSink(DataFlow::Node sink) {
+    exists(DataFlow::MethodCallNode mcn |
+      mcn.getTarget() instanceof GinContextSetCookieMethod and
+      mcn.getArgument(6) = sink and
+      exists(DataFlow::Node nameArg |
+        NameToGinSetCookieTrackingFlow::flowTo(nameArg) and
         mcn.getArgument(0) = nameArg
       )
     )
   }
 }
 
+module BoolToGinSetCookieTrackingFlow = DataFlow::Global<BoolToGinSetCookieTrackingConfig>;
+
 /**
  * A taint-tracking configuration for tracking flow from sensitive names to
  * `gin-gonic/gin.Context.SetCookie`.
  */
-private class NameToGinSetCookieTrackingConfiguration extends DataFlow2::Configuration {
-  NameToGinSetCookieTrackingConfiguration() { this = "NameToGinSetCookieTrackingConfiguration" }
-
-  override predicate isSource(DataFlow::Node source) { isAuthVariable(source.asExpr()) }
+private module NameToGinSetCookieTrackingConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { isAuthVariable(source.asExpr()) }
 
-  override predicate isSink(DataFlow::Node sink) {
+  predicate isSink(DataFlow::Node sink) {
     exists(DataFlow::MethodCallNode mcn |
       mcn.getTarget() instanceof GinContextSetCookieMethod and
       mcn.getArgument(0) = sink
     )
   }
 }
 
+private module NameToGinSetCookieTrackingFlow = DataFlow::Global<NameToGinSetCookieTrackingConfig>;
+
 /**
  * The receiver of `gorilla/sessions.Session.Save` call.
  */
@@ -168,10 +225,12 @@ private class GorillaStoreSaveSink extends DataFlow::Node {
 }
 
 /**
+ * DEPRECATED: Use `GorillaCookieStoreSaveTrackingFlow` instead.
+ *
  * A taint-tracking configuration for tracking flow from gorilla cookie store
  * creation to `gorilla/sessions.Session.Save`.
  */
-class GorillaCookieStoreSaveTrackingConfiguration extends DataFlow::Configuration {
+deprecated class GorillaCookieStoreSaveTrackingConfiguration extends DataFlow::Configuration {
   GorillaCookieStoreSaveTrackingConfiguration() {
     this = "GorillaCookieStoreSaveTrackingConfiguration"
   }
@@ -198,11 +257,38 @@ class GorillaCookieStoreSaveTrackingConfiguration extends DataFlow::Configuratio
   }
 }
 
+private module GorillaCookieStoreSaveTrackingConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    source
+        .(DataFlow::CallNode)
+        .getTarget()
+        .hasQualifiedName(package("github.com/gorilla/sessions", ""), "NewCookieStore")
+  }
+
+  predicate isSink(DataFlow::Node sink) {
+    sink instanceof GorillaSessionSaveSink or
+    sink instanceof GorillaStoreSaveSink
+  }
+
+  predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(DataFlow::MethodCallNode cn |
+      cn.getTarget()
+          .hasQualifiedName(package("github.com/gorilla/sessions", ""), "CookieStore", "Get") and
+      pred = cn.getReceiver() and
+      succ = cn.getResult(0)
+    )
+  }
+}
+
+module GorillaCookieStoreSaveTrackingFlow = DataFlow::Global<GorillaCookieStoreSaveTrackingConfig>;
+
 /**
+ * DEPRECATED: Use `GorillaSessionOptionsTrackingFlow` instead.
+ *
  * A taint-tracking configuration for tracking flow from session options to
  * `gorilla/sessions.Session.Save`.
  */
-class GorillaSessionOptionsTrackingConfiguration extends TaintTracking::Configuration {
+deprecated class GorillaSessionOptionsTrackingConfiguration extends TaintTracking::Configuration {
   GorillaSessionOptionsTrackingConfiguration() {
     this = "GorillaSessionOptionsTrackingConfiguration"
   }
@@ -224,11 +310,35 @@ class GorillaSessionOptionsTrackingConfiguration extends TaintTracking::Configur
   }
 }
 
+private module GorillaSessionOptionsTrackingConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    exists(StructLit sl |
+      sl.getType().hasQualifiedName(package("github.com/gorilla/sessions", ""), "Options") and
+      source.asExpr() = sl
+    )
+  }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof GorillaSessionSaveSink }
+
+  predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(GorillaSessionOptionsField f, DataFlow::Write w, DataFlow::Node base |
+      w.writesField(base, f, pred) and
+      succ = base
+    )
+  }
+}
+
+module GorillaSessionOptionsTrackingFlow =
+  TaintTracking::Global<GorillaSessionOptionsTrackingConfig>;
+
 /**
+ * DEPRECATED: Use `BoolToGorillaSessionOptionsTrackingFlow` instead.
+ *
  * A taint-tracking configuration for tracking flow from a `bool` assigned to
  * `HttpOnly` to `gorilla/sessions.Session.Save`.
  */
-class BoolToGorillaSessionOptionsTrackingConfiguration extends TaintTracking::Configuration {
+deprecated class BoolToGorillaSessionOptionsTrackingConfiguration extends TaintTracking::Configuration
+{
   BoolToGorillaSessionOptionsTrackingConfiguration() {
     this = "BoolToGorillaSessionOptionsTrackingConfiguration"
   }
@@ -251,3 +361,26 @@ class BoolToGorillaSessionOptionsTrackingConfiguration extends TaintTracking::Co
     )
   }
 }
+
+private module BoolToGorillaSessionOptionsTrackingConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    source.getType().getUnderlyingType() instanceof BoolType
+  }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof GorillaSessionSaveSink }
+
+  predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(StructLit sl |
+      getValueForFieldWrite(sl, "HttpOnly") = pred and
+      sl = succ.asExpr()
+    )
+    or
+    exists(GorillaSessionOptionsField f, DataFlow::Write w, DataFlow::Node base |
+      w.writesField(base, f, pred) and
+      succ = base
+    )
+  }
+}
+
+module BoolToGorillaSessionOptionsTrackingFlow =
+  TaintTracking::Global<BoolToGorillaSessionOptionsTrackingConfig>;
diff --git a/go/ql/src/experimental/CWE-1004/CookieWithoutHttpOnly.ql b/go/ql/src/experimental/CWE-1004/CookieWithoutHttpOnly.ql
@@ -15,25 +15,40 @@
 
 import go
 import AuthCookie
-import DataFlow::PathGraph
+
+module MergedFlow1 =
+  DataFlow::MergePathGraph<NameToNetHttpCookieTrackingFlow::PathNode,
+    BoolToNetHttpCookieTrackingFlow::PathNode, NameToNetHttpCookieTrackingFlow::PathGraph,
+    BoolToNetHttpCookieTrackingFlow::PathGraph>;
+
+module MergedFlow2 =
+  DataFlow::MergePathGraph3<GorillaCookieStoreSaveTrackingFlow::PathNode,
+    GorillaSessionOptionsTrackingFlow::PathNode, BoolToGorillaSessionOptionsTrackingFlow::PathNode,
+    GorillaCookieStoreSaveTrackingFlow::PathGraph, GorillaSessionOptionsTrackingFlow::PathGraph,
+    BoolToGorillaSessionOptionsTrackingFlow::PathGraph>;
+
+module MergedFlow =
+  DataFlow::MergePathGraph3<MergedFlow1::PathNode, BoolToGinSetCookieTrackingFlow::PathNode,
+    MergedFlow2::PathNode, MergedFlow1::PathGraph, BoolToGinSetCookieTrackingFlow::PathGraph,
+    MergedFlow2::PathGraph>;
+
+import MergedFlow::PathGraph
 
 /** Holds if `HttpOnly` of `net/http.SetCookie` is set to `false` or not set (default value is used). */
-predicate isNetHttpCookieFlow(DataFlow::PathNode source, DataFlow::PathNode sink) {
-  exists(DataFlow::PathNode sensitiveName, DataFlow::PathNode setCookieSink |
-    exists(NameToNetHttpCookieTrackingConfiguration cfg |
-      cfg.hasFlowPath(sensitiveName, setCookieSink)
-    ) and
+predicate isNetHttpCookieFlow(MergedFlow1::PathNode source, MergedFlow1::PathNode sink) {
+  exists(
+    NameToNetHttpCookieTrackingFlow::PathNode sensitiveName,
+    NameToNetHttpCookieTrackingFlow::PathNode setCookieSink
+  |
+    NameToNetHttpCookieTrackingFlow::flowPath(sensitiveName, setCookieSink) and
     (
-      not any(BoolToNetHttpCookieTrackingConfiguration cfg).hasFlowTo(setCookieSink.getNode()) and
-      source = sensitiveName and
-      sink = setCookieSink
+      not BoolToNetHttpCookieTrackingFlow::flowTo(sink.getNode()) and
+      source.asPathNode1() = sensitiveName and
+      sink.asPathNode1() = setCookieSink
       or
-      exists(BoolToNetHttpCookieTrackingConfiguration cfg, DataFlow::PathNode setCookieSink2 |
-        cfg.hasFlowPath(source, setCookieSink2) and
-        source.getNode().getBoolValue() = false and
-        sink = setCookieSink2 and
-        setCookieSink.getNode() = setCookieSink2.getNode()
-      )
+      BoolToNetHttpCookieTrackingFlow::flowPath(source.asPathNode2(), sink.asPathNode2()) and
+      source.getNode().getBoolValue() = false and
+      setCookieSink.getNode() = sink.getNode()
     )
   )
 }
@@ -42,44 +57,38 @@ predicate isNetHttpCookieFlow(DataFlow::PathNode source, DataFlow::PathNode sink
  * Holds if there is gorilla cookie store creation to `Save` path and
  * `HttpOnly` is set to `false` or not set (default value is used).
  */
-predicate isGorillaSessionsCookieFlow(DataFlow::PathNode source, DataFlow::PathNode sink) {
-  exists(DataFlow::PathNode cookieStoreCreate, DataFlow::PathNode sessionSave |
-    any(GorillaCookieStoreSaveTrackingConfiguration cfg).hasFlowPath(cookieStoreCreate, sessionSave) and
+predicate isGorillaSessionsCookieFlow(MergedFlow2::PathNode source, MergedFlow2::PathNode sink) {
+  exists(
+    GorillaCookieStoreSaveTrackingFlow::PathNode cookieStoreCreate,
+    GorillaCookieStoreSaveTrackingFlow::PathNode sessionSave
+  |
+    GorillaCookieStoreSaveTrackingFlow::flowPath(cookieStoreCreate, sessionSave) and
     (
-      not any(GorillaSessionOptionsTrackingConfiguration cfg).hasFlowTo(sessionSave.getNode()) and
-      source = cookieStoreCreate and
-      sink = sessionSave
+      not GorillaSessionOptionsTrackingFlow::flowTo(sink.getNode()) and
+      source.asPathNode1() = cookieStoreCreate and
+      sink.asPathNode1() = sessionSave
       or
-      exists(
-        GorillaSessionOptionsTrackingConfiguration cfg, DataFlow::PathNode options,
-        DataFlow::PathNode sessionSave2
-      |
-        cfg.hasFlowPath(options, sessionSave2) and
+      exists(MergedFlow2::PathNode options, MergedFlow2::PathNode sessionSave2 |
+        GorillaSessionOptionsTrackingFlow::flowPath(options.asPathNode2(),
+          sessionSave2.asPathNode2()) and
         (
-          not any(BoolToGorillaSessionOptionsTrackingConfiguration boolCfg)
-              .hasFlowTo(sessionSave.getNode()) and
-          sink = sessionSave2 and
+          not BoolToGorillaSessionOptionsTrackingFlow::flowTo(sink.getNode()) and
           source = options and
+          sink = sessionSave2 and
           sessionSave.getNode() = sessionSave2.getNode()
           or
-          exists(
-            BoolToGorillaSessionOptionsTrackingConfiguration boolCfg,
-            DataFlow::PathNode sessionSave3
-          |
-            boolCfg.hasFlowPath(source, sessionSave3) and
-            source.getNode().getBoolValue() = false and
-            sink = sessionSave3 and
-            sessionSave.getNode() = sessionSave3.getNode()
-          )
+          BoolToGorillaSessionOptionsTrackingFlow::flowPath(source.asPathNode3(), sink.asPathNode3()) and
+          source.getNode().getBoolValue() = false and
+          sink.getNode() = sessionSave.getNode()
         )
       )
     )
   )
 }
 
-from DataFlow::PathNode source, DataFlow::PathNode sink
+from MergedFlow::PathNode source, MergedFlow::PathNode sink
 where
-  isNetHttpCookieFlow(source, sink) or
-  any(BoolToGinSetCookieTrackingConfiguration cfg).hasFlowPath(source, sink) or
-  isGorillaSessionsCookieFlow(source, sink)
+  isNetHttpCookieFlow(source.asPathNode1(), sink.asPathNode1()) or
+  BoolToGinSetCookieTrackingFlow::flowPath(source.asPathNode2(), sink.asPathNode2()) or
+  isGorillaSessionsCookieFlow(source.asPathNode3(), sink.asPathNode3())
 select sink.getNode(), source, sink, "Cookie attribute 'HttpOnly' is not set to true."
diff --git a/go/ql/test/experimental/CWE-1004/CookieWithoutHttpOnly.expected b/go/ql/test/experimental/CWE-1004/CookieWithoutHttpOnly.expected
