Merge pull request #6360 from jorgectf/jorgectf/python/insecure-cookie

Python: Add cookie security-related queries

Author: RasmusWL

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

@@ -0,0 +1,16 @@
+from flask import request, make_response
+
+
+@app.route("/1")
+def true():
+    resp = make_response()
+    resp.set_cookie(request.args["name"],
+                    value=request.args["name"])
+    return resp
+
+
+@app.route("/2")
+def flask_make_response():
+    resp = make_response("hello")
+    resp.headers['Set-Cookie'] = f"{request.args['name']}={request.args['name']};"
+    return resp

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

@@ -0,0 +1,28 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>Constructing cookies from user input may allow an attacker to perform a Cookie Poisoning attack. 
+It is possible, however, to perform other parameter-like attacks through cookie poisoning techniques, 
+such as SQL Injection, Directory Traversal, or Stealth Commanding, etc. Additionally, 
+cookie injection may relate to attempts to perform Access of Administrative Interface.
+</p>
+</overview>
+
+<recommendation>
+<p>Do not use raw user input to construct cookies.</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
+and the second sets a cookie's raw value through a header, both using user-supplied input.</p>
+<sample src="CookieInjection.py" />
+</example>
+
+<references>
+<li>Imperva: <a href="https://docs.imperva.com/bundle/on-premises-knowledgebase-reference-guide/page/cookie_injection.htm">Cookie injection</a>.</li>
+</references>
+
+</qhelp>

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

@@ -0,0 +1,28 @@
+/**
+ * @name Construction of a cookie using user-supplied input.
+ * @description Constructing cookies from user input may allow an attacker to perform a Cookie Poisoning attack.
+ * @kind path-problem
+ * @problem.severity error
+ * @id py/cookie-injection
+ * @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
+import DataFlow::PathGraph
+
+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 its " + sink.getNode().(CookieSink).getFlag() + " flag is not properly set."
+  else insecure = "."
+select sink.getNode(), source, sink, "Cookie is constructed from a $@" + insecure, source.getNode(),
+  "user-supplied input"

python/ql/src/experimental/Security/CWE-614/InsecureCookie.py

@@ -0,0 +1,15 @@
+from flask import Flask, request, make_response, Response
+
+
+@app.route("/1")
+def true():
+    resp = make_response()
+    resp.set_cookie("name", value="value", secure=True)
+    return resp
+
+
+@app.route("/2")
+def flask_make_response():
+    resp = make_response("hello")
+    resp.headers['Set-Cookie'] = "name=value; Secure;"
+    return resp

python/ql/src/experimental/Security/CWE-614/InsecureCookie.qhelp

@@ -0,0 +1,31 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<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>
+</overview>
+
+<recommendation>
+<p>Always set <code>secure</code> to <code>True</code> or add "; Secure;" to the cookie's raw value.</p>
+<p>Always set <code>httponly</code> to <code>True</code> or add "; HttpOnly;" to the cookie's raw value.</p>
+<p>Always set <code>samesite</code> to <code>Lax</code> or <code>Strict</code>, or add "; SameSite=Lax;", or
+"; Samesite=Strict;" to the cookie's raw header value.</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" />
+</example>
+
+<references>
+<li>Detectify: <a href="https://support.detectify.com/support/solutions/articles/48001048982-cookie-lack-secure-flag">Cookie lack Secure flag</a>.</li>
+<li>PortSwigger: <a href="https://portswigger.net/kb/issues/00500200_tls-cookie-without-secure-flag-set">TLS cookie without secure flag set</a>.</li>
+</references>
+
+</qhelp>

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

@@ -0,0 +1,30 @@
+/**
+ * @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
+ * @security-severity 5.0
+ * @precision ???
+ * @id py/insecure-cookie
+ * @tags security
+ *       external/cwe/cwe-614
+ */
+
+// TODO: determine precision above
+import python
+import semmle.python.dataflow.new.DataFlow
+import experimental.semmle.python.Concepts
+import experimental.semmle.python.CookieHeader
+
+from Cookie cookie, string alert
+where
+  not cookie.isSecure() and
+  alert = "secure"
+  or
+  not cookie.isHttpOnly() and
+  alert = "httponly"
+  or
+  not cookie.isSameSite() and
+  alert = "samesite"
+select cookie, "Cookie is added without the '" + alert + "' flag properly set."

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

@@ -13,6 +13,7 @@ private import semmle.python.dataflow.new.DataFlow
 private import semmle.python.dataflow.new.RemoteFlowSources
 private import semmle.python.dataflow.new.TaintTracking
 private import experimental.semmle.python.Frameworks
