Merge pull request #16105 from joefarebrother/python-promote-header-injection

Python: Promote Header Injection query from experimental

python/ql/src/Security/CWE-113/HeaderInjection.qhelp

@@ -0,0 +1,41 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>Directly writing user input (for example, an HTTP request parameter) to an HTTP header
+can lead to an HTTP response-splitting vulnerability.</p>
+
+<p>If user-controlled input is used in an HTTP header that allows line break characters, an attacker can 
+inject additional headers or control the response body, leading to vulnerabilities such as XSS or cache poisoning.
+</p>
+
+</overview>
+
+<recommendation>
+<p>Ensure that user input containing line break characters is not written to an HTTP header.</p>
+</recommendation>
+
+<example>
+<p>In the following example, the case marked BAD writes user input to the header name. 
+In the GOOD case, input is first escaped to not contain any line break characters.</p>
+<sample src="examples/header_injection.py" />
+</example>
+
+<references>
+<li>
+SecLists.org: <a href="https://seclists.org/bugtraq/2005/Apr/187">HTTP response splitting</a>.
+</li>
+<li>
+OWASP:
+<a href="https://www.owasp.org/index.php/HTTP_Response_Splitting">HTTP Response Splitting</a>.
+</li>
+<li>
+Wikipedia: <a href="http://en.wikipedia.org/wiki/HTTP_response_splitting">HTTP response splitting</a>.
+</li>
+<li>
+CAPEC: <a href="https://capec.mitre.org/data/definitions/105.html">CAPEC-105: HTTP Request Splitting</a>
+</li>
+</references>
+</qhelp>

python/ql/src/Security/CWE-113/HeaderInjection.ql

@@ -1,19 +1,19 @@
 /**
- * @name HTTP Header Injection
- * @description User input should not be used in HTTP headers, otherwise a malicious user
- *              may be able to inject a value that could manipulate the response.
+ * @name HTTP Response Splitting
+ * @description Writing user input directly to an HTTP header
+ *              makes code vulnerable to attack by header splitting.
  * @kind path-problem
  * @problem.severity error
- * @id py/header-injection
+ * @security-severity 6.1
+ * @precision high
+ * @id py/http-response-splitting
  * @tags security
- *       experimental
  *       external/cwe/cwe-113
  *       external/cwe/cwe-079
  */
 
-// determine precision above
 import python
-import experimental.semmle.python.security.injection.HTTPHeaders
+import semmle.python.security.dataflow.HttpHeaderInjectionQuery
 import HeaderInjectionFlow::PathGraph
 
 from HeaderInjectionFlow::PathNode source, HeaderInjectionFlow::PathNode sink

python/ql/src/Security/CWE-113/examples/header_injection.py

@@ -0,0 +1,17 @@
+@app.route("/example_bad")
+def example_bad():
+    rfs_header = request.args["rfs_header"]
+    response = Response()
+    custom_header = "X-MyHeader-" + rfs_header
+    # BAD: User input is used as part of the header name.
+    response.headers[custom_header] = "HeaderValue" 
+    return response
+
+@app.route("/example_good")
+def example_bad():
+    rfs_header = request.args["rfs_header"]
+    response = Response()
+    custom_header = "X-MyHeader-" + rfs_header.replace("\n", "").replace("\r","").replace(":","")
+    # GOOD: Line break characters are removed from the input.
+    response.headers[custom_header] = "HeaderValue" 
+    return response

python/ql/src/change-notes/2024-04-08-header-injection-promotion.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* The `py/header-injection` query, originally contributed to the experimental query pack by @jorgectf, has been promoted to the main query pack and renamed to `py/http-response-splitting`. This query finds instances of http header injection / response splitting vulnerabilities.

python/ql/src/experimental/Security/CWE-113/HeaderInjection.qhelp

