Python: sinks -> decodings

Query operators that interpret JavaScript are no longer considered sinks. Instead they are considered decodings and the output is the tainted dictionary. The state changes to `DictInput` to reflect that the user now controls a dangerous dictionary. This fixes the spurious result and moves the error reporting to a more logical place.
2026-04-26 09:15:12 +02:00 · 2023-09-11 16:33:20 +02:00
parent d9f63e1ed3
commit a063d7d510
4 changed files with 44 additions and 27 deletions
--- a/python/ql/lib/semmle/python/frameworks/NoSQL.qll
+++ b/python/ql/lib/semmle/python/frameworks/NoSQL.qll
@@ -123,40 +123,49 @@ private module NoSql {
  }

  /** The `$where` query operator executes a string as JavaScript. */
-  private class WhereQueryOperator extends API::CallNode, NoSqlQuery::Range {
+  private class WhereQueryOperator extends DataFlow::Node, Decoding::Range {
+    API::Node dictionary;
    DataFlow::Node query;

    WhereQueryOperator() {
-      this = mongoCollection().getMember(mongoCollectionMethodName()).getACall() and
-      query = this.getParameter(0).getSubscript("$where").asSink()
+      dictionary =
+        mongoCollection().getMember(mongoCollectionMethodName()).getACall().getParameter(0) and
+      query = dictionary.getSubscript("$where").asSink() and
+      this = dictionary.asSink()
    }

-    override DataFlow::Node getQuery() { result = query }
+    override DataFlow::Node getAnInput() { result = query }

-    override predicate interpretsDict() { none() }
+    override DataFlow::Node getOutput() { result = this }

-    override predicate vulnerableToStrings() { any() }
+    override string getFormat() { result = "NoSQL" }
+
+    override predicate mayExecuteInput() { any() }
  }

  /** The `$function` query operator executes its `body` string as JavaScript. */
-  private class FunctionQueryOperator extends API::CallNode, NoSqlQuery::Range {
+  private class FunctionQueryOperator extends DataFlow::Node, Decoding::Range {
+    API::Node dictionary;
    DataFlow::Node query;

    FunctionQueryOperator() {
-      this = mongoCollection().getMember(mongoCollectionMethodName()).getACall() and
-      query =
-        this.getParameter(0)
-            .getASubscript*()
-            .getSubscript("$function")
-            .getSubscript("body")
-            .asSink()
+      dictionary =
+        mongoCollection()
+            .getMember(mongoCollectionMethodName())
+            .getACall()
+            .getParameter(0)
+            .getASubscript*() and
+      query = dictionary.getSubscript("$function").getSubscript("body").asSink() and
+      this = dictionary.asSink()
    }

-    override DataFlow::Node getQuery() { result = query }
+    override DataFlow::Node getAnInput() { result = query }

-    override predicate interpretsDict() { none() }
+    override DataFlow::Node getOutput() { result = this }

-    override predicate vulnerableToStrings() { any() }
+    override string getFormat() { result = "NoSQL" }
+
+    override predicate mayExecuteInput() { any() }
  }

  /**
--- a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll
+++ b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll
@@ -75,7 +75,7 @@ module NoSqlInjection {

  /** A JSON decoding converts a string to a dictionary. */
  class JsonDecoding extends Decoding, StringToDictConversion {
-    JsonDecoding() { this.getFormat() = "JSON" }
+    JsonDecoding() { this.getFormat() in ["JSON", "NoSQL"] }

    override DataFlow::Node getAnInput() { result = Decoding.super.getAnInput() }

--- a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected
+++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected
@@ -2,14 +2,19 @@ edges
 | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | PoC/server.py:1:26:1:32 | GSSA Variable request |
 | PoC/server.py:1:26:1:32 | GSSA Variable request | PoC/server.py:26:21:26:27 | ControlFlowNode for request |
 | PoC/server.py:1:26:1:32 | GSSA Variable request | PoC/server.py:42:14:42:20 | ControlFlowNode for request |
+| PoC/server.py:1:26:1:32 | GSSA Variable request | PoC/server.py:51:14:51:20 | ControlFlowNode for request |
 | PoC/server.py:26:5:26:17 | SSA variable author_string | PoC/server.py:27:25:27:37 | ControlFlowNode for author_string |
 | PoC/server.py:26:21:26:27 | ControlFlowNode for request | PoC/server.py:26:5:26:17 | SSA variable author_string |
 | PoC/server.py:27:5:27:10 | SSA variable author | PoC/server.py:30:27:30:44 | ControlFlowNode for Dict |
 | PoC/server.py:27:14:27:38 | ControlFlowNode for Attribute() | PoC/server.py:27:5:27:10 | SSA variable author |
 | PoC/server.py:27:25:27:37 | ControlFlowNode for author_string | PoC/server.py:27:14:27:38 | ControlFlowNode for Attribute() |
-| PoC/server.py:42:5:42:10 | SSA variable author | PoC/server.py:46:27:46:68 | ControlFlowNode for Dict |
 | PoC/server.py:42:5:42:10 | SSA variable author | PoC/server.py:46:38:46:67 | ControlFlowNode for BinaryExpr |
 | PoC/server.py:42:14:42:20 | ControlFlowNode for request | PoC/server.py:42:5:42:10 | SSA variable author |
+| PoC/server.py:46:38:46:67 | ControlFlowNode for BinaryExpr | PoC/server.py:46:27:46:68 | ControlFlowNode for Dict |
+| PoC/server.py:51:5:51:10 | SSA variable author | PoC/server.py:53:17:53:70 | ControlFlowNode for BinaryExpr |
+| PoC/server.py:51:14:51:20 | ControlFlowNode for request | PoC/server.py:51:5:51:10 | SSA variable author |
+| PoC/server.py:53:17:53:70 | ControlFlowNode for BinaryExpr | PoC/server.py:60:37:60:57 | ControlFlowNode for Dict |
+| PoC/server.py:60:37:60:57 | ControlFlowNode for Dict | PoC/server.py:60:27:60:58 | ControlFlowNode for Dict |
 | flask_mongoengine_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | flask_mongoengine_bad.py:1:26:1:32 | GSSA Variable request |
 | flask_mongoengine_bad.py:1:26:1:32 | GSSA Variable request | flask_mongoengine_bad.py:19:21:19:27 | ControlFlowNode for request |
 | flask_mongoengine_bad.py:1:26:1:32 | GSSA Variable request | flask_mongoengine_bad.py:26:21:26:27 | ControlFlowNode for request |
@@ -76,16 +81,16 @@ edges
 | pymongo_test.py:13:5:13:15 | SSA variable json_search | pymongo_test.py:15:42:15:62 | ControlFlowNode for Dict |
 | pymongo_test.py:13:19:13:43 | ControlFlowNode for Attribute() | pymongo_test.py:13:5:13:15 | SSA variable json_search |
 | pymongo_test.py:13:30:13:42 | ControlFlowNode for unsafe_search | pymongo_test.py:13:19:13:43 | ControlFlowNode for Attribute() |
-| pymongo_test.py:29:5:29:12 | SSA variable event_id | pymongo_test.py:33:34:33:73 | ControlFlowNode for Dict |
 | pymongo_test.py:29:5:29:12 | SSA variable event_id | pymongo_test.py:33:45:33:72 | ControlFlowNode for Fstring |
 | pymongo_test.py:29:16:29:51 | ControlFlowNode for Attribute() | pymongo_test.py:29:5:29:12 | SSA variable event_id |
 | pymongo_test.py:29:27:29:33 | ControlFlowNode for request | pymongo_test.py:29:27:29:50 | ControlFlowNode for Subscript |
 | pymongo_test.py:29:27:29:50 | ControlFlowNode for Subscript | pymongo_test.py:29:16:29:51 | ControlFlowNode for Attribute() |
-| pymongo_test.py:39:5:39:12 | SSA variable event_id | pymongo_test.py:43:34:43:73 | ControlFlowNode for Dict |
+| pymongo_test.py:33:45:33:72 | ControlFlowNode for Fstring | pymongo_test.py:33:34:33:73 | ControlFlowNode for Dict |
 | pymongo_test.py:39:5:39:12 | SSA variable event_id | pymongo_test.py:43:45:43:72 | ControlFlowNode for Fstring |
 | pymongo_test.py:39:16:39:51 | ControlFlowNode for Attribute() | pymongo_test.py:39:5:39:12 | SSA variable event_id |
 | pymongo_test.py:39:27:39:33 | ControlFlowNode for request | pymongo_test.py:39:27:39:50 | ControlFlowNode for Subscript |
 | pymongo_test.py:39:27:39:50 | ControlFlowNode for Subscript | pymongo_test.py:39:16:39:51 | ControlFlowNode for Attribute() |
+| pymongo_test.py:43:45:43:72 | ControlFlowNode for Fstring | pymongo_test.py:43:34:43:73 | ControlFlowNode for Dict |
 nodes
 | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | semmle.label | ControlFlowNode for ImportMember |
 | PoC/server.py:1:26:1:32 | GSSA Variable request | semmle.label | GSSA Variable request |
@@ -99,6 +104,11 @@ nodes
 | PoC/server.py:42:14:42:20 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
 | PoC/server.py:46:27:46:68 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict |
 | PoC/server.py:46:38:46:67 | ControlFlowNode for BinaryExpr | semmle.label | ControlFlowNode for BinaryExpr |
+| PoC/server.py:51:5:51:10 | SSA variable author | semmle.label | SSA variable author |
+| PoC/server.py:51:14:51:20 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
+| PoC/server.py:53:17:53:70 | ControlFlowNode for BinaryExpr | semmle.label | ControlFlowNode for BinaryExpr |
+| PoC/server.py:60:27:60:58 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict |
+| PoC/server.py:60:37:60:57 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict |
 | flask_mongoengine_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | semmle.label | ControlFlowNode for ImportMember |
 | flask_mongoengine_bad.py:1:26:1:32 | GSSA Variable request | semmle.label | GSSA Variable request |
 | flask_mongoengine_bad.py:19:5:19:17 | SSA variable unsafe_search | semmle.label | SSA variable unsafe_search |
@@ -183,7 +193,7 @@ subpaths
 #select
 | PoC/server.py:30:27:30:44 | ControlFlowNode for Dict | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | PoC/server.py:30:27:30:44 | ControlFlowNode for Dict | This NoSQL query contains an unsanitized $@. | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
 | PoC/server.py:46:27:46:68 | ControlFlowNode for Dict | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | PoC/server.py:46:27:46:68 | ControlFlowNode for Dict | This NoSQL query contains an unsanitized $@. | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
-| PoC/server.py:46:38:46:67 | ControlFlowNode for BinaryExpr | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | PoC/server.py:46:38:46:67 | ControlFlowNode for BinaryExpr | This NoSQL query contains an unsanitized $@. | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
+| PoC/server.py:60:27:60:58 | ControlFlowNode for Dict | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | PoC/server.py:60:27:60:58 | ControlFlowNode for Dict | This NoSQL query contains an unsanitized $@. | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
 | flask_mongoengine_bad.py:22:34:22:44 | ControlFlowNode for json_search | flask_mongoengine_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | flask_mongoengine_bad.py:22:34:22:44 | ControlFlowNode for json_search | This NoSQL query contains an unsanitized $@. | flask_mongoengine_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
 | flask_mongoengine_bad.py:30:39:30:59 | ControlFlowNode for Dict | flask_mongoengine_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | flask_mongoengine_bad.py:30:39:30:59 | ControlFlowNode for Dict | This NoSQL query contains an unsanitized $@. | flask_mongoengine_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
 | flask_pymongo_bad.py:14:31:14:51 | ControlFlowNode for Dict | flask_pymongo_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | flask_pymongo_bad.py:14:31:14:51 | ControlFlowNode for Dict | This NoSQL query contains an unsanitized $@. | flask_pymongo_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
@@ -195,6 +205,4 @@ subpaths
 | mongoengine_bad.py:61:29:61:49 | ControlFlowNode for Dict | mongoengine_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | mongoengine_bad.py:61:29:61:49 | ControlFlowNode for Dict | This NoSQL query contains an unsanitized $@. | mongoengine_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
 | pymongo_test.py:15:42:15:62 | ControlFlowNode for Dict | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | pymongo_test.py:15:42:15:62 | ControlFlowNode for Dict | This NoSQL query contains an unsanitized $@. | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
 | pymongo_test.py:33:34:33:73 | ControlFlowNode for Dict | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | pymongo_test.py:33:34:33:73 | ControlFlowNode for Dict | This NoSQL query contains an unsanitized $@. | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
-| pymongo_test.py:33:45:33:72 | ControlFlowNode for Fstring | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | pymongo_test.py:33:45:33:72 | ControlFlowNode for Fstring | This NoSQL query contains an unsanitized $@. | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
 | pymongo_test.py:43:34:43:73 | ControlFlowNode for Dict | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | pymongo_test.py:43:34:43:73 | ControlFlowNode for Dict | This NoSQL query contains an unsanitized $@. | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
-| pymongo_test.py:43:45:43:72 | ControlFlowNode for Fstring | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | pymongo_test.py:43:45:43:72 | ControlFlowNode for Fstring | This NoSQL query contains an unsanitized $@. | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
--- a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/server.py
+++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/server.py
@@ -50,7 +50,7 @@ def by_where():
 def by_function():
    author = request.args['author']
    search = {
-        "body": 'function(author) { return(author === "'+author+'") }', # $ result=BAD
+        "body": 'function(author) { return(author === "'+author+'") }',
        "args": [ "$author" ],
        "lang": "js"
    }
@@ -64,11 +64,11 @@ def by_function():
 def by_function_arg():
    author = request.args['author']
    search = {
-        "body": 'function(author, target) { return(author === target) }', # $ result=OK
+        "body": 'function(author, target) { return(author === target) }',
        "args": [ "$author", author ],
        "lang": "js"
    }
-    post = posts.find_one({'$expr': {'$function': search}}) # $ SPURIOUS: result=BAD
+    post = posts.find_one({'$expr': {'$function': search}}) # $ result=OK
    return show_post(post, author)

@app.route('/', methods=['GET'])