Python: Model (most of) twisted

RasmusWL · RasmusWL · commit a21039170b42 · 2021-06-08T16:11:18.000+02:00
diff --git a/docs/codeql/support/reusables/frameworks.rst b/docs/codeql/support/reusables/frameworks.rst
@@ -155,6 +155,7 @@ Python built-in support
    Django, Web framework
    Flask, Web framework
    Tornado, Web framework
+   Twisted, Web framework
    PyYAML, Serialization
    dill, Serialization
    simplejson, Serialization
diff --git a/python/ql/src/semmle/python/Frameworks.qll b/python/ql/src/semmle/python/Frameworks.qll
@@ -19,5 +19,6 @@ private import semmle.python.frameworks.PyMySQL
 private import semmle.python.frameworks.Simplejson
 private import semmle.python.frameworks.Stdlib
 private import semmle.python.frameworks.Tornado
+private import semmle.python.frameworks.Twisted
 private import semmle.python.frameworks.Ujson
 private import semmle.python.frameworks.Yaml
diff --git a/python/ql/src/semmle/python/frameworks/Twisted.qll b/python/ql/src/semmle/python/frameworks/Twisted.qll
@@ -0,0 +1,229 @@
+/**
+ * Provides classes modeling security-relevant aspects of the `twisted` PyPI package.
+ * See https://twistedmatrix.com/.
+ */
+
+private import python
+private import semmle.python.dataflow.new.DataFlow
+private import semmle.python.dataflow.new.RemoteFlowSources
+private import semmle.python.dataflow.new.TaintTracking
+private import semmle.python.Concepts
+private import semmle.python.ApiGraphs
+
+/**
+ * Provides models for the `twisted` PyPI package.
+ * See https://twistedmatrix.com/.
+ */
+private module Twisted {
+  // ---------------------------------------------------------------------------
+  // request handler modeling
+  // ---------------------------------------------------------------------------
+  /**
+   * A class that is a subclass of `twisted.web.resource.Resource`, thereby
+   * being able to handle HTTP requests.
+   *
+   * See https://twistedmatrix.com/documents/21.2.0/api/twisted.web.resource.Resource.html
+   */
+  class TwistedResourceSubclass extends Class {
+    TwistedResourceSubclass() {
+      this.getABase() =
+        API::moduleImport("twisted")
+            .getMember("web")
+            .getMember("resource")
+            .getMember("Resource")
+            .getASubclass*()
+            .getAUse()
+            .asExpr()
+    }
+
+    /** Gets a function that could handle incoming requests, if any. */
+    Function getARequestHandler() {
+      // TODO: This doesn't handle attribute assignment. Should be OK, but analysis is not as complete as with
+      // points-to and `.lookup`, which would handle `post = my_post_handler` inside class def
+      result = this.getAMethod() and
+      resourceMethodRequestParamIndex(result.getName(), _)
+    }
+  }
+
+  /**
+   * Holds if the request parameter is supposed to be at index `requestParamIndex` for
+   * the method named `methodName` in `twisted.web.resource.Resource`.
+   */
+  bindingset[methodName]
+  private predicate resourceMethodRequestParamIndex(string methodName, int requestParamIndex) {
+    methodName.matches("render_%") and requestParamIndex = 1
+    or
+    methodName in ["render", "listDynamicEntities", "getChildForRequest"] and requestParamIndex = 1
+    or
+    methodName = ["getDynamicEntity", "getChild", "getChildWithDefault"] and requestParamIndex = 2
+  }
+
+  /** A method that handles incoming requests, on a `twisted.web.resource.Resource` subclass. */
+  class TwistedResourceRequestHandler extends HTTP::Server::RequestHandler::Range {
+    TwistedResourceRequestHandler() {
+      any(TwistedResourceSubclass cls).getAMethod() = this and
+      resourceMethodRequestParamIndex(this.getName(), _)
+    }
+
+    Parameter getRequestParameter() {
+      exists(int i |
+        resourceMethodRequestParamIndex(this.getName(), i) and
+        result = this.getArg(i)
+      )
+    }
+
+    override Parameter getARoutedParameter() { none() }
+
+    override string getFramework() { result = "twisted" }
+  }
+
+  /**
+   * A "render" method on a `twisted.web.resource.Resource` subclass, whose return value
+   * is written as the body fo the HTTP response.
+   */
+  class TwistedResourceRenderMethod extends TwistedResourceRequestHandler {
+    TwistedResourceRenderMethod() {
+      this.getName() = "render" or this.getName().matches("render_%")
+    }
+  }
+
+  // ---------------------------------------------------------------------------
+  // request modeling
+  // ---------------------------------------------------------------------------
+  /**
+   * Provides models for the `twisted.web.server.Request` class
+   *
+   * See https://twistedmatrix.com/documents/21.2.0/api/twisted.web.server.Request.html
+   */
+  module Request {
+    /**
+     * A source of instances of `twisted.web.server.Request`, extend this class to model new instances.
+     *
+     * This can include instantiations of the class, return values from function
+     * calls, or a special parameter that will be set when functions are called by an external
+     * library.
+     *
+     * Use `Request::instance()` predicate to get
+     * references to instances of `twisted.web.server.Request`.
+     */
+    abstract class InstanceSource extends DataFlow::LocalSourceNode { }
+
+    /** Gets a reference to an instance of `twisted.web.server.Request`. */
+    private DataFlow::LocalSourceNode instance(DataFlow::TypeTracker t) {
+      t.start() and
+      result instanceof InstanceSource
+      or
+      exists(DataFlow::TypeTracker t2 | result = instance(t2).track(t2, t))
+    }
+
+    /** Gets a reference to an instance of `twisted.web.server.Request`. */
+    DataFlow::Node instance() { instance(DataFlow::TypeTracker::end()).flowsTo(result) }
+  }
+
+  /**
+   * A parameter that will receive a `twisted.web.server.Request` instance,
+   * when a twisted request handler is called.
+   */
+  class TwistedResourceRequestHandlerRequestParam extends RemoteFlowSource::Range,
+    Request::InstanceSource, DataFlow::ParameterNode {
+    TwistedResourceRequestHandlerRequestParam() {
+      this.getParameter() = any(TwistedResourceRequestHandler handler).getRequestParameter()
+    }
+
+    override string getSourceType() { result = "twisted.web.server.Request" }
+  }
+
+  /**
+   * Taint propagation for `twisted.web.server.Request`.
+   */
+  private class TwistedRequestAdditionalTaintStep extends TaintTracking::AdditionalTaintStep {
+    override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+      // Methods
+      //
+      // TODO: When we have tools that make it easy, model these properly to handle
+      // `meth = obj.meth; meth()`. Until then, we'll use this more syntactic approach
+      // (since it allows us to at least capture the most common cases).
+      nodeFrom = Request::instance() and
+      exists(DataFlow::AttrRead attr | attr.getObject() = nodeFrom |
+        // normal (non-async) methods
+        attr.getAttributeName() in [
+            "getCookie", "getHeader", "getAllHeaders", "getUser", "getPassword", "getHost",
+            "getRequestHostname"
+          ] and
+        nodeTo.(DataFlow::CallCfgNode).getFunction() = attr
+      )
+      or
+      // Attributes
+      nodeFrom = Request::instance() and
+      nodeTo.(DataFlow::AttrRead).getObject() = nodeFrom and
+      nodeTo.(DataFlow::AttrRead).getAttributeName() in [
+          "uri", "path", "prepath", "postpath", "content", "args", "received_cookies",
+          "requestHeaders", "user", "password", "host"
+        ]
+    }
+  }
+
+  /**
+   * A parameter of a request handler method (on a `twisted.web.resource.Resource` subclass)
+   * that is also given remote user input. (a bit like RoutedParameter).
+   */
+  class TwistedResourceRequestHandlerExtraSources extends RemoteFlowSource::Range,
+    DataFlow::ParameterNode {
+    TwistedResourceRequestHandlerExtraSources() {
+      exists(TwistedResourceRequestHandler func, int i |
+        func.getName() in ["getChild", "getChildWithDefault"] and i = 1
+        or
+        func.getName() = "getDynamicEntity" and i = 1
+      |
+        this.getParameter() = func.getArg(i)
+      )
+    }
+
+    override string getSourceType() { result = "twisted Resource method extra parameter" }
+  }
+
+  // ---------------------------------------------------------------------------
+  // response modeling
+  // ---------------------------------------------------------------------------
+  /**
+   * Implicit response from returns of render methods.
+   */
+  private class TwistedResourceRenderMethodReturn extends HTTP::Server::HttpResponse::Range,
+    DataFlow::CfgNode {
+    TwistedResourceRenderMethodReturn() {
+      this.asCfgNode() = any(TwistedResourceRenderMethod meth).getAReturnValueFlowNode()
+    }
+
+    override DataFlow::Node getBody() { result = this }
+
+    override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
+
+    override string getMimetypeDefault() { result = "text/html" }
+  }
+
+  /**
+   * A call to the `twisted.web.server.Request.write` function.
+   *
+   * See https://twistedmatrix.com/documents/21.2.0/api/twisted.web.server.Request.html#write
+   */
+  class TwistedRequestWriteCall extends HTTP::Server::HttpResponse::Range, DataFlow::CallCfgNode {
+    TwistedRequestWriteCall() {
+      // TODO: When we have tools that make it easy, model these properly to handle
+      // `meth = obj.meth; meth()`. Until then, we'll use this more syntactic approach
+      // (since it allows us to at least capture the most common cases).
+      exists(DataFlow::AttrRead read |
+        this.getFunction() = read and
+        read.getObject() = Request::instance() and
+        read.getAttributeName() = "write"
+      )
+    }
+
+    override DataFlow::Node getBody() {
+      result.asCfgNode() in [node.getArg(0), node.getArgByName("data")]
+    }
+
+    override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
+
+    override string getMimetypeDefault() { result = "text/html" }
+  }
+}
diff --git a/python/ql/test/library-tests/frameworks/twisted/response_test.py b/python/ql/test/library-tests/frameworks/twisted/response_test.py
@@ -6,55 +6,55 @@
 root = Resource()
 
 class Now(Resource):