@@ -1,26 +0,0 @@
-<!DOCTYPE qhelp PUBLIC
-  "-//Semmle//qhelp//EN"
-  "qhelp.dtd">
-<qhelp>
-<overview>
-<p>If an HTTP Header is built using string concatenation or string formatting, and the
-components of the concatenation include user input, a user 
-is likely to be able to manipulate the response.</p>
-</overview>
-
-<recommendation>
-<p>User input should not be included in an HTTP Header.</p>
-</recommendation>
-
-<example>
-<p>In the following example, the code appends a user-provided value into a header.</p>
-
-<sample src="header_injection.py" />
-</example>
-
-<references>
-<li>OWASP: <a href="https://owasp.org/www-community/attacks/HTTP_Response_Splitting">HTTP Response Splitting</a>.</li>
-<li>Python Security: <a href="https://python-security.readthedocs.io/vuln/http-header-injection.html">HTTP header injection</a>.</li>
-<li>SonarSource: <a href="https://rules.sonarsource.com/python/RSPEC-5167">RSPEC-5167</a>.</li>
-</references>
-</qhelp>

python/ql/src/experimental/Security/CWE-113/header_injection.py

@@ -1,9 +0,0 @@
-from flask import Response, request, Flask, make_response
-
-
-@app.route("/flask_Response")
-def flask_Response():
-    rfs_header = request.args["rfs_header"]
-    response = Response()
-    response.headers['HeaderName'] = rfs_header
-    return response

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

@@ -216,45 +216,6 @@ class SqlEscape extends DataFlow::Node instanceof SqlEscape::Range {
   DataFlow::Node getAnInput() { result = super.getAnInput() }
 }
 
-/** Provides classes for modeling HTTP Header APIs. */
-module HeaderDeclaration {
-  /**
-   * A data-flow node that collects functions setting HTTP Headers.
-   *
-   * Extend this class to model new APIs. If you want to refine existing API models,
-   * extend `HeaderDeclaration` instead.
-   */
-  abstract class Range extends DataFlow::Node {
-    /**
-     * Gets the argument containing the header name.
-     */
-    abstract DataFlow::Node getNameArg();
-
-    /**
-     * Gets the argument containing the header value.
-     */
-    abstract DataFlow::Node getValueArg();
-  }
-}
-
-/**
- * A data-flow node that collects functions setting HTTP Headers.
- *
- * Extend this class to refine existing API models. If you want to model new APIs,
- * extend `HeaderDeclaration::Range` instead.
- */
-class HeaderDeclaration extends DataFlow::Node instanceof HeaderDeclaration::Range {
-  /**
-   * Gets the argument containing the header name.
-   */
-  DataFlow::Node getNameArg() { result = super.getNameArg() }
-
-  /**
-   * Gets the argument containing the header value.
-   */
-  DataFlow::Node getValueArg() { result = super.getValueArg() }
-}
-
 /** Provides classes for modeling Csv writer APIs. */
 module CsvWriter {
   /**

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

@@ -6,6 +6,7 @@ import python
 import semmle.python.dataflow.new.DataFlow
 import semmle.python.dataflow.new.TaintTracking
 import experimental.semmle.python.Concepts
+import semmle.python.Concepts
 
 /**
  * Gets a header setting a cookie.
@@ -26,13 +27,13 @@ import experimental.semmle.python.Concepts
  * * `isSameSite()` predicate would fail.
  * * `getName()` and `getValue()` results would be `"name=value; Secure;"`.
  */
-class CookieHeader extends Cookie::Range instanceof HeaderDeclaration {
+class CookieHeader extends Cookie::Range instanceof Http::Server::ResponseHeaderWrite {
   CookieHeader() {
     exists(StringLiteral str |
       str.getText() = "Set-Cookie" and
       DataFlow::exprNode(str)
           .(DataFlow::LocalSourceNode)
-          .flowsTo(this.(HeaderDeclaration).getNameArg())
+          .flowsTo(this.(Http::Server::ResponseHeaderWrite).getNameArg())
     )
   }
 
@@ -41,7 +42,7 @@ class CookieHeader extends Cookie::Range instanceof HeaderDeclaration {
       str.getText().regexpMatch(".*; *Secure;.*") and
       DataFlow::exprNode(str)
           .(DataFlow::LocalSourceNode)
-          .flowsTo(this.(HeaderDeclaration).getValueArg())
+          .flowsTo(this.(Http::Server::ResponseHeaderWrite).getValueArg())
     )
   }
 
@@ -50,7 +51,7 @@ class CookieHeader extends Cookie::Range instanceof HeaderDeclaration {
       str.getText().regexpMatch(".*; *HttpOnly;.*") and
       DataFlow::exprNode(str)
           .(DataFlow::LocalSourceNode)
-          .flowsTo(this.(HeaderDeclaration).getValueArg())
+          .flowsTo(this.(Http::Server::ResponseHeaderWrite).getValueArg())
     )
   }
 
@@ -59,13 +60,17 @@ class CookieHeader extends Cookie::Range instanceof HeaderDeclaration {
       str.getText().regexpMatch(".*; *SameSite=(Strict|Lax);.*") and
       DataFlow::exprNode(str)
           .(DataFlow::LocalSourceNode)
-          .flowsTo(this.(HeaderDeclaration).getValueArg())
+          .flowsTo(this.(Http::Server::ResponseHeaderWrite).getValueArg())
     )
   }
 
