From 0c62916af5d42aaf88667dc2d04f82ec12537b37 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 2 May 2022 14:05:35 +0200 Subject: [PATCH 1/4] Python: Highlight problem with Flask `request.files` modeling --- python/ql/test/library-tests/frameworks/flask/taint_test.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/python/ql/test/library-tests/frameworks/flask/taint_test.py b/python/ql/test/library-tests/frameworks/flask/taint_test.py index 1c1f88eab68..609e4caf091 100644 --- a/python/ql/test/library-tests/frameworks/flask/taint_test.py +++ b/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, # $ MISSING: tainted ) # aliasing tests From fb0133d276c2f8c62f79bcea3f3d1087ebbfcdd7 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 2 May 2022 14:14:29 +0200 Subject: [PATCH 2/4] Python: Fix Flask `request.files` modeling --- python/ql/lib/semmle/python/frameworks/Flask.qll | 2 +- python/ql/test/library-tests/frameworks/flask/taint_test.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/Flask.qll b/python/ql/lib/semmle/python/frameworks/Flask.qll index 8fe9e9bdd4b..19400aba739 100644 --- a/python/ql/lib/semmle/python/frameworks/Flask.qll +++ b/python/ql/lib/semmle/python/frameworks/Flask.qll @@ -418,7 +418,7 @@ module Flask { // 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() | + exists(DataFlow::Node files | files = request().getMember("files").getAUse() | this.asCfgNode().(SubscriptNode).getObject() = files.asCfgNode() or this.(DataFlow::MethodCallNode).calls(files, "get") diff --git a/python/ql/test/library-tests/frameworks/flask/taint_test.py b/python/ql/test/library-tests/frameworks/flask/taint_test.py index 609e4caf091..dcca8ff6681 100644 --- a/python/ql/test/library-tests/frameworks/flask/taint_test.py +++ b/python/ql/test/library-tests/frameworks/flask/taint_test.py @@ -204,7 +204,7 @@ def test_taint(name = "World!", number="0", foo="foo"): # $requestHandler route b.getlist('key'), # $ tainted gl('key'), # $ tainted - files.get('key').filename, # $ MISSING: tainted + files.get('key').filename, # $ tainted ) # aliasing tests From de4390cdf6500f352591e69ad4568243cc7408d1 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 2 May 2022 14:19:45 +0200 Subject: [PATCH 3/4] Python: Improve Flask `request.files` handling even more --- python/ql/lib/semmle/python/frameworks/Flask.qll | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/Flask.qll b/python/ql/lib/semmle/python/frameworks/Flask.qll index 19400aba739..02331ed316e 100644 --- a/python/ql/lib/semmle/python/frameworks/Flask.qll +++ b/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::Node files | files = request().getMember("files").getAUse() | - 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() ) } } From 7e1be3172e946da4620eb52c112391ad6241b36b Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 2 May 2022 14:24:13 +0200 Subject: [PATCH 4/4] Python: Add change-note --- .../change-notes/2022-05-02-flask-request-files-modeling.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 python/ql/lib/change-notes/2022-05-02-flask-request-files-modeling.md diff --git a/python/ql/lib/change-notes/2022-05-02-flask-request-files-modeling.md b/python/ql/lib/change-notes/2022-05-02-flask-request-files-modeling.md new file mode 100644 index 00000000000..9b80811a608 --- /dev/null +++ b/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`).