Merge pull request #21234 from owen-mc/python/convert-sanitizers-to-mad

Python: Allow models-as-data sanitizers

python/ql/lib/semmle/python/Concepts.qll

@@ -116,6 +116,16 @@ module SystemCommandExecution {
 class FileSystemAccess extends DataFlow::Node instanceof FileSystemAccess::Range {
   /** Gets an argument to this file system access that is interpreted as a path. */
   DataFlow::Node getAPathArgument() { result = super.getAPathArgument() }
+
+  /**
+   * Gets an argument to this file system access that is interpreted as a path
+   * which is vulnerable to path injection.
+   *
+   * By default all path arguments are considered vulnerable, but this can be overridden to
+   * exclude certain arguments that are known to be safe, for example because they are
+   * restricted to a specific directory.
+   */
+  DataFlow::Node getAVulnerablePathArgument() { result = super.getAVulnerablePathArgument() }
 }
 
 /** Provides a class for modeling new file system access APIs. */
@@ -130,6 +140,16 @@ module FileSystemAccess {
   abstract class Range extends DataFlow::Node {
     /** Gets an argument to this file system access that is interpreted as a path. */
     abstract DataFlow::Node getAPathArgument();
+
+    /**
+     * Gets an argument to this file system access that is interpreted as a path
+     * which is vulnerable to path injection.
+     *
+     * By default all path arguments are considered vulnerable, but this can be overridden to
+     * exclude certain arguments that are known to be safe, for example because they are
+     * restricted to a specific directory.
+     */
+    DataFlow::Node getAVulnerablePathArgument() { result = this.getAPathArgument() }
   }
 }
 

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

@@ -621,24 +621,15 @@ module Flask {
     }
 
     override DataFlow::Node getAPathArgument() {
-      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")
-        ]
+      result = this.getArg([0, 1]) or
+      result = this.getArgByName(["directory", "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")
+    override DataFlow::Node getAVulnerablePathArgument() {
+      result = this.getAPathArgument() and
+      // as described in the docs, the `filename` argument is restricted to be within
+      // the provided directory, so is not exposed to path-injection.
+      not result in [this.getArg(1), this.getArgByName("filename")]
     }
   }
 

python/ql/lib/semmle/python/security/dataflow/CodeInjectionCustomizations.qll

@@ -60,4 +60,11 @@ module CodeInjection {
 
   /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */
   deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;
+
+  /**
+   * A sanitizer defined via models-as-data with kind "code-injection".
+   */
+  class SanitizerFromModel extends Sanitizer {
+    SanitizerFromModel() { ModelOutput::barrierNode(this, "code-injection") }
+  }
 }

python/ql/lib/semmle/python/security/dataflow/CommandInjectionCustomizations.qll

@@ -95,4 +95,11 @@ module CommandInjection {
 
   /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */
   deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;
+
+  /**
+   * A sanitizer defined via models-as-data with kind "command-injection".
+   */
+  class SanitizerFromModel extends Sanitizer {
+    SanitizerFromModel() { ModelOutput::barrierNode(this, "command-injection") }
+  }
 }

python/ql/lib/semmle/python/security/dataflow/LogInjectionCustomizations.qll

@@ -106,4 +106,11 @@ module LogInjection {
       this.getArg(0).asExpr().(StringLiteral).getText() in ["\r\n", "\n"]
     }
   }
+
+  /**
+   * A sanitizer defined via models-as-data with kind "log-injection".
+   */
+  class SanitizerFromModel extends Sanitizer {
+    SanitizerFromModel() { ModelOutput::barrierNode(this, "log-injection") }
+  }
 }

python/ql/lib/semmle/python/security/dataflow/PathInjectionCustomizations.qll

@@ -57,7 +57,7 @@ module PathInjection {
    */
   class FileSystemAccessAsSink extends Sink {
     FileSystemAccessAsSink() {
-      this = any(FileSystemAccess e).getAPathArgument() and
+      this = any(FileSystemAccess e).getAVulnerablePathArgument() and
       // since implementation of Path.open in pathlib.py is like
       // ```py
       // def open(self, ...):
@@ -98,4 +98,11 @@ module PathInjection {
 
   /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */
   deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;
+
+  /**
+   * A sanitizer defined via models-as-data with kind "path-injection".
+   */
+  class SanitizerFromModel extends Sanitizer {
+    SanitizerFromModel() { ModelOutput::barrierNode(this, "path-injection") }
+  }
 }

python/ql/lib/semmle/python/security/dataflow/ReflectedXSSCustomizations.qll

@@ -84,4 +84,11 @@ module ReflectedXss {
 
   /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */
   deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;
+
+  /**
+   * A sanitizer defined via models-as-data with kind "html-injection" or "js-injection".
+   */
+  class SanitizerFromModel extends Sanitizer {
+    SanitizerFromModel() { ModelOutput::barrierNode(this, ["html-injection", "js-injection"]) }
+  }
 }

python/ql/lib/semmle/python/security/dataflow/SqlInjectionCustomizations.qll

@@ -69,4 +69,11 @@ module SqlInjection {
   private class DataAsSqlSink extends Sink {
     DataAsSqlSink() { ModelOutput::sinkNode(this, "sql-injection") }
   }
+
+  /**
+   * A sanitizer defined via models-as-data with kind "sql-injection".
+   */
+  class SanitizerFromModel extends Sanitizer {
+    SanitizerFromModel() { ModelOutput::barrierNode(this, "sql-injection") }
+  }
 }

python/ql/lib/semmle/python/security/dataflow/UnsafeDeserializationCustomizations.qll

@@ -65,4 +65,11 @@ module UnsafeDeserialization {
 
   /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */
   deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;
+
+  /**
+   * A sanitizer defined via models-as-data with kind "unsafe-deserialization".
+   */
+  class SanitizerFromModel extends Sanitizer {
+    SanitizerFromModel() { ModelOutput::barrierNode(this, "unsafe-deserialization") }
+  }
 }

python/ql/lib/semmle/python/security/dataflow/UrlRedirectCustomizations.qll

@@ -162,7 +162,8 @@ module UrlRedirect {
   deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;
 
   /**
-   * A sanitizer defined via models-as-data with kind "url-redirection".
+   * A sanitizer which sanitizes all flow states, defined via models-as-data
+   * with kind "url-redirection".
    */
   class SanitizerFromModel extends Sanitizer {
     SanitizerFromModel() { ModelOutput::barrierNode(this, "url-redirection") }

python/ql/lib/utils/test/PrettyPrintModels.ql

@@ -0,0 +1,6 @@
+/**
+ * @kind test-postprocess
+ */
+
+import semmle.python.frameworks.data.internal.ApiGraphModels
+import codeql.dataflow.test.ProvenancePathGraph::TestPostProcessing::TranslateProvenanceResults<interpretModelForTest/2>