diff --git a/python/change-notes/2021-10-28-flask-send_file.md b/python/change-notes/2021-10-28-flask-send_file.md new file mode 100644 index 000000000000..1e875bd54122 --- /dev/null +++ b/python/change-notes/2021-10-28-flask-send_file.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* Added modeling of the `send_from_directory` and `send_file` functions from the `flask` PyPI package, resulting in additional sinks for the _Uncontrolled data used in path expression_ (`py/path-injection`) query. This addition was originally [submitted as an external contribution by @porcupineyhairs](https://github.com/github/codeql/pull/6330). diff --git a/python/ql/lib/semmle/python/frameworks/Flask.qll b/python/ql/lib/semmle/python/frameworks/Flask.qll index fa31bd772208..5bb838447cbe 100644 --- a/python/ql/lib/semmle/python/frameworks/Flask.qll +++ b/python/ql/lib/semmle/python/frameworks/Flask.qll @@ -11,6 +11,7 @@ private import semmle.python.Concepts private import semmle.python.frameworks.Werkzeug private import semmle.python.ApiGraphs private import semmle.python.frameworks.internal.InstanceTaintStepsHelper +private import semmle.python.security.dataflow.PathInjectionCustomizations /** * Provides models for the `flask` PyPI package. @@ -525,13 +526,30 @@ module Flask { * * See https://flask.palletsprojects.com/en/1.1.x/api/#flask.send_from_directory */ - class FlaskSendFromDirectory extends FileSystemAccess::Range, DataFlow::CallCfgNode { - FlaskSendFromDirectory() { + private class FlaskSendFromDirectoryCall extends FileSystemAccess::Range, DataFlow::CallCfgNode { + FlaskSendFromDirectoryCall() { this = API::moduleImport("flask").getMember("send_from_directory").getACall() } override DataFlow::Node getAPathArgument() { - result in [this.getArg(_), this.getArgByName(["directory", "filename"])] + result in [ + this.getArg(0), this.getArgByName("directory"), + // as described in the docs, the `filename` argument is restrained to be within + // the provided directory, so is not exposed to path-injection. (but is still a + // path-argument). + this.getArg(1), this.getArgByName("filename") + ] + } + } + + /** + * To exclude `filename` argument to `flask.send_from_directory` as a path-injection sink. + */ + private class FlaskSendFromDirectoryCallFilenameSanitizer extends PathInjection::Sanitizer { + FlaskSendFromDirectoryCallFilenameSanitizer() { + this = any(FlaskSendFromDirectoryCall c).getArg(1) + or + this = any(FlaskSendFromDirectoryCall c).getArgByName("filename") } } @@ -540,8 +558,8 @@ module Flask { * * See https://flask.palletsprojects.com/en/1.1.x/api/#flask.send_file */ - class FlaskSendFile extends FileSystemAccess::Range, DataFlow::CallCfgNode { - FlaskSendFile() { this = API::moduleImport("flask").getMember("send_file").getACall() } + private class FlaskSendFileCall extends FileSystemAccess::Range, DataFlow::CallCfgNode { + FlaskSendFileCall() { this = API::moduleImport("flask").getMember("send_file").getACall() } override DataFlow::Node getAPathArgument() { result in [this.getArg(0), this.getArgByName("filename_or_fp")] diff --git a/python/ql/lib/semmle/python/security/dataflow/PathInjection.qll b/python/ql/lib/semmle/python/security/dataflow/PathInjection.qll index 30570c32f58e..827fe806d71c 100644 --- a/python/ql/lib/semmle/python/security/dataflow/PathInjection.qll +++ b/python/ql/lib/semmle/python/security/dataflow/PathInjection.qll @@ -26,7 +26,11 @@ class PathNotNormalizedConfiguration extends TaintTracking::Configuration { override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } - override predicate isSanitizer(DataFlow::Node node) { node instanceof Path::PathNormalization } + override predicate isSanitizer(DataFlow::Node node) { + node instanceof Sanitizer + or + node instanceof Path::PathNormalization + } override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { guard instanceof SanitizerGuard @@ -52,6 +56,8 @@ class FirstNormalizationConfiguration extends TaintTracking::Configuration { override predicate isSink(DataFlow::Node sink) { sink instanceof Path::PathNormalization } + override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer } + override predicate isSanitizerOut(DataFlow::Node node) { node instanceof Path::PathNormalization } override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { @@ -67,6 +73,8 @@ class NormalizedPathNotCheckedConfiguration extends TaintTracking2::Configuratio override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } + override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer } + override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { guard instanceof Path::SafeAccessCheck or diff --git a/python/ql/lib/semmle/python/security/dataflow/PathInjectionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/PathInjectionCustomizations.qll index 21a6a2093e52..410eee50b299 100644 --- a/python/ql/lib/semmle/python/security/dataflow/PathInjectionCustomizations.qll +++ b/python/ql/lib/semmle/python/security/dataflow/PathInjectionCustomizations.qll @@ -32,6 +32,16 @@ module PathInjection { */ abstract class Sink extends DataFlow::Node { } + /** + * A sanitizer for "path injection" vulnerabilities. + * + * This should only be used for things like calls to library functions that perform their own + * (correct) normalization/escaping of untrusted paths. + * + * Please also see `Path::SafeAccessCheck` and `Path::PathNormalization` Concepts. + */ + abstract class Sanitizer extends DataFlow::Node { } + /** * A sanitizer guard for "path injection" vulnerabilities. */ diff --git a/python/ql/test/library-tests/frameworks/flask/file_sending.py b/python/ql/test/library-tests/frameworks/flask/file_sending.py new file mode 100644 index 000000000000..9abebda146da --- /dev/null +++ b/python/ql/test/library-tests/frameworks/flask/file_sending.py @@ -0,0 +1,7 @@ +from flask import send_from_directory, send_file + +send_from_directory("dir", "file") # $ getAPathArgument="dir" getAPathArgument="file" +send_from_directory(directory="dir", filename="file") # $ getAPathArgument="dir" getAPathArgument="file" + +send_file("file") # $ getAPathArgument="file" +send_file(filename_or_fp="file") # $ getAPathArgument="file" diff --git a/python/ql/test/library-tests/frameworks/flask/save_uploaded_file.py b/python/ql/test/library-tests/frameworks/flask/save_uploaded_file.py index b61de94a3651..8aba97d1876f 100644 --- a/python/ql/test/library-tests/frameworks/flask/save_uploaded_file.py +++ b/python/ql/test/library-tests/frameworks/flask/save_uploaded_file.py @@ -1,17 +1,6 @@ -from flask import Flask, request, send_from_directory, send_file +from flask import Flask, request app = Flask(__name__) @app.route("/save-uploaded-file") # $routeSetup="/save-uploaded-file" def test_taint(): # $requestHandler request.files['key'].save("path") # $ getAPathArgument="path" - - -@app.route("/path-injection") # $routeSetup="/path-injection" -def test_path(): # $requestHandler - - send_from_directory("filepath","file") # $ getAPathArgument="filepath" getAPathArgument="file" - send_file("file") # $ getAPathArgument="file" - - send_from_directory(directory="filepath","file") # $ getAPathArgument="filepath" getAPathArgument="file" - send_from_directory(filename="filepath","file") # $ getAPathArgument="filepath" getAPathArgument="file" - send_file(filename_or_fp="file") # $ getAPathArgument="file" diff --git a/python/ql/test/query-tests/Security/CWE-022-PathInjection/PathInjection.expected b/python/ql/test/query-tests/Security/CWE-022-PathInjection/PathInjection.expected index 446e80f33f14..1229e3ba716e 100644 --- a/python/ql/test/query-tests/Security/CWE-022-PathInjection/PathInjection.expected +++ b/python/ql/test/query-tests/Security/CWE-022-PathInjection/PathInjection.expected @@ -1,4 +1,6 @@ edges +| flask_path_injection.py:19:15:19:21 | ControlFlowNode for request | flask_path_injection.py:19:15:19:26 | ControlFlowNode for Attribute | +| flask_path_injection.py:19:15:19:26 | ControlFlowNode for Attribute | flask_path_injection.py:21:32:21:38 | ControlFlowNode for dirname | | path_injection.py:12:16:12:22 | ControlFlowNode for request | path_injection.py:12:16:12:27 | ControlFlowNode for Attribute | | path_injection.py:12:16:12:27 | ControlFlowNode for Attribute | path_injection.py:13:14:13:47 | ControlFlowNode for Attribute() | | path_injection.py:19:16:19:22 | ControlFlowNode for request | path_injection.py:19:16:19:27 | ControlFlowNode for Attribute | @@ -68,6 +70,9 @@ edges | test_chaining.py:41:9:41:16 | ControlFlowNode for source() | test_chaining.py:42:9:42:19 | ControlFlowNode for normpath() | | test_chaining.py:44:13:44:23 | ControlFlowNode for normpath() | test_chaining.py:45:14:45:14 | ControlFlowNode for z | nodes +| flask_path_injection.py:19:15:19:21 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | +| flask_path_injection.py:19:15:19:26 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | +| flask_path_injection.py:21:32:21:38 | ControlFlowNode for dirname | semmle.label | ControlFlowNode for dirname | | path_injection.py:12:16:12:22 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | | path_injection.py:12:16:12:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | | path_injection.py:13:14:13:47 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | @@ -153,6 +158,7 @@ nodes | test_chaining.py:44:13:44:23 | ControlFlowNode for normpath() | semmle.label | ControlFlowNode for normpath() | | test_chaining.py:45:14:45:14 | ControlFlowNode for z | semmle.label | ControlFlowNode for z | #select +| flask_path_injection.py:21:32:21:38 | ControlFlowNode for dirname | flask_path_injection.py:19:15:19:21 | ControlFlowNode for request | flask_path_injection.py:21:32:21:38 | ControlFlowNode for dirname | This path depends on $@. | flask_path_injection.py:19:15:19:21 | ControlFlowNode for request | a user-provided value | | path_injection.py:13:14:13:47 | ControlFlowNode for Attribute() | path_injection.py:12:16:12:22 | ControlFlowNode for request | path_injection.py:13:14:13:47 | ControlFlowNode for Attribute() | This path depends on $@. | path_injection.py:12:16:12:22 | ControlFlowNode for request | a user-provided value | | path_injection.py:21:14:21:18 | ControlFlowNode for npath | path_injection.py:19:16:19:22 | ControlFlowNode for request | path_injection.py:21:14:21:18 | ControlFlowNode for npath | This path depends on $@. | path_injection.py:19:16:19:22 | ControlFlowNode for request | a user-provided value | | path_injection.py:31:14:31:18 | ControlFlowNode for npath | path_injection.py:27:16:27:22 | ControlFlowNode for request | path_injection.py:31:14:31:18 | ControlFlowNode for npath | This path depends on $@. | path_injection.py:27:16:27:22 | ControlFlowNode for request | a user-provided value | diff --git a/python/ql/test/query-tests/Security/CWE-022-PathInjection/flask_path_injection.py b/python/ql/test/query-tests/Security/CWE-022-PathInjection/flask_path_injection.py new file mode 100644 index 000000000000..ee531fead469 --- /dev/null +++ b/python/ql/test/query-tests/Security/CWE-022-PathInjection/flask_path_injection.py @@ -0,0 +1,21 @@ +from flask import Flask, request, send_from_directory +app = Flask(__name__) + + +STATIC_DIR = "/server/static/" + + +# see https://flask.palletsprojects.com/en/1.1.x/api/#flask.send_from_directory +@app.route("/provide-filename") +def download_file(): + filename = request.args.get('filename', '') + # ok since `send_from_directory` ensure this stays within `STATIC_DIR` + return send_from_directory(STATIC_DIR, filename) # OK + + +# see https://flask.palletsprojects.com/en/1.1.x/api/#flask.send_from_directory +@app.route("/also-provide-dirname") +def download_file(): + dirname = request.args.get('dirname', '') + filename = request.args.get('filename', '') + return send_from_directory(dirname, filename) # NOT OK