github · joefarebrother · Aug 7, 2024 · May 28, 2024 · Jun 28, 2024 · Jul 8, 2024
@@ -1203,6 +1203,77 @@ module Http {
        * Gets the argument, if any, specifying the cookie value.
        */
       DataFlow::Node getValueArg() { result = super.getValueArg() }
+
+      /**
+       * Holds if the `Secure` flag of the cookie is known to have a value of `b`.
+       */
+      predicate hasSecureFlag(boolean b) { super.hasSecureFlag(b) }
+
+      /**
+       * Holds if the `HttpOnly` flag of the cookie is known to have a value of `b`.
+       */
+      predicate hasHttpOnlyFlag(boolean b) { super.hasHttpOnlyFlag(b) }
+
+      /**
+       * Holds if the `SameSite` attribute of the cookie is known to have a value of `v`.
+       */
+      predicate hasSameSiteAttribute(CookieWrite::SameSiteValue v) { super.hasSameSiteAttribute(v) }
+    }
+
+    /**
+     * A dataflow call node to a method that sets a cookie in an http response,
+     * and has common keyword arguments `secure`, `httponly`, and `samesite` to set the attributes of the cookie.
+     *
+     * See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie
+     */
+    abstract class SetCookieCall extends CookieWrite::Range, DataFlow::CallCfgNode {
+      override predicate hasSecureFlag(boolean b) {
+        super.hasSecureFlag(b)
+        or
+        exists(DataFlow::Node arg, BooleanLiteral bool | arg = this.getArgByName("secure") |
+          DataFlow::localFlow(DataFlow::exprNode(bool), arg) and
+          b = bool.booleanValue()
+        )
+        or
+        not exists(this.getArgByName("secure")) and
+        not exists(this.getKwargs()) and
+        b = false
+      }
+
+      override predicate hasHttpOnlyFlag(boolean b) {
+        super.hasHttpOnlyFlag(b)
+        or
+        exists(DataFlow::Node arg, BooleanLiteral bool | arg = this.getArgByName("httponly") |
+          DataFlow::localFlow(DataFlow::exprNode(bool), arg) and
+          b = bool.booleanValue()
+        )
+        or
+        not exists(this.getArgByName("httponly")) and
+        not exists(this.getKwargs()) and
+        b = false
+      }
+
+      override predicate hasSameSiteAttribute(CookieWrite::SameSiteValue v) {
+        super.hasSameSiteAttribute(v)
+        or
+        exists(DataFlow::Node arg, StringLiteral str | arg = this.getArgByName("samesite") |
+          DataFlow::localFlow(DataFlow::exprNode(str), arg) and
+          (
+            str.getText().toLowerCase() = "strict" and
+            v instanceof CookieWrite::SameSiteStrict
+            or
+            str.getText().toLowerCase() = "lax" and
+            v instanceof CookieWrite::SameSiteLax
+            or
+            str.getText().toLowerCase() = "none" and
+            v instanceof CookieWrite::SameSiteNone
+          )
+        )
+        or
+        not exists(this.getArgByName("samesite")) and
+        not exists(this.getKwargs()) and
+        v instanceof CookieWrite::SameSiteLax // Lax is the default
+      }
     }
 
     /** Provides a class for modeling new cookie writes on HTTP responses. */
