Merge pull request #6989 from RasmusWL/flask-file-sending-fixup

Python: Small fixup for `flask.send_from_directory`

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).

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")]

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

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.
    */

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"

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"

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 |

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