-    def render(self, request: Request):
-        return b"now"
+    def render(self, request: Request): # $ requestHandler
+        return b"now" # $ HttpResponse mimetype=text/html responseBody=b"now"
 
 
 class AlsoNow(Resource):
-    def render(self, request: Request):
-        request.write(b"also now")
-        return b""
+    def render(self, request: Request): # $ requestHandler
+        request.write(b"also now") # $ HttpResponse mimetype=text/html responseBody=b"also now"
+        return b"" # $ HttpResponse mimetype=text/html responseBody=b""
 
 
 def process_later(request: Request):
     print("process_later called")
-    request.write(b"later")
+    request.write(b"later") # $ MISSING: responseBody=b"later"
     request.finish()
 
 
 class Later(Resource):
-    def render(self, request: Request):
+    def render(self, request: Request): # $ requestHandler
         # process the request in 1 second
         print("setting up callback for process_later")
         reactor.callLater(1, process_later, request)
-        return NOT_DONE_YET
+        return NOT_DONE_YET # $ SPURIOUS: HttpResponse mimetype=text/html responseBody=NOT_DONE_YET
 
 
 class PlainText(Resource):
-    def render(self, request: Request):
+    def render(self, request: Request): # $ requestHandler
         request.setHeader(b"content-type", "text/plain")
