Merge pull request #10629 from RasmusWL/fix-flask-source

Python: Fix flask request modeling

Author: RasmusWL

python/ql/lib/semmle/python/frameworks/Flask.qll

@@ -354,13 +354,7 @@ module Flask {
    * See https://flask.palletsprojects.com/en/1.1.x/api/#flask.Request
    */
   private class FlaskRequestSource extends RemoteFlowSource::Range {
-    FlaskRequestSource() {
-      this = request().getAValueReachableFromSource() and
-      not any(Import imp).contains(this.asExpr()) and
-      not exists(ControlFlowNode def | this.asVar().getSourceVariable().hasDefiningNode(def) |
-        any(Import imp).contains(def.getNode())
-      )
-    }
+    FlaskRequestSource() { this = request().asSource() }
 
     override string getSourceType() { result = "flask.request" }
   }

python/ql/lib/semmle/python/frameworks/Tornado.qll

@@ -14,10 +14,12 @@ private import semmle.python.frameworks.Stdlib
 private import semmle.python.frameworks.internal.InstanceTaintStepsHelper
 
 /**
+ * INTERNAL: Do not use.
+ *
  * Provides models for the `tornado` PyPI package.
  * See https://www.tornadoweb.org/en/stable/.
  */
-private module Tornado {
+module Tornado {
   /**
    * Provides models for the `tornado.httputil.HTTPHeaders` class
    *
@@ -126,8 +128,7 @@ private module Tornado {
         abstract class InstanceSource extends DataFlow::LocalSourceNode { }
 
         /** The `self` parameter in a method on the `tornado.web.RequestHandler` class or any subclass. */
-        private class SelfParam extends InstanceSource, RemoteFlowSource::Range,
-          DataFlow::ParameterNode {
+        class SelfParam extends InstanceSource, RemoteFlowSource::Range, DataFlow::ParameterNode {
           SelfParam() {
             exists(RequestHandlerClass cls | cls.getAMethod().getArg(0) = this.getParameter())
           }

python/ql/src/change-notes/2022-09-29-flask-source-modeling.md

@@ -0,0 +1,4 @@
+---
+category: fix
+---
+* Fixed how `flask.request` is modeled as a RemoteFlowSource, such that we show fewer duplicated alert messages for Code Scanning alerts. The import, such as `from flask import request`, will now be shown as the first step in a path explanation.

python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll

@@ -1,73 +1,57 @@
 private import python
 private import semmle.python.Concepts
 private import semmle.python.ApiGraphs
-private import semmle.python.dataflow.new.RemoteFlowSources
+private import semmle.python.frameworks.Flask
+private import semmle.python.frameworks.Django
+private import semmle.python.frameworks.Tornado
 
 /**
  * A data flow source of the client ip obtained according to the remote endpoint identifier specified
  * (`X-Forwarded-For`, `X-Real-IP`, `Proxy-Client-IP`, etc.) in the header.
  *
  * For example: `request.headers.get("X-Forwarded-For")`.
+ *
+ * A call to `request.headers.get` or `request.headers.get_all` or `request.headers.getlist`.
  */
-abstract class ClientSuppliedIpUsedInSecurityCheck extends DataFlow::CallCfgNode { }
+abstract class ClientSuppliedIpUsedInSecurityCheck extends DataFlow::Node { }
 
-private class FlaskClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck {
+private class FlaskClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck,
+  DataFlow::MethodCallNode {
   FlaskClientSuppliedIpUsedInSecurityCheck() {
-    exists(RemoteFlowSource rfs, DataFlow::AttrRead get |
-      rfs.getSourceType() = "flask.request" and this.getFunction() = get
-    |
-      // `get` is a call to request.headers.get or request.headers.get_all or request.headers.getlist
-      // request.headers
-      get.getObject()
-          .(DataFlow::AttrRead)
-          // request
-          .getObject()
-          .getALocalSource() = rfs and
-      get.getAttributeName() in ["get", "get_all", "getlist"] and
-      get.getObject().(DataFlow::AttrRead).getAttributeName() = "headers" and
-      this.getArg(0).asExpr().(StrConst).getText().toLowerCase() = clientIpParameterName()
-    )
+    this = Flask::request().getMember("headers").getMember(["get", "get_all", "getlist"]).getACall() and
+    this.getArg(0).asExpr().(StrConst).getText().toLowerCase() = clientIpParameterName()
   }
 }
 
-private class DjangoClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck {
+private class DjangoClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck,
+  DataFlow::MethodCallNode {
   DjangoClientSuppliedIpUsedInSecurityCheck() {
-    exists(RemoteFlowSource rfs, DataFlow::AttrRead get |
-      rfs.getSourceType() = "django.http.request.HttpRequest" and this.getFunction() = get
-    |
-      // `get` is a call to request.headers.get or request.META.get
-      // request.headers
-      get.getObject()
-          .(DataFlow::AttrRead)
-          // request
-          .getObject()
-          .getALocalSource() = rfs and
-      get.getAttributeName() = "get" and
-      get.getObject().(DataFlow::AttrRead).getAttributeName() in ["headers", "META"] and
-      this.getArg(0).asExpr().(StrConst).getText().toLowerCase() = clientIpParameterName()
-    )
+    exists(DataFlow::Node req, DataFlow::AttrRead headers |
+      // a call to request.headers.get or request.META.get
+      req = PrivateDjango::DjangoImpl::DjangoHttp::Request::HttpRequest::instance() and
+      headers.getObject().getALocalSource() = req and
+      headers.getAttributeName() in ["headers", "META"] and
+      this.calls(headers, "get")
+    ) and
+    this.getArg(0).asExpr().(StrConst).getText().toLowerCase() = clientIpParameterName()
   }
 }
 
-private class TornadoClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck {
+private class TornadoClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck,
+  DataFlow::MethodCallNode {
   TornadoClientSuppliedIpUsedInSecurityCheck() {
-    exists(RemoteFlowSource rfs, DataFlow::AttrRead get |
-      rfs.getSourceType() = "tornado.web.RequestHandler" and this.getFunction() = get
+    // a call to self.request.headers.get or self.request.headers.get_list inside a tornado requesthandler
+    exists(
+      Tornado::TornadoModule::Web::RequestHandler::SelfParam selfParam, DataFlow::AttrRead headers,
+      DataFlow::AttrRead req
     |
-      // `get` is a call to `rfs`.request.headers.get
-      // `rfs`.request.headers
-      get.getObject()
-          .(DataFlow::AttrRead)
-          // `rfs`.request
-          .getObject()
-          .(DataFlow::AttrRead)
-          // `rfs`
-          .getObject()
-          .getALocalSource() = rfs and
-      get.getAttributeName() in ["get", "get_list"] and
-      get.getObject().(DataFlow::AttrRead).getAttributeName() = "headers" and
-      this.getArg(0).asExpr().(StrConst).getText().toLowerCase() = clientIpParameterName()
-    )
+      req.getObject().getALocalSource() = selfParam and
+      req.getAttributeName() = "request" and
+      headers.getObject().getALocalSource() = req and
+      headers.getAttributeName() = "headers" and
+      this.calls(headers, ["get", "get_list"])
+    ) and
+    this.getArg(0).asExpr().(StrConst).getText().toLowerCase() = clientIpParameterName()
   }
 }
 

python/ql/src/meta/alerts/RemoteFlowSourcesReach.ql

@@ -25,28 +25,15 @@ class RemoteFlowSourceReach extends TaintTracking::Configuration {
   }
 
   override predicate isSink(DataFlow::Node node) {
-    not node.getLocation().getFile() instanceof IgnoredFile and
-    (
-      node instanceof RemoteFlowSource
-      or
-      this.isAdditionalFlowStep(_, node)
-    ) and
-    // In september 2021 we changed how we do taint-propagation for method calls (mostly
-    // relating to modeled frameworks/libraries). We used to do `obj -> obj.meth` and
-    // `obj.meth -> obj.meth()` in two separate steps, and now do them in one
-    // `obj -> obj.meth()`. To be able to compare the overall reach between these two
-    // version, we don't want this query to alert us to the fact that we no longer taint
-    // the node in the middle (since that is just noise).
-    // see https://github.com/github/codeql/pull/6349
+    not node.getLocation().getFile() instanceof IgnoredFile
+    // We could try to reduce the number of sinks in this configuration, by only
+    // allowing something that is on one end of a localFlowStep, readStep or storeStep,
+    // however, it's a brittle solution that requires us to remember to update this file
+    // if/when adding something new to the data-flow library.
     //
-    // We should be able to remove the following few lines of code once we don't care to
-    // compare with the old (before September 2021) way of doing taint-propagation for
-    // method calls.
-    not exists(DataFlow::MethodCallNode c |
-      node = c.getFunction() and
-      this.isAdditionalFlowStep(c.getObject(), node) and
-      this.isAdditionalFlowStep(node, c)
-    )
+    // From testing on a few projects, trying to reduce the number of nodes, we only
+    // gain a reduction in the range of 40%, and while that's nice, it doesn't seem
+    // worth it to me for a meta query.
   }
 }