diff --git a/python/ql/lib/change-notes/2023-07-12-aiohttp-improvements.md b/python/ql/lib/change-notes/2023-07-12-aiohttp-improvements.md new file mode 100644 index 00000000000..5805ee1baa9 --- /dev/null +++ b/python/ql/lib/change-notes/2023-07-12-aiohttp-improvements.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Improvements of the `aiohttp` models including remote-flow-sources from type annotations, new path manipulation, and SSRF sinks. diff --git a/python/ql/lib/semmle/python/frameworks/Aiohttp.qll b/python/ql/lib/semmle/python/frameworks/Aiohttp.qll index 54decc5dcb8..546014c2dbf 100644 --- a/python/ql/lib/semmle/python/frameworks/Aiohttp.qll +++ b/python/ql/lib/semmle/python/frameworks/Aiohttp.qll @@ -468,6 +468,27 @@ module AiohttpWebModel { override string getSourceType() { result = "aiohttp.web.Request" } } + /** + * A parameter that has a type annotation of `aiohttp.web.Request`, so with all + * likelihood will receive an `aiohttp.web.Request` instance at some point when a + * request handler is invoked. + */ + class AiohttpRequestParamFromTypeAnnotation extends Request::InstanceSource, + DataFlow::ParameterNode, RemoteFlowSource::Range + { + AiohttpRequestParamFromTypeAnnotation() { + not this instanceof AiohttpRequestHandlerRequestParam and + this.getParameter().getAnnotation() = + API::moduleImport("aiohttp") + .getMember("web") + .getMember("Request") + .getAValueReachableFromSource() + .asExpr() + } + + override string getSourceType() { result = "aiohttp.web.Request from type-annotation" } + } + /** * A read of the `request` attribute on an instance of an aiohttp.web View class, * which is the request being processed currently. @@ -498,14 +519,17 @@ module AiohttpWebModel { * - https://docs.aiohttp.org/en/stable/web_quickstart.html#aiohttp-web-exceptions */ class AiohttpWebResponseInstantiation extends Http::Server::HttpResponse::Range, - Response::InstanceSource, DataFlow::CallCfgNode + Response::InstanceSource, API::CallNode { API::Node apiNode; AiohttpWebResponseInstantiation() { this = apiNode.getACall() and ( - apiNode = API::moduleImport("aiohttp").getMember("web").getMember("Response") + apiNode = + API::moduleImport("aiohttp") + .getMember("web") + .getMember(["FileResponse", "Response", "StreamResponse"]) or exists(string httpExceptionClassName | httpExceptionClassName in [ @@ -545,6 +569,10 @@ module AiohttpWebModel { override DataFlow::Node getMimetypeOrContentTypeArg() { result = this.getArgByName("content_type") + or + exists(string key | key.toLowerCase() = "content-type" | + result = this.getKeywordParameter("headers").getSubscript(key).getAValueReachingSink() + ) } override string getMimetypeDefault() { @@ -556,6 +584,37 @@ module AiohttpWebModel { } } + /** + * A call to the `aiohttp.web.FileResponse` constructor as a sink for Filesystem access. + */ + class FileResponseCall extends FileSystemAccess::Range, API::CallNode { + FileResponseCall() { + this = API::moduleImport("aiohttp").getMember("web").getMember("FileResponse").getACall() + } + + override DataFlow::Node getAPathArgument() { result = this.getParameter(0, "path").asSink() } + } + + /** + * An instantiation of `aiohttp.web.StreamResponse`. + * + * See https://docs.aiohttp.org/en/stable/web_reference.html#aiohttp.web.StreamResponse + */ + class StreamResponse extends AiohttpWebResponseInstantiation { + StreamResponse() { + this = API::moduleImport("aiohttp").getMember("web").getMember("StreamResponse").getACall() + } + + override DataFlow::Node getBody() { + result = + this.getReturn() + .getMember(["write", "write_eof"]) + .getACall() + .getParameter(0, "data") + .asSink() + } + } + /** Gets an HTTP response instance. */ private API::Node aiohttpResponseInstance() { result = any(AiohttpWebResponseInstantiation call).getApiNode().getReturn() @@ -670,14 +729,14 @@ private module AiohttpClientModel { string methodName; OutgoingRequestCall() { - methodName in [Http::httpVerbLower(), "request"] and + methodName in [Http::httpVerbLower(), "request", "ws_connect"] and this = instance().getMember(methodName).getACall() } override DataFlow::Node getAUrlPart() { result = this.getArgByName("url") or - not methodName = "request" and + methodName in [Http::httpVerbLower(), "ws_connect"] and result = this.getArg(0) or methodName = "request" and diff --git a/python/ql/test/experimental/meta/ConceptsTest.qll b/python/ql/test/experimental/meta/ConceptsTest.qll index 13dac722c3b..255923d4e7f 100644 --- a/python/ql/test/experimental/meta/ConceptsTest.qll +++ b/python/ql/test/experimental/meta/ConceptsTest.qll @@ -292,10 +292,11 @@ module HttpServerHttpResponseTest implements TestSig { exists(DedicatedResponseTest d | d.isDedicatedFile(file)) ) and ( - exists(Http::Server::HttpResponse response | - location = response.getLocation() and - element = response.toString() and - value = prettyNodeForInlineTest(response.getBody()) and + exists(Http::Server::HttpResponse response, DataFlow::Node body | + body = response.getBody() and + location = body.getLocation() and + element = body.toString() and + value = prettyNodeForInlineTest(body) and tag = "responseBody" ) or diff --git a/python/ql/test/library-tests/frameworks/aiohttp/InlineTaintTest.ql b/python/ql/test/library-tests/frameworks/aiohttp/InlineTaintTest.ql index 8524da5fe7d..caaa22ef194 100644 --- a/python/ql/test/library-tests/frameworks/aiohttp/InlineTaintTest.ql +++ b/python/ql/test/library-tests/frameworks/aiohttp/InlineTaintTest.ql @@ -1,2 +1,19 @@ import experimental.meta.InlineTaintTest -import MakeInlineTaintTest + +predicate isSafe(DataFlow::GuardNode g, ControlFlowNode node, boolean branch) { + g.(CallNode).getFunction().(NameNode).getId() = "is_safe" and + node = g.(CallNode).getArg(_) and + branch = true +} + +module CustomSanitizerOverridesConfig implements DataFlow::ConfigSig { + predicate isSource = TestTaintTrackingConfig::isSource/1; + + predicate isSink = TestTaintTrackingConfig::isSink/1; + + predicate isBarrier(DataFlow::Node node) { + node = DataFlow::BarrierGuard::getABarrierNode() + } +} + +import MakeInlineTaintTest diff --git a/python/ql/test/library-tests/frameworks/aiohttp/client_request.py b/python/ql/test/library-tests/frameworks/aiohttp/client_request.py index 1bafb4ef583..d7958cf660f 100644 --- a/python/ql/test/library-tests/frameworks/aiohttp/client_request.py +++ b/python/ql/test/library-tests/frameworks/aiohttp/client_request.py @@ -33,3 +33,5 @@ async def test(): assert context.verify_mode == ssl.VerifyMode.CERT_NONE s.get("url", ssl=context) # $ clientRequestUrlPart="url" MISSING: clientRequestCertValidationDisabled + + s.ws_connect("url") # $ clientRequestUrlPart="url" diff --git a/python/ql/test/library-tests/frameworks/aiohttp/response_test.py b/python/ql/test/library-tests/frameworks/aiohttp/response_test.py index e800c28234a..bc9bc8d3bda 100644 --- a/python/ql/test/library-tests/frameworks/aiohttp/response_test.py +++ b/python/ql/test/library-tests/frameworks/aiohttp/response_test.py @@ -23,6 +23,9 @@ async def html_text(request): # $ requestHandler async def html_body(request): # $ requestHandler return web.Response(body=b"foo", content_type="text/html") # $ HttpResponse mimetype=text/html responseBody=b"foo" +@routes.get("/html_body_header") # $ routeSetup="/html_body_header" +async def html_body_header(request): # $ requestHandler + return web.Response(headers={"content-type": "text/html"}, text="foo") # $ HttpResponse mimetype=text/html responseBody="foo" @routes.get("/html_body_set_later") # $ routeSetup="/html_body_set_later" async def html_body_set_later(request): # $ requestHandler @@ -65,6 +68,26 @@ async def redirect_302(request): # $ requestHandler else: raise web.HTTPFound(location="/logout") # $ HttpResponse HttpRedirectResponse mimetype=application/octet-stream redirectLocation="/logout" + +@routes.get("/file_response") # $ routeSetup="/file_response" +async def file_response(request): # $ requestHandler + filename = "foo.txt" + resp = web.FileResponse(filename) # $ HttpResponse mimetype=application/octet-stream getAPathArgument=filename + resp = web.FileResponse(path=filename) # $ HttpResponse mimetype=application/octet-stream getAPathArgument=filename + return resp + + +@routes.get("/streaming_response") # $ routeSetup="/streaming_response" +async def streaming_response(request): # $ requestHandler + resp = web.StreamResponse() # $ HttpResponse mimetype=application/octet-stream + await resp.prepare(request) + + await resp.write(b"foo") # $ responseBody=b"foo" + await resp.write(data=b"bar") # $ responseBody=b"bar" + await resp.write_eof(b"baz") # $ responseBody=b"baz" + + return resp + ################################################################################ # Cookies ################################################################################ diff --git a/python/ql/test/library-tests/frameworks/aiohttp/taint_test.py b/python/ql/test/library-tests/frameworks/aiohttp/taint_test.py index 54da5726803..fb65a17554d 100644 --- a/python/ql/test/library-tests/frameworks/aiohttp/taint_test.py +++ b/python/ql/test/library-tests/frameworks/aiohttp/taint_test.py @@ -142,10 +142,36 @@ class TaintTestClass(web.View): self.request.url # $ tainted ) +# not a request handler, and not called, but since we have type-annotation, should be a +# remote-flow-source. +async def test_source_from_type_annotation(request: web.Request): + # picking out just a few of the tests from `test_taint` above, to show that we have + # the same taint-steps :) + ensure_tainted( + request, # $ tainted + request.url, # $ tainted + await request.content.read(), # $ tainted + ) + +# Test that since we can reach the `request` object in the helper function, we don't +# introduce a new remote-flow-source, but instead use the one from the caller. (which is +# checked to not be tainted) +async def test_sanitizer(request): # $ requestHandler + ensure_tainted(request, request.url, await request.content.read()) # $ tainted + + if (is_safe(request)): + ensure_not_tainted(request, request.url, await request.content.read()) + test_safe_helper_function_no_route_with_type(request) + + +async def test_safe_helper_function_no_route_with_type(request: web.Request): + ensure_not_tainted(request, request.url, await request.content.read()) # $ SPURIOUS: tainted + app = web.Application() app.router.add_get(r"/test_taint/{name}/{number:\d+}", test_taint) # $ routeSetup="/test_taint/{name}/{number:\d+}" app.router.add_view(r"/test_taint_class", TaintTestClass) # $ routeSetup="/test_taint_class" +app.router.add_view(r"/test_sanitizer", test_sanitizer) # $ routeSetup="/test_sanitizer" if __name__ == "__main__":