github · yoff · Sep 29, 2023 · Aug 15, 2023 · Aug 15, 2023 · Aug 23, 2023
@@ -378,6 +378,68 @@ module SqlExecution {
   }
 }
 
+/** Provides a class for modeling NoSql execution APIs. */
+module NoSqlQuery {
+  /**
+   * A data-flow node that executes NoSQL queries.
+   *
+   * Extend this class to model new APIs. If you want to refine existing API models,
+   * extend `NoSQLQuery` instead.
+   */
+  abstract class Range extends DataFlow::Node {
+    /** Gets the argument that specifies the NoSql query to be executed. */
+    abstract DataFlow::Node getQuery();
+
+    /** Holds if this query will unpack/interpret a dictionary */
+    abstract predicate interpretsDict();
+
+    /** Holds if this query can be dangerous when run on a user-controlled string */
+    abstract predicate vulnerableToStrings();
+  }
+}
+
+/**
+ * A data-flow node that executes NoSQL queries.
+ *
+ * Extend this class to refine existing API models. If you want to model new APIs,
+ * extend `NoSQLQuery::Range` instead.
+ */
+class NoSqlQuery extends DataFlow::Node instanceof NoSqlQuery::Range {
+  /** Gets the argument that specifies the NoSql query to be executed. */
+  DataFlow::Node getQuery() { result = super.getQuery() }
+
+  /** Holds if this query will unpack/interpret a dictionary */
+  predicate interpretsDict() { super.interpretsDict() }
+
+  /** Holds if this query can be dangerous when run on a user-controlled string */
+  predicate vulnerableToStrings() { super.vulnerableToStrings() }
+}
+
+/** Provides classes for modeling NoSql sanitization-related APIs. */
+module NoSqlSanitizer {
+  /**
+   * A data-flow node that collects functions sanitizing NoSQL queries.
+   *
+   * Extend this class to model new APIs. If you want to refine existing API models,
+   * extend `NoSQLSanitizer` instead.
+   */
+  abstract class Range extends DataFlow::Node {
+    /** Gets the argument that specifies the NoSql query to be sanitized. */
+    abstract DataFlow::Node getAnInput();
+  }
+}
+
+/**
+ * A data-flow node that collects functions sanitizing NoSQL queries.
+ *
+ * Extend this class to model new APIs. If you want to refine existing API models,
+ * extend `NoSQLSanitizer::Range` instead.
+ */
+class NoSqlSanitizer extends DataFlow::Node instanceof NoSqlSanitizer::Range {
+  /** Gets the argument that specifies the NoSql query to be sanitized. */
+  DataFlow::Node getAnInput() { result = super.getAnInput() }
+}
+
 /**
  * A data-flow node that executes a regular expression.
  *

@@ -36,6 +36,7 @@ private import semmle.python.frameworks.MarkupSafe
 private import semmle.python.frameworks.Multidict
 private import semmle.python.frameworks.Mysql
 private import semmle.python.frameworks.MySQLdb
+private import semmle.python.frameworks.NoSQL
 private import semmle.python.frameworks.Oracledb
 private import semmle.python.frameworks.Peewee
 private import semmle.python.frameworks.Phoenixdb

diff --git a/...mental/semmle/python/frameworks/NoSQL.qll → ...ql/lib/semmle/python/frameworks/NoSQL.qll b/...mental/semmle/python/frameworks/NoSQL.qll → ...ql/lib/semmle/python/frameworks/NoSQL.qll
@@ -7,7 +7,7 @@ private import python
 private import semmle.python.dataflow.new.DataFlow
 private import semmle.python.dataflow.new.TaintTracking
 private import semmle.python.dataflow.new.RemoteFlowSources
-private import experimental.semmle.python.Concepts
+private import semmle.python.Concepts
 private import semmle.python.ApiGraphs
 
 private module NoSql {
@@ -91,16 +91,17 @@ private module NoSql {
     result = mongoDBInstance().getMember(["get_collection", "create_collection"]).getReturn()
   }
 
-  /** This class represents names of find_* relevant `Mongo` collection-level operation methods. */
-  private class MongoCollectionMethodNames extends string {
-    MongoCollectionMethodNames() {
-      this in [
-          "find", "find_raw_batches", "find_one", "find_one_and_delete", "find_and_modify",
-          "find_one_and_replace", "find_one_and_update", "find_one_or_404"
-        ]
-    }
+  /** Gets the name of a find_* relevant `Mongo` collection-level operation method. */
+  private string mongoCollectionMethodName() {
+    result in [
+        "find", "find_raw_batches", "find_one", "find_one_and_delete", "find_and_modify",
+        "find_one_and_replace", "find_one_and_update", "find_one_or_404"
+      ]
   }
 
+  /** Gets the name of a mongo query operator that will interpret JavaScript. */
+  private string mongoQueryOperator() { result in ["$where", "$function"] }
+
   /**
    * Gets a reference to a `Mongo` collection method call
    *
@@ -114,10 +115,29 @@ private module NoSql {
    */
   private class MongoCollectionCall extends DataFlow::CallCfgNode, NoSqlQuery::Range {
     MongoCollectionCall() {
-      this = mongoCollection().getMember(any(MongoCollectionMethodNames m)).getACall()
+      this = mongoCollection().getMember(mongoCollectionMethodName()).getACall()
     }
 
     override DataFlow::Node getQuery() { result = this.getArg(0) }
+
+    override predicate interpretsDict() { any() }
+
+    override predicate vulnerableToStrings() { none() }
+  }
+
+  private class MongoCollectionQueryOperator extends API::CallNode, NoSqlQuery::Range {
+    DataFlow::Node query;
+
+    MongoCollectionQueryOperator() {
+      this = mongoCollection().getMember(mongoCollectionMethodName()).getACall() and
+      query = this.getParameter(0).getSubscript(mongoQueryOperator()).asSink()
+    }
+
+    override DataFlow::Node getQuery() { result = query }
+
+    override predicate interpretsDict() { none() }
+
+    override predicate vulnerableToStrings() { any() }
   }
 
   /**
@@ -146,6 +166,10 @@ private module NoSql {
     }
 
     override DataFlow::Node getQuery() { result = this.getArgByName(_) }
+
+    override predicate interpretsDict() { any() }
+
+    override predicate vulnerableToStrings() { none() }
   }
 
   /** Gets a reference to `mongosanitizer.sanitizer.sanitize` */
@@ -176,4 +200,23 @@ private module NoSql {
 
     override DataFlow::Node getAnInput() { result = this.getArg(0) }
   }
+
+  /**
+   * An equality operator can protect against dictionary interpretation.
+   * For instance, in `{'password': {"$eq": password} }`, if a dictionary is injected into
+   * `password`, it will not match.
+   */
+  private class EqualityOperator extends DataFlow::Node, NoSqlSanitizer::Range {
+    EqualityOperator() {
+      this =
+        mongoCollection()
+            .getMember(mongoCollectionMethodName())
+            .getParameter(0)
+            .getASubscript*()
+            .getSubscript("$eq")
+            .asSink()
+    }
+
+    override DataFlow::Node getAnInput() { result = this }
+  }
 }