-        return b"this is plain text"
+        return b"this is plain text" # $ HttpResponse responseBody=b"this is plain text" SPURIOUS: mimetype=text/html MISSING: mimetype=text/plain
 
 
 class Redirect(Resource):
-    def render_GET(self, request: Request):
-        request.redirect("/new-location")
+    def render_GET(self, request: Request): # $ requestHandler
+        request.redirect("/new-location") # $ MISSING: HttpRedirectResponse
         # By default, this `hello` output is not returned... not even when
         # requested with curl.
-        return b"hello"
+        return b"hello" # $ SPURIOUS: HttpResponse mimetype=text/html responseBody=b"hello"
 
 
 class NonHttpBodyOutput(Resource):
     """Examples of provides values in response that is not in the body
     """
-    def render_GET(self, request: Request):
+    def render_GET(self, request: Request): # $ requestHandler
         request.responseHeaders.addRawHeader("key", "value")
         request.setHeader("key2", "value")
 
         request.addCookie("key", "value")
         request.cookies.append(b"key2=value")
 
-        return b""
+        return b"" # $ HttpResponse mimetype=text/html responseBody=b""
 
 
 root.putChild(b"now", Now())
diff --git a/python/ql/test/library-tests/frameworks/twisted/routing_test.py b/python/ql/test/library-tests/frameworks/twisted/routing_test.py
@@ -7,11 +7,11 @@
 
 
 class Foo(Resource):
-    def render(self, request: Request):
+    def render(self, request: Request): # $ requestHandler
         print(f"{request.content=}")
         print(f"{request.cookies=}")
         print(f"{request.received_cookies=}")
-        return b"I am Foo"
+        return b"I am Foo" # $ HttpResponse
 
 
 root.putChild(b"foo", Foo())
@@ -21,17 +21,17 @@ class Child(Resource):
     def __init__(self, name):
         self.name = name.decode("utf-8")
 
-    def render_GET(self, request):
-        return f"Hi, I'm child '{self.name}'".encode("utf-8")
+    def render_GET(self, request): # $ requestHandler
+        return f"Hi, I'm child '{self.name}'".encode("utf-8") # $ HttpResponse
 
 
 class Parent(Resource):
-    def getChild(self, path, request):
+    def getChild(self, path, request): # $ requestHandler
         print(path, type(path))
         return Child(path)
 
-    def render_GET(self, request):
-        return b"Hi, I'm parent"
+    def render_GET(self, request): # $ requestHandler
+        return b"Hi, I'm parent" # $ HttpResponse
 
 
 root.putChild(b"parent", Parent())
diff --git a/python/ql/test/library-tests/frameworks/twisted/taint_test.py b/python/ql/test/library-tests/frameworks/twisted/taint_test.py