-  override DataFlow::Node getNameArg() { result = this.(HeaderDeclaration).getValueArg() }
+  override DataFlow::Node getNameArg() {
+    result = this.(Http::Server::ResponseHeaderWrite).getValueArg()
+  }
 
-  override DataFlow::Node getValueArg() { result = this.(HeaderDeclaration).getValueArg() }
+  override DataFlow::Node getValueArg() {
+    result = this.(Http::Server::ResponseHeaderWrite).getValueArg()
+  }
 
   override DataFlow::Node getHeaderArg() { none() }
 }

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

@@ -5,7 +5,6 @@
 private import experimental.semmle.python.frameworks.Stdlib
 private import experimental.semmle.python.frameworks.Flask
 private import experimental.semmle.python.frameworks.Django
-private import experimental.semmle.python.frameworks.Werkzeug
 private import experimental.semmle.python.frameworks.LDAP
 private import experimental.semmle.python.frameworks.JWT
 private import experimental.semmle.python.frameworks.Csv

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

@@ -88,31 +88,6 @@ private module ExperimentalPrivateDjango {
             result = baseClassRef().getReturn().getAMember()
           }
 
-          class DjangoResponseSetItemCall extends DataFlow::CallCfgNode, HeaderDeclaration::Range {
-            DjangoResponseSetItemCall() {
-              this = baseClassRef().getReturn().getMember("__setitem__").getACall()
-            }
-
-            override DataFlow::Node getNameArg() { result = this.getArg(0) }
-
-            override DataFlow::Node getValueArg() { result = this.getArg(1) }
-          }
-
-          class DjangoResponseDefinition extends DataFlow::Node, HeaderDeclaration::Range {
-            DataFlow::Node headerInput;
-
-            DjangoResponseDefinition() {
-              headerInput = headerInstance().asSink() and
-              headerInput.asCfgNode() = this.asCfgNode().(DefinitionNode).getValue()
-            }
-
-            override DataFlow::Node getNameArg() {
-              result.asExpr() = this.asExpr().(Subscript).getIndex()
-            }
-
-            override DataFlow::Node getValueArg() { result = headerInput }
-          }
-
           /**
            * Gets a call to `set_cookie()`.
            *

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

@@ -11,76 +11,6 @@ private import semmle.python.ApiGraphs
 private import semmle.python.frameworks.Flask
 
 module ExperimentalFlask {
-  /**
-   * A reference to either `flask.make_response` function, or the `make_response` method on
-   * an instance of `flask.Flask`. This creates an instance of the `flask_response`
-   * class (class-attribute on a flask application), which by default is
-   * `flask.Response`.
-   *
-   * See
-   * - https://flask.palletsprojects.com/en/1.1.x/api/#flask.Flask.make_response
-   * - https://flask.palletsprojects.com/en/1.1.x/api/#flask.make_response
-   */
-  private API::Node flaskMakeResponse() {
-    result =
-      [API::moduleImport("flask"), Flask::FlaskApp::instance()]
-          .getMember(["make_response", "jsonify", "make_default_options_response"])
-  }
-
-  /** Gets a reference to a header instance. */
-  private DataFlow::LocalSourceNode headerInstance() {
-    result =
-      [Flask::Response::classRef(), flaskMakeResponse()]
-          .getReturn()
-          .getAMember()
-          .getAValueReachableFromSource()
-  }
-
-  /** Gets a reference to a header instance call/subscript */
-  private DataFlow::Node headerInstanceCall() {
-    headerInstance() in [result.(DataFlow::AttrRead), result.(DataFlow::AttrRead).getObject()] or
-    headerInstance().asExpr() = result.asExpr().(Subscript).getObject()
-  }
-
-  class FlaskHeaderDefinition extends DataFlow::Node, HeaderDeclaration::Range {
-    DataFlow::Node headerInput;
-
-    FlaskHeaderDefinition() {
-      this.asCfgNode().(DefinitionNode) = headerInstanceCall().asCfgNode() and
-      headerInput.asCfgNode() = this.asCfgNode().(DefinitionNode).getValue()
-    }
-
-    override DataFlow::Node getNameArg() { result.asExpr() = this.asExpr().(Subscript).getIndex() }
-
-    override DataFlow::Node getValueArg() { result = headerInput }
-  }
-
-  private class FlaskMakeResponseExtend extends DataFlow::CallCfgNode, HeaderDeclaration::Range {
-    KeyValuePair item;
-
-    FlaskMakeResponseExtend() {
-      this.getFunction() = headerInstanceCall() and
-      item = this.getArg(_).asExpr().(Dict).getAnItem()
-    }
-
-    override DataFlow::Node getNameArg() { result.asExpr() = item.getKey() }
-
-    override DataFlow::Node getValueArg() { result.asExpr() = item.getValue() }
-  }
-
-  private class FlaskResponse extends DataFlow::CallCfgNode, HeaderDeclaration::Range {
-    KeyValuePair item;
-
-    FlaskResponse() {
-      this = Flask::Response::classRef().getACall() and
-      item = this.getArg(_).asExpr().(Dict).getAnItem()
-    }
-
-    override DataFlow::Node getNameArg() { result.asExpr() = item.getKey() }
-
-    override DataFlow::Node getValueArg() { result.asExpr() = item.getValue() }
-  }
-
   /**
    * Gets a call to `set_cookie()`.
    *

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

@@ -1,33 +0,0 @@
-/**
- * Provides classes modeling security-relevant aspects of the `Werkzeug` PyPI package.
- * See
- * - https://pypi.org/project/Werkzeug/
- * - https://werkzeug.palletsprojects.com/en/1.0.x/#werkzeug
- */
-
-private import python
-private import semmle.python.frameworks.Flask
-private import semmle.python.dataflow.new.DataFlow
-private import experimental.semmle.python.Concepts
-private import semmle.python.ApiGraphs
-
-private module Werkzeug {
-  module Datastructures {
-    module Headers {
-      class WerkzeugHeaderAddCall extends DataFlow::CallCfgNode, HeaderDeclaration::Range {
-        WerkzeugHeaderAddCall() {
-          this.getFunction().(DataFlow::AttrRead).getObject().getALocalSource() =
-            API::moduleImport("werkzeug")
-                .getMember("datastructures")
-                .getMember("Headers")
-                .getACall() and
-          this.getFunction().(DataFlow::AttrRead).getAttributeName() = "add"
-        }
-
-        override DataFlow::Node getNameArg() { result = this.getArg(0) }
-
-        override DataFlow::Node getValueArg() { result = this.getArg(1) }
-      }
-    }
-  }
-}

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

@@ -1,21 +0,0 @@
-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
-
-/**
- * A taint-tracking configuration for detecting HTTP Header injections.
- */
-private module HeaderInjectionConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
-
-  predicate isSink(DataFlow::Node sink) {
-    exists(HeaderDeclaration headerDeclaration |
-      sink in [headerDeclaration.getNameArg(), headerDeclaration.getValueArg()]
-    )
-  }
-}
-
-/** Global taint-tracking for detecting "HTTP Header injection" vulnerabilities. */
-module HeaderInjectionFlow = TaintTracking::Global<HeaderInjectionConfig>;