diff --git a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll
@@ -0,0 +1,84 @@
+/**
+ * Provides default sources, sinks and sanitizers for detecting
+ * "NoSql injection"
+ * vulnerabilities, as well as extension points for adding your own.
+ */
+
+import python
+import semmle.python.dataflow.new.DataFlow
+import semmle.python.dataflow.new.RemoteFlowSources
+import semmle.python.Concepts
+
+/**
+ * Provides default sources, sinks and sanitizers for detecting
+ * "NoSql injection"
+ * vulnerabilities, as well as extension points for adding your own.
+ */
+module NoSqlInjection {
+  private newtype TFlowState =
+    TStringInput() or
+    TDictInput()
+
+  /** A flow state, tracking the structure of the input. */
+  abstract class FlowState extends TFlowState {
+    /** Gets a textual representation of this element. */
+    abstract string toString();
+  }
+
+  /** A state where input is only a string. */
+  class StringInput extends FlowState, TStringInput {
+    override string toString() { result = "StringInput" }
+  }
+
+  /** A state where input is a dictionary. */
+  class DictInput extends FlowState, TDictInput {
+    override string toString() { result = "DictInput" }
+  }
+
+  /** A source allowing string inputs. */
+  abstract class StringSource extends DataFlow::Node { }
+
+  /** A source allowing dictionary inputs. */
+  abstract class DictSource extends DataFlow::Node { }
+
+  /** A sink vulnerable to user controlled strings. */
+  abstract class StringSink extends DataFlow::Node { }
+
+  /** A sink vulnerable to user controlled dictionaries. */
+  abstract class DictSink extends DataFlow::Node { }
+
+  /** A data flow node where a string is converted into a dictionary. */
+  abstract class StringToDictConversion extends DataFlow::Node {
+    /** Gets the argument that specifies the string to be converted. */
+    abstract DataFlow::Node getAnInput();
+
+    /** Gets the resulting dictionary. */
+    abstract DataFlow::Node getOutput();
+  }
+
+  /** A remote flow source considered a source of user controlled strings. */
+  class RemoteFlowSourceAsStringSource extends RemoteFlowSource, StringSource { }
+
+  /** A NoSQL query that is vulnerable to user controlled strings. */
+  class NoSqlQueryAsStringSink extends StringSink {
+    NoSqlQueryAsStringSink() {
+      exists(NoSqlQuery noSqlQuery | this = noSqlQuery.getQuery() |
+        noSqlQuery.vulnerableToStrings()
+      )
+    }
+  }
+
+  /** A NoSQL query that is vulnerable to user controlled dictionaries. */
+  class NoSqlQueryAsDictSink extends DictSink {
+    NoSqlQueryAsDictSink() { this = any(NoSqlQuery noSqlQuery).getQuery() }
+  }
+
+  /** A JSON decoding converts a string to a dictionary. */
+  class JsonDecoding extends Decoding, StringToDictConversion {
+    JsonDecoding() { this.getFormat() = "JSON" }
+
+    override DataFlow::Node getAnInput() { result = Decoding.super.getAnInput() }
+
+    override DataFlow::Node getOutput() { result = Decoding.super.getOutput() }
+  }
+}
diff --git a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll
@@ -0,0 +1,60 @@
+/**
+ * Provides a taint-tracking configuration for detecting NoSQL injection vulnerabilities
+ */
+
+import python
+import semmle.python.dataflow.new.DataFlow
+import semmle.python.dataflow.new.TaintTracking
+import semmle.python.Concepts
+private import NoSQLInjectionCustomizations::NoSqlInjection as C
+
+/**
+ * A taint-tracking configuration for detecting NoSQL injection vulnerabilities.
+ */
+module NoSQLInjectionConfig implements DataFlow::StateConfigSig {
+  class FlowState = C::FlowState;
+
+  predicate isSource(DataFlow::Node source, FlowState state) {
+    source instanceof C::StringSource and
+    state instanceof C::StringInput
+    or
+    source instanceof C::DictSource and
+    state instanceof C::DictInput
+  }
+
+  predicate isSink(DataFlow::Node source, FlowState state) {
+    source instanceof C::StringSink and
+    (
+      state instanceof C::StringInput
+      or
+      // since dictionaries can encode strings
+      state instanceof C::DictInput
+    )
+    or
+    source instanceof C::DictSink and
+    state instanceof C::DictInput
+  }
+
+  predicate isBarrier(DataFlow::Node node, FlowState state) {
+    // Block `StringInput` paths here, since they change state to `DictInput`
+    exists(C::StringToDictConversion c | node = c.getOutput()) and
+    state instanceof C::StringInput
+  }
-  predicate isSink(DataFlow::Node source, FlowState state) {
-    source instanceof C::StringSink and
-    (
-      state instanceof C::StringInput
-      or
-      // since dictionaries can encode strings
-      state instanceof C::DictInput
-    )
-    or
-    source instanceof C::DictSink and
-    state instanceof C::DictInput
-  }
-
-  predicate isBarrier(DataFlow::Node node, FlowState state) {
-    // Block `StringInput` paths here, since they change state to `DictInput`
-    exists(C::StringToDictConversion c | node = c.getOutput()) and
-    state instanceof C::StringInput
-  }
+  predicate isSink(DataFlow::Node source, FlowState state) {
+    source instanceof C::StringSink and
+    state instanceof C::StringInput
+    or
+    source instanceof C::DictSink and
+    state instanceof C::DictInput
+  }
+
+  predicate isBarrier(DataFlow::Node node, FlowState state) {
+    // NOTE: not blocking `StringInput` when converted to Dictionary, since
+    // `StringToDictConversion` also covers things like parsing JSON objects,
+    // where you don't actually know if it's a string or a dictionary that is
+    // returned.
+  }
-  predicate isSink(DataFlow::Node source, FlowState state) {
-    source instanceof C::StringSink and
-    (
-      state instanceof C::StringInput
-      or
-      // since dictionaries can encode strings
-      state instanceof C::DictInput
-    )
-    or
-    source instanceof C::DictSink and
-    state instanceof C::DictInput
-  }
-
-  predicate isBarrier(DataFlow::Node node, FlowState state) {
-    // Block `StringInput` paths here, since they change state to `DictInput`
-    exists(C::StringToDictConversion c | node = c.getOutput()) and
-    state instanceof C::StringInput
-  }
+  predicate isSink(DataFlow::Node source, FlowState state) {
+    source instanceof C::StringSink and
+    state instanceof C::StringInput
+    or
+    source instanceof C::DictSink and
+    state instanceof C::DictInput
+  }
+
+  predicate isBarrier(DataFlow::Node node, FlowState state) {
+    // NOTE: not blocking `StringInput` when converted to Dictionary, since
+    // `StringToDictConversion` also covers things like parsing JSON objects,
+    // where you don't actually know if it's a string or a dictionary that is
+    // returned.
+  }
+
+  predicate isAdditionalFlowStep(
+    DataFlow::Node nodeFrom, FlowState stateFrom, DataFlow::Node nodeTo, FlowState stateTo
+  ) {
+    exists(C::StringToDictConversion c |
+      nodeFrom = c.getAnInput() and
+      nodeTo = c.getOutput()
+    ) and
+    stateFrom instanceof C::StringInput and
+    stateTo instanceof C::DictInput
+  }
+
+  predicate isBarrier(DataFlow::Node node) {
+    node = any(NoSqlSanitizer noSqlSanitizer).getAnInput()
+  }
+}
+
+module Flow = TaintTracking::GlobalWithState<NoSQLInjectionConfig>;
diff --git a/...tal/Security/CWE-943/NoSQLInjection.qhelp → ...src/Security/CWE-943/NoSQLInjection.qhelp b/...tal/Security/CWE-943/NoSQLInjection.qhelp → ...src/Security/CWE-943/NoSQLInjection.qhelp
diff --git a/...mental/Security/CWE-943/NoSQLInjection.ql → ...ql/src/Security/CWE-943/NoSQLInjection.ql b/...mental/Security/CWE-943/NoSQLInjection.ql → ...ql/src/Security/CWE-943/NoSQLInjection.ql
@@ -11,10 +11,10 @@
  */
 
 import python
