Merge pull request #9001 from RasmusWL/files-refactoring

Python: Flask: Improve `request.files` modeing

Author: yoff

python/ql/lib/change-notes/2022-05-02-flask-request-files-modeling.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+The modeling of `request.files` in Flask has been fixed, so we now properly handle
+assignments to local variables (such as `files = request.files; files['key'].filename`).

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

@@ -411,21 +411,16 @@ module Flask {
   /** An `FileStorage` instance that originates from a flask request. */
   private class FlaskRequestFileStorageInstances extends Werkzeug::FileStorage::InstanceSource {
     FlaskRequestFileStorageInstances() {
-      // TODO: this currently only works in local-scope, since writing type-trackers for
-      // this is a little too much effort. Once API-graphs are available for more
-      // things, we can rewrite this.
-      //
       // TODO: This approach for identifying member-access is very adhoc, and we should
       // be able to do something more structured for providing modeling of the members
       // of a container-object.
-      exists(DataFlow::AttrRead files | files = request().getMember("files").getAnImmediateUse() |
-        this.asCfgNode().(SubscriptNode).getObject() = files.asCfgNode()
+      exists(API::Node files | files = request().getMember("files") |
+        this.asCfgNode().(SubscriptNode).getObject() = files.getAUse().asCfgNode()
         or
-        this.(DataFlow::MethodCallNode).calls(files, "get")
+        this = files.getMember("get").getACall()
         or
-        exists(DataFlow::MethodCallNode getlistCall | getlistCall.calls(files, "getlist") |
-          this.asCfgNode().(SubscriptNode).getObject() = getlistCall.asCfgNode()
-        )
+        this.asCfgNode().(SubscriptNode).getObject() =
+          files.getMember("getlist").getReturn().getAUse().asCfgNode()
       )
     }
   }

python/ql/test/library-tests/frameworks/flask/taint_test.py

@@ -189,6 +189,7 @@ def test_taint(name = "World!", number="0", foo="foo"):  # $requestHandler route
     a = request.args
     b = a
     gl = b.getlist
+    files = request.files
     ensure_tainted(
         request.args, # $ tainted
         a, # $ tainted
@@ -202,6 +203,8 @@ def test_taint(name = "World!", number="0", foo="foo"):  # $requestHandler route
         a.getlist('key'), # $ tainted
         b.getlist('key'), # $ tainted
         gl('key'), # $ tainted
+
+        files.get('key').filename, # $ tainted
     )
 
     # aliasing tests