Add cookie injection query missing proper tests

Author: jorgectf

python/ql/src/experimental/Security/CWE-614/CookieInjection.ql

@@ -0,0 +1,28 @@
+/**
+ * @name Failure to use secure cookies
+ * @description Insecure cookies may be sent in cleartext, which makes them vulnerable to
+ *              interception.
+ * @kind problem
+ * @problem.severity error
+ * @id py/insecure-cookie
+ * @tags security
+ *       external/cwe/cwe-614
+ */
+
+// determine precision above
+import python
+import semmle.python.dataflow.new.DataFlow
+import experimental.semmle.python.Concepts
+import experimental.semmle.python.CookieHeader
+import experimental.semmle.python.security.injection.CookieInjection
+
+from
+  CookieInjectionFlowConfig config, DataFlow::PathNode source, DataFlow::PathNode sink,
+  string insecure
+where
+  config.hasFlowPath(source, sink) and
+  if exists(sink.getNode().(CookieSink))
+  then insecure = "and it's " + sink.getNode().(CookieSink).getFlag() + " flag is not properly set"
+  else insecure = ""
+select sink.getNode(), "Cookie is constructed from a", source.getNode(), "user-supplied input",
+  insecure

python/ql/src/experimental/semmle/python/Concepts.qll

@@ -322,6 +322,16 @@ class Cookie extends DataFlow::Node {
    * Holds if the cookie is SameSite
    */
   predicate isSameSite() { range.isSameSite() }
+
+  /**
+   * Gets the argument containing the header name.
+   */
+  DataFlow::Node getName() { result = range.getName() }
+
+  /**
+   * Gets the argument containing the header value.
+   */
+  DataFlow::Node getValue() { result = range.getValue() }
 }
 
 /** Provides a class for modeling new cookie writes on HTTP responses. */
@@ -347,5 +357,15 @@ module Cookie {
      * Holds if the cookie is SameSite.
      */
     abstract predicate isSameSite();
+
+    /**
+     * Gets the argument containing the header name.
+     */
+    abstract DataFlow::Node getName();
+
+    /**
+     * Gets the argument containing the header value.
+     */
+    abstract DataFlow::Node getValue();
   }
 }

python/ql/src/experimental/semmle/python/CookieHeader.qll

@@ -9,18 +9,28 @@ import experimental.semmle.python.Concepts
 
 class CookieHeader extends HeaderDeclaration, Cookie::Range {
   CookieHeader() {
-    this instanceof HeaderDeclaration and this.getNameArg().asExpr().(Str_).getS() = "Set-Cookie"
+    this instanceof HeaderDeclaration and
+    this.(HeaderDeclaration).getNameArg().asExpr().(Str_).getS() = "Set-Cookie"
   }
 
   override predicate isSecure() {
-    this.getValueArg().asExpr().(Str_).getS().regexpMatch(".*; *Secure;.*")
+    this.(HeaderDeclaration).getValueArg().asExpr().(Str_).getS().regexpMatch(".*; *Secure;.*")
   }
 
   override predicate isHttpOnly() {
-    this.getValueArg().asExpr().(Str_).getS().regexpMatch(".*; *HttpOnly;.*")
+    this.(HeaderDeclaration).getValueArg().asExpr().(Str_).getS().regexpMatch(".*; *HttpOnly;.*")
   }
 
   override predicate isSameSite() {
-    this.getValueArg().asExpr().(Str_).getS().regexpMatch(".*; *SameSite=(Strict|Lax);.*")
+    this.(HeaderDeclaration)
+        .getValueArg()
+        .asExpr()
+        .(Str_)
+        .getS()
+        .regexpMatch(".*; *SameSite=(Strict|Lax);.*")
   }
+
+  override DataFlow::Node getName() { result = this.(HeaderDeclaration).getValueArg() }
+
+  override DataFlow::Node getValue() { result = this.(HeaderDeclaration).getValueArg() }
 }

python/ql/src/experimental/semmle/python/frameworks/Django.qll

@@ -90,6 +90,10 @@ private module PrivateDjango {
           class DjangoSetCookieCall extends DataFlow::CallCfgNode, Cookie::Range {
             DjangoSetCookieCall() { this = baseClassRef().getMember("set_cookie").getACall() }
 
+            override DataFlow::Node getName() { result = this.getArg(0) }
+
+            override DataFlow::Node getValue() { result = this.getArgByName("value") }
+
             override predicate isSecure() {
               DataFlow::exprNode(any(True t))
                   .(DataFlow::LocalSourceNode)

python/ql/src/experimental/semmle/python/frameworks/Flask.qll

@@ -91,6 +91,10 @@ module ExperimentalFlask {
             .getACall()
     }
 
+    override DataFlow::Node getName() { result = this.getArg(0) }
+
+    override DataFlow::Node getValue() { result = this.getArgByName("value") }
+
     override predicate isSecure() {
       DataFlow::exprNode(any(True t))
           .(DataFlow::LocalSourceNode)

python/ql/src/experimental/semmle/python/security/injection/CookieInjection.qll

@@ -0,0 +1,40 @@
+import python
+import experimental.semmle.python.Concepts
+import semmle.python.dataflow.new.DataFlow
+import semmle.python.dataflow.new.TaintTracking
+import semmle.python.dataflow.new.RemoteFlowSources
+
+class CookieSink extends DataFlow::Node {
+  string flag;
+
+  CookieSink() {
+    exists(Cookie cookie |
+      this in [cookie.getName(), cookie.getValue()] and
+      (
+        not cookie.isSecure() and
+        flag = "secure"
+        or
+        not cookie.isHttpOnly() and
+        flag = "httponly"
+        or
+        not cookie.isSameSite() and
+        flag = "samesite"
+      )
+    )
+  }
+
+  string getFlag() { result = flag }
+}
+
+/**
+ * A taint-tracking configuration for detecting Cookie injections.
+ */
+class CookieInjectionFlowConfig extends TaintTracking::Configuration {
+  CookieInjectionFlowConfig() { this = "CookieInjectionFlowConfig" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(Cookie c | sink in [c.getName(), c.getValue()])
+  }
+}

python/ql/test/experimental/query-tests/Security/CWE-614/flask_bad.py

@@ -6,7 +6,7 @@
 @app.route("/false")
 def false():
     resp = make_response()
-    resp.set_cookie("name", value="value", secure=False,
+    resp.set_cookie(request.args["name"], value=request.args["value"], secure=False,
                     httponly=False, samesite='None')
     return resp