@@ -1231,6 +1302,91 @@ module Http {
          * Gets the argument, if any, specifying the cookie value.
          */
         abstract DataFlow::Node getValueArg();
+
+        /**
+         * Holds if the `Secure` flag of the cookie is known to have a value of `b`.
+         */
+        predicate hasSecureFlag(boolean b) {
+          exists(StringLiteral sl |
+            // `sl` is likely a substring of the header
+            TaintTracking::localTaint(DataFlow::exprNode(sl), this.getHeaderArg()) and
+            sl.getText().regexpMatch("(?i).*;\\s*secure(;.*|\\s*)") and
+            b = true
+            or
+            // `sl` is the entire header
+            DataFlow::localFlow(DataFlow::exprNode(sl), this.getHeaderArg()) and
+            not sl.getText().regexpMatch("(?i).*;\\s*secure(;.*|\\s*)") and
+            b = false
+          )
+        }
+
+        /**
+         * Holds if the `HttpOnly` flag of the cookie is known to have a value of `b`.
+         */
+        predicate hasHttpOnlyFlag(boolean b) {
+          exists(StringLiteral sl |
+            // `sl` is likely a substring of the header
+            TaintTracking::localTaint(DataFlow::exprNode(sl), this.getHeaderArg()) and
+            sl.getText().regexpMatch("(?i).*;\\s*httponly(;.*|\\s*)") and
+            b = true
+            or
+            // `sl` is the entire header
+            DataFlow::localFlow(DataFlow::exprNode(sl), this.getHeaderArg()) and
+            not sl.getText().regexpMatch("(?i).*;\\s*httponly(;.*|\\s*)") and
+            b = false
+          )
+        }
+
+        /**
+         * Holds if the `SameSite` flag of the cookie is known to have a value of `v`.
+         */
+        predicate hasSameSiteAttribute(SameSiteValue v) {
+          exists(StringLiteral sl |
+            // `sl` is likely a substring of the header
+            TaintTracking::localTaint(DataFlow::exprNode(sl), this.getHeaderArg()) and
+            (
+              sl.getText().regexpMatch("(?i).*;\\s*samesite=strict(;.*|\\s*)") and
+              v instanceof SameSiteStrict
+              or
+              sl.getText().regexpMatch("(?i).*;\\s*samesite=lax(;.*|\\s*)") and
+              v instanceof SameSiteLax
+              or
+              sl.getText().regexpMatch("(?i).*;\\s*samesite=none(;.*|\\s*)") and
+              v instanceof SameSiteNone
+            )
+            or
+            // `sl` is the entire header
+            DataFlow::localFlow(DataFlow::exprNode(sl), this.getHeaderArg()) and
+            not sl.getText().regexpMatch("(?i).*;\\s*samesite=(strict|lax|none)(;.*|\\s*)") and
+            v instanceof SameSiteLax // Lax is the default
+          )
+        }
+      }
+
+      private newtype TSameSiteValue =
+        TSameSiteStrict() or
+        TSameSiteLax() or
+        TSameSiteNone()
+
+      /** A possible value for the SameSite attribute of a cookie. */
+      class SameSiteValue extends TSameSiteValue {
+        /** Gets a string representation of this value. */
+        string toString() { none() }
+      }
+
+      /** A `Strict` value of the `SameSite` attribute. */
+      class SameSiteStrict extends SameSiteValue, TSameSiteStrict {
+        override string toString() { result = "Strict" }
+      }
+
+      /** A `Lax` value of the `SameSite` attribute. */
+      class SameSiteLax extends SameSiteValue, TSameSiteLax {
+        override string toString() { result = "Lax" }
+      }
+
+      /** A `None` value of the `SameSite` attribute. */
+      class SameSiteNone extends SameSiteValue, TSameSiteNone {
+        override string toString() { result = "None" }
       }
     }
 

@@ -219,6 +219,12 @@ class CallCfgNode extends CfgNode, LocalSourceNode {
 
   /** Gets the data-flow node corresponding to the named argument of the call corresponding to this data-flow node */
   Node getArgByName(string name) { result.asCfgNode() = node.getArgByName(name) }
+
+  /** Gets the data-flow node corresponding to the first tuple (*) argument of the call corresponding to this data-flow node, if any. */
+  Node getStarArg() { result.asCfgNode() = node.getStarArg() }
+
+  /** Gets the data-flow node corresponding to a dictionary (**) argument of the call corresponding to this data-flow node, if any. */
+  Node getKwargs() { result.asCfgNode() = node.getKwargs() }
 }
 
 /**

@@ -653,8 +653,7 @@ module AiohttpWebModel {
   /**
    * A call to `set_cookie` on a HTTP Response.
    */