+private import semmle.python.Concepts
 
 /** Provides classes for modeling copying file related APIs. */
 module CopyFile {
@@ -370,6 +371,55 @@ class HeaderDeclaration extends DataFlow::Node {
   DataFlow::Node getValueArg() { result = range.getValueArg() }
 }
 
+/**
+ * A data-flow node that sets a cookie in an HTTP response.
+ *
+ * Extend this class to refine existing API models. If you want to model new APIs,
+ * extend `Cookie::Range` instead.
+ */
+class Cookie extends HTTP::Server::CookieWrite instanceof Cookie::Range {
+  /**
+   * Holds if this cookie is secure.
+   */
+  predicate isSecure() { super.isSecure() }
+
+  /**
+   * Holds if this cookie is HttpOnly.
+   */
+  predicate isHttpOnly() { super.isHttpOnly() }
+
+  /**
+   * Holds if the cookie is SameSite
+   */
+  predicate isSameSite() { super.isSameSite() }
+}
+
+/** Provides a class for modeling new cookie writes on HTTP responses. */
+module Cookie {
+  /**
+   * A data-flow node that sets a cookie in an HTTP response.
+   *
+   * Extend this class to model new APIs. If you want to refine existing API models,
+   * extend `Cookie` instead.
+   */
+  abstract class Range extends HTTP::Server::CookieWrite::Range {
+    /**
+     * Holds if this cookie is secure.
+     */
+    abstract predicate isSecure();
+
+    /**
+     * Holds if this cookie is HttpOnly.
+     */
+    abstract predicate isHttpOnly();
+
+    /**
+     * Holds if the cookie is SameSite.
+     */
+    abstract predicate isSameSite();
+  }
+}
+
 /** Provides classes for modeling JWT encoding-related APIs. */
 module JwtEncoding {
   /**

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

@@ -0,0 +1,72 @@
+/**
+ * Temporary: provides a class to extend current cookies to header declarations
+ */
+
+import python
+import semmle.python.dataflow.new.DataFlow
+import semmle.python.dataflow.new.TaintTracking
+import experimental.semmle.python.Concepts
+
+/**
+ * Gets a header setting a cookie.
+ *
+ * Given the following example:
+ *
+ * ```py
+ * @app.route("/")
+ * def flask_make_response():
+ *    resp = make_response("")
+ *    resp.headers['Set-Cookie'] = "name=value; Secure;"
+ *    return resp
+ * ```
+ *
+ * * `this` would be `resp.headers['Set-Cookie'] = "name=value; Secure;"`.
+ * * `isSecure()` predicate would succeed.
+ * * `isHttpOnly()` predicate would fail.
+ * * `isSameSite()` predicate would fail.
+ * * `getName()` and `getValue()` results would be `"name=value; Secure;"`.
+ */
+class CookieHeader extends Cookie::Range instanceof HeaderDeclaration {
+  CookieHeader() {
+    this instanceof HeaderDeclaration and
+    exists(StrConst str |
+      str.getText() = "Set-Cookie" and
+      DataFlow::exprNode(str)
+          .(DataFlow::LocalSourceNode)
+          .flowsTo(this.(HeaderDeclaration).getNameArg())
+    )
+  }
+
+  override predicate isSecure() {
+    exists(StrConst str |
+      str.getText().regexpMatch(".*; *Secure;.*") and
+      DataFlow::exprNode(str)
+          .(DataFlow::LocalSourceNode)
+          .flowsTo(this.(HeaderDeclaration).getValueArg())
+    )
+  }
+
+  override predicate isHttpOnly() {
+    exists(StrConst str |
+      str.getText().regexpMatch(".*; *HttpOnly;.*") and
+      DataFlow::exprNode(str)
+          .(DataFlow::LocalSourceNode)
+          .flowsTo(this.(HeaderDeclaration).getValueArg())
+    )
+  }
+
+  override predicate isSameSite() {
+    exists(StrConst str |
+      str.getText().regexpMatch(".*; *SameSite=(Strict|Lax);.*") and
+      DataFlow::exprNode(str)
+          .(DataFlow::LocalSourceNode)
+          .flowsTo(this.(HeaderDeclaration).getValueArg())
+    )
+  }
+
+  override DataFlow::Node getNameArg() { result = this.(HeaderDeclaration).getValueArg() }
+
+  override DataFlow::Node getValueArg() { result = this.(HeaderDeclaration).getValueArg() }
+
+  override DataFlow::Node getHeaderArg() { none() }
+}