From 151a733ff2468c859179ffd4850a958b7d2c91a7 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 8 Jun 2021 15:06:10 +0200 Subject: [PATCH 1/6] Python: Add tests for twisted These were largely based on the old tests in https://github.com/github/codeql/blob/6011cb74f8df6f7b0c304e59c0abe8acf79fc7b4/python/ql/test/library-tests/web/twisted/test.py --- .../frameworks/twisted/ConceptsTest.expected | 0 .../frameworks/twisted/ConceptsTest.ql | 12 +++ .../twisted/InlineTaintTest.expected | 3 + .../frameworks/twisted/InlineTaintTest.ql | 1 + .../frameworks/twisted/response_test.py | 75 +++++++++++++++++++ .../frameworks/twisted/routing_test.py | 47 ++++++++++++ .../frameworks/twisted/taint_test.py | 70 +++++++++++++++++ 7 files changed, 208 insertions(+) create mode 100644 python/ql/test/library-tests/frameworks/twisted/ConceptsTest.expected create mode 100644 python/ql/test/library-tests/frameworks/twisted/ConceptsTest.ql create mode 100644 python/ql/test/library-tests/frameworks/twisted/InlineTaintTest.expected create mode 100644 python/ql/test/library-tests/frameworks/twisted/InlineTaintTest.ql create mode 100644 python/ql/test/library-tests/frameworks/twisted/response_test.py create mode 100644 python/ql/test/library-tests/frameworks/twisted/routing_test.py create mode 100644 python/ql/test/library-tests/frameworks/twisted/taint_test.py diff --git a/python/ql/test/library-tests/frameworks/twisted/ConceptsTest.expected b/python/ql/test/library-tests/frameworks/twisted/ConceptsTest.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/python/ql/test/library-tests/frameworks/twisted/ConceptsTest.ql b/python/ql/test/library-tests/frameworks/twisted/ConceptsTest.ql new file mode 100644 index 00000000000..1e2c1fab3ee --- /dev/null +++ b/python/ql/test/library-tests/frameworks/twisted/ConceptsTest.ql @@ -0,0 +1,12 @@ +import python +import experimental.meta.ConceptsTest + +class DedicatedResponseTest extends HttpServerHttpResponseTest { + DedicatedResponseTest() { file.getShortName() = "response_test.py" } +} + +class OtherResponseTest extends HttpServerHttpResponseTest { + OtherResponseTest() { not this instanceof DedicatedResponseTest } + + override string getARelevantTag() { result = "HttpResponse" } +} diff --git a/python/ql/test/library-tests/frameworks/twisted/InlineTaintTest.expected b/python/ql/test/library-tests/frameworks/twisted/InlineTaintTest.expected new file mode 100644 index 00000000000..79d760d87f4 --- /dev/null +++ b/python/ql/test/library-tests/frameworks/twisted/InlineTaintTest.expected @@ -0,0 +1,3 @@ +argumentToEnsureNotTaintedNotMarkedAsSpurious +untaintedArgumentToEnsureTaintedNotMarkedAsMissing +failures diff --git a/python/ql/test/library-tests/frameworks/twisted/InlineTaintTest.ql b/python/ql/test/library-tests/frameworks/twisted/InlineTaintTest.ql new file mode 100644 index 00000000000..027ad8667be --- /dev/null +++ b/python/ql/test/library-tests/frameworks/twisted/InlineTaintTest.ql @@ -0,0 +1 @@ +import experimental.meta.InlineTaintTest diff --git a/python/ql/test/library-tests/frameworks/twisted/response_test.py b/python/ql/test/library-tests/frameworks/twisted/response_test.py new file mode 100644 index 00000000000..b29b59cf415 --- /dev/null +++ b/python/ql/test/library-tests/frameworks/twisted/response_test.py @@ -0,0 +1,75 @@ +from twisted.web.server import Site, Request, NOT_DONE_YET +from twisted.web.resource import Resource +from twisted.internet import reactor, endpoints, defer + + +root = Resource() + +class Now(Resource): + def render(self, request: Request): + return b"now" + + +class AlsoNow(Resource): + def render(self, request: Request): + request.write(b"also now") + return b"" + + +def process_later(request: Request): + print("process_later called") + request.write(b"later") + request.finish() + + +class Later(Resource): + def render(self, request: Request): + # process the request in 1 second + print("setting up callback for process_later") + reactor.callLater(1, process_later, request) + return NOT_DONE_YET + + +class PlainText(Resource): + def render(self, request: Request): + request.setHeader(b"content-type", "text/plain") + return b"this is plain text" + + +class Redirect(Resource): + def render_GET(self, request: Request): + request.redirect("/new-location") + # By default, this `hello` output is not returned... not even when + # requested with curl. + return b"hello" + + +class NonHttpBodyOutput(Resource): + """Examples of provides values in response that is not in the body + """ + def render_GET(self, request: Request): + request.responseHeaders.addRawHeader("key", "value") + request.setHeader("key2", "value") + + request.addCookie("key", "value") + request.cookies.append(b"key2=value") + + return b"" + + +root.putChild(b"now", Now()) +root.putChild(b"also-now", AlsoNow()) +root.putChild(b"later", Later()) +root.putChild(b"plain-text", PlainText()) +root.putChild(b"redirect", Redirect()) +root.putChild(b"non-body", NonHttpBodyOutput()) + + +if __name__ == "__main__": + factory = Site(root) + endpoint = endpoints.TCP4ServerEndpoint(reactor, 8880) + endpoint.listen(factory) + + print("Will run on http://localhost:8880") + + reactor.run() diff --git a/python/ql/test/library-tests/frameworks/twisted/routing_test.py b/python/ql/test/library-tests/frameworks/twisted/routing_test.py new file mode 100644 index 00000000000..0d8c7caefa6 --- /dev/null +++ b/python/ql/test/library-tests/frameworks/twisted/routing_test.py @@ -0,0 +1,47 @@ +from twisted.web.server import Site, Request +from twisted.web.resource import Resource +from twisted.internet import reactor, endpoints + + +root = Resource() + + +class Foo(Resource): + def render(self, request: Request): + print(f"{request.content=}") + print(f"{request.cookies=}") + print(f"{request.received_cookies=}") + return b"I am Foo" + + +root.putChild(b"foo", Foo()) + + +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") + + +class Parent(Resource): + def getChild(self, path, request): + print(path, type(path)) + return Child(path) + + def render_GET(self, request): + return b"Hi, I'm parent" + + +root.putChild(b"parent", Parent()) + + +if __name__ == "__main__": + factory = Site(root) + endpoint = endpoints.TCP4ServerEndpoint(reactor, 8880) + endpoint.listen(factory) + + print("Will run on http://localhost:8880") + + reactor.run() diff --git a/python/ql/test/library-tests/frameworks/twisted/taint_test.py b/python/ql/test/library-tests/frameworks/twisted/taint_test.py new file mode 100644 index 00000000000..b566672f82d --- /dev/null +++ b/python/ql/test/library-tests/frameworks/twisted/taint_test.py @@ -0,0 +1,70 @@ +from twisted.web.resource import Resource +from twisted.web.server import Request + +class MyTaintTest(Resource): + def getChild(self, path, request): + ensure_tainted(path, request) # $ MISSING: tainted + + def render(self, request): + ensure_tainted(request) # $ MISSING: tainted + + def render_GET(self, request: Request): + # see https://twistedmatrix.com/documents/21.2.0/api/twisted.web.server.Request.html + ensure_tainted( + request, # $ MISSING: tainted + + request.uri, # $ MISSING: tainted + request.path, # $ MISSING: tainted + request.prepath, # $ MISSING: tainted + request.postpath, # $ MISSING: tainted + + # file-like + request.content, # $ MISSING: tainted + request.content.read(), # $ MISSING: tainted + + # Dict[bytes, List[bytes]] (for query args) + request.args, # $ MISSING: tainted + request.args[b"key"], # $ MISSING: tainted + request.args[b"key"][0], # $ MISSING: tainted + request.args.get(b"key"), # $ MISSING: tainted + request.args.get(b"key")[0], # $ MISSING: tainted + + request.received_cookies, # $ MISSING: tainted + request.received_cookies["key"], # $ MISSING: tainted + request.received_cookies.get("key"), # $ MISSING: tainted + request.getCookie(b"key"), # $ MISSING: tainted + + # twisted.web.http_headers.Headers + # see https://twistedmatrix.com/documents/21.2.0/api/twisted.web.http_headers.Headers.html + request.requestHeaders, # $ MISSING: tainted + request.requestHeaders.getRawHeaders("key"), # $ MISSING: tainted + request.requestHeaders.getRawHeaders("key")[0], # $ MISSING: tainted + request.requestHeaders.getAllRawHeaders(), # $ MISSING: tainted + list(request.requestHeaders.getAllRawHeaders()), # $ MISSING: tainted + + request.getHeader("key"), # $ MISSING: tainted + request.getAllHeaders(), # $ MISSING: tainted + request.getAllHeaders()["key"], # $ MISSING: tainted + + request.user, # $ MISSING: tainted + request.getUser(), # $ MISSING: tainted + + request.password, # $ MISSING: tainted + request.getPassword(), # $ MISSING: tainted + + request.host, # $ MISSING: tainted + request.getHost(), # $ MISSING: tainted + request.getRequestHostname(), # $ MISSING: tainted + ) + + # technically user-controlled, but unlike to lead to vulnerabilities. + ensure_not_tainted( + request.method, + ) + + # not tainted at all + ensure_not_tainted( + # outgoing things + request.cookies, + request.responseHeaders, + ) From a21039170b42c08e46c80c8b10392b12edf62a2b Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 8 Jun 2021 16:11:18 +0200 Subject: [PATCH 2/6] Python: Model (most of) twisted --- docs/codeql/support/reusables/frameworks.rst | 1 + python/ql/src/semmle/python/Frameworks.qll | 1 + .../src/semmle/python/frameworks/Twisted.qll | 229 ++++++++++++++++++ .../frameworks/twisted/response_test.py | 30 +-- .../frameworks/twisted/routing_test.py | 14 +- .../frameworks/twisted/taint_test.py | 62 ++--- 6 files changed, 284 insertions(+), 53 deletions(-) create mode 100644 python/ql/src/semmle/python/frameworks/Twisted.qll diff --git a/docs/codeql/support/reusables/frameworks.rst b/docs/codeql/support/reusables/frameworks.rst index be9949aad77..3e169a0949b 100644 --- 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 index 3115c3ffac6..ecbf4eb6c86 100644 --- 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 new file mode 100644 index 00000000000..1c9b1ea301d --- /dev/null +++ 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 index b29b59cf415..171cad82dab 100644 --- 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 @@ from twisted.internet import reactor, endpoints, defer 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 index 0d8c7caefa6..bcbcc6521af 100644 --- 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 @@ root = Resource() 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 index b566672f82d..03353f8cf97 100644 --- a/python/ql/test/library-tests/frameworks/twisted/taint_test.py +++ b/python/ql/test/library-tests/frameworks/twisted/taint_test.py @@ -2,59 +2,59 @@ from twisted.web.resource import Resource from twisted.web.server import Request class MyTaintTest(Resource): - def getChild(self, path, request): - ensure_tainted(path, request) # $ MISSING: tainted + def getChild(self, path, request): # $ requestHandler + ensure_tainted(path, request) # $ tainted - def render(self, request): - ensure_tainted(request) # $ MISSING: tainted + def render(self, request): # $ requestHandler + ensure_tainted(request) # $ tainted - def render_GET(self, request: Request): + def render_GET(self, request: Request): # $ requestHandler # see https://twistedmatrix.com/documents/21.2.0/api/twisted.web.server.Request.html ensure_tainted( - request, # $ MISSING: tainted + request, # $ tainted - request.uri, # $ MISSING: tainted - request.path, # $ MISSING: tainted - request.prepath, # $ MISSING: tainted - request.postpath, # $ MISSING: tainted + request.uri, # $ tainted + request.path, # $ tainted + request.prepath, # $ tainted + request.postpath, # $ tainted # file-like - request.content, # $ MISSING: tainted + request.content, # $ tainted request.content.read(), # $ MISSING: tainted # Dict[bytes, List[bytes]] (for query args) - request.args, # $ MISSING: tainted - request.args[b"key"], # $ MISSING: tainted - request.args[b"key"][0], # $ MISSING: tainted - request.args.get(b"key"), # $ MISSING: tainted - request.args.get(b"key")[0], # $ MISSING: tainted + request.args, # $ tainted + request.args[b"key"], # $ tainted + request.args[b"key"][0], # $ tainted + request.args.get(b"key"), # $ tainted + request.args.get(b"key")[0], # $ tainted - request.received_cookies, # $ MISSING: tainted - request.received_cookies["key"], # $ MISSING: tainted - request.received_cookies.get("key"), # $ MISSING: tainted - request.getCookie(b"key"), # $ MISSING: tainted + request.received_cookies, # $ tainted + request.received_cookies["key"], # $ tainted + request.received_cookies.get("key"), # $ tainted + request.getCookie(b"key"), # $ tainted # twisted.web.http_headers.Headers # see https://twistedmatrix.com/documents/21.2.0/api/twisted.web.http_headers.Headers.html - request.requestHeaders, # $ MISSING: tainted + request.requestHeaders, # $ tainted request.requestHeaders.getRawHeaders("key"), # $ MISSING: tainted request.requestHeaders.getRawHeaders("key")[0], # $ MISSING: tainted request.requestHeaders.getAllRawHeaders(), # $ MISSING: tainted list(request.requestHeaders.getAllRawHeaders()), # $ MISSING: tainted - request.getHeader("key"), # $ MISSING: tainted - request.getAllHeaders(), # $ MISSING: tainted - request.getAllHeaders()["key"], # $ MISSING: tainted + request.getHeader("key"), # $ tainted + request.getAllHeaders(), # $ tainted + request.getAllHeaders()["key"], # $ tainted - request.user, # $ MISSING: tainted - request.getUser(), # $ MISSING: tainted + request.user, # $ tainted + request.getUser(), # $ tainted - request.password, # $ MISSING: tainted - request.getPassword(), # $ MISSING: tainted + request.password, # $ tainted + request.getPassword(), # $ tainted - request.host, # $ MISSING: tainted - request.getHost(), # $ MISSING: tainted - request.getRequestHostname(), # $ MISSING: tainted + request.host, # $ tainted + request.getHost(), # $ tainted + request.getRequestHostname(), # $ tainted ) # technically user-controlled, but unlike to lead to vulnerabilities. From 23f668f8eef97e7d00975274ff4bf116ca4a6f49 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 8 Jun 2021 16:16:56 +0200 Subject: [PATCH 3/6] Python: Model redirects in twisted --- .../src/semmle/python/frameworks/Twisted.qll | 29 +++++++++++++++++++ .../frameworks/twisted/response_test.py | 2 +- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/python/ql/src/semmle/python/frameworks/Twisted.qll b/python/ql/src/semmle/python/frameworks/Twisted.qll index 1c9b1ea301d..5390513ca3a 100644 --- a/python/ql/src/semmle/python/frameworks/Twisted.qll +++ b/python/ql/src/semmle/python/frameworks/Twisted.qll @@ -226,4 +226,33 @@ private module Twisted { override string getMimetypeDefault() { result = "text/html" } } + + /** + * A call to the `redirect` function on a twisted request. + * + * See https://twistedmatrix.com/documents/21.2.0/api/twisted.web.http.Request.html#redirect + */ + class TwistedRequestRedirectCall extends HTTP::Server::HttpRedirectResponse::Range, + DataFlow::CallCfgNode { + TwistedRequestRedirectCall() { + // 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() = "redirect" + ) + } + + override DataFlow::Node getBody() { none() } + + override DataFlow::Node getRedirectLocation() { + result.asCfgNode() in [node.getArg(0), node.getArgByName("url")] + } + + 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 index 171cad82dab..f418330c47a 100644 --- a/python/ql/test/library-tests/frameworks/twisted/response_test.py +++ b/python/ql/test/library-tests/frameworks/twisted/response_test.py @@ -38,7 +38,7 @@ class PlainText(Resource): class Redirect(Resource): def render_GET(self, request: Request): # $ requestHandler - request.redirect("/new-location") # $ MISSING: HttpRedirectResponse + request.redirect("/new-location") # $ HttpRedirectResponse redirectLocation="/new-location" HttpResponse mimetype=text/html # By default, this `hello` output is not returned... not even when # requested with curl. return b"hello" # $ SPURIOUS: HttpResponse mimetype=text/html responseBody=b"hello" From 7c758f5c81a600d4327d9d1ef284ebd62b6916c4 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 8 Jun 2021 16:20:29 +0200 Subject: [PATCH 4/6] Python: Add change-note for twisted --- python/change-notes/2021-06-08-twisted-add-modeling.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 python/change-notes/2021-06-08-twisted-add-modeling.md diff --git a/python/change-notes/2021-06-08-twisted-add-modeling.md b/python/change-notes/2021-06-08-twisted-add-modeling.md new file mode 100644 index 00000000000..364b8c02707 --- /dev/null +++ b/python/change-notes/2021-06-08-twisted-add-modeling.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* Added modeling of sources/sinks when using `twisted` to create web servers. From 8208aebd7e34caca3af5ed012abf3bef4601af3e Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 21 Jun 2021 10:43:25 +0200 Subject: [PATCH 5/6] Python: Apply suggestions from code review Co-authored-by: yoff --- python/ql/src/semmle/python/frameworks/Twisted.qll | 2 +- .../ql/test/library-tests/frameworks/twisted/response_test.py | 2 +- python/ql/test/library-tests/frameworks/twisted/taint_test.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/python/ql/src/semmle/python/frameworks/Twisted.qll b/python/ql/src/semmle/python/frameworks/Twisted.qll index 5390513ca3a..b9b0942bbdc 100644 --- a/python/ql/src/semmle/python/frameworks/Twisted.qll +++ b/python/ql/src/semmle/python/frameworks/Twisted.qll @@ -79,7 +79,7 @@ private module Twisted { /** * A "render" method on a `twisted.web.resource.Resource` subclass, whose return value - * is written as the body fo the HTTP response. + * is written as the body of the HTTP response. */ class TwistedResourceRenderMethod extends TwistedResourceRequestHandler { TwistedResourceRenderMethod() { diff --git a/python/ql/test/library-tests/frameworks/twisted/response_test.py b/python/ql/test/library-tests/frameworks/twisted/response_test.py index f418330c47a..f845f7f693a 100644 --- a/python/ql/test/library-tests/frameworks/twisted/response_test.py +++ b/python/ql/test/library-tests/frameworks/twisted/response_test.py @@ -45,7 +45,7 @@ class Redirect(Resource): class NonHttpBodyOutput(Resource): - """Examples of provides values in response that is not in the body + """Examples of providing values in response that is not in the body """ def render_GET(self, request: Request): # $ requestHandler request.responseHeaders.addRawHeader("key", "value") diff --git a/python/ql/test/library-tests/frameworks/twisted/taint_test.py b/python/ql/test/library-tests/frameworks/twisted/taint_test.py index 03353f8cf97..cdc6592f90e 100644 --- a/python/ql/test/library-tests/frameworks/twisted/taint_test.py +++ b/python/ql/test/library-tests/frameworks/twisted/taint_test.py @@ -57,7 +57,7 @@ class MyTaintTest(Resource): request.getRequestHostname(), # $ tainted ) - # technically user-controlled, but unlike to lead to vulnerabilities. + # technically user-controlled, but unlikely to lead to vulnerabilities. ensure_not_tainted( request.method, ) From d6ec4d30fcc41714d51881f83fe7a7055d6b215a Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 21 Jun 2021 10:54:28 +0200 Subject: [PATCH 6/6] Python: Twisted refactor of getRequestParamIndex --- .../src/semmle/python/frameworks/Twisted.qll | 26 +++++++------------ 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/python/ql/src/semmle/python/frameworks/Twisted.qll b/python/ql/src/semmle/python/frameworks/Twisted.qll index b9b0942bbdc..5b014597157 100644 --- a/python/ql/src/semmle/python/frameworks/Twisted.qll +++ b/python/ql/src/semmle/python/frameworks/Twisted.qll @@ -41,36 +41,28 @@ private module Twisted { // 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(), _) + exists(getRequestParamIndex(result.getName())) } } /** - * Holds if the request parameter is supposed to be at index `requestParamIndex` for - * the method named `methodName` in `twisted.web.resource.Resource`. + * Gets the index the request parameter is supposed to be at for the method named + * `methodName` in a `twisted.web.resource.Resource` subclass. */ bindingset[methodName] - private predicate resourceMethodRequestParamIndex(string methodName, int requestParamIndex) { - methodName.matches("render_%") and requestParamIndex = 1 + private int getRequestParamIndex(string methodName) { + methodName.matches("render_%") and result = 1 or - methodName in ["render", "listDynamicEntities", "getChildForRequest"] and requestParamIndex = 1 + methodName in ["render", "listDynamicEntities", "getChildForRequest"] and result = 1 or - methodName = ["getDynamicEntity", "getChild", "getChildWithDefault"] and requestParamIndex = 2 + methodName = ["getDynamicEntity", "getChild", "getChildWithDefault"] and result = 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(), _) - } + TwistedResourceRequestHandler() { this = any(TwistedResourceSubclass cls).getARequestHandler() } - Parameter getRequestParameter() { - exists(int i | - resourceMethodRequestParamIndex(this.getName(), i) and - result = this.getArg(i) - ) - } + Parameter getRequestParameter() { result = this.getArg(getRequestParamIndex(this.getName())) } override Parameter getARoutedParameter() { none() }