Merge branch 'main' into py-last-msg

python/ql/src/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 0.5.1
+
+No user-facing changes.
+
 ## 0.5.0
 
 ### Query Metadata Changes

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/change-notes/released/0.5.1.md

@@ -0,0 +1,3 @@
+## 0.5.1
+
+No user-facing changes.

python/ql/src/codeql-pack.release.yml

@@ -1,2 +1,2 @@
 ---
-lastReleaseVersion: 0.5.0
+lastReleaseVersion: 0.5.1

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.
   }
 }
 

python/ql/src/qlpack.yml

@@ -1,5 +1,5 @@
 name: codeql/python-queries
-version: 0.5.1-dev
+version: 0.5.2-dev
 groups: 
   - python
   - queries