-import experimental.semmle.python.security.injection.NoSQLInjection
-import DataFlow::PathGraph
+import semmle.python.security.dataflow.NoSQLInjectionQuery
+import Flow::PathGraph
 
-from NoSqlInjection::Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
-where config.hasFlowPath(source, sink)
-select sink, source, sink, "This NoSQL query contains an unsanitized $@.", source,
+from Flow::PathNode source, Flow::PathNode sink
+where Flow::flowPath(source, sink)
+select sink.getNode(), source, sink, "This NoSQL query contains an unsanitized $@.", source,
   "user-provided value"
@@ -216,56 +216,6 @@ class SqlEscape extends DataFlow::Node instanceof SqlEscape::Range {
   DataFlow::Node getAnInput() { result = super.getAnInput() }
 }
 
-/** Provides a class for modeling NoSql execution APIs. */
-module NoSqlQuery {
-  /**
-   * A data-flow node that executes NoSQL queries.
-   *
-   * Extend this class to model new APIs. If you want to refine existing API models,
-   * extend `NoSQLQuery` instead.
-   */
-  abstract class Range extends DataFlow::Node {
-    /** Gets the argument that specifies the NoSql query to be executed. */
-    abstract DataFlow::Node getQuery();
-  }
-}
-
-/**
- * A data-flow node that executes NoSQL queries.
- *
- * Extend this class to refine existing API models. If you want to model new APIs,
- * extend `NoSQLQuery::Range` instead.
- */
-class NoSqlQuery extends DataFlow::Node instanceof NoSqlQuery::Range {
-  /** Gets the argument that specifies the NoSql query to be executed. */
-  DataFlow::Node getQuery() { result = super.getQuery() }
-}
-
-/** Provides classes for modeling NoSql sanitization-related APIs. */
-module NoSqlSanitizer {
-  /**
-   * A data-flow node that collects functions sanitizing NoSQL queries.
-   *
-   * Extend this class to model new APIs. If you want to refine existing API models,
-   * extend `NoSQLSanitizer` instead.
-   */
-  abstract class Range extends DataFlow::Node {
-    /** Gets the argument that specifies the NoSql query to be sanitized. */
-    abstract DataFlow::Node getAnInput();
-  }
-}
-
-/**
- * A data-flow node that collects functions sanitizing NoSQL queries.
- *
- * Extend this class to model new APIs. If you want to refine existing API models,
- * extend `NoSQLSanitizer::Range` instead.
- */
-class NoSqlSanitizer extends DataFlow::Node instanceof NoSqlSanitizer::Range {
-  /** Gets the argument that specifies the NoSql query to be sanitized. */
-  DataFlow::Node getAnInput() { result = super.getAnInput() }
-}
-
 /** Provides classes for modeling HTTP Header APIs. */
 module HeaderDeclaration {
   /**