-  class AiohttpResponseSetCookieCall extends Http::Server::CookieWrite::Range, DataFlow::CallCfgNode
-  {
+  class AiohttpResponseSetCookieCall extends Http::Server::SetCookieCall {
     AiohttpResponseSetCookieCall() {
       this = aiohttpResponseInstance().getMember("set_cookie").getACall()
     }

@@ -2170,7 +2170,7 @@ module PrivateDjango {
         /**
          * A call to `set_cookie` on a HTTP Response.
          */
-        class DjangoResponseSetCookieCall extends Http::Server::CookieWrite::Range,
+        class DjangoResponseSetCookieCall extends Http::Server::SetCookieCall,
           DataFlow::MethodCallNode
         {
           DjangoResponseSetCookieCall() {

@@ -348,7 +348,7 @@ module FastApi {
     /**
      * A call to `set_cookie` on a FastAPI Response.
      */
-    private class SetCookieCall extends Http::Server::CookieWrite::Range, DataFlow::MethodCallNode {
+    private class SetCookieCall extends Http::Server::SetCookieCall, DataFlow::MethodCallNode {
       SetCookieCall() { this.calls(instance(), "set_cookie") }
 
       override DataFlow::Node getHeaderArg() { none() }

@@ -583,9 +583,7 @@ module Flask {
    *
    * See https://flask.palletsprojects.com/en/2.0.x/api/#flask.Response.set_cookie
    */
-  class FlaskResponseSetCookieCall extends Http::Server::CookieWrite::Range,
-    DataFlow::MethodCallNode
-  {
+  class FlaskResponseSetCookieCall extends Http::Server::SetCookieCall, DataFlow::MethodCallNode {
     FlaskResponseSetCookieCall() { this.calls(Flask::Response::instance(), "set_cookie") }
 
     override DataFlow::Node getHeaderArg() { none() }

@@ -255,7 +255,7 @@ module Pyramid {
     }
 
     /** A call to `response.set_cookie`. */
-    private class SetCookieCall extends Http::Server::CookieWrite::Range, DataFlow::MethodCallNode {
+    private class SetCookieCall extends Http::Server::SetCookieCall, DataFlow::MethodCallNode {
       SetCookieCall() { this.calls(instance(), "set_cookie") }
 
       override DataFlow::Node getHeaderArg() { none() }

@@ -592,7 +592,7 @@ module Tornado {
    *
    * See https://www.tornadoweb.org/en/stable/web.html#tornado.web.RequestHandler.set_cookie
    */
-  class TornadoRequestHandlerSetCookieCall extends Http::Server::CookieWrite::Range,
+  class TornadoRequestHandlerSetCookieCall extends Http::Server::SetCookieCall,
     DataFlow::MethodCallNode
   {
     TornadoRequestHandlerSetCookieCall() {

@@ -235,9 +235,7 @@ private module Twisted {
    *
    * See https://twistedmatrix.com/documents/21.2.0/api/twisted.web.http.Request.html#addCookie
    */
-  class TwistedRequestAddCookieCall extends Http::Server::CookieWrite::Range,
-    DataFlow::MethodCallNode
-  {
+  class TwistedRequestAddCookieCall extends Http::Server::SetCookieCall, DataFlow::MethodCallNode {
     TwistedRequestAddCookieCall() { this.calls(Twisted::Request::instance(), "addCookie") }
 
     override DataFlow::Node getHeaderArg() { none() }

@@ -4,10 +4,9 @@
 <qhelp>
 
 <overview>
-<p>Setting the 'secure' flag on a cookie to <code>False</code> can cause it to be sent in cleartext.
-Setting the 'httponly' flag on a cookie to <code>False</code> may allow attackers access it via JavaScript.
-Setting the 'samesite' flag on a cookie to <code>'None'</code> will make the cookie to be sent in third-party 
-contexts which may be attacker-controlled.</p>
+<p>Cookies without the <code>Secure</code> flag set may be transmittd using HTTP instead of HTTPS, which leaves it vulnerable to being read by a third party.</p>
+<p>Cookies without the <code>HttpOnly</code> flag set are accessible to JavaScript running in the same origin. In case of a Cross-Site Scripting (XSS) vulnerability, the cookie can be stolen by a malicious script.</p>
+<p>Cookies with the <code>SameSite</code> attribute set to <code>'None'</code> will be sent with cross-origin requests, which can be controlled by third-party JavaScript code and allow for Cross-Site Request Forgery (CSRF) attacks.</p>
 </overview>
 
 <recommendation>
@@ -18,9 +17,8 @@ contexts which may be attacker-controlled.</p>
 </recommendation>
 
 <example>
-<p>This example shows two ways of adding a cookie to a Flask response. The first way uses <code>set_cookie</code>'s
-secure flag and the second adds the secure flag in the cookie's raw value.</p>
-<sample src="InsecureCookie.py" />
+<p>In the following examples, the cases marked GOOD show secure cookie attributes being set; whereas in the cases marked BAD they are not set.</p>
+<sample src="examples/InsecureCookie.py" />
 </example>
 
 <references>

@@ -0,0 +1,51 @@
+/**
+ * @name Failure to use secure cookies
+ * @description Insecure cookies may be sent in cleartext, which makes them vulnerable to
+ *              interception.
+ * @kind problem
+ * @problem.severity warning
+ * @security-severity 5.0
+ * @precision high
+ * @id py/insecure-cookie
+ * @tags security
+ *       external/cwe/cwe-614
+ *       external/cwe/cwe-1004
+ *       external/cwe/cwe-1275
+ */
+
+import python
+import semmle.python.dataflow.new.DataFlow
+import semmle.python.Concepts
+
+predicate hasProblem(Http::Server::CookieWrite cookie, string alert, int idx) {
+  cookie.hasSecureFlag(false) and
+  alert = "Secure" and
+  idx = 0
+  or
+  cookie.hasHttpOnlyFlag(false) and
+  alert = "HttpOnly" and
+  idx = 1
+  or
+  cookie.hasSameSiteAttribute(any(Http::Server::CookieWrite::SameSiteNone v)) and
+  alert = "SameSite" and
+  idx = 2
+}
+
+predicate hasAlert(Http::Server::CookieWrite cookie, string alert) {
+  exists(int numProblems | numProblems = strictcount(string p | hasProblem(cookie, p, _)) |
+    numProblems = 1 and
+    alert = any(string prob | hasProblem(cookie, prob, _)) + " attribute"
+    or
+    numProblems = 2 and
+    alert =
+      strictconcat(string prob, int idx | hasProblem(cookie, prob, idx) | prob, " and " order by idx)
+        + " attributes"
+    or
+    numProblems = 3 and
+    alert = "Secure, HttpOnly, and SameSite attributes"
+  )
+}
+
+from Http::Server::CookieWrite cookie, string alert
+where hasAlert(cookie, alert)
+select cookie, "Cookie is added without the " + alert + " properly set."
@@ -0,0 +1,20 @@
+from flask import Flask, request, make_response, Response
+
+
+@app.route("/good1")
+def good1():
+    resp = make_response()
+    resp.set_cookie("name", value="value", secure=True, httponly=True, samesite='Strict') # GOOD: Attributes are securely set
+    return resp
+
+
+@app.route("/good2")
+def good2():
+    resp = make_response()
+    resp.headers['Set-Cookie'] = "name=value; Secure; HttpOnly; SameSite=Strict" # GOOD: Attributes are securely set 
+    return resp
+
+@app.route("/bad1")
+    resp = make_response()
+    resp.set_cookie("name", value="value", samesite='None') # BAD: the SameSite attribute is set to 'None' and the 'Secure' and 'HttpOnly' attributes are set to False by default.
+    return resp
@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* The `py/cookie-injection` query, originally contributed to the experimental query pack by @jorgectf, has been promoted to the main query pack. This query finds instances of cookies being set without the `Secure`, `HttpOnly`, or `SameSite` attributes set to secure values.