From 60dc1afbc06358070edf1c28a875917d97194c38 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Tue, 15 Aug 2023 10:01:20 +0200 Subject: [PATCH 01/45] Python: prepare to promote NoSqlInjection Mostly move files, preserving authourship. This will not compile. --- python/ql/lib/semmle/python/Frameworks.qll | 1 + .../{src/experimental => lib}/semmle/python/frameworks/NoSQL.qll | 0 .../semmle/python/security/dataflow}/NoSQLInjection.qll | 0 .../src/{experimental => }/Security/CWE-943/NoSQLInjection.qhelp | 0 .../ql/src/{experimental => }/Security/CWE-943/NoSQLInjection.ql | 0 .../Security/CWE-943/examples/NoSQLInjection-bad.py | 0 .../Security/CWE-943/examples/NoSQLInjection-good.py | 0 python/ql/src/experimental/semmle/python/Frameworks.qll | 1 - .../query-tests/Security/CWE-943/NoSQLInjection.expected | 0 .../query-tests/Security/CWE-943/NoSQLInjection.qlref | 0 .../query-tests/Security/CWE-943/flask_mongoengine_bad.py | 0 .../query-tests/Security/CWE-943/flask_mongoengine_good.py | 0 .../query-tests/Security/CWE-943/flask_pymongo_bad.py | 0 .../query-tests/Security/CWE-943/flask_pymongo_good.py | 0 .../query-tests/Security/CWE-943/mongoengine_bad.py | 0 .../query-tests/Security/CWE-943/mongoengine_good.py | 0 .../query-tests/Security/CWE-943/pymongo_test.py | 0 17 files changed, 1 insertion(+), 1 deletion(-) rename python/ql/{src/experimental => lib}/semmle/python/frameworks/NoSQL.qll (100%) rename python/ql/{src/experimental/semmle/python/security/injection => lib/semmle/python/security/dataflow}/NoSQLInjection.qll (100%) rename python/ql/src/{experimental => }/Security/CWE-943/NoSQLInjection.qhelp (100%) rename python/ql/src/{experimental => }/Security/CWE-943/NoSQLInjection.ql (100%) rename python/ql/src/{experimental => }/Security/CWE-943/examples/NoSQLInjection-bad.py (100%) rename python/ql/src/{experimental => }/Security/CWE-943/examples/NoSQLInjection-good.py (100%) rename python/ql/test/{experimental => }/query-tests/Security/CWE-943/NoSQLInjection.expected (100%) rename python/ql/test/{experimental => }/query-tests/Security/CWE-943/NoSQLInjection.qlref (100%) rename python/ql/test/{experimental => }/query-tests/Security/CWE-943/flask_mongoengine_bad.py (100%) rename python/ql/test/{experimental => }/query-tests/Security/CWE-943/flask_mongoengine_good.py (100%) rename python/ql/test/{experimental => }/query-tests/Security/CWE-943/flask_pymongo_bad.py (100%) rename python/ql/test/{experimental => }/query-tests/Security/CWE-943/flask_pymongo_good.py (100%) rename python/ql/test/{experimental => }/query-tests/Security/CWE-943/mongoengine_bad.py (100%) rename python/ql/test/{experimental => }/query-tests/Security/CWE-943/mongoengine_good.py (100%) rename python/ql/test/{experimental => }/query-tests/Security/CWE-943/pymongo_test.py (100%) diff --git a/python/ql/lib/semmle/python/Frameworks.qll b/python/ql/lib/semmle/python/Frameworks.qll index 82cb69679cba..171918349c64 100644 --- a/python/ql/lib/semmle/python/Frameworks.qll +++ b/python/ql/lib/semmle/python/Frameworks.qll @@ -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/python/ql/src/experimental/semmle/python/frameworks/NoSQL.qll b/python/ql/lib/semmle/python/frameworks/NoSQL.qll similarity index 100% rename from python/ql/src/experimental/semmle/python/frameworks/NoSQL.qll rename to python/ql/lib/semmle/python/frameworks/NoSQL.qll diff --git a/python/ql/src/experimental/semmle/python/security/injection/NoSQLInjection.qll b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjection.qll similarity index 100% rename from python/ql/src/experimental/semmle/python/security/injection/NoSQLInjection.qll rename to python/ql/lib/semmle/python/security/dataflow/NoSQLInjection.qll diff --git a/python/ql/src/experimental/Security/CWE-943/NoSQLInjection.qhelp b/python/ql/src/Security/CWE-943/NoSQLInjection.qhelp similarity index 100% rename from python/ql/src/experimental/Security/CWE-943/NoSQLInjection.qhelp rename to python/ql/src/Security/CWE-943/NoSQLInjection.qhelp diff --git a/python/ql/src/experimental/Security/CWE-943/NoSQLInjection.ql b/python/ql/src/Security/CWE-943/NoSQLInjection.ql similarity index 100% rename from python/ql/src/experimental/Security/CWE-943/NoSQLInjection.ql rename to python/ql/src/Security/CWE-943/NoSQLInjection.ql diff --git a/python/ql/src/experimental/Security/CWE-943/examples/NoSQLInjection-bad.py b/python/ql/src/Security/CWE-943/examples/NoSQLInjection-bad.py similarity index 100% rename from python/ql/src/experimental/Security/CWE-943/examples/NoSQLInjection-bad.py rename to python/ql/src/Security/CWE-943/examples/NoSQLInjection-bad.py diff --git a/python/ql/src/experimental/Security/CWE-943/examples/NoSQLInjection-good.py b/python/ql/src/Security/CWE-943/examples/NoSQLInjection-good.py similarity index 100% rename from python/ql/src/experimental/Security/CWE-943/examples/NoSQLInjection-good.py rename to python/ql/src/Security/CWE-943/examples/NoSQLInjection-good.py diff --git a/python/ql/src/experimental/semmle/python/Frameworks.qll b/python/ql/src/experimental/semmle/python/Frameworks.qll index 98fb86f6d92b..2fc78b7f53b5 100644 --- a/python/ql/src/experimental/semmle/python/Frameworks.qll +++ b/python/ql/src/experimental/semmle/python/Frameworks.qll @@ -7,7 +7,6 @@ private import experimental.semmle.python.frameworks.Flask private import experimental.semmle.python.frameworks.Django private import experimental.semmle.python.frameworks.Werkzeug private import experimental.semmle.python.frameworks.LDAP -private import experimental.semmle.python.frameworks.NoSQL private import experimental.semmle.python.frameworks.JWT private import experimental.semmle.python.frameworks.Csv private import experimental.semmle.python.libraries.PyJWT diff --git a/python/ql/test/experimental/query-tests/Security/CWE-943/NoSQLInjection.expected b/python/ql/test/query-tests/Security/CWE-943/NoSQLInjection.expected similarity index 100% rename from python/ql/test/experimental/query-tests/Security/CWE-943/NoSQLInjection.expected rename to python/ql/test/query-tests/Security/CWE-943/NoSQLInjection.expected diff --git a/python/ql/test/experimental/query-tests/Security/CWE-943/NoSQLInjection.qlref b/python/ql/test/query-tests/Security/CWE-943/NoSQLInjection.qlref similarity index 100% rename from python/ql/test/experimental/query-tests/Security/CWE-943/NoSQLInjection.qlref rename to python/ql/test/query-tests/Security/CWE-943/NoSQLInjection.qlref diff --git a/python/ql/test/experimental/query-tests/Security/CWE-943/flask_mongoengine_bad.py b/python/ql/test/query-tests/Security/CWE-943/flask_mongoengine_bad.py similarity index 100% rename from python/ql/test/experimental/query-tests/Security/CWE-943/flask_mongoengine_bad.py rename to python/ql/test/query-tests/Security/CWE-943/flask_mongoengine_bad.py diff --git a/python/ql/test/experimental/query-tests/Security/CWE-943/flask_mongoengine_good.py b/python/ql/test/query-tests/Security/CWE-943/flask_mongoengine_good.py similarity index 100% rename from python/ql/test/experimental/query-tests/Security/CWE-943/flask_mongoengine_good.py rename to python/ql/test/query-tests/Security/CWE-943/flask_mongoengine_good.py diff --git a/python/ql/test/experimental/query-tests/Security/CWE-943/flask_pymongo_bad.py b/python/ql/test/query-tests/Security/CWE-943/flask_pymongo_bad.py similarity index 100% rename from python/ql/test/experimental/query-tests/Security/CWE-943/flask_pymongo_bad.py rename to python/ql/test/query-tests/Security/CWE-943/flask_pymongo_bad.py diff --git a/python/ql/test/experimental/query-tests/Security/CWE-943/flask_pymongo_good.py b/python/ql/test/query-tests/Security/CWE-943/flask_pymongo_good.py similarity index 100% rename from python/ql/test/experimental/query-tests/Security/CWE-943/flask_pymongo_good.py rename to python/ql/test/query-tests/Security/CWE-943/flask_pymongo_good.py diff --git a/python/ql/test/experimental/query-tests/Security/CWE-943/mongoengine_bad.py b/python/ql/test/query-tests/Security/CWE-943/mongoengine_bad.py similarity index 100% rename from python/ql/test/experimental/query-tests/Security/CWE-943/mongoengine_bad.py rename to python/ql/test/query-tests/Security/CWE-943/mongoengine_bad.py diff --git a/python/ql/test/experimental/query-tests/Security/CWE-943/mongoengine_good.py b/python/ql/test/query-tests/Security/CWE-943/mongoengine_good.py similarity index 100% rename from python/ql/test/experimental/query-tests/Security/CWE-943/mongoengine_good.py rename to python/ql/test/query-tests/Security/CWE-943/mongoengine_good.py diff --git a/python/ql/test/experimental/query-tests/Security/CWE-943/pymongo_test.py b/python/ql/test/query-tests/Security/CWE-943/pymongo_test.py similarity index 100% rename from python/ql/test/experimental/query-tests/Security/CWE-943/pymongo_test.py rename to python/ql/test/query-tests/Security/CWE-943/pymongo_test.py From 55707d395e4949640e6b523414988f28d8cfd2a5 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Tue, 15 Aug 2023 10:34:43 +0200 Subject: [PATCH 02/45] Python: Make things compile in their new location - Move NoSQL concepts to the non-experimental concepts file - fix references --- python/ql/lib/semmle/python/Concepts.qll | 50 +++++++++++++++++++ .../ql/lib/semmle/python/frameworks/NoSQL.qll | 2 +- .../security/dataflow/NoSQLInjection.qll | 1 - .../ql/src/Security/CWE-943/NoSQLInjection.ql | 2 +- .../experimental/semmle/python/Concepts.qll | 50 ------------------- .../Security/CWE-943/NoSQLInjection.qlref | 2 +- 6 files changed, 53 insertions(+), 54 deletions(-) diff --git a/python/ql/lib/semmle/python/Concepts.qll b/python/ql/lib/semmle/python/Concepts.qll index f3026d8faadc..0a7b67440166 100644 --- a/python/ql/lib/semmle/python/Concepts.qll +++ b/python/ql/lib/semmle/python/Concepts.qll @@ -378,6 +378,56 @@ 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(); + } +} + +/** + * 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() } +} + /** * A data-flow node that executes a regular expression. * diff --git a/python/ql/lib/semmle/python/frameworks/NoSQL.qll b/python/ql/lib/semmle/python/frameworks/NoSQL.qll index ed0ce0315df0..fafc555a6386 100644 --- a/python/ql/lib/semmle/python/frameworks/NoSQL.qll +++ b/python/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 { diff --git a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjection.qll b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjection.qll index f7a40fb3ee6f..6c1a52efe68e 100644 --- a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjection.qll +++ b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjection.qll @@ -2,7 +2,6 @@ import python import semmle.python.dataflow.new.DataFlow import semmle.python.dataflow.new.TaintTracking import semmle.python.dataflow.new.RemoteFlowSources -import experimental.semmle.python.Concepts import semmle.python.Concepts module NoSqlInjection { diff --git a/python/ql/src/Security/CWE-943/NoSQLInjection.ql b/python/ql/src/Security/CWE-943/NoSQLInjection.ql index a73202ca0828..2d9dbc0bb6d3 100644 --- a/python/ql/src/Security/CWE-943/NoSQLInjection.ql +++ b/python/ql/src/Security/CWE-943/NoSQLInjection.ql @@ -11,7 +11,7 @@ */ import python -import experimental.semmle.python.security.injection.NoSQLInjection +import semmle.python.security.dataflow.NoSQLInjection import DataFlow::PathGraph from NoSqlInjection::Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink diff --git a/python/ql/src/experimental/semmle/python/Concepts.qll b/python/ql/src/experimental/semmle/python/Concepts.qll index 3331e1117e07..a289cedb6dfd 100644 --- a/python/ql/src/experimental/semmle/python/Concepts.qll +++ b/python/ql/src/experimental/semmle/python/Concepts.qll @@ -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 { /** diff --git a/python/ql/test/query-tests/Security/CWE-943/NoSQLInjection.qlref b/python/ql/test/query-tests/Security/CWE-943/NoSQLInjection.qlref index 3ca00df892bb..a3f577c4cdca 100644 --- a/python/ql/test/query-tests/Security/CWE-943/NoSQLInjection.qlref +++ b/python/ql/test/query-tests/Security/CWE-943/NoSQLInjection.qlref @@ -1 +1 @@ -experimental/Security/CWE-943/NoSQLInjection.ql +Security/CWE-943/NoSQLInjection.ql From db0459739f5b837a008816475ccd3632eefef6de Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Wed, 23 Aug 2023 13:48:43 +0200 Subject: [PATCH 03/45] Python: rename file --- .../dataflow/{NoSQLInjection.qll => NoSQLInjectionQuery.qll} | 0 python/ql/src/Security/CWE-943/NoSQLInjection.ql | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename python/ql/lib/semmle/python/security/dataflow/{NoSQLInjection.qll => NoSQLInjectionQuery.qll} (100%) diff --git a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjection.qll b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll similarity index 100% rename from python/ql/lib/semmle/python/security/dataflow/NoSQLInjection.qll rename to python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll diff --git a/python/ql/src/Security/CWE-943/NoSQLInjection.ql b/python/ql/src/Security/CWE-943/NoSQLInjection.ql index 2d9dbc0bb6d3..bfa753e78a8d 100644 --- a/python/ql/src/Security/CWE-943/NoSQLInjection.ql +++ b/python/ql/src/Security/CWE-943/NoSQLInjection.ql @@ -11,7 +11,7 @@ */ import python -import semmle.python.security.dataflow.NoSQLInjection +import semmle.python.security.dataflow.NoSQLInjectionQuery import DataFlow::PathGraph from NoSqlInjection::Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink From 087961d179fa9809bf379aff916644605654fe6b Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Wed, 23 Aug 2023 15:03:37 +0200 Subject: [PATCH 04/45] Python: Refactor to allow customizations Also use new DataFlow API --- .../dataflow/NoSQLInjectionCustomizations.qll | 50 +++++++++++++ .../security/dataflow/NoSQLInjectionQuery.qll | 73 +++++++++---------- .../ql/src/Security/CWE-943/NoSQLInjection.ql | 8 +- 3 files changed, 88 insertions(+), 43 deletions(-) create mode 100644 python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll diff --git a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll new file mode 100644 index 000000000000..eb7516f8fa0d --- /dev/null +++ b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll @@ -0,0 +1,50 @@ +import python +import semmle.python.dataflow.new.DataFlow +import semmle.python.dataflow.new.RemoteFlowSources +import semmle.python.Concepts + +module NoSqlInjection { + private newtype TFlowState = + TStringInput() or + TDictInput() + + abstract class FlowState extends TFlowState { + abstract string toString(); + } + + class StringInput extends FlowState, TStringInput { + override string toString() { result = "StringInput" } + } + + class DictInput extends FlowState, TDictInput { + override string toString() { result = "DictInput" } + } + + abstract class StringSource extends DataFlow::Node { } + + abstract class DictSource extends DataFlow::Node { } + + abstract class StringSink extends DataFlow::Node { } + + abstract class DictSink extends DataFlow::Node { } + + abstract class StringToDictConversion extends DataFlow::Node { + abstract DataFlow::Node getAnInput(); + + abstract DataFlow::Node getOutput(); + } + + class RemoteFlowSourceAsStringSource extends RemoteFlowSource, StringSource { } + + class NoSqlQueryAsDictSink extends DictSink { + NoSqlQueryAsDictSink() { this = any(NoSqlQuery noSqlQuery).getQuery() } + } + + 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 index 6c1a52efe68e..0f5748a9ecc4 100644 --- a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll +++ b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll @@ -1,53 +1,48 @@ import python import semmle.python.dataflow.new.DataFlow import semmle.python.dataflow.new.TaintTracking -import semmle.python.dataflow.new.RemoteFlowSources import semmle.python.Concepts +private import NoSQLInjectionCustomizations::NoSqlInjection as C -module NoSqlInjection { - class Configuration extends TaintTracking::Configuration { - Configuration() { this = "NoSQLInjection" } +module Config implements DataFlow::StateConfigSig { + class FlowState = C::FlowState; - override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) { - source instanceof RemoteFlowSource and - state instanceof RemoteInput - } - - override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) { - sink = any(NoSqlQuery noSqlQuery).getQuery() and - state instanceof ConvertedToDict - } - - override predicate isSanitizer(DataFlow::Node node, DataFlow::FlowState state) { - // Block `RemoteInput` paths here, since they change state to `ConvertedToDict` - exists(Decoding decoding | decoding.getFormat() = "JSON" and node = decoding.getOutput()) and - state instanceof RemoteInput - } + 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 + } - override predicate isAdditionalTaintStep( - DataFlow::Node nodeFrom, DataFlow::FlowState stateFrom, DataFlow::Node nodeTo, - DataFlow::FlowState stateTo - ) { - exists(Decoding decoding | decoding.getFormat() = "JSON" | - nodeFrom = decoding.getAnInput() and - nodeTo = decoding.getOutput() - ) and - stateFrom instanceof RemoteInput and - stateTo instanceof ConvertedToDict - } + 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 + } - override predicate isSanitizer(DataFlow::Node sanitizer) { - sanitizer = any(NoSqlSanitizer noSqlSanitizer).getAnInput() - } + 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 } - /** A flow state signifying remote input. */ - class RemoteInput extends DataFlow::FlowState { - RemoteInput() { this = "RemoteInput" } + 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 } - /** A flow state signifying remote input converted to a dictionary. */ - class ConvertedToDict extends DataFlow::FlowState { - ConvertedToDict() { this = "ConvertedToDict" } + predicate isBarrier(DataFlow::Node node) { + node = any(NoSqlSanitizer noSqlSanitizer).getAnInput() } } + +module Flow = TaintTracking::GlobalWithState; diff --git a/python/ql/src/Security/CWE-943/NoSQLInjection.ql b/python/ql/src/Security/CWE-943/NoSQLInjection.ql index bfa753e78a8d..fd588f009f0b 100644 --- a/python/ql/src/Security/CWE-943/NoSQLInjection.ql +++ b/python/ql/src/Security/CWE-943/NoSQLInjection.ql @@ -12,9 +12,9 @@ import python import semmle.python.security.dataflow.NoSQLInjectionQuery -import DataFlow::PathGraph +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" From 19046ea417e1efae5308c17012d974ca5b1a0762 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Wed, 23 Aug 2023 19:58:18 +0200 Subject: [PATCH 05/45] Python: more renames --- .../NoSqlInjection.expected} | 0 .../NoSqlInjection.qlref} | 0 .../{CWE-943 => CWE-943-NoSqlInjection}/flask_mongoengine_bad.py | 0 .../{CWE-943 => CWE-943-NoSqlInjection}/flask_mongoengine_good.py | 0 .../{CWE-943 => CWE-943-NoSqlInjection}/flask_pymongo_bad.py | 0 .../{CWE-943 => CWE-943-NoSqlInjection}/flask_pymongo_good.py | 0 .../{CWE-943 => CWE-943-NoSqlInjection}/mongoengine_bad.py | 0 .../{CWE-943 => CWE-943-NoSqlInjection}/mongoengine_good.py | 0 .../Security/{CWE-943 => CWE-943-NoSqlInjection}/pymongo_test.py | 0 9 files changed, 0 insertions(+), 0 deletions(-) rename python/ql/test/query-tests/Security/{CWE-943/NoSQLInjection.expected => CWE-943-NoSqlInjection/NoSqlInjection.expected} (100%) rename python/ql/test/query-tests/Security/{CWE-943/NoSQLInjection.qlref => CWE-943-NoSqlInjection/NoSqlInjection.qlref} (100%) rename python/ql/test/query-tests/Security/{CWE-943 => CWE-943-NoSqlInjection}/flask_mongoengine_bad.py (100%) rename python/ql/test/query-tests/Security/{CWE-943 => CWE-943-NoSqlInjection}/flask_mongoengine_good.py (100%) rename python/ql/test/query-tests/Security/{CWE-943 => CWE-943-NoSqlInjection}/flask_pymongo_bad.py (100%) rename python/ql/test/query-tests/Security/{CWE-943 => CWE-943-NoSqlInjection}/flask_pymongo_good.py (100%) rename python/ql/test/query-tests/Security/{CWE-943 => CWE-943-NoSqlInjection}/mongoengine_bad.py (100%) rename python/ql/test/query-tests/Security/{CWE-943 => CWE-943-NoSqlInjection}/mongoengine_good.py (100%) rename python/ql/test/query-tests/Security/{CWE-943 => CWE-943-NoSqlInjection}/pymongo_test.py (100%) diff --git a/python/ql/test/query-tests/Security/CWE-943/NoSQLInjection.expected b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected similarity index 100% rename from python/ql/test/query-tests/Security/CWE-943/NoSQLInjection.expected rename to python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected diff --git a/python/ql/test/query-tests/Security/CWE-943/NoSQLInjection.qlref b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.qlref similarity index 100% rename from python/ql/test/query-tests/Security/CWE-943/NoSQLInjection.qlref rename to python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.qlref diff --git a/python/ql/test/query-tests/Security/CWE-943/flask_mongoengine_bad.py b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_mongoengine_bad.py similarity index 100% rename from python/ql/test/query-tests/Security/CWE-943/flask_mongoengine_bad.py rename to python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_mongoengine_bad.py diff --git a/python/ql/test/query-tests/Security/CWE-943/flask_mongoengine_good.py b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_mongoengine_good.py similarity index 100% rename from python/ql/test/query-tests/Security/CWE-943/flask_mongoengine_good.py rename to python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_mongoengine_good.py diff --git a/python/ql/test/query-tests/Security/CWE-943/flask_pymongo_bad.py b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_pymongo_bad.py similarity index 100% rename from python/ql/test/query-tests/Security/CWE-943/flask_pymongo_bad.py rename to python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_pymongo_bad.py diff --git a/python/ql/test/query-tests/Security/CWE-943/flask_pymongo_good.py b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_pymongo_good.py similarity index 100% rename from python/ql/test/query-tests/Security/CWE-943/flask_pymongo_good.py rename to python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_pymongo_good.py diff --git a/python/ql/test/query-tests/Security/CWE-943/mongoengine_bad.py b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/mongoengine_bad.py similarity index 100% rename from python/ql/test/query-tests/Security/CWE-943/mongoengine_bad.py rename to python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/mongoengine_bad.py diff --git a/python/ql/test/query-tests/Security/CWE-943/mongoengine_good.py b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/mongoengine_good.py similarity index 100% rename from python/ql/test/query-tests/Security/CWE-943/mongoengine_good.py rename to python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/mongoengine_good.py diff --git a/python/ql/test/query-tests/Security/CWE-943/pymongo_test.py b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/pymongo_test.py similarity index 100% rename from python/ql/test/query-tests/Security/CWE-943/pymongo_test.py rename to python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/pymongo_test.py From bf8bfd91cd6e47c068a2e5bc31cf82513d2a1140 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Thu, 7 Sep 2023 10:22:30 +0200 Subject: [PATCH 06/45] Python: Add inline query test --- .../DataflowQueryTest.expected | 3 +++ .../CWE-943-NoSqlInjection/DataflowQueryTest.ql | 4 ++++ .../CWE-943-NoSqlInjection/flask_mongoengine_bad.py | 4 ++-- .../CWE-943-NoSqlInjection/flask_mongoengine_good.py | 2 +- .../CWE-943-NoSqlInjection/flask_pymongo_bad.py | 2 +- .../CWE-943-NoSqlInjection/flask_pymongo_good.py | 2 +- .../CWE-943-NoSqlInjection/mongoengine_bad.py | 12 ++++++------ .../CWE-943-NoSqlInjection/mongoengine_good.py | 2 +- .../Security/CWE-943-NoSqlInjection/pymongo_test.py | 8 ++++---- 9 files changed, 23 insertions(+), 16 deletions(-) create mode 100644 python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/DataflowQueryTest.expected create mode 100644 python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/DataflowQueryTest.ql diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/DataflowQueryTest.expected b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/DataflowQueryTest.expected new file mode 100644 index 000000000000..303d04688ff5 --- /dev/null +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/DataflowQueryTest.expected @@ -0,0 +1,3 @@ +failures +missingAnnotationOnSink +testFailures diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/DataflowQueryTest.ql b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/DataflowQueryTest.ql new file mode 100644 index 000000000000..f3709acc913f --- /dev/null +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/DataflowQueryTest.ql @@ -0,0 +1,4 @@ +import python +import experimental.dataflow.TestUtil.DataflowQueryTest +import semmle.python.security.dataflow.NoSQLInjectionQuery +import FromTaintTrackingStateConfig diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_mongoengine_bad.py b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_mongoengine_bad.py index 9fc8aaefc0fc..76ac28edf799 100644 --- a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_mongoengine_bad.py +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_mongoengine_bad.py @@ -19,7 +19,7 @@ def subclass_objects(): unsafe_search = request.args['search'] json_search = json.loads(unsafe_search) - return Movie.objects(__raw__=json_search) + return Movie.objects(__raw__=json_search) #$ result=BAD @app.route("/get_db_find") def get_db_find(): @@ -27,7 +27,7 @@ def get_db_find(): json_search = json.loads(unsafe_search) retrieved_db = db.get_db() - return retrieved_db["Movie"].find({'name': json_search}) + return retrieved_db["Movie"].find({'name': json_search}) #$ result=BAD # if __name__ == "__main__": # app.run(debug=True) diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_mongoengine_good.py b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_mongoengine_good.py index 29a2c75d664f..1ce065569f26 100644 --- a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_mongoengine_good.py +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_mongoengine_good.py @@ -21,7 +21,7 @@ def subclass_objects(): json_search = json.loads(unsafe_search) safe_search = sanitize(json_search) - return Movie.objects(__raw__=safe_search) + return Movie.objects(__raw__=safe_search) #$ result=OK # if __name__ == "__main__": # app.run(debug=True) diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_pymongo_bad.py b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_pymongo_bad.py index 0c1023971da0..735fbff9b346 100644 --- a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_pymongo_bad.py +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_pymongo_bad.py @@ -11,7 +11,7 @@ def home_page(): unsafe_search = request.args['search'] json_search = json.loads(unsafe_search) - return mongo.db.user.find({'name': json_search}) + return mongo.db.user.find({'name': json_search}) #$ result=BAD # if __name__ == "__main__": # app.run(debug=True) diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_pymongo_good.py b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_pymongo_good.py index 6576ba88af81..f2458f91b898 100644 --- a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_pymongo_good.py +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/flask_pymongo_good.py @@ -13,7 +13,7 @@ def home_page(): json_search = json.loads(unsafe_search) safe_search = sanitize(json_search) - return mongo.db.user.find({'name': safe_search}) + return mongo.db.user.find({'name': safe_search}) #$ result=OK # if __name__ == "__main__": # app.run(debug=True) diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/mongoengine_bad.py b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/mongoengine_bad.py index 81800425e46a..4367f9e1ff77 100644 --- a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/mongoengine_bad.py +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/mongoengine_bad.py @@ -19,7 +19,7 @@ def connect_find(): json_search = json.loads(unsafe_search) db = me.connect('mydb') - return db.movie.find({'name': json_search}) + return db.movie.find({'name': json_search}) #$ result=BAD @app.route("/connection_connect_find") def connection_connect_find(): @@ -27,7 +27,7 @@ def connection_connect_find(): json_search = json.loads(unsafe_search) db = connect('mydb') - return db.movie.find({'name': json_search}) + return db.movie.find({'name': json_search}) #$ result=BAD @app.route("/get_db_find") def get_db_find(): @@ -35,7 +35,7 @@ def get_db_find(): json_search = json.loads(unsafe_search) db = me.get_db() - return db.movie.find({'name': json_search}) + return db.movie.find({'name': json_search}) #$ result=BAD @app.route("/connection_get_db_find") def connection_get_db_find(): @@ -43,14 +43,14 @@ def connection_get_db_find(): json_search = json.loads(unsafe_search) db = get_db() - return db.movie.find({'name': json_search}) + return db.movie.find({'name': json_search}) #$ result=BAD @app.route("/subclass_objects") def subclass_objects(): unsafe_search = request.args['search'] json_search = json.loads(unsafe_search) - return Movie.objects(__raw__=json_search) + return Movie.objects(__raw__=json_search) #$ result=BAD @app.route("/subscript_find") def subscript_find(): @@ -58,7 +58,7 @@ def subscript_find(): json_search = json.loads(unsafe_search) db = me.connect('mydb') - return db['movie'].find({'name': json_search}) + return db['movie'].find({'name': json_search}) #$ result=BAD # if __name__ == "__main__": # app.run(debug=True) diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/mongoengine_good.py b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/mongoengine_good.py index c9b2b8e762fe..e8ac68cccba2 100644 --- a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/mongoengine_good.py +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/mongoengine_good.py @@ -21,7 +21,7 @@ def connect_find(): safe_search = sanitize(json_search) db = me.connect('mydb') - return db.movie.find({'name': safe_search}) + return db.movie.find({'name': safe_search}) #$ result=OK # if __name__ == "__main__": # app.run(debug=True) diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/pymongo_test.py b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/pymongo_test.py index ecf53ec4f9a2..3dd667a3f2d5 100644 --- a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/pymongo_test.py +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/pymongo_test.py @@ -12,7 +12,7 @@ def bad(): unsafe_search = request.args['search'] json_search = json.loads(unsafe_search) - return client.db.collection.find_one({'data': json_search}) + return client.db.collection.find_one({'data': json_search}) #$ result=BAD @app.route("/good") @@ -21,7 +21,7 @@ def good(): json_search = json.loads(unsafe_search) safe_search = sanitize(json_search) - return client.db.collection.find_one({'data': safe_search}) + return client.db.collection.find_one({'data': safe_search}) #$ result=OK @app.route("/bad2") @@ -30,7 +30,7 @@ def bad2(): client = MongoClient("localhost", 27017, maxPoolSize=50) db = client.localhost collection = db['collection'] - cursor = collection.find_one({"$where": f"this._id == '${event_id}'"}) + cursor = collection.find_one({"$where": f"this._id == '${event_id}'"}) #$ result=BAD @app.route("/bad3") @@ -40,7 +40,7 @@ def bad3(): client = MongoClient("localhost", 27017, maxPoolSize=50) db = client.get_database(name="localhost") collection = db.get_collection("collection") - cursor = collection.find_one({"$where": f"this._id == '${event_id}'"}) + cursor = collection.find_one({"$where": f"this._id == '${event_id}'"}) #$ result=BAD if __name__ == "__main__": From 114984bd8cb92ba8f06dbb3902eb48d3481a1a67 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Tue, 29 Aug 2023 16:16:38 +0200 Subject: [PATCH 07/45] Python: Added tests based on security analysis currently we do not: - recognize the pattern `{'author': {"$eq": author}}` as protected - recognize arguements to `$where` (and friends) as vulnerable --- .../CWE-943-NoSqlInjection/PoC/populate.py | 16 ++++++ .../CWE-943-NoSqlInjection/PoC/readme.md | 19 +++++++ .../PoC/requirements.txt | 2 + .../CWE-943-NoSqlInjection/PoC/server.py | 55 +++++++++++++++++++ .../Security/CWE-943-NoSqlInjection/options | 1 + 5 files changed, 93 insertions(+) create mode 100644 python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/populate.py create mode 100644 python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/readme.md create mode 100644 python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/requirements.txt create mode 100644 python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/server.py create mode 100644 python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/options diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/populate.py b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/populate.py new file mode 100644 index 000000000000..026f72bdfe59 --- /dev/null +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/populate.py @@ -0,0 +1,16 @@ +from pymongo import MongoClient +client = MongoClient() + +db = client.test_database + +import datetime +post = { + "author": "Mike", + "text": "My first blog post!", + "tags": ["mongodb", "python", "pymongo"], + "date": datetime.datetime.now(tz=datetime.timezone.utc), +} + +posts = db.posts +post_id = posts.insert_one(post).inserted_id +post_id diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/readme.md b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/readme.md new file mode 100644 index 000000000000..78e7e92e48f4 --- /dev/null +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/readme.md @@ -0,0 +1,19 @@ +Tutorials: +- [pymongo](https://pymongo.readthedocs.io/en/stable/tutorial.html) +- [install mongodb](https://www.mongodb.com/docs/manual/tutorial/install-mongodb-on-os-x/#std-label-install-mdb-community-macos) + +I recommend creating a virtual environment with venv and then installing dependencies via +``` +python -m pip --install -r requirements.txt +``` + +Start mongodb: +``` +mongod --config /usr/local/etc/mongod.conf --fork +``` +run flask app: +``` +flask --app server run +``` + +Navigate to the root to see routes. For each route try to get the system to reveal the person in the database. If you know the name, you can just input it, but in some cases you can get to the person without knowing the name! diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/requirements.txt b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/requirements.txt new file mode 100644 index 000000000000..a98715ed1e4a --- /dev/null +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/requirements.txt @@ -0,0 +1,2 @@ +flask +pymongo diff --git 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 new file mode 100644 index 000000000000..59b519635834 --- /dev/null +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/server.py @@ -0,0 +1,55 @@ +from flask import Flask, request +from pymongo import MongoClient +import json + +client = MongoClient() +db = client.test_database +posts = db.posts + +app = Flask(__name__) + + +def show_post(post, query): + if post: + return "You found " + post['author'] + "!" + else: + return "You did not find " + query + +@app.route('/plain', methods=['GET']) +def plain(): + author = request.args['author'] + post = posts.find_one({'author': author}) # $ result=OK + return show_post(post, author) + +@app.route('/dict', methods=['GET']) +def as_dict(): + author_string = request.args['author'] + author = json.loads(author_string) + # Use {"$ne": 1} as author + # Found by http://127.0.0.1:5000/dict?author={%22$ne%22:1} + post = posts.find_one({'author': author}) # $ result=BAD + return show_post(post, author) + +@app.route('/dictHardened', methods=['GET']) +def as_dict_hardened(): + author_string = request.args['author'] + author = json.loads(author_string) + post = posts.find_one({'author': {"$eq": author}}) # $ SPURIOUS: result=BAD + return show_post(post, author) + +@app.route('/byWhere', methods=['GET']) +def by_where(): + author = request.args['author'] + # Use `" | "a" === "a` as author + # making the query `this.author === "" | "a" === "a"` + # Found by http://127.0.0.1:5000/byWhere?author=%22%20|%20%22a%22%20===%20%22a + post = posts.find_one({'$where': 'this.author === "'+author+'"'}) # $ MISSING: result=BAD + return show_post(post, author) + +@app.route('/', methods=['GET']) +def show_routes(): + links = [] + for rule in app.url_map.iter_rules(): + if "GET" in rule.methods: + links.append((rule.rule, rule.endpoint)) + return links diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/options b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/options new file mode 100644 index 000000000000..48ecb907736e --- /dev/null +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/options @@ -0,0 +1 @@ +semmle-extractor-options: --max-import-depth=1 -r PoC From c0b3245a5393306660a04026357cf1ae2eb2a4e2 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Thu, 31 Aug 2023 20:30:03 +0200 Subject: [PATCH 08/45] Python: Enrich the NoSql concept This allows us to make more precise modelling The query tests now pass. I do wonder, if there is a cleaner approach, similar to `TaintedObject` in JavaScript. I want the option to get this query in the hands of the custumors before such an investigation, though. --- python/ql/lib/semmle/python/Concepts.qll | 12 ++++ .../ql/lib/semmle/python/frameworks/NoSQL.qll | 61 ++++++++++++++++--- .../dataflow/NoSQLInjectionCustomizations.qll | 8 +++ .../security/dataflow/NoSQLInjectionQuery.qll | 7 ++- .../CWE-943-NoSqlInjection/PoC/server.py | 4 +- 5 files changed, 80 insertions(+), 12 deletions(-) diff --git a/python/ql/lib/semmle/python/Concepts.qll b/python/ql/lib/semmle/python/Concepts.qll index 0a7b67440166..ea6310e326c0 100644 --- a/python/ql/lib/semmle/python/Concepts.qll +++ b/python/ql/lib/semmle/python/Concepts.qll @@ -389,6 +389,12 @@ module NoSqlQuery { 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(); } } @@ -401,6 +407,12 @@ module NoSqlQuery { 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. */ diff --git a/python/ql/lib/semmle/python/frameworks/NoSQL.qll b/python/ql/lib/semmle/python/frameworks/NoSQL.qll index fafc555a6386..ba17d593a603 100644 --- a/python/ql/lib/semmle/python/frameworks/NoSQL.qll +++ b/python/ql/lib/semmle/python/frameworks/NoSQL.qll @@ -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 index eb7516f8fa0d..54c85ccf195b 100644 --- a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll +++ b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll @@ -36,6 +36,14 @@ module NoSqlInjection { class RemoteFlowSourceAsStringSource extends RemoteFlowSource, StringSource { } + class NoSqlQueryAsStringSink extends StringSink { + NoSqlQueryAsStringSink() { + exists(NoSqlQuery noSqlQuery | this = noSqlQuery.getQuery() | + noSqlQuery.vulnerableToStrings() + ) + } + } + class NoSqlQueryAsDictSink extends DictSink { NoSqlQueryAsDictSink() { this = any(NoSqlQuery noSqlQuery).getQuery() } } diff --git a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll index 0f5748a9ecc4..8a0e31ed5445 100644 --- a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll +++ b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll @@ -17,7 +17,12 @@ module Config implements DataFlow::StateConfigSig { predicate isSink(DataFlow::Node source, FlowState state) { source instanceof C::StringSink and - state instanceof C::StringInput + ( + state instanceof C::StringInput + or + // since dictionaries can encode strings + state instanceof C::DictInput + ) or source instanceof C::DictSink and state instanceof C::DictInput diff --git 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 index 59b519635834..f2b4a4f0a43a 100644 --- 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 @@ -34,7 +34,7 @@ def as_dict(): def as_dict_hardened(): author_string = request.args['author'] author = json.loads(author_string) - post = posts.find_one({'author': {"$eq": author}}) # $ SPURIOUS: result=BAD + post = posts.find_one({'author': {"$eq": author}}) # $ result=OK return show_post(post, author) @app.route('/byWhere', methods=['GET']) @@ -43,7 +43,7 @@ def by_where(): # Use `" | "a" === "a` as author # making the query `this.author === "" | "a" === "a"` # Found by http://127.0.0.1:5000/byWhere?author=%22%20|%20%22a%22%20===%20%22a - post = posts.find_one({'$where': 'this.author === "'+author+'"'}) # $ MISSING: result=BAD + post = posts.find_one({'$where': 'this.author === "'+author+'"'}) # $ result=BAD return show_post(post, author) @app.route('/', methods=['GET']) From 7edebbeaffe10c5d8b7774666c34d8d58d6e194c Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Wed, 6 Sep 2023 11:46:49 +0200 Subject: [PATCH 09/45] Python: Add QLDocs --- .../dataflow/NoSQLInjectionCustomizations.qll | 26 +++++++++++++++++++ .../security/dataflow/NoSQLInjectionQuery.qll | 7 +++++ 2 files changed, 33 insertions(+) diff --git a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll index 54c85ccf195b..d3348b8df8d5 100644 --- a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll +++ b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll @@ -1,41 +1,65 @@ +/** + * 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() | @@ -44,10 +68,12 @@ module NoSqlInjection { } } + /** 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" } diff --git a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll index 8a0e31ed5445..da3432e44ada 100644 --- a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll +++ b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll @@ -1,9 +1,16 @@ +/** + * 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 Config implements DataFlow::StateConfigSig { class FlowState = C::FlowState; From f253f9797fe448c076ceb639734f3111ce8a7ebe Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Wed, 6 Sep 2023 11:53:44 +0200 Subject: [PATCH 10/45] Python: update test expectations --- .../NoSqlInjection.expected | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected index 015328031223..89b0684744c5 100644 --- a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected @@ -1,4 +1,15 @@ 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: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 | | 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 | @@ -66,14 +77,28 @@ edges | 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: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() | 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 | +| PoC/server.py:26:5:26:17 | SSA variable author_string | semmle.label | SSA variable author_string | +| PoC/server.py:26:21:26:27 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | +| PoC/server.py:27:5:27:10 | SSA variable author | semmle.label | SSA variable author | +| PoC/server.py:27:14:27:38 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| PoC/server.py:27:25:27:37 | ControlFlowNode for author_string | semmle.label | ControlFlowNode for author_string | +| PoC/server.py:30:27:30:44 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | +| PoC/server.py:42:5:42:10 | SSA variable author | semmle.label | SSA variable author | +| 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 | | 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 | @@ -147,13 +172,18 @@ nodes | pymongo_test.py:29:27:29:33 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | | pymongo_test.py:29:27:29:50 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript | | pymongo_test.py:33:34:33:73 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | +| pymongo_test.py:33:45:33:72 | ControlFlowNode for Fstring | semmle.label | ControlFlowNode for Fstring | | pymongo_test.py:39:5:39:12 | SSA variable event_id | semmle.label | SSA variable event_id | | pymongo_test.py:39:16:39:51 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | | pymongo_test.py:39:27:39:33 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | | pymongo_test.py:39:27:39:50 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript | | pymongo_test.py:43:34:43:73 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | +| pymongo_test.py:43:45:43:72 | ControlFlowNode for Fstring | semmle.label | ControlFlowNode for Fstring | 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 | | 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 | @@ -165,4 +195,6 @@ 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 | From 970e881697b0e8a3301eb068e779a3810cd11664 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Thu, 7 Sep 2023 15:03:51 +0200 Subject: [PATCH 11/45] Python: Follow naming convention --- .../semmle/python/security/dataflow/NoSQLInjectionQuery.qll | 4 ++-- .../Security/CWE-943-NoSqlInjection/DataflowQueryTest.ql | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll index da3432e44ada..8c6f06949d69 100644 --- a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll +++ b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll @@ -11,7 +11,7 @@ private import NoSQLInjectionCustomizations::NoSqlInjection as C /** * A taint-tracking configuration for detecting NoSQL injection vulnerabilities. */ -module Config implements DataFlow::StateConfigSig { +module NoSQLInjectionConfig implements DataFlow::StateConfigSig { class FlowState = C::FlowState; predicate isSource(DataFlow::Node source, FlowState state) { @@ -57,4 +57,4 @@ module Config implements DataFlow::StateConfigSig { } } -module Flow = TaintTracking::GlobalWithState; +module Flow = TaintTracking::GlobalWithState; diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/DataflowQueryTest.ql b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/DataflowQueryTest.ql index f3709acc913f..e71c10b3d56e 100644 --- a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/DataflowQueryTest.ql +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/DataflowQueryTest.ql @@ -1,4 +1,4 @@ import python import experimental.dataflow.TestUtil.DataflowQueryTest import semmle.python.security.dataflow.NoSQLInjectionQuery -import FromTaintTrackingStateConfig +import FromTaintTrackingStateConfig From b07d085157e64ab774e99201da4244f9249d7b70 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Thu, 7 Sep 2023 15:04:27 +0200 Subject: [PATCH 12/45] Python: make test PoC a proper package --- .../query-tests/Security/CWE-943-NoSqlInjection/PoC/__init.py__ | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/__init.py__ diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/__init.py__ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/__init.py__ new file mode 100644 index 000000000000..e69de29bb2d1 From d91cd21204c9951ff5675ef063387fafbcab5a08 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Fri, 8 Sep 2023 13:37:54 +0200 Subject: [PATCH 13/45] Python: rename file --- .../CWE-943-NoSqlInjection/PoC/{__init.py__ => __init__.py} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/{__init.py__ => __init__.py} (100%) diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/__init.py__ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/__init__.py similarity index 100% rename from python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/__init.py__ rename to python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/__init__.py From 154a36934d87a21e1cc0932960bf9d15fb694fa7 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Mon, 11 Sep 2023 14:49:03 +0200 Subject: [PATCH 14/45] Python: Add test for function --- .../CWE-943-NoSqlInjection/PoC/server.py | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git 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 index f2b4a4f0a43a..a94583d8177f 100644 --- 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 @@ -46,6 +46,32 @@ def by_where(): post = posts.find_one({'$where': 'this.author === "'+author+'"'}) # $ result=BAD return show_post(post, author) + +@app.route('/byFunction', methods=['GET']) +def by_function(): + author = request.args['author'] + search = { + "body": 'function(author) { return(author === "'+author+'") }', + "args": [ "$author" ], + "lang": "js" + } + # Use `" | "a" === "a` as author + # making the query `this.author === "" | "a" === "a"` + # Found by http://127.0.0.1:5000/byFunction?author=%22%20|%20%22a%22%20===%20%22a + post = posts.find_one({'$expr': {'$function': search}}) # $ MISING: result=BAD + return show_post(post, author) + +@app.route('/byFunctionArg', methods=['GET']) +def by_function_arg(): + author = request.args['author'] + search = { + "body": 'function(author, target) { return(author === target) }', + "args": [ "$author", author ], + "lang": "js" + } + post = posts.find_one({'$expr': {'$function': search}}) # $ result=OK + return show_post(post, author) + @app.route('/', methods=['GET']) def show_routes(): links = [] From d9f63e1ed3ee01a580fa19fd03bb2590daeb3ccb Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Mon, 11 Sep 2023 15:54:00 +0200 Subject: [PATCH 15/45] Python: Split modelling of query operators `$where` and `$function` behave quite differently. --- .../ql/lib/semmle/python/frameworks/NoSQL.qll | 31 +++++++++++++++---- .../CWE-943-NoSqlInjection/PoC/server.py | 9 +++--- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/NoSQL.qll b/python/ql/lib/semmle/python/frameworks/NoSQL.qll index ba17d593a603..d7a0848ae9c4 100644 --- a/python/ql/lib/semmle/python/frameworks/NoSQL.qll +++ b/python/ql/lib/semmle/python/frameworks/NoSQL.qll @@ -99,9 +99,6 @@ private module NoSql { ] } - /** 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 * @@ -125,12 +122,34 @@ private module NoSql { override predicate vulnerableToStrings() { none() } } - private class MongoCollectionQueryOperator extends API::CallNode, NoSqlQuery::Range { + /** The `$where` query operator executes a string as JavaScript. */ + private class WhereQueryOperator extends API::CallNode, NoSqlQuery::Range { DataFlow::Node query; - MongoCollectionQueryOperator() { + WhereQueryOperator() { this = mongoCollection().getMember(mongoCollectionMethodName()).getACall() and - query = this.getParameter(0).getSubscript(mongoQueryOperator()).asSink() + query = this.getParameter(0).getSubscript("$where").asSink() + } + + override DataFlow::Node getQuery() { result = query } + + override predicate interpretsDict() { none() } + + override predicate vulnerableToStrings() { any() } + } + + /** The `$function` query operator executes its `body` string as JavaScript. */ + private class FunctionQueryOperator extends API::CallNode, NoSqlQuery::Range { + DataFlow::Node query; + + FunctionQueryOperator() { + this = mongoCollection().getMember(mongoCollectionMethodName()).getACall() and + query = + this.getParameter(0) + .getASubscript*() + .getSubscript("$function") + .getSubscript("body") + .asSink() } override DataFlow::Node getQuery() { result = query } diff --git 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 index a94583d8177f..6a0099a4f5d6 100644 --- 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 @@ -46,30 +46,29 @@ def by_where(): post = posts.find_one({'$where': 'this.author === "'+author+'"'}) # $ result=BAD return show_post(post, author) - @app.route('/byFunction', methods=['GET']) def by_function(): author = request.args['author'] search = { - "body": 'function(author) { return(author === "'+author+'") }', + "body": 'function(author) { return(author === "'+author+'") }', # $ result=BAD "args": [ "$author" ], "lang": "js" } # Use `" | "a" === "a` as author # making the query `this.author === "" | "a" === "a"` # Found by http://127.0.0.1:5000/byFunction?author=%22%20|%20%22a%22%20===%20%22a - post = posts.find_one({'$expr': {'$function': search}}) # $ MISING: result=BAD + post = posts.find_one({'$expr': {'$function': search}}) # $ result=BAD return show_post(post, author) @app.route('/byFunctionArg', methods=['GET']) def by_function_arg(): author = request.args['author'] search = { - "body": 'function(author, target) { return(author === target) }', + "body": 'function(author, target) { return(author === target) }', # $ result=OK "args": [ "$author", author ], "lang": "js" } - post = posts.find_one({'$expr': {'$function': search}}) # $ result=OK + post = posts.find_one({'$expr': {'$function': search}}) # $ SPURIOUS: result=BAD return show_post(post, author) @app.route('/', methods=['GET']) From a063d7d5104c12f785532cdc9b6358cb87a84777 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Mon, 11 Sep 2023 16:33:20 +0200 Subject: [PATCH 16/45] 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. --- .../ql/lib/semmle/python/frameworks/NoSQL.qll | 43 +++++++++++-------- .../dataflow/NoSQLInjectionCustomizations.qll | 2 +- .../NoSqlInjection.expected | 20 ++++++--- .../CWE-943-NoSqlInjection/PoC/server.py | 6 +-- 4 files changed, 44 insertions(+), 27 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/NoSQL.qll b/python/ql/lib/semmle/python/frameworks/NoSQL.qll index d7a0848ae9c4..f122f4b20006 100644 --- 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 DataFlow::Node getOutput() { result = this } - override predicate interpretsDict() { none() } + override string getFormat() { result = "NoSQL" } - override predicate vulnerableToStrings() { any() } + override predicate mayExecuteInput() { any() } } /** diff --git a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll index d3348b8df8d5..debefd525a73 100644 --- 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() } diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected index 89b0684744c5..fdabe8cb9778 100644 --- 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 | diff --git 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 index 6a0099a4f5d6..4f608c5e5841 100644 --- 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']) From 4614b1ae9cadaf39da9de6f8912d16ca4f3d384f Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Mon, 18 Sep 2023 14:34:03 +0200 Subject: [PATCH 17/45] Python: add change note --- .../change-notes/2023-09-18-promoted-nosql-injection-query.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 python/ql/src/change-notes/2023-09-18-promoted-nosql-injection-query.md diff --git a/python/ql/src/change-notes/2023-09-18-promoted-nosql-injection-query.md b/python/ql/src/change-notes/2023-09-18-promoted-nosql-injection-query.md new file mode 100644 index 000000000000..2b30fd492d53 --- /dev/null +++ b/python/ql/src/change-notes/2023-09-18-promoted-nosql-injection-query.md @@ -0,0 +1,4 @@ +--- +category: newQuery +--- +* The query `py/nosql-injection` for finding NoSQL injection vulnerabilities is now available in the default security suite. From 5611bda7ee7a971963c6b586e01430e82d420f4c Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Tue, 19 Sep 2023 17:04:28 +0200 Subject: [PATCH 18/45] Python: add test for `$accumulator` --- .../CWE-943-NoSqlInjection/PoC/server.py | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git 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 index 4f608c5e5841..74618fa1bbe2 100644 --- 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 @@ -71,6 +71,26 @@ def by_function_arg(): post = posts.find_one({'$expr': {'$function': search}}) # $ result=OK return show_post(post, author) +@app.route('/byGroup', methods=['GET']) +def by_group(): + author = request.args['author'] + accumulator = { + "init": 'function() { return "Not found" }', + "accumulate": 'function(state, author) { return (author === "'+author+'") ? author : state }', + "accumulateArgs": ["$author"], + "merge": 'function(state1, state2) { return (state1 === "Not found") ? state2 : state1 }' + } + group = { + "_id": "null", + "author": { "$accumulator": accumulator } + } + # Use `" | "a" === "a` as author + # making the query `this.author === "" | "a" === "a"` + # Found by http://127.0.0.1:5000/byGroup?author=%22%20|%20%22a%22%20===%20%22a + post = posts.aggregate([{ "$group": group }]).next() # $ MISSING: result=BAD + app.logger.error("post", post) + return show_post(post, author) + @app.route('/', methods=['GET']) def show_routes(): links = [] From 30c37ca8cba10da4fe10eef193f5e6e7a91703ae Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Tue, 19 Sep 2023 18:26:11 +0200 Subject: [PATCH 19/45] =?UTF-8?q?Python:=20model=20`=C2=A7accumulator`=20a?= =?UTF-8?q?lso=20slightly=20rearrange=20the=20modelling?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../ql/lib/semmle/python/frameworks/NoSQL.qll | 51 +++++++++++++++++-- .../NoSqlInjection.expected | 19 +++++-- .../CWE-943-NoSqlInjection/PoC/server.py | 3 +- 3 files changed, 65 insertions(+), 8 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/NoSQL.qll b/python/ql/lib/semmle/python/frameworks/NoSQL.qll index f122f4b20006..ea69f565aa02 100644 --- a/python/ql/lib/semmle/python/frameworks/NoSQL.qll +++ b/python/ql/lib/semmle/python/frameworks/NoSQL.qll @@ -122,6 +122,16 @@ private module NoSql { override predicate vulnerableToStrings() { none() } } + private class MongoCollectionAggregation extends API::CallNode, NoSqlQuery::Range { + MongoCollectionAggregation() { this = mongoCollection().getMember("aggregate").getACall() } + + override DataFlow::Node getQuery() { result = this.getParameter(0).getASubscript().asSink() } + + override predicate interpretsDict() { any() } + + override predicate vulnerableToStrings() { none() } + } + /** The `$where` query operator executes a string as JavaScript. */ private class WhereQueryOperator extends DataFlow::Node, Decoding::Range { API::Node dictionary; @@ -143,7 +153,11 @@ private module NoSql { override predicate mayExecuteInput() { any() } } - /** The `$function` query operator executes its `body` string as JavaScript. */ + /** + * The `$function` query operator executes its `body` string as JavaScript. + * + * See https://www.mongodb.com/docs/manual/reference/operator/aggregation/function/#mongodb-expression-exp.-function + */ private class FunctionQueryOperator extends DataFlow::Node, Decoding::Range { API::Node dictionary; DataFlow::Node query; @@ -154,8 +168,39 @@ private module NoSql { .getMember(mongoCollectionMethodName()) .getACall() .getParameter(0) - .getASubscript*() and - query = dictionary.getSubscript("$function").getSubscript("body").asSink() and + .getASubscript*() + .getSubscript("$function") and + query = dictionary.getSubscript("body").asSink() and + this = dictionary.asSink() + } + + override DataFlow::Node getAnInput() { result = query } + + override DataFlow::Node getOutput() { result = this } + + override string getFormat() { result = "NoSQL" } + + override predicate mayExecuteInput() { any() } + } + + /** + * The `$accumulator` query operator executes strings in some of its fields as JavaScript. + * + * See https://www.mongodb.com/docs/manual/reference/operator/aggregation/accumulator/#mongodb-group-grp.-accumulator + */ + private class AccumulatorQueryOperator extends DataFlow::Node, Decoding::Range { + API::Node dictionary; + DataFlow::Node query; + + AccumulatorQueryOperator() { + dictionary = + mongoCollection() + .getMember("aggregate") + .getACall() + .getParameter(0) + .getASubscript*() + .getSubscript("$accumulator") and + query = dictionary.getSubscript(["init", "accumulate", "merge", "finalize"]).asSink() and this = dictionary.asSink() } diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected index fdabe8cb9778..ac13418edcdf 100644 --- a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected @@ -3,6 +3,7 @@ edges | 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:1:26:1:32 | GSSA Variable request | PoC/server.py:76:14:76: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 | @@ -13,8 +14,13 @@ edges | 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 | +| PoC/server.py:53:17:53:70 | ControlFlowNode for BinaryExpr | PoC/server.py:60:51:60:56 | ControlFlowNode for search | +| PoC/server.py:60:51:60:56 | ControlFlowNode for search | PoC/server.py:60:27:60:58 | ControlFlowNode for Dict | +| PoC/server.py:76:5:76:10 | SSA variable author | PoC/server.py:79:23:79:101 | ControlFlowNode for BinaryExpr | +| PoC/server.py:76:14:76:20 | ControlFlowNode for request | PoC/server.py:76:5:76:10 | SSA variable author | +| PoC/server.py:79:23:79:101 | ControlFlowNode for BinaryExpr | PoC/server.py:85:37:85:47 | ControlFlowNode for accumulator | +| PoC/server.py:83:5:83:9 | SSA variable group | PoC/server.py:90:29:90:47 | ControlFlowNode for Dict | +| PoC/server.py:85:37:85:47 | ControlFlowNode for accumulator | PoC/server.py:83:5:83:9 | SSA variable group | | 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 | @@ -108,7 +114,13 @@ nodes | 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 | +| PoC/server.py:60:51:60:56 | ControlFlowNode for search | semmle.label | ControlFlowNode for search | +| PoC/server.py:76:5:76:10 | SSA variable author | semmle.label | SSA variable author | +| PoC/server.py:76:14:76:20 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | +| PoC/server.py:79:23:79:101 | ControlFlowNode for BinaryExpr | semmle.label | ControlFlowNode for BinaryExpr | +| PoC/server.py:83:5:83:9 | SSA variable group | semmle.label | SSA variable group | +| PoC/server.py:85:37:85:47 | ControlFlowNode for accumulator | semmle.label | ControlFlowNode for accumulator | +| PoC/server.py:90:29:90:47 | 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 | @@ -194,6 +206,7 @@ subpaths | 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: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 | +| PoC/server.py:90:29:90:47 | ControlFlowNode for Dict | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | PoC/server.py:90:29:90:47 | 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 | diff --git 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 index 74618fa1bbe2..611fd90d3ed8 100644 --- 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 @@ -87,8 +87,7 @@ def by_group(): # Use `" | "a" === "a` as author # making the query `this.author === "" | "a" === "a"` # Found by http://127.0.0.1:5000/byGroup?author=%22%20|%20%22a%22%20===%20%22a - post = posts.aggregate([{ "$group": group }]).next() # $ MISSING: result=BAD - app.logger.error("post", post) + post = posts.aggregate([{ "$group": group }]).next() # $ result=BAD return show_post(post, author) @app.route('/', methods=['GET']) From 7c085ecc61b17c187d134fd3c6ed5bc209fbd08f Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Wed, 20 Sep 2023 15:23:18 +0200 Subject: [PATCH 20/45] Python: Add test for `map_reduce` Also log requirement for old versions of `pymongo` --- .../CWE-943-NoSqlInjection/PoC/requirements.txt | 2 +- .../Security/CWE-943-NoSqlInjection/PoC/server.py | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/requirements.txt b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/requirements.txt index a98715ed1e4a..36b236179535 100644 --- a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/requirements.txt +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/requirements.txt @@ -1,2 +1,2 @@ flask -pymongo +pymongo==3.9 diff --git 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 index 611fd90d3ed8..a0ce2944f697 100644 --- 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 @@ -90,6 +90,21 @@ def by_group(): post = posts.aggregate([{ "$group": group }]).next() # $ result=BAD return show_post(post, author) +# works with pymongo 3.9, `map_reduce` is removed in pymongo 4.0 +@app.route('/byMapReduce', methods=['GET']) +def by_map_reduce(): + author = request.args['author'] + mapper = 'function() { emit(this.author, this.author === "'+author+'") }' + reducer = "function(key, values) { return values.some( x => x ) }" + results = posts.map_reduce(mapper, reducer, "results") + # Use `" | "a" === "a` as author + # making the query `this.author === "" | "a" === "a"` + # Found by http://127.0.0.1:5000/byMapReduce?author=%22%20|%20%22a%22%20===%20%22a + post = results.find_one({'value': True}) # $ MISSING: result=BAD + if(post): + post["author"] = post["_id"] + return show_post(post, author) + @app.route('/', methods=['GET']) def show_routes(): links = [] From 4ec8b3f02fac3f8132b4627acb4b790fd207de52 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Wed, 20 Sep 2023 15:44:12 +0200 Subject: [PATCH 21/45] Python: Model `map_reduce` --- .../ql/lib/semmle/python/frameworks/NoSQL.qll | 20 +++++++++++++++++++ .../NoSqlInjection.expected | 9 +++++++++ .../CWE-943-NoSqlInjection/PoC/server.py | 7 +++++-- 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/NoSQL.qll b/python/ql/lib/semmle/python/frameworks/NoSQL.qll index ea69f565aa02..eb777b7199d7 100644 --- a/python/ql/lib/semmle/python/frameworks/NoSQL.qll +++ b/python/ql/lib/semmle/python/frameworks/NoSQL.qll @@ -132,6 +132,26 @@ private module NoSql { override predicate vulnerableToStrings() { none() } } + private class MongoMapReduce extends API::CallNode, NoSqlQuery::Range { + MongoMapReduce() { this = mongoCollection().getMember("map_reduce").getACall() } + + override DataFlow::Node getQuery() { result in [this.getArg(0), this.getArg(1)] } + + override predicate interpretsDict() { none() } + + override predicate vulnerableToStrings() { any() } + } + + private class MongoMapReduceQuery extends API::CallNode, NoSqlQuery::Range { + MongoMapReduceQuery() { this = mongoCollection().getMember("map_reduce").getACall() } + + override DataFlow::Node getQuery() { result in [this.getArgByName("query")] } + + override predicate interpretsDict() { any() } + + override predicate vulnerableToStrings() { none() } + } + /** The `$where` query operator executes a string as JavaScript. */ private class WhereQueryOperator extends DataFlow::Node, Decoding::Range { API::Node dictionary; diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected index ac13418edcdf..0eae28cecc1b 100644 --- a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected @@ -4,6 +4,7 @@ edges | 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:1:26:1:32 | GSSA Variable request | PoC/server.py:76:14:76:20 | ControlFlowNode for request | +| PoC/server.py:1:26:1:32 | GSSA Variable request | PoC/server.py:96:14:96: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 | @@ -21,6 +22,9 @@ edges | PoC/server.py:79:23:79:101 | ControlFlowNode for BinaryExpr | PoC/server.py:85:37:85:47 | ControlFlowNode for accumulator | | PoC/server.py:83:5:83:9 | SSA variable group | PoC/server.py:90:29:90:47 | ControlFlowNode for Dict | | PoC/server.py:85:37:85:47 | ControlFlowNode for accumulator | PoC/server.py:83:5:83:9 | SSA variable group | +| PoC/server.py:96:5:96:10 | SSA variable author | PoC/server.py:97:5:97:10 | SSA variable mapper | +| PoC/server.py:96:14:96:20 | ControlFlowNode for request | PoC/server.py:96:5:96:10 | SSA variable author | +| PoC/server.py:97:5:97:10 | SSA variable mapper | PoC/server.py:100:9:100:14 | ControlFlowNode for mapper | | 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 | @@ -121,6 +125,10 @@ nodes | PoC/server.py:83:5:83:9 | SSA variable group | semmle.label | SSA variable group | | PoC/server.py:85:37:85:47 | ControlFlowNode for accumulator | semmle.label | ControlFlowNode for accumulator | | PoC/server.py:90:29:90:47 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | +| PoC/server.py:96:5:96:10 | SSA variable author | semmle.label | SSA variable author | +| PoC/server.py:96:14:96:20 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | +| PoC/server.py:97:5:97:10 | SSA variable mapper | semmle.label | SSA variable mapper | +| PoC/server.py:100:9:100:14 | ControlFlowNode for mapper | semmle.label | ControlFlowNode for mapper | | 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 | @@ -207,6 +215,7 @@ subpaths | 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: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 | | PoC/server.py:90:29:90:47 | ControlFlowNode for Dict | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | PoC/server.py:90:29:90:47 | 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:100:9:100:14 | ControlFlowNode for mapper | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | PoC/server.py:100:9:100:14 | ControlFlowNode for mapper | 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 | diff --git 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 index a0ce2944f697..893d972baadb 100644 --- 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 @@ -96,11 +96,14 @@ def by_map_reduce(): author = request.args['author'] mapper = 'function() { emit(this.author, this.author === "'+author+'") }' reducer = "function(key, values) { return values.some( x => x ) }" - results = posts.map_reduce(mapper, reducer, "results") + results = posts.map_reduce( + mapper, # $ result=BAD + reducer, # $ result=OK + "results") # Use `" | "a" === "a` as author # making the query `this.author === "" | "a" === "a"` # Found by http://127.0.0.1:5000/byMapReduce?author=%22%20|%20%22a%22%20===%20%22a - post = results.find_one({'value': True}) # $ MISSING: result=BAD + post = results.find_one({'value': True}) if(post): post["author"] = post["_id"] return show_post(post, author) From 12dab88ec7bffac9ecd387c34ad133aa5dfe45d9 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Wed, 20 Sep 2023 15:49:35 +0200 Subject: [PATCH 22/45] Python: rename concept `NoSqlQuery` -> `NoSqlExecution` --- python/ql/lib/semmle/python/Concepts.qll | 4 ++-- python/ql/lib/semmle/python/frameworks/NoSQL.qll | 10 +++++----- .../dataflow/NoSQLInjectionCustomizations.qll | 12 ++++++------ 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/python/ql/lib/semmle/python/Concepts.qll b/python/ql/lib/semmle/python/Concepts.qll index ea6310e326c0..5966000583c4 100644 --- a/python/ql/lib/semmle/python/Concepts.qll +++ b/python/ql/lib/semmle/python/Concepts.qll @@ -379,7 +379,7 @@ module SqlExecution { } /** Provides a class for modeling NoSql execution APIs. */ -module NoSqlQuery { +module NoSqlExecution { /** * A data-flow node that executes NoSQL queries. * @@ -404,7 +404,7 @@ module NoSqlQuery { * 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 { +class NoSqlExecution extends DataFlow::Node instanceof NoSqlExecution::Range { /** Gets the argument that specifies the NoSql query to be executed. */ DataFlow::Node getQuery() { result = super.getQuery() } diff --git a/python/ql/lib/semmle/python/frameworks/NoSQL.qll b/python/ql/lib/semmle/python/frameworks/NoSQL.qll index eb777b7199d7..9365dfa45e28 100644 --- a/python/ql/lib/semmle/python/frameworks/NoSQL.qll +++ b/python/ql/lib/semmle/python/frameworks/NoSQL.qll @@ -110,7 +110,7 @@ private module NoSql { * * `mongo.db.user.find({'name': safe_search})` would be a collection method call. */ - private class MongoCollectionCall extends DataFlow::CallCfgNode, NoSqlQuery::Range { + private class MongoCollectionCall extends DataFlow::CallCfgNode, NoSqlExecution::Range { MongoCollectionCall() { this = mongoCollection().getMember(mongoCollectionMethodName()).getACall() } @@ -122,7 +122,7 @@ private module NoSql { override predicate vulnerableToStrings() { none() } } - private class MongoCollectionAggregation extends API::CallNode, NoSqlQuery::Range { + private class MongoCollectionAggregation extends API::CallNode, NoSqlExecution::Range { MongoCollectionAggregation() { this = mongoCollection().getMember("aggregate").getACall() } override DataFlow::Node getQuery() { result = this.getParameter(0).getASubscript().asSink() } @@ -132,7 +132,7 @@ private module NoSql { override predicate vulnerableToStrings() { none() } } - private class MongoMapReduce extends API::CallNode, NoSqlQuery::Range { + private class MongoMapReduce extends API::CallNode, NoSqlExecution::Range { MongoMapReduce() { this = mongoCollection().getMember("map_reduce").getACall() } override DataFlow::Node getQuery() { result in [this.getArg(0), this.getArg(1)] } @@ -142,7 +142,7 @@ private module NoSql { override predicate vulnerableToStrings() { any() } } - private class MongoMapReduceQuery extends API::CallNode, NoSqlQuery::Range { + private class MongoMapReduceQuery extends API::CallNode, NoSqlExecution::Range { MongoMapReduceQuery() { this = mongoCollection().getMember("map_reduce").getACall() } override DataFlow::Node getQuery() { result in [this.getArgByName("query")] } @@ -248,7 +248,7 @@ private module NoSql { * * `Movie.objects(__raw__=json_search)` would be the result. */ - private class MongoEngineObjectsCall extends DataFlow::CallCfgNode, NoSqlQuery::Range { + private class MongoEngineObjectsCall extends DataFlow::CallCfgNode, NoSqlExecution::Range { MongoEngineObjectsCall() { this = [mongoEngine(), flask_MongoEngine()] diff --git a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll index debefd525a73..6026ae01f075 100644 --- a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll +++ b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll @@ -60,17 +60,17 @@ module NoSqlInjection { 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() + class NoSqlExecutionAsStringSink extends StringSink { + NoSqlExecutionAsStringSink() { + exists(NoSqlExecution noSqlExecution | this = noSqlExecution.getQuery() | + noSqlExecution.vulnerableToStrings() ) } } /** A NoSQL query that is vulnerable to user controlled dictionaries. */ - class NoSqlQueryAsDictSink extends DictSink { - NoSqlQueryAsDictSink() { this = any(NoSqlQuery noSqlQuery).getQuery() } + class NoSqlExecutionAsDictSink extends DictSink { + NoSqlExecutionAsDictSink() { this = any(NoSqlExecution noSqlExecution).getQuery() } } /** A JSON decoding converts a string to a dictionary. */ From 8156fa9a4df09b00c46981d93a8e66b2054c8a5d Mon Sep 17 00:00:00 2001 From: yoff Date: Thu, 28 Sep 2023 11:47:10 +0200 Subject: [PATCH 23/45] Apply naming suggestions from code review Co-authored-by: Rasmus Wriedt Larsen --- python/ql/lib/semmle/python/Concepts.qll | 6 +++--- .../semmle/python/security/dataflow/NoSQLInjectionQuery.qll | 4 ++-- .../Security/CWE-943-NoSqlInjection/DataflowQueryTest.ql | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/python/ql/lib/semmle/python/Concepts.qll b/python/ql/lib/semmle/python/Concepts.qll index 5966000583c4..f3dcddf3bfde 100644 --- a/python/ql/lib/semmle/python/Concepts.qll +++ b/python/ql/lib/semmle/python/Concepts.qll @@ -378,13 +378,13 @@ module SqlExecution { } } -/** Provides a class for modeling NoSql execution APIs. */ +/** Provides a class for modeling NoSQL execution APIs. */ module NoSqlExecution { /** * 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. + * extend `NoSqlExecution` instead. */ abstract class Range extends DataFlow::Node { /** Gets the argument that specifies the NoSql query to be executed. */ @@ -402,7 +402,7 @@ module NoSqlExecution { * 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. + * extend `NoSqlExecution::Range` instead. */ class NoSqlExecution extends DataFlow::Node instanceof NoSqlExecution::Range { /** Gets the argument that specifies the NoSql query to be executed. */ diff --git a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll index 8c6f06949d69..19e395860376 100644 --- a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll +++ b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll @@ -11,7 +11,7 @@ private import NoSQLInjectionCustomizations::NoSqlInjection as C /** * A taint-tracking configuration for detecting NoSQL injection vulnerabilities. */ -module NoSQLInjectionConfig implements DataFlow::StateConfigSig { +module NoSqlInjectionConfig implements DataFlow::StateConfigSig { class FlowState = C::FlowState; predicate isSource(DataFlow::Node source, FlowState state) { @@ -57,4 +57,4 @@ module NoSQLInjectionConfig implements DataFlow::StateConfigSig { } } -module Flow = TaintTracking::GlobalWithState; +module NoSqlInjectionFlow = TaintTracking::GlobalWithState; diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/DataflowQueryTest.ql b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/DataflowQueryTest.ql index e71c10b3d56e..79c28c399f38 100644 --- a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/DataflowQueryTest.ql +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/DataflowQueryTest.ql @@ -1,4 +1,4 @@ import python import experimental.dataflow.TestUtil.DataflowQueryTest import semmle.python.security.dataflow.NoSQLInjectionQuery -import FromTaintTrackingStateConfig +import FromTaintTrackingStateConfig From 37a4f356509446ebf410eea1f93204ee7c595b43 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Thu, 28 Sep 2023 11:49:42 +0200 Subject: [PATCH 24/45] Python: further rename --- .../semmle/python/security/dataflow/NoSQLInjectionQuery.qll | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll index 19e395860376..12bc207c6621 100644 --- a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll +++ b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll @@ -22,8 +22,8 @@ module NoSqlInjectionConfig implements DataFlow::StateConfigSig { state instanceof C::DictInput } - predicate isSink(DataFlow::Node source, FlowState state) { - source instanceof C::StringSink and + predicate isSink(DataFlow::Node sink, FlowState state) { + sink instanceof C::StringSink and ( state instanceof C::StringInput or @@ -31,7 +31,7 @@ module NoSqlInjectionConfig implements DataFlow::StateConfigSig { state instanceof C::DictInput ) or - source instanceof C::DictSink and + sink instanceof C::DictSink and state instanceof C::DictInput } From 3fb579eaffa56233f1ff03313dde1bbc2f81cba4 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Thu, 28 Sep 2023 12:14:12 +0200 Subject: [PATCH 25/45] Python: add test for type tracking --- .../CWE-943-NoSqlInjection/pymongo_test.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/pymongo_test.py b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/pymongo_test.py index 3dd667a3f2d5..69377260715a 100644 --- a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/pymongo_test.py +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/pymongo_test.py @@ -43,5 +43,24 @@ def bad3(): cursor = collection.find_one({"$where": f"this._id == '${event_id}'"}) #$ result=BAD +@app.route("/bad4") +def bad4(): + client = MongoClient("localhost", 27017, maxPoolSize=50) + db = client.get_database(name="localhost") + collection = db.get_collection("collection") + + decoded = json.loads(request.args['event_id']) + + search = { + "body": decoded, + "args": [ "$event_id" ], + "lang": "js" + } + collection.find_one({'$expr': {'$function': search}}) # $ result=BAD + + collection.find_one({'$expr': {'$function': decoded}}) # $ result=BAD + collection.find_one({'$expr': decoded}) # $ result=BAD + collection.find_one(decoded) # $ result=BAD + if __name__ == "__main__": app.run(debug=True) From d90630aa66e8fffcaee61366e4adbf2de34a96d2 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Thu, 28 Sep 2023 12:34:10 +0200 Subject: [PATCH 26/45] Python: fix query file --- python/ql/src/Security/CWE-943/NoSQLInjection.ql | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/python/ql/src/Security/CWE-943/NoSQLInjection.ql b/python/ql/src/Security/CWE-943/NoSQLInjection.ql index fd588f009f0b..0b9ac2139e71 100644 --- a/python/ql/src/Security/CWE-943/NoSQLInjection.ql +++ b/python/ql/src/Security/CWE-943/NoSQLInjection.ql @@ -4,17 +4,17 @@ * malicious NoSQL code by the user. * @kind path-problem * @problem.severity error + * @security-severity 8.8 * @id py/nosql-injection * @tags security - * experimental * external/cwe/cwe-943 */ import python import semmle.python.security.dataflow.NoSQLInjectionQuery -import Flow::PathGraph +import NoSqlInjectionFlow::PathGraph -from Flow::PathNode source, Flow::PathNode sink -where Flow::flowPath(source, sink) +from NoSqlInjectionFlow::PathNode source, NoSqlInjectionFlow::PathNode sink +where NoSqlInjectionFlow::flowPath(source, sink) select sink.getNode(), source, sink, "This NoSQL query contains an unsanitized $@.", source, "user-provided value" From c2b63830f1688fe352ab6764b7c7d3375f842326 Mon Sep 17 00:00:00 2001 From: yoff Date: Thu, 28 Sep 2023 12:40:37 +0200 Subject: [PATCH 27/45] Apply suggestions from code review Claim conversions do not execute inputs in order to remove interaction with `py/unsafe-deserialization`. Co-authored-by: Rasmus Wriedt Larsen --- python/ql/lib/semmle/python/frameworks/NoSQL.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/NoSQL.qll b/python/ql/lib/semmle/python/frameworks/NoSQL.qll index 9365dfa45e28..55c60138ff7b 100644 --- a/python/ql/lib/semmle/python/frameworks/NoSQL.qll +++ b/python/ql/lib/semmle/python/frameworks/NoSQL.qll @@ -170,7 +170,7 @@ private module NoSql { override string getFormat() { result = "NoSQL" } - override predicate mayExecuteInput() { any() } + override predicate mayExecuteInput() { none() } } /** @@ -200,7 +200,7 @@ private module NoSql { override string getFormat() { result = "NoSQL" } - override predicate mayExecuteInput() { any() } + override predicate mayExecuteInput() { none() } } /** From 9682c8218afbff12acb757d6c23cbfa032879cb4 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Thu, 28 Sep 2023 12:50:36 +0200 Subject: [PATCH 28/45] Python: rename file --- python/ql/lib/semmle/python/Frameworks.qll | 2 +- .../ql/lib/semmle/python/frameworks/{NoSQL.qll => PyMongo.qll} | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) rename python/ql/lib/semmle/python/frameworks/{NoSQL.qll => PyMongo.qll} (98%) diff --git a/python/ql/lib/semmle/python/Frameworks.qll b/python/ql/lib/semmle/python/Frameworks.qll index 171918349c64..b9452e5d4ddc 100644 --- a/python/ql/lib/semmle/python/Frameworks.qll +++ b/python/ql/lib/semmle/python/Frameworks.qll @@ -36,13 +36,13 @@ 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 private import semmle.python.frameworks.Psycopg2 private import semmle.python.frameworks.Pycurl private import semmle.python.frameworks.Pydantic +private import semmle.python.frameworks.PyMongo private import semmle.python.frameworks.Pymssql private import semmle.python.frameworks.PyMySQL private import semmle.python.frameworks.Pyodbc diff --git a/python/ql/lib/semmle/python/frameworks/NoSQL.qll b/python/ql/lib/semmle/python/frameworks/PyMongo.qll similarity index 98% rename from python/ql/lib/semmle/python/frameworks/NoSQL.qll rename to python/ql/lib/semmle/python/frameworks/PyMongo.qll index 55c60138ff7b..360389ac462e 100644 --- a/python/ql/lib/semmle/python/frameworks/NoSQL.qll +++ b/python/ql/lib/semmle/python/frameworks/PyMongo.qll @@ -1,6 +1,5 @@ /** - * Provides classes modeling security-relevant aspects of the standard libraries. - * Note: some modeling is done internally in the dataflow/taint tracking implementation. + * Provides classes modeling security-relevant aspects of the PyMongo bindings. */ private import python From 2a739b3b7a04bdbd40331b2fd5e4c2c2f043819d Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Thu, 28 Sep 2023 12:51:11 +0200 Subject: [PATCH 29/45] Python: rename module --- python/ql/lib/semmle/python/frameworks/PyMongo.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/lib/semmle/python/frameworks/PyMongo.qll b/python/ql/lib/semmle/python/frameworks/PyMongo.qll index 360389ac462e..c3f38169d01e 100644 --- a/python/ql/lib/semmle/python/frameworks/PyMongo.qll +++ b/python/ql/lib/semmle/python/frameworks/PyMongo.qll @@ -9,7 +9,7 @@ private import semmle.python.dataflow.new.RemoteFlowSources private import semmle.python.Concepts private import semmle.python.ApiGraphs -private module NoSql { +private module PyMongo { // API Nodes returning `Mongo` instances. /** Gets a reference to `pymongo.MongoClient` */ private API::Node pyMongo() { From eb1be08bce7ce353a9455359fcf54e6d58724898 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Thu, 28 Sep 2023 12:52:56 +0200 Subject: [PATCH 30/45] Python: split modelling --- .../ql/lib/semmle/python/frameworks/BSon.qll | 29 +++++++++++++++++++ .../lib/semmle/python/frameworks/PyMongo.qll | 19 ------------ 2 files changed, 29 insertions(+), 19 deletions(-) create mode 100644 python/ql/lib/semmle/python/frameworks/BSon.qll diff --git a/python/ql/lib/semmle/python/frameworks/BSon.qll b/python/ql/lib/semmle/python/frameworks/BSon.qll new file mode 100644 index 000000000000..aa8b1c58288f --- /dev/null +++ b/python/ql/lib/semmle/python/frameworks/BSon.qll @@ -0,0 +1,29 @@ +/** + * Provides classes modeling security-relevant aspects of the PyMongo bindings. + */ + +private import python +private import semmle.python.dataflow.new.DataFlow +private import semmle.python.Concepts +private import semmle.python.ApiGraphs + +module BSon { + /** + * ObjectId returns a string representing an id. + * If at any time ObjectId can't parse it's input (like when a tainted dict in passed in), + * then ObjectId will throw an error preventing the query from running. + */ + private class BsonObjectIdCall extends DataFlow::CallCfgNode, NoSqlSanitizer::Range { + BsonObjectIdCall() { + exists(API::Node mod | + mod = API::moduleImport("bson") + or + mod = API::moduleImport("bson").getMember(["objectid", "json_util"]) + | + this = mod.getMember("ObjectId").getACall() + ) + } + + override DataFlow::Node getAnInput() { result = this.getArg(0) } + } +} diff --git a/python/ql/lib/semmle/python/frameworks/PyMongo.qll b/python/ql/lib/semmle/python/frameworks/PyMongo.qll index c3f38169d01e..2c3d6195a5fd 100644 --- a/python/ql/lib/semmle/python/frameworks/PyMongo.qll +++ b/python/ql/lib/semmle/python/frameworks/PyMongo.qll @@ -274,25 +274,6 @@ private module PyMongo { override DataFlow::Node getAnInput() { result = this.getArg(0) } } - /** - * ObjectId returns a string representing an id. - * If at any time ObjectId can't parse it's input (like when a tainted dict in passed in), - * then ObjectId will throw an error preventing the query from running. - */ - private class BsonObjectIdCall extends DataFlow::CallCfgNode, NoSqlSanitizer::Range { - BsonObjectIdCall() { - exists(API::Node mod | - mod = API::moduleImport("bson") - or - mod = API::moduleImport("bson").getMember(["objectid", "json_util"]) - | - this = mod.getMember("ObjectId").getACall() - ) - } - - 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 From 2a7b59328583f98e0aeaca885d8e6b762cf97e12 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Thu, 28 Sep 2023 13:35:29 +0200 Subject: [PATCH 31/45] Python: Fix QL alerts --- .../ql/lib/semmle/python/frameworks/BSon.qll | 2 +- .../lib/semmle/python/frameworks/PyMongo.qll | 44 ++++++++++--------- 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/BSon.qll b/python/ql/lib/semmle/python/frameworks/BSon.qll index aa8b1c58288f..05bfb108cc7b 100644 --- a/python/ql/lib/semmle/python/frameworks/BSon.qll +++ b/python/ql/lib/semmle/python/frameworks/BSon.qll @@ -7,7 +7,7 @@ private import semmle.python.dataflow.new.DataFlow private import semmle.python.Concepts private import semmle.python.ApiGraphs -module BSon { +private module BSon { /** * ObjectId returns a string representing an id. * If at any time ObjectId can't parse it's input (like when a tainted dict in passed in), diff --git a/python/ql/lib/semmle/python/frameworks/PyMongo.qll b/python/ql/lib/semmle/python/frameworks/PyMongo.qll index 2c3d6195a5fd..bdc120c3b598 100644 --- a/python/ql/lib/semmle/python/frameworks/PyMongo.qll +++ b/python/ql/lib/semmle/python/frameworks/PyMongo.qll @@ -144,7 +144,7 @@ private module PyMongo { private class MongoMapReduceQuery extends API::CallNode, NoSqlExecution::Range { MongoMapReduceQuery() { this = mongoCollection().getMember("map_reduce").getACall() } - override DataFlow::Node getQuery() { result in [this.getArgByName("query")] } + override DataFlow::Node getQuery() { result = this.getArgByName("query") } override predicate interpretsDict() { any() } @@ -178,19 +178,20 @@ private module PyMongo { * See https://www.mongodb.com/docs/manual/reference/operator/aggregation/function/#mongodb-expression-exp.-function */ private class FunctionQueryOperator extends DataFlow::Node, Decoding::Range { - API::Node dictionary; DataFlow::Node query; FunctionQueryOperator() { - dictionary = - mongoCollection() - .getMember(mongoCollectionMethodName()) - .getACall() - .getParameter(0) - .getASubscript*() - .getSubscript("$function") and - query = dictionary.getSubscript("body").asSink() and - this = dictionary.asSink() + exists(API::Node dictionary | + dictionary = + mongoCollection() + .getMember(mongoCollectionMethodName()) + .getACall() + .getParameter(0) + .getASubscript*() + .getSubscript("$function") and + query = dictionary.getSubscript("body").asSink() and + this = dictionary.asSink() + ) } override DataFlow::Node getAnInput() { result = query } @@ -208,19 +209,20 @@ private module PyMongo { * See https://www.mongodb.com/docs/manual/reference/operator/aggregation/accumulator/#mongodb-group-grp.-accumulator */ private class AccumulatorQueryOperator extends DataFlow::Node, Decoding::Range { - API::Node dictionary; DataFlow::Node query; AccumulatorQueryOperator() { - dictionary = - mongoCollection() - .getMember("aggregate") - .getACall() - .getParameter(0) - .getASubscript*() - .getSubscript("$accumulator") and - query = dictionary.getSubscript(["init", "accumulate", "merge", "finalize"]).asSink() and - this = dictionary.asSink() + exists(API::Node dictionary | + dictionary = + mongoCollection() + .getMember("aggregate") + .getACall() + .getParameter(0) + .getASubscript*() + .getSubscript("$accumulator") and + query = dictionary.getSubscript(["init", "accumulate", "merge", "finalize"]).asSink() and + this = dictionary.asSink() + ) } override DataFlow::Node getAnInput() { result = query } From a8e0023f390a53d6f17b55ab2e29d6f31670f113 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Thu, 28 Sep 2023 13:42:33 +0200 Subject: [PATCH 32/45] Python: forgot to list framework --- python/ql/lib/semmle/python/Frameworks.qll | 1 + 1 file changed, 1 insertion(+) diff --git a/python/ql/lib/semmle/python/Frameworks.qll b/python/ql/lib/semmle/python/Frameworks.qll index b9452e5d4ddc..ff0650fcda09 100644 --- a/python/ql/lib/semmle/python/Frameworks.qll +++ b/python/ql/lib/semmle/python/Frameworks.qll @@ -10,6 +10,7 @@ private import semmle.python.frameworks.Aiomysql private import semmle.python.frameworks.Aiosqlite private import semmle.python.frameworks.Aiopg private import semmle.python.frameworks.Asyncpg +private import semmle.python.frameworks.BSon private import semmle.python.frameworks.CassandraDriver private import semmle.python.frameworks.ClickhouseDriver private import semmle.python.frameworks.Cryptodome From d5b64c5ff2abe8565c14f4dd4939ab0d99a81b04 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Thu, 28 Sep 2023 14:20:30 +0200 Subject: [PATCH 33/45] Python: update test expectations --- .../NoSqlInjection.expected | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected index 0eae28cecc1b..448196bff3a3 100644 --- a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected @@ -86,6 +86,7 @@ edges | pymongo_test.py:1:26:1:32 | GSSA Variable request | pymongo_test.py:12:21:12:27 | ControlFlowNode for request | | pymongo_test.py:1:26:1:32 | GSSA Variable request | pymongo_test.py:29:27:29:33 | ControlFlowNode for request | | pymongo_test.py:1:26:1:32 | GSSA Variable request | pymongo_test.py:39:27:39:33 | ControlFlowNode for request | +| pymongo_test.py:1:26:1:32 | GSSA Variable request | pymongo_test.py:52:26:52:32 | ControlFlowNode for request | | pymongo_test.py:12:5:12:17 | SSA variable unsafe_search | pymongo_test.py:13:30:13:42 | ControlFlowNode for unsafe_search | | pymongo_test.py:12:21:12:27 | ControlFlowNode for request | pymongo_test.py:12:5:12:17 | SSA variable unsafe_search | | pymongo_test.py:13:5:13:15 | SSA variable json_search | pymongo_test.py:15:42:15:62 | ControlFlowNode for Dict | @@ -101,6 +102,17 @@ edges | 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 | +| pymongo_test.py:52:5:52:11 | SSA variable decoded | pymongo_test.py:55:17:55:23 | ControlFlowNode for decoded | +| pymongo_test.py:52:15:52:50 | ControlFlowNode for Attribute() | pymongo_test.py:52:5:52:11 | SSA variable decoded | +| pymongo_test.py:52:26:52:32 | ControlFlowNode for request | pymongo_test.py:52:26:52:49 | ControlFlowNode for Subscript | +| pymongo_test.py:52:26:52:49 | ControlFlowNode for Subscript | pymongo_test.py:52:15:52:50 | ControlFlowNode for Attribute() | +| pymongo_test.py:54:5:54:10 | SSA variable search | pymongo_test.py:59:49:59:54 | ControlFlowNode for search | +| pymongo_test.py:55:17:55:23 | ControlFlowNode for decoded | pymongo_test.py:54:5:54:10 | SSA variable search | +| pymongo_test.py:55:17:55:23 | ControlFlowNode for decoded | pymongo_test.py:59:49:59:54 | ControlFlowNode for search | +| pymongo_test.py:55:17:55:23 | ControlFlowNode for decoded | pymongo_test.py:61:25:61:57 | ControlFlowNode for Dict | +| pymongo_test.py:55:17:55:23 | ControlFlowNode for decoded | pymongo_test.py:62:25:62:42 | ControlFlowNode for Dict | +| pymongo_test.py:55:17:55:23 | ControlFlowNode for decoded | pymongo_test.py:63:25:63:31 | ControlFlowNode for decoded | +| pymongo_test.py:59:49:59:54 | ControlFlowNode for search | pymongo_test.py:59:25:59:56 | 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 | @@ -209,6 +221,17 @@ nodes | pymongo_test.py:39:27:39:50 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript | | pymongo_test.py:43:34:43:73 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | | pymongo_test.py:43:45:43:72 | ControlFlowNode for Fstring | semmle.label | ControlFlowNode for Fstring | +| pymongo_test.py:52:5:52:11 | SSA variable decoded | semmle.label | SSA variable decoded | +| pymongo_test.py:52:15:52:50 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| pymongo_test.py:52:26:52:32 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | +| pymongo_test.py:52:26:52:49 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript | +| pymongo_test.py:54:5:54:10 | SSA variable search | semmle.label | SSA variable search | +| pymongo_test.py:55:17:55:23 | ControlFlowNode for decoded | semmle.label | ControlFlowNode for decoded | +| pymongo_test.py:59:25:59:56 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | +| pymongo_test.py:59:49:59:54 | ControlFlowNode for search | semmle.label | ControlFlowNode for search | +| pymongo_test.py:61:25:61:57 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | +| pymongo_test.py:62:25:62:42 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | +| pymongo_test.py:63:25:63:31 | ControlFlowNode for decoded | semmle.label | ControlFlowNode for decoded | 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 | @@ -228,3 +251,7 @@ subpaths | 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: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:59:25:59:56 | ControlFlowNode for Dict | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | pymongo_test.py:59:25:59:56 | 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:61:25:61:57 | ControlFlowNode for Dict | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | pymongo_test.py:61:25:61:57 | 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:62:25:62:42 | ControlFlowNode for Dict | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | pymongo_test.py:62:25:62:42 | 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:63:25:63:31 | ControlFlowNode for decoded | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | pymongo_test.py:63:25:63:31 | ControlFlowNode for decoded | This NoSQL query contains an unsanitized $@. | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value | From 3043633d9c16a9eb1cf0aab3249d38430a062ad6 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Thu, 28 Sep 2023 14:24:49 +0200 Subject: [PATCH 34/45] Python: Some renaming of flow states --- .../dataflow/NoSQLInjectionCustomizations.qll | 49 ++++++++++++------- .../security/dataflow/NoSQLInjectionQuery.qll | 21 ++++---- 2 files changed, 42 insertions(+), 28 deletions(-) diff --git a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll index 6026ae01f075..f516a61e0aff 100644 --- a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll +++ b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll @@ -17,7 +17,7 @@ import semmle.python.Concepts module NoSqlInjection { private newtype TFlowState = TStringInput() or - TDictInput() + TInterpretedStringInput() /** A flow state, tracking the structure of the input. */ abstract class FlowState extends TFlowState { @@ -30,29 +30,33 @@ module NoSqlInjection { override string toString() { result = "StringInput" } } - /** A state where input is a dictionary. */ - class DictInput extends FlowState, TDictInput { - override string toString() { result = "DictInput" } + /** + * A state where input is a string that has been interpreted. + * For instance, it could have been turned into a dictionary, + * or evaluated as javascript code. + */ + class InterpretedStringInput extends FlowState, TInterpretedStringInput { + override string toString() { result = "InterpretedStringInput" } } /** A source allowing string inputs. */ abstract class StringSource extends DataFlow::Node { } - /** A source allowing dictionary inputs. */ - abstract class DictSource extends DataFlow::Node { } + /** A source of interpreted strings. */ + abstract class InterpretedStringSource 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 sink vulnerable to user controlled interpreted strings. */ + abstract class InterpretedStringSink 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. */ + /** A data flow node where a string is being interpreted. */ + abstract class StringInterpretation extends DataFlow::Node { + /** Gets the argument that specifies the string to be interpreted. */ abstract DataFlow::Node getAnInput(); - /** Gets the resulting dictionary. */ + /** Gets the result of interpreting the string. */ abstract DataFlow::Node getOutput(); } @@ -68,14 +72,23 @@ module NoSqlInjection { } } - /** A NoSQL query that is vulnerable to user controlled dictionaries. */ - class NoSqlExecutionAsDictSink extends DictSink { - NoSqlExecutionAsDictSink() { this = any(NoSqlExecution noSqlExecution).getQuery() } + /** A NoSQL query that is vulnerable to user controlled InterpretedStringionaries. */ + class NoSqlExecutionAsInterpretedStringSink extends InterpretedStringSink { + NoSqlExecutionAsInterpretedStringSink() { this = any(NoSqlExecution noSqlExecution).getQuery() } } - /** A JSON decoding converts a string to a dictionary. */ - class JsonDecoding extends Decoding, StringToDictConversion { - JsonDecoding() { this.getFormat() in ["JSON", "NoSQL"] } + /** A JSON decoding converts a string to a Dictionary. */ + class JsonDecoding extends Decoding, StringInterpretation { + JsonDecoding() { this.getFormat() = "JSON" } + + override DataFlow::Node getAnInput() { result = Decoding.super.getAnInput() } + + override DataFlow::Node getOutput() { result = Decoding.super.getOutput() } + } + + /** A NoSQL decoding interprets a string. */ + class NoSqlDecoding extends Decoding, StringInterpretation { + NoSqlDecoding() { this.getFormat() = "NoSQL" } override DataFlow::Node getAnInput() { result = Decoding.super.getAnInput() } diff --git a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll index 12bc207c6621..2e8e827b55c4 100644 --- a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll +++ b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll @@ -18,8 +18,8 @@ module NoSqlInjectionConfig implements DataFlow::StateConfigSig { source instanceof C::StringSource and state instanceof C::StringInput or - source instanceof C::DictSource and - state instanceof C::DictInput + source instanceof C::InterpretedStringSource and + state instanceof C::InterpretedStringInput } predicate isSink(DataFlow::Node sink, FlowState state) { @@ -27,29 +27,30 @@ module NoSqlInjectionConfig implements DataFlow::StateConfigSig { ( state instanceof C::StringInput or - // since dictionaries can encode strings - state instanceof C::DictInput + // since InterpretedStrings can include strings, + // e.g. JSON objects can encode strings. + state instanceof C::InterpretedStringInput ) or - sink instanceof C::DictSink and - state instanceof C::DictInput + sink instanceof C::InterpretedStringSink and + state instanceof C::InterpretedStringInput } 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 + // Block `StringInput` paths here, since they change state to `InterpretedStringInput` + exists(C::StringInterpretation c | node = c.getOutput()) and state instanceof C::StringInput } predicate isAdditionalFlowStep( DataFlow::Node nodeFrom, FlowState stateFrom, DataFlow::Node nodeTo, FlowState stateTo ) { - exists(C::StringToDictConversion c | + exists(C::StringInterpretation c | nodeFrom = c.getAnInput() and nodeTo = c.getOutput() ) and stateFrom instanceof C::StringInput and - stateTo instanceof C::DictInput + stateTo instanceof C::InterpretedStringInput } predicate isBarrier(DataFlow::Node node) { From 2e028a41eeff2517574345e4a0d5963d93ab68e1 Mon Sep 17 00:00:00 2001 From: yoff Date: Fri, 29 Sep 2023 11:32:51 +0200 Subject: [PATCH 35/45] Apply suggestions from code review Co-authored-by: Rasmus Wriedt Larsen --- python/ql/lib/semmle/python/frameworks/BSon.qll | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/python/ql/lib/semmle/python/frameworks/BSon.qll b/python/ql/lib/semmle/python/frameworks/BSon.qll index 05bfb108cc7b..4888c7786c03 100644 --- a/python/ql/lib/semmle/python/frameworks/BSon.qll +++ b/python/ql/lib/semmle/python/frameworks/BSon.qll @@ -1,5 +1,8 @@ /** - * Provides classes modeling security-relevant aspects of the PyMongo bindings. + * Provides classes modeling security-relevant aspects of the `bson` PyPI package. + * See + * - https://pypi.org/project/bson/ + * - https://github.com/py-bson/bson */ private import python @@ -7,6 +10,12 @@ private import semmle.python.dataflow.new.DataFlow private import semmle.python.Concepts private import semmle.python.ApiGraphs +/** + * Provides models for the `bson` PyPI package. + * See + * - https://pypi.org/project/bson/ + * - https://github.com/py-bson/bson + */ private module BSon { /** * ObjectId returns a string representing an id. From 74d6f37467d976d60f3f15dcd3605a692389b1bf Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Fri, 29 Sep 2023 11:28:17 +0200 Subject: [PATCH 36/45] Python: update meta query `TaintSinks` --- python/ql/src/meta/alerts/TaintSinks.ql | 1 + 1 file changed, 1 insertion(+) diff --git a/python/ql/src/meta/alerts/TaintSinks.ql b/python/ql/src/meta/alerts/TaintSinks.ql index 634ef18f1294..193cdac61712 100644 --- a/python/ql/src/meta/alerts/TaintSinks.ql +++ b/python/ql/src/meta/alerts/TaintSinks.ql @@ -17,6 +17,7 @@ import semmle.python.security.dataflow.CodeInjectionCustomizations import semmle.python.security.dataflow.CommandInjectionCustomizations import semmle.python.security.dataflow.LdapInjectionCustomizations import semmle.python.security.dataflow.LogInjectionCustomizations +import semmle.python.security.dataflow.NoSQLInjectionCustomizations import semmle.python.security.dataflow.PathInjectionCustomizations import semmle.python.security.dataflow.PolynomialReDoSCustomizations import semmle.python.security.dataflow.ReflectedXSSCustomizations From 2d845e3e55899e75e3016454453f61a9c3f6434c Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Fri, 29 Sep 2023 12:01:41 +0200 Subject: [PATCH 37/45] Python: nicer paths turn "the long jump" that would end up straight at the argument into a short jump that ends up at the dictionary being written to. Dataflow takes care of the rest of the path. --- .../lib/semmle/python/frameworks/PyMongo.qll | 6 ++--- .../NoSqlInjection.expected | 25 +++++++++++-------- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/PyMongo.qll b/python/ql/lib/semmle/python/frameworks/PyMongo.qll index bdc120c3b598..27c40f1e1c64 100644 --- a/python/ql/lib/semmle/python/frameworks/PyMongo.qll +++ b/python/ql/lib/semmle/python/frameworks/PyMongo.qll @@ -160,7 +160,7 @@ private module PyMongo { dictionary = mongoCollection().getMember(mongoCollectionMethodName()).getACall().getParameter(0) and query = dictionary.getSubscript("$where").asSink() and - this = dictionary.asSink() + this = dictionary.getAValueReachingSink() } override DataFlow::Node getAnInput() { result = query } @@ -190,7 +190,7 @@ private module PyMongo { .getASubscript*() .getSubscript("$function") and query = dictionary.getSubscript("body").asSink() and - this = dictionary.asSink() + this = dictionary.getAValueReachingSink() ) } @@ -221,7 +221,7 @@ private module PyMongo { .getASubscript*() .getSubscript("$accumulator") and query = dictionary.getSubscript(["init", "accumulate", "merge", "finalize"]).asSink() and - this = dictionary.asSink() + this = dictionary.getAValueReachingSink() ) } diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected index 448196bff3a3..beaa85d53122 100644 --- a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected @@ -15,13 +15,15 @@ edges | 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:51:60:56 | ControlFlowNode for search | -| PoC/server.py:60:51:60:56 | ControlFlowNode for search | PoC/server.py:60:27:60:58 | ControlFlowNode for Dict | +| PoC/server.py:52:5:52:10 | SSA variable search | PoC/server.py:60:27:60:58 | ControlFlowNode for Dict | +| PoC/server.py:52:14:56:5 | ControlFlowNode for Dict | PoC/server.py:52:5:52:10 | SSA variable search | +| PoC/server.py:53:17:53:70 | ControlFlowNode for BinaryExpr | PoC/server.py:52:14:56:5 | ControlFlowNode for Dict | | PoC/server.py:76:5:76:10 | SSA variable author | PoC/server.py:79:23:79:101 | ControlFlowNode for BinaryExpr | | PoC/server.py:76:14:76:20 | ControlFlowNode for request | PoC/server.py:76:5:76:10 | SSA variable author | -| PoC/server.py:79:23:79:101 | ControlFlowNode for BinaryExpr | PoC/server.py:85:37:85:47 | ControlFlowNode for accumulator | +| PoC/server.py:77:5:77:15 | SSA variable accumulator | PoC/server.py:83:5:83:9 | SSA variable group | +| PoC/server.py:77:19:82:5 | ControlFlowNode for Dict | PoC/server.py:77:5:77:15 | SSA variable accumulator | +| PoC/server.py:79:23:79:101 | ControlFlowNode for BinaryExpr | PoC/server.py:77:19:82:5 | ControlFlowNode for Dict | | PoC/server.py:83:5:83:9 | SSA variable group | PoC/server.py:90:29:90:47 | ControlFlowNode for Dict | -| PoC/server.py:85:37:85:47 | ControlFlowNode for accumulator | PoC/server.py:83:5:83:9 | SSA variable group | | PoC/server.py:96:5:96:10 | SSA variable author | PoC/server.py:97:5:97:10 | SSA variable mapper | | PoC/server.py:96:14:96:20 | ControlFlowNode for request | PoC/server.py:96:5:96:10 | SSA variable author | | PoC/server.py:97:5:97:10 | SSA variable mapper | PoC/server.py:100:9:100:14 | ControlFlowNode for mapper | @@ -106,13 +108,12 @@ edges | pymongo_test.py:52:15:52:50 | ControlFlowNode for Attribute() | pymongo_test.py:52:5:52:11 | SSA variable decoded | | pymongo_test.py:52:26:52:32 | ControlFlowNode for request | pymongo_test.py:52:26:52:49 | ControlFlowNode for Subscript | | pymongo_test.py:52:26:52:49 | ControlFlowNode for Subscript | pymongo_test.py:52:15:52:50 | ControlFlowNode for Attribute() | -| pymongo_test.py:54:5:54:10 | SSA variable search | pymongo_test.py:59:49:59:54 | ControlFlowNode for search | -| pymongo_test.py:55:17:55:23 | ControlFlowNode for decoded | pymongo_test.py:54:5:54:10 | SSA variable search | -| pymongo_test.py:55:17:55:23 | ControlFlowNode for decoded | pymongo_test.py:59:49:59:54 | ControlFlowNode for search | +| pymongo_test.py:54:5:54:10 | SSA variable search | pymongo_test.py:59:25:59:56 | ControlFlowNode for Dict | +| pymongo_test.py:54:14:58:5 | ControlFlowNode for Dict | pymongo_test.py:54:5:54:10 | SSA variable search | +| pymongo_test.py:55:17:55:23 | ControlFlowNode for decoded | pymongo_test.py:54:14:58:5 | ControlFlowNode for Dict | | pymongo_test.py:55:17:55:23 | ControlFlowNode for decoded | pymongo_test.py:61:25:61:57 | ControlFlowNode for Dict | | pymongo_test.py:55:17:55:23 | ControlFlowNode for decoded | pymongo_test.py:62:25:62:42 | ControlFlowNode for Dict | | pymongo_test.py:55:17:55:23 | ControlFlowNode for decoded | pymongo_test.py:63:25:63:31 | ControlFlowNode for decoded | -| pymongo_test.py:59:49:59:54 | ControlFlowNode for search | pymongo_test.py:59:25:59:56 | 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 | @@ -128,14 +129,16 @@ nodes | 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:52:5:52:10 | SSA variable search | semmle.label | SSA variable search | +| PoC/server.py:52:14:56:5 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | | 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:51:60:56 | ControlFlowNode for search | semmle.label | ControlFlowNode for search | | PoC/server.py:76:5:76:10 | SSA variable author | semmle.label | SSA variable author | | PoC/server.py:76:14:76:20 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | +| PoC/server.py:77:5:77:15 | SSA variable accumulator | semmle.label | SSA variable accumulator | +| PoC/server.py:77:19:82:5 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | | PoC/server.py:79:23:79:101 | ControlFlowNode for BinaryExpr | semmle.label | ControlFlowNode for BinaryExpr | | PoC/server.py:83:5:83:9 | SSA variable group | semmle.label | SSA variable group | -| PoC/server.py:85:37:85:47 | ControlFlowNode for accumulator | semmle.label | ControlFlowNode for accumulator | | PoC/server.py:90:29:90:47 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | | PoC/server.py:96:5:96:10 | SSA variable author | semmle.label | SSA variable author | | PoC/server.py:96:14:96:20 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | @@ -226,9 +229,9 @@ nodes | pymongo_test.py:52:26:52:32 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | | pymongo_test.py:52:26:52:49 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript | | pymongo_test.py:54:5:54:10 | SSA variable search | semmle.label | SSA variable search | +| pymongo_test.py:54:14:58:5 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | | pymongo_test.py:55:17:55:23 | ControlFlowNode for decoded | semmle.label | ControlFlowNode for decoded | | pymongo_test.py:59:25:59:56 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | -| pymongo_test.py:59:49:59:54 | ControlFlowNode for search | semmle.label | ControlFlowNode for search | | pymongo_test.py:61:25:61:57 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | | pymongo_test.py:62:25:62:42 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | | pymongo_test.py:63:25:63:31 | ControlFlowNode for decoded | semmle.label | ControlFlowNode for decoded | From e1708054a46c460bc45a98e856c7ad654ca39ee4 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Fri, 29 Sep 2023 12:06:51 +0200 Subject: [PATCH 38/45] Python: fix QL alert --- python/ql/lib/semmle/python/frameworks/PyMongo.qll | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/PyMongo.qll b/python/ql/lib/semmle/python/frameworks/PyMongo.qll index 27c40f1e1c64..148cb074a104 100644 --- a/python/ql/lib/semmle/python/frameworks/PyMongo.qll +++ b/python/ql/lib/semmle/python/frameworks/PyMongo.qll @@ -153,14 +153,15 @@ private module PyMongo { /** The `$where` query operator executes a string as JavaScript. */ private class WhereQueryOperator extends DataFlow::Node, Decoding::Range { - API::Node dictionary; DataFlow::Node query; WhereQueryOperator() { - dictionary = - mongoCollection().getMember(mongoCollectionMethodName()).getACall().getParameter(0) and - query = dictionary.getSubscript("$where").asSink() and - this = dictionary.getAValueReachingSink() + exists(API::Node dictionary | + dictionary = + mongoCollection().getMember(mongoCollectionMethodName()).getACall().getParameter(0) and + query = dictionary.getSubscript("$where").asSink() and + this = dictionary.getAValueReachingSink() + ) } override DataFlow::Node getAnInput() { result = query } From f3a01612e84e8d5a5fcc51526ff0f41ab26967dc Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Fri, 29 Sep 2023 13:23:36 +0200 Subject: [PATCH 39/45] Python: rename flow states Close to being a revert of https://github.com/github/codeql/pull/14070/commits/3043633d9c16a9eb1cf0aab3249d38430a062ad6 but with slightly shorter names and added comments. --- .../dataflow/NoSQLInjectionCustomizations.qll | 55 ++++++++++--------- .../security/dataflow/NoSQLInjectionQuery.qll | 28 +++++----- 2 files changed, 43 insertions(+), 40 deletions(-) diff --git a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll index f516a61e0aff..a07d3caf8236 100644 --- a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll +++ b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll @@ -16,47 +16,50 @@ import semmle.python.Concepts */ module NoSqlInjection { private newtype TFlowState = - TStringInput() or - TInterpretedStringInput() + TString() or + TDict() - /** A flow state, tracking the structure of the input. */ + /** A flow state, tracking the structure of the data. */ 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 the tracked data is only a string. */ + class String extends FlowState, TString { + override string toString() { result = "String" } } /** - * A state where input is a string that has been interpreted. - * For instance, it could have been turned into a dictionary, - * or evaluated as javascript code. + * A state where the tracked data has been converted to + * a dictionary. + * + * We include cases where data represent JSON objects, so + * it could actually still be just a string. It could + * also contain query operators, or even JavaScript code. */ - class InterpretedStringInput extends FlowState, TInterpretedStringInput { - override string toString() { result = "InterpretedStringInput" } + class Dict extends FlowState, TDict { + override string toString() { result = "Dict" } } /** A source allowing string inputs. */ abstract class StringSource extends DataFlow::Node { } - /** A source of interpreted strings. */ - abstract class InterpretedStringSource extends DataFlow::Node { } + /** A source of allowing dictionaries. */ + 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 interpreted strings. */ - abstract class InterpretedStringSink extends DataFlow::Node { } + /** A sink vulnerable to user controlled dictionaries. */ + abstract class DictSink extends DataFlow::Node { } - /** A data flow node where a string is being interpreted. */ - abstract class StringInterpretation extends DataFlow::Node { - /** Gets the argument that specifies the string to be interpreted. */ + /** 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 result of interpreting the string. */ + /** Gets the resulting dictionary. */ abstract DataFlow::Node getOutput(); } @@ -72,13 +75,13 @@ module NoSqlInjection { } } - /** A NoSQL query that is vulnerable to user controlled InterpretedStringionaries. */ - class NoSqlExecutionAsInterpretedStringSink extends InterpretedStringSink { - NoSqlExecutionAsInterpretedStringSink() { this = any(NoSqlExecution noSqlExecution).getQuery() } + /** A NoSQL query that is vulnerable to user controlled dictionaries. */ + class NoSqlExecutionAsDictSink extends DictSink { + NoSqlExecutionAsDictSink() { this = any(NoSqlExecution noSqlExecution).getQuery() } } - /** A JSON decoding converts a string to a Dictionary. */ - class JsonDecoding extends Decoding, StringInterpretation { + /** 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() } @@ -86,8 +89,8 @@ module NoSqlInjection { override DataFlow::Node getOutput() { result = Decoding.super.getOutput() } } - /** A NoSQL decoding interprets a string. */ - class NoSqlDecoding extends Decoding, StringInterpretation { + /** A NoSQL decoding interprets a string as a dictionary. */ + class NoSqlDecoding extends Decoding, StringToDictConversion { NoSqlDecoding() { this.getFormat() = "NoSQL" } override DataFlow::Node getAnInput() { result = Decoding.super.getAnInput() } diff --git a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll index 2e8e827b55c4..790b1785f7ad 100644 --- a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll +++ b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll @@ -16,41 +16,41 @@ module NoSqlInjectionConfig implements DataFlow::StateConfigSig { predicate isSource(DataFlow::Node source, FlowState state) { source instanceof C::StringSource and - state instanceof C::StringInput + state instanceof C::String or - source instanceof C::InterpretedStringSource and - state instanceof C::InterpretedStringInput + source instanceof C::DictSource and + state instanceof C::Dict } predicate isSink(DataFlow::Node sink, FlowState state) { sink instanceof C::StringSink and ( - state instanceof C::StringInput + state instanceof C::String or - // since InterpretedStrings can include strings, + // since Dicts can include strings, // e.g. JSON objects can encode strings. - state instanceof C::InterpretedStringInput + state instanceof C::Dict ) or - sink instanceof C::InterpretedStringSink and - state instanceof C::InterpretedStringInput + sink instanceof C::DictSink and + state instanceof C::Dict } predicate isBarrier(DataFlow::Node node, FlowState state) { - // Block `StringInput` paths here, since they change state to `InterpretedStringInput` - exists(C::StringInterpretation c | node = c.getOutput()) and - state instanceof C::StringInput + // Block `String` paths here, since they change state to `Dict` + exists(C::StringToDictConversion c | node = c.getOutput()) and + state instanceof C::String } predicate isAdditionalFlowStep( DataFlow::Node nodeFrom, FlowState stateFrom, DataFlow::Node nodeTo, FlowState stateTo ) { - exists(C::StringInterpretation c | + exists(C::StringToDictConversion c | nodeFrom = c.getAnInput() and nodeTo = c.getOutput() ) and - stateFrom instanceof C::StringInput and - stateTo instanceof C::InterpretedStringInput + stateFrom instanceof C::String and + stateTo instanceof C::Dict } predicate isBarrier(DataFlow::Node node) { From 97696680e67e6de2ff83a754adf1236adf5571d4 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Fri, 29 Sep 2023 13:45:23 +0200 Subject: [PATCH 40/45] Python: require dict sinks be dangerous. --- .../security/dataflow/NoSQLInjectionCustomizations.qll | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll index a07d3caf8236..d708081a7f51 100644 --- a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll +++ b/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll @@ -77,7 +77,11 @@ module NoSqlInjection { /** A NoSQL query that is vulnerable to user controlled dictionaries. */ class NoSqlExecutionAsDictSink extends DictSink { - NoSqlExecutionAsDictSink() { this = any(NoSqlExecution noSqlExecution).getQuery() } + NoSqlExecutionAsDictSink() { + exists(NoSqlExecution noSqlExecution | this = noSqlExecution.getQuery() | + noSqlExecution.interpretsDict() + ) + } } /** A JSON decoding converts a string to a dictionary. */ From 16e1a00e882a4b85f06255a5726c218fb506f769 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Fri, 29 Sep 2023 13:16:30 +0200 Subject: [PATCH 41/45] Python: NoSQLInjection -> NoSqlInjection --- ...ctionCustomizations.qll => NoSqlInjectionCustomizations.qll} | 0 .../{NoSQLInjectionQuery.qll => NoSqlInjectionQuery.qll} | 2 +- .../CWE-943/{NoSQLInjection.qhelp => NoSqlInjection.qhelp} | 0 .../Security/CWE-943/{NoSQLInjection.ql => NoSqlInjection.ql} | 2 +- python/ql/src/meta/alerts/TaintSinks.ql | 2 +- .../Security/CWE-943-NoSqlInjection/DataflowQueryTest.ql | 2 +- .../Security/CWE-943-NoSqlInjection/NoSqlInjection.qlref | 2 +- 7 files changed, 5 insertions(+), 5 deletions(-) rename python/ql/lib/semmle/python/security/dataflow/{NoSQLInjectionCustomizations.qll => NoSqlInjectionCustomizations.qll} (100%) rename python/ql/lib/semmle/python/security/dataflow/{NoSQLInjectionQuery.qll => NoSqlInjectionQuery.qll} (96%) rename python/ql/src/Security/CWE-943/{NoSQLInjection.qhelp => NoSqlInjection.qhelp} (100%) rename python/ql/src/Security/CWE-943/{NoSQLInjection.ql => NoSqlInjection.ql} (91%) diff --git a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/NoSqlInjectionCustomizations.qll similarity index 100% rename from python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll rename to python/ql/lib/semmle/python/security/dataflow/NoSqlInjectionCustomizations.qll diff --git a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll b/python/ql/lib/semmle/python/security/dataflow/NoSqlInjectionQuery.qll similarity index 96% rename from python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll rename to python/ql/lib/semmle/python/security/dataflow/NoSqlInjectionQuery.qll index 790b1785f7ad..5b0daacb737b 100644 --- a/python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionQuery.qll +++ b/python/ql/lib/semmle/python/security/dataflow/NoSqlInjectionQuery.qll @@ -6,7 +6,7 @@ import python import semmle.python.dataflow.new.DataFlow import semmle.python.dataflow.new.TaintTracking import semmle.python.Concepts -private import NoSQLInjectionCustomizations::NoSqlInjection as C +private import NoSqlInjectionCustomizations::NoSqlInjection as C /** * A taint-tracking configuration for detecting NoSQL injection vulnerabilities. diff --git a/python/ql/src/Security/CWE-943/NoSQLInjection.qhelp b/python/ql/src/Security/CWE-943/NoSqlInjection.qhelp similarity index 100% rename from python/ql/src/Security/CWE-943/NoSQLInjection.qhelp rename to python/ql/src/Security/CWE-943/NoSqlInjection.qhelp diff --git a/python/ql/src/Security/CWE-943/NoSQLInjection.ql b/python/ql/src/Security/CWE-943/NoSqlInjection.ql similarity index 91% rename from python/ql/src/Security/CWE-943/NoSQLInjection.ql rename to python/ql/src/Security/CWE-943/NoSqlInjection.ql index 0b9ac2139e71..b559159055fc 100644 --- a/python/ql/src/Security/CWE-943/NoSQLInjection.ql +++ b/python/ql/src/Security/CWE-943/NoSqlInjection.ql @@ -11,7 +11,7 @@ */ import python -import semmle.python.security.dataflow.NoSQLInjectionQuery +import semmle.python.security.dataflow.NoSqlInjectionQuery import NoSqlInjectionFlow::PathGraph from NoSqlInjectionFlow::PathNode source, NoSqlInjectionFlow::PathNode sink diff --git a/python/ql/src/meta/alerts/TaintSinks.ql b/python/ql/src/meta/alerts/TaintSinks.ql index 193cdac61712..0930da284510 100644 --- a/python/ql/src/meta/alerts/TaintSinks.ql +++ b/python/ql/src/meta/alerts/TaintSinks.ql @@ -17,7 +17,7 @@ import semmle.python.security.dataflow.CodeInjectionCustomizations import semmle.python.security.dataflow.CommandInjectionCustomizations import semmle.python.security.dataflow.LdapInjectionCustomizations import semmle.python.security.dataflow.LogInjectionCustomizations -import semmle.python.security.dataflow.NoSQLInjectionCustomizations +import semmle.python.security.dataflow.NoSqlInjectionCustomizations import semmle.python.security.dataflow.PathInjectionCustomizations import semmle.python.security.dataflow.PolynomialReDoSCustomizations import semmle.python.security.dataflow.ReflectedXSSCustomizations diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/DataflowQueryTest.ql b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/DataflowQueryTest.ql index 79c28c399f38..b665aefd6fb3 100644 --- a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/DataflowQueryTest.ql +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/DataflowQueryTest.ql @@ -1,4 +1,4 @@ import python import experimental.dataflow.TestUtil.DataflowQueryTest -import semmle.python.security.dataflow.NoSQLInjectionQuery +import semmle.python.security.dataflow.NoSqlInjectionQuery import FromTaintTrackingStateConfig diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.qlref b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.qlref index a3f577c4cdca..4bc8c29a1044 100644 --- a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.qlref +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.qlref @@ -1 +1 @@ -Security/CWE-943/NoSQLInjection.ql +Security/CWE-943/NoSqlInjection.ql From d7ad5a0f2314d0b351c9656cd49f5e5762fc4ef1 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Fri, 29 Sep 2023 13:16:50 +0200 Subject: [PATCH 42/45] Python: List NoSQL injection sinks --- python/ql/src/meta/alerts/TaintSinks.ql | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/python/ql/src/meta/alerts/TaintSinks.ql b/python/ql/src/meta/alerts/TaintSinks.ql index 0930da284510..55d88f8c359e 100644 --- a/python/ql/src/meta/alerts/TaintSinks.ql +++ b/python/ql/src/meta/alerts/TaintSinks.ql @@ -58,6 +58,10 @@ DataFlow::Node relevantTaintSink(string kind) { or kind = "RegexInjection" and result instanceof RegexInjection::Sink or + kind = "NoSqlInjection (string sink)" and result instanceof NoSqlInjection::StringSink + or + kind = "NoSqlInjection (dict sink)" and result instanceof NoSqlInjection::DictSink + or kind = "ServerSideRequestForgery" and result instanceof ServerSideRequestForgery::Sink or kind = "SqlInjection" and result instanceof SqlInjection::Sink From 3676262313dd6520d1127bee806a82e77a63fa20 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Fri, 29 Sep 2023 13:35:14 +0200 Subject: [PATCH 43/45] Python: Clean trailing whitespace --- .../Security/CWE-943-NoSqlInjection/mongoengine_bad.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/mongoengine_bad.py b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/mongoengine_bad.py index 4367f9e1ff77..8dcedda62a20 100644 --- a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/mongoengine_bad.py +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/mongoengine_bad.py @@ -18,7 +18,7 @@ def connect_find(): unsafe_search = request.args['search'] json_search = json.loads(unsafe_search) - db = me.connect('mydb') + db = me.connect('mydb') return db.movie.find({'name': json_search}) #$ result=BAD @app.route("/connection_connect_find") @@ -57,7 +57,7 @@ def subscript_find(): unsafe_search = request.args['search'] json_search = json.loads(unsafe_search) - db = me.connect('mydb') + db = me.connect('mydb') return db['movie'].find({'name': json_search}) #$ result=BAD # if __name__ == "__main__": From d6d13f84a99c149180f24dfdd2d4a09039cd6dd0 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Fri, 29 Sep 2023 13:50:34 +0200 Subject: [PATCH 44/45] Python: -> NoSQL in QLDocs --- python/ql/lib/semmle/python/Concepts.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/ql/lib/semmle/python/Concepts.qll b/python/ql/lib/semmle/python/Concepts.qll index f3dcddf3bfde..81c7bd2990ee 100644 --- a/python/ql/lib/semmle/python/Concepts.qll +++ b/python/ql/lib/semmle/python/Concepts.qll @@ -387,7 +387,7 @@ module NoSqlExecution { * extend `NoSqlExecution` instead. */ abstract class Range extends DataFlow::Node { - /** Gets the argument that specifies the NoSql query to be executed. */ + /** Gets the argument that specifies the NoSQL query to be executed. */ abstract DataFlow::Node getQuery(); /** Holds if this query will unpack/interpret a dictionary */ @@ -405,7 +405,7 @@ module NoSqlExecution { * extend `NoSqlExecution::Range` instead. */ class NoSqlExecution extends DataFlow::Node instanceof NoSqlExecution::Range { - /** Gets the argument that specifies the NoSql query to be executed. */ + /** 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 */ From 9b73bbfc31cab1c8d605a151a6d3449bd9d98568 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Fri, 29 Sep 2023 13:51:55 +0200 Subject: [PATCH 45/45] Python: Add keyword argument support and a fair bit of refactoring --- .../lib/semmle/python/frameworks/PyMongo.qll | 35 +++---- .../NoSqlInjection.expected | 98 ++++++++++--------- .../CWE-943-NoSqlInjection/PoC/server.py | 2 + 3 files changed, 72 insertions(+), 63 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/PyMongo.qll b/python/ql/lib/semmle/python/frameworks/PyMongo.qll index 148cb074a104..de36f2ebcec9 100644 --- a/python/ql/lib/semmle/python/frameworks/PyMongo.qll +++ b/python/ql/lib/semmle/python/frameworks/PyMongo.qll @@ -109,22 +109,34 @@ private module PyMongo { * * `mongo.db.user.find({'name': safe_search})` would be a collection method call. */ - private class MongoCollectionCall extends DataFlow::CallCfgNode, NoSqlExecution::Range { + private class MongoCollectionCall extends API::CallNode, NoSqlExecution::Range { MongoCollectionCall() { this = mongoCollection().getMember(mongoCollectionMethodName()).getACall() } - override DataFlow::Node getQuery() { result = this.getArg(0) } + /** Gets the argument that specifies the NoSQL query to be executed, as an API::node */ + pragma[inline] + API::Node getQueryAsApiNode() { + // 'filter' is allowed keyword in pymongo, see https://pymongo.readthedocs.io/en/stable/api/pymongo/collection.html#pymongo.collection.Collection.find + result = this.getParameter(0, "filter") + } + + override DataFlow::Node getQuery() { result = this.getQueryAsApiNode().asSink() } override predicate interpretsDict() { any() } override predicate vulnerableToStrings() { none() } } + /** + * See https://pymongo.readthedocs.io/en/stable/api/pymongo/collection.html#pymongo.collection.Collection.aggregate + */ private class MongoCollectionAggregation extends API::CallNode, NoSqlExecution::Range { MongoCollectionAggregation() { this = mongoCollection().getMember("aggregate").getACall() } - override DataFlow::Node getQuery() { result = this.getParameter(0).getASubscript().asSink() } + override DataFlow::Node getQuery() { + result = this.getParameter(0, "pipeline").getASubscript().asSink() + } override predicate interpretsDict() { any() } @@ -157,8 +169,7 @@ private module PyMongo { WhereQueryOperator() { exists(API::Node dictionary | - dictionary = - mongoCollection().getMember(mongoCollectionMethodName()).getACall().getParameter(0) and + dictionary = any(MongoCollectionCall c).getQueryAsApiNode() and query = dictionary.getSubscript("$where").asSink() and this = dictionary.getAValueReachingSink() ) @@ -184,12 +195,7 @@ private module PyMongo { FunctionQueryOperator() { exists(API::Node dictionary | dictionary = - mongoCollection() - .getMember(mongoCollectionMethodName()) - .getACall() - .getParameter(0) - .getASubscript*() - .getSubscript("$function") and + any(MongoCollectionCall c).getQueryAsApiNode().getASubscript*().getSubscript("$function") and query = dictionary.getSubscript("body").asSink() and this = dictionary.getAValueReachingSink() ) @@ -285,12 +291,7 @@ private module PyMongo { private class EqualityOperator extends DataFlow::Node, NoSqlSanitizer::Range { EqualityOperator() { this = - mongoCollection() - .getMember(mongoCollectionMethodName()) - .getParameter(0) - .getASubscript*() - .getSubscript("$eq") - .asSink() + any(MongoCollectionCall c).getQueryAsApiNode().getASubscript*().getSubscript("$eq").asSink() } override DataFlow::Node getAnInput() { result = this } diff --git a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected index beaa85d53122..c1b5889d02bd 100644 --- a/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected +++ b/python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected @@ -1,32 +1,34 @@ 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:1:26:1:32 | GSSA Variable request | PoC/server.py:76:14:76:20 | ControlFlowNode for request | -| PoC/server.py:1:26:1:32 | GSSA Variable request | PoC/server.py:96:14:96:20 | ControlFlowNode for request | +| PoC/server.py:1:26:1:32 | GSSA Variable request | PoC/server.py:43:14:43:20 | ControlFlowNode for request | +| PoC/server.py:1:26:1:32 | GSSA Variable request | PoC/server.py:52:14:52:20 | ControlFlowNode for request | +| PoC/server.py:1:26:1:32 | GSSA Variable request | PoC/server.py:77:14:77:20 | ControlFlowNode for request | +| PoC/server.py:1:26:1:32 | GSSA Variable request | PoC/server.py:98:14:98: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:5:27:10 | SSA variable author | PoC/server.py:31:34:31:51 | 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: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:52:5:52:10 | SSA variable search | PoC/server.py:60:27:60:58 | ControlFlowNode for Dict | -| PoC/server.py:52:14:56:5 | ControlFlowNode for Dict | PoC/server.py:52:5:52:10 | SSA variable search | -| PoC/server.py:53:17:53:70 | ControlFlowNode for BinaryExpr | PoC/server.py:52:14:56:5 | ControlFlowNode for Dict | -| PoC/server.py:76:5:76:10 | SSA variable author | PoC/server.py:79:23:79:101 | ControlFlowNode for BinaryExpr | -| PoC/server.py:76:14:76:20 | ControlFlowNode for request | PoC/server.py:76:5:76:10 | SSA variable author | -| PoC/server.py:77:5:77:15 | SSA variable accumulator | PoC/server.py:83:5:83:9 | SSA variable group | -| PoC/server.py:77:19:82:5 | ControlFlowNode for Dict | PoC/server.py:77:5:77:15 | SSA variable accumulator | -| PoC/server.py:79:23:79:101 | ControlFlowNode for BinaryExpr | PoC/server.py:77:19:82:5 | ControlFlowNode for Dict | -| PoC/server.py:83:5:83:9 | SSA variable group | PoC/server.py:90:29:90:47 | ControlFlowNode for Dict | -| PoC/server.py:96:5:96:10 | SSA variable author | PoC/server.py:97:5:97:10 | SSA variable mapper | -| PoC/server.py:96:14:96:20 | ControlFlowNode for request | PoC/server.py:96:5:96:10 | SSA variable author | -| PoC/server.py:97:5:97:10 | SSA variable mapper | PoC/server.py:100:9:100:14 | ControlFlowNode for mapper | +| PoC/server.py:43:5:43:10 | SSA variable author | PoC/server.py:47:38:47:67 | ControlFlowNode for BinaryExpr | +| PoC/server.py:43:14:43:20 | ControlFlowNode for request | PoC/server.py:43:5:43:10 | SSA variable author | +| PoC/server.py:47:38:47:67 | ControlFlowNode for BinaryExpr | PoC/server.py:47:27:47:68 | ControlFlowNode for Dict | +| PoC/server.py:52:5:52:10 | SSA variable author | PoC/server.py:54:17:54:70 | ControlFlowNode for BinaryExpr | +| PoC/server.py:52:14:52:20 | ControlFlowNode for request | PoC/server.py:52:5:52:10 | SSA variable author | +| PoC/server.py:53:5:53:10 | SSA variable search | PoC/server.py:61:27:61:58 | ControlFlowNode for Dict | +| PoC/server.py:53:14:57:5 | ControlFlowNode for Dict | PoC/server.py:53:5:53:10 | SSA variable search | +| PoC/server.py:54:17:54:70 | ControlFlowNode for BinaryExpr | PoC/server.py:53:14:57:5 | ControlFlowNode for Dict | +| PoC/server.py:77:5:77:10 | SSA variable author | PoC/server.py:80:23:80:101 | ControlFlowNode for BinaryExpr | +| PoC/server.py:77:14:77:20 | ControlFlowNode for request | PoC/server.py:77:5:77:10 | SSA variable author | +| PoC/server.py:78:5:78:15 | SSA variable accumulator | PoC/server.py:84:5:84:9 | SSA variable group | +| PoC/server.py:78:19:83:5 | ControlFlowNode for Dict | PoC/server.py:78:5:78:15 | SSA variable accumulator | +| PoC/server.py:80:23:80:101 | ControlFlowNode for BinaryExpr | PoC/server.py:78:19:83:5 | ControlFlowNode for Dict | +| PoC/server.py:84:5:84:9 | SSA variable group | PoC/server.py:91:29:91:47 | ControlFlowNode for Dict | +| PoC/server.py:84:5:84:9 | SSA variable group | PoC/server.py:92:38:92:56 | ControlFlowNode for Dict | +| PoC/server.py:98:5:98:10 | SSA variable author | PoC/server.py:99:5:99:10 | SSA variable mapper | +| PoC/server.py:98:14:98:20 | ControlFlowNode for request | PoC/server.py:98:5:98:10 | SSA variable author | +| PoC/server.py:99:5:99:10 | SSA variable mapper | PoC/server.py:102:9:102:14 | ControlFlowNode for mapper | | 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 | @@ -123,27 +125,29 @@ nodes | PoC/server.py:27:14:27:38 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | | PoC/server.py:27:25:27:37 | ControlFlowNode for author_string | semmle.label | ControlFlowNode for author_string | | PoC/server.py:30:27:30:44 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | -| PoC/server.py:42:5:42:10 | SSA variable author | semmle.label | SSA variable author | -| 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:52:5:52:10 | SSA variable search | semmle.label | SSA variable search | -| PoC/server.py:52:14:56:5 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | -| 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:76:5:76:10 | SSA variable author | semmle.label | SSA variable author | -| PoC/server.py:76:14:76:20 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | -| PoC/server.py:77:5:77:15 | SSA variable accumulator | semmle.label | SSA variable accumulator | -| PoC/server.py:77:19:82:5 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | -| PoC/server.py:79:23:79:101 | ControlFlowNode for BinaryExpr | semmle.label | ControlFlowNode for BinaryExpr | -| PoC/server.py:83:5:83:9 | SSA variable group | semmle.label | SSA variable group | -| PoC/server.py:90:29:90:47 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | -| PoC/server.py:96:5:96:10 | SSA variable author | semmle.label | SSA variable author | -| PoC/server.py:96:14:96:20 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | -| PoC/server.py:97:5:97:10 | SSA variable mapper | semmle.label | SSA variable mapper | -| PoC/server.py:100:9:100:14 | ControlFlowNode for mapper | semmle.label | ControlFlowNode for mapper | +| PoC/server.py:31:34:31:51 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | +| PoC/server.py:43:5:43:10 | SSA variable author | semmle.label | SSA variable author | +| PoC/server.py:43:14:43:20 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | +| PoC/server.py:47:27:47:68 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | +| PoC/server.py:47:38:47:67 | ControlFlowNode for BinaryExpr | semmle.label | ControlFlowNode for BinaryExpr | +| PoC/server.py:52:5:52:10 | SSA variable author | semmle.label | SSA variable author | +| PoC/server.py:52:14:52:20 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | +| PoC/server.py:53:5:53:10 | SSA variable search | semmle.label | SSA variable search | +| PoC/server.py:53:14:57:5 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | +| PoC/server.py:54:17:54:70 | ControlFlowNode for BinaryExpr | semmle.label | ControlFlowNode for BinaryExpr | +| PoC/server.py:61:27:61:58 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | +| PoC/server.py:77:5:77:10 | SSA variable author | semmle.label | SSA variable author | +| PoC/server.py:77:14:77:20 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | +| PoC/server.py:78:5:78:15 | SSA variable accumulator | semmle.label | SSA variable accumulator | +| PoC/server.py:78:19:83:5 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | +| PoC/server.py:80:23:80:101 | ControlFlowNode for BinaryExpr | semmle.label | ControlFlowNode for BinaryExpr | +| PoC/server.py:84:5:84:9 | SSA variable group | semmle.label | SSA variable group | +| PoC/server.py:91:29:91:47 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | +| PoC/server.py:92:38:92:56 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict | +| PoC/server.py:98:5:98:10 | SSA variable author | semmle.label | SSA variable author | +| PoC/server.py:98:14:98:20 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | +| PoC/server.py:99:5:99:10 | SSA variable mapper | semmle.label | SSA variable mapper | +| PoC/server.py:102:9:102:14 | ControlFlowNode for mapper | semmle.label | ControlFlowNode for mapper | | 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 | @@ -238,10 +242,12 @@ nodes 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: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 | -| PoC/server.py:90:29:90:47 | ControlFlowNode for Dict | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | PoC/server.py:90:29:90:47 | 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:100:9:100:14 | ControlFlowNode for mapper | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | PoC/server.py:100:9:100:14 | ControlFlowNode for mapper | This NoSQL query contains an unsanitized $@. | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value | +| PoC/server.py:31:34:31:51 | ControlFlowNode for Dict | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | PoC/server.py:31:34:31:51 | 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:47:27:47:68 | ControlFlowNode for Dict | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | PoC/server.py:47:27:47: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:61:27:61:58 | ControlFlowNode for Dict | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | PoC/server.py:61:27:61:58 | 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:91:29:91:47 | ControlFlowNode for Dict | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | PoC/server.py:91:29:91:47 | 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:92:38:92:56 | ControlFlowNode for Dict | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | PoC/server.py:92:38:92:56 | 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:102:9:102:14 | ControlFlowNode for mapper | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | PoC/server.py:102:9:102:14 | ControlFlowNode for mapper | 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 | diff --git 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 index 893d972baadb..956d6c921ca7 100644 --- 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 @@ -28,6 +28,7 @@ def as_dict(): # Use {"$ne": 1} as author # Found by http://127.0.0.1:5000/dict?author={%22$ne%22:1} post = posts.find_one({'author': author}) # $ result=BAD + post = posts.find_one(filter={'author': author}) # $ result=BAD return show_post(post, author) @app.route('/dictHardened', methods=['GET']) @@ -88,6 +89,7 @@ def by_group(): # making the query `this.author === "" | "a" === "a"` # Found by http://127.0.0.1:5000/byGroup?author=%22%20|%20%22a%22%20===%20%22a post = posts.aggregate([{ "$group": group }]).next() # $ result=BAD + post = posts.aggregate(pipeline=[{ "$group": group }]).next() # $ result=BAD return show_post(post, author) # works with pymongo 3.9, `map_reduce` is removed in pymongo 4.0