From d7b82b2355f7f338340c16e0dbe8189d239987ad Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Wed, 20 May 2020 10:04:59 +0100 Subject: [PATCH 1/5] Rename a few modules and classes to reflect the fact that NoSQL queries are not usually strings. --- ql/src/semmle/go/frameworks/NoSQL.qll | 29 ++++++++++--------- ql/src/semmle/go/security/SqlInjection.qll | 4 +-- .../security/SqlInjectionCustomizations.qll | 4 +-- .../{QueryString.expected => Query.expected} | 0 .../semmle/go/frameworks/NoSQL/Query.ql | 3 ++ .../semmle/go/frameworks/NoSQL/QueryString.ql | 5 ---- 6 files changed, 22 insertions(+), 23 deletions(-) rename ql/test/library-tests/semmle/go/frameworks/NoSQL/{QueryString.expected => Query.expected} (100%) create mode 100644 ql/test/library-tests/semmle/go/frameworks/NoSQL/Query.ql delete mode 100644 ql/test/library-tests/semmle/go/frameworks/NoSQL/QueryString.ql diff --git a/ql/src/semmle/go/frameworks/NoSQL.qll b/ql/src/semmle/go/frameworks/NoSQL.qll index d80fe3f20b7..da198d02183 100644 --- a/ql/src/semmle/go/frameworks/NoSQL.qll +++ b/ql/src/semmle/go/frameworks/NoSQL.qll @@ -7,24 +7,24 @@ import go /** Provides classes for working with NoSQL-related APIs. */ module NoSQL { /** - * A data-flow node whose string value is interpreted as (part of) a NoSQL query. + * A data-flow node whose value is interpreted as (part of) a NoSQL query. * * Extend this class to refine existing API models. If you want to model new APIs, - * extend `NoSQL::QueryString::Range` instead. + * extend `NoSQL::Query::Range` instead. */ - class NoSQLQueryString extends DataFlow::Node { - NoSQLQueryString::Range self; + class Query extends DataFlow::Node { + Query::Range self; - NoSQLQueryString() { this = self } + Query() { this = self } } - /** Provides classes for working with SQL query strings. */ - module NoSQLQueryString { + /** Provides classes for working with NoSQL queries. */ + module Query { /** - * A data-flow node whose string value is interpreted as (part of) a NoSQL query. + * A data-flow node whose value is interpreted as (part of) a NoSQL query. * * Extend this class to model new APIs. If you want to refine existing API models, - * extend `NoSQL::QueryString` instead. + * extend `NoSQL::Query` instead. */ abstract class Range extends DataFlow::Node { } @@ -33,7 +33,8 @@ module NoSQL { * package interprets parameter `n` as a query. */ private predicate collectionMethods(string name, int n) { - // func (coll *Collection) CountDocuments(ctx context.Context, filter interface{}, opts ...*options.CountOptions) (int64, error) + // func (coll *Collection) CountDocuments(ctx context.Context, filter interface{}, + // opts ...*options.CountOptions) (int64, error) name = "CountDocuments" and n = 1 or // func (coll *Collection) DeleteMany(ctx context.Context, filter interface{}, opts ...*options.DeleteOptions) (*DeleteResult, error) @@ -77,11 +78,11 @@ module NoSQL { } /** - * A query string used in an API function acting on a `Collection` struct of - * `go.mongodb.org/mongo-driver/mongo` package + * A query used in an API function acting on a `Collection` struct of package + * [go.mongodb.org/mongo-driver/mongo](https://pkg.go.dev/go.mongodb.org/mongo-driver/mongo). */ - private class MongoDbCollectionQueryString extends Range { - MongoDbCollectionQueryString() { + private class MongoDbCollectionQuery extends Range { + MongoDbCollectionQuery() { exists(Method meth, string methodName, int n | collectionMethods(methodName, n) and meth.hasQualifiedName("go.mongodb.org/mongo-driver/mongo", "Collection", methodName) and diff --git a/ql/src/semmle/go/security/SqlInjection.qll b/ql/src/semmle/go/security/SqlInjection.qll index 0397ee8c665..82ea9a2a232 100644 --- a/ql/src/semmle/go/security/SqlInjection.qll +++ b/ql/src/semmle/go/security/SqlInjection.qll @@ -23,8 +23,8 @@ module SqlInjection { override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } - override predicate isAdditionalTaintStep(DataFlow::Node prev, DataFlow::Node succ) { - NoSQL::isAdditionalMongoTaintStep(prev, succ) + override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { + NoSQL::isAdditionalMongoTaintStep(pred, succ) } override predicate isSanitizer(DataFlow::Node node) { diff --git a/ql/src/semmle/go/security/SqlInjectionCustomizations.qll b/ql/src/semmle/go/security/SqlInjectionCustomizations.qll index d5b87271546..006c308ed4f 100644 --- a/ql/src/semmle/go/security/SqlInjectionCustomizations.qll +++ b/ql/src/semmle/go/security/SqlInjectionCustomizations.qll @@ -40,8 +40,8 @@ module SqlInjection { SqlQueryAsSink() { this instanceof SQL::QueryString } } - /** An NoSQL string, considered as a taint sink for SQL injection. */ + /** A NoSQL query, considered as a taint sink for SQL injection. */ class NoSqlQueryAsSink extends Sink { - NoSqlQueryAsSink() { this instanceof NoSQL::NoSQLQueryString } + NoSqlQueryAsSink() { this instanceof NoSQL::Query } } } diff --git a/ql/test/library-tests/semmle/go/frameworks/NoSQL/QueryString.expected b/ql/test/library-tests/semmle/go/frameworks/NoSQL/Query.expected similarity index 100% rename from ql/test/library-tests/semmle/go/frameworks/NoSQL/QueryString.expected rename to ql/test/library-tests/semmle/go/frameworks/NoSQL/Query.expected diff --git a/ql/test/library-tests/semmle/go/frameworks/NoSQL/Query.ql b/ql/test/library-tests/semmle/go/frameworks/NoSQL/Query.ql new file mode 100644 index 00000000000..c5a89c0b8ef --- /dev/null +++ b/ql/test/library-tests/semmle/go/frameworks/NoSQL/Query.ql @@ -0,0 +1,3 @@ +import go + +select any(NoSQL::Query q) diff --git a/ql/test/library-tests/semmle/go/frameworks/NoSQL/QueryString.ql b/ql/test/library-tests/semmle/go/frameworks/NoSQL/QueryString.ql deleted file mode 100644 index 2bf41639937..00000000000 --- a/ql/test/library-tests/semmle/go/frameworks/NoSQL/QueryString.ql +++ /dev/null @@ -1,5 +0,0 @@ -import go -import semmle.go.frameworks.NoSQL - -from NoSQL::NoSQLQueryString qs -select qs From 8cc76edee41b5b535f77268b35b306db89743c44 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Wed, 20 May 2020 10:05:26 +0100 Subject: [PATCH 2/5] Rephrase a comment and split up some very long lines. --- ql/src/semmle/go/frameworks/NoSQL.qll | 44 ++++++++++++++++++--------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/ql/src/semmle/go/frameworks/NoSQL.qll b/ql/src/semmle/go/frameworks/NoSQL.qll index da198d02183..ba3f4f9f11b 100644 --- a/ql/src/semmle/go/frameworks/NoSQL.qll +++ b/ql/src/semmle/go/frameworks/NoSQL.qll @@ -29,51 +29,65 @@ module NoSQL { abstract class Range extends DataFlow::Node { } /** - * Holds if method `name` of `Collection` struct of `go.mongodb.org/mongo-driver/mongo` - * package interprets parameter `n` as a query. + * Holds if method `name` of struct `Collection` from package + * [go.mongodb.org/mongo-driver/mongo](https://pkg.go.dev/go.mongodb.org/mongo-driver/mongo) + * interprets parameter `n` as a query. */ private predicate collectionMethods(string name, int n) { // func (coll *Collection) CountDocuments(ctx context.Context, filter interface{}, // opts ...*options.CountOptions) (int64, error) name = "CountDocuments" and n = 1 or - // func (coll *Collection) DeleteMany(ctx context.Context, filter interface{}, opts ...*options.DeleteOptions) (*DeleteResult, error) + // func (coll *Collection) DeleteMany(ctx context.Context, filter interface{}, + // opts ...*options.DeleteOptions) (*DeleteResult, error) name = "DeleteMany" and n = 1 or - // func (coll *Collection) DeleteOne(ctx context.Context, filter interface{}, opts ...*options.DeleteOptions) (*DeleteResult, error) + // func (coll *Collection) DeleteOne(ctx context.Context, filter interface{}, + // opts ...*options.DeleteOptions) (*DeleteResult, error) name = "DeleteOne" and n = 1 or - // func (coll *Collection) Distinct(ctx context.Context, fieldName string, filter interface{}, ...) ([]interface{}, error) + // func (coll *Collection) Distinct(ctx context.Context, fieldName string, filter interface{}, + // ...) ([]interface{}, error) name = "Distinct" and n = 2 or - // func (coll *Collection) Find(ctx context.Context, filter interface{}, opts ...*options.FindOptions) (*Cursor, error) + // func (coll *Collection) Find(ctx context.Context, filter interface{}, + // opts ...*options.FindOptions) (*Cursor, error) name = "Find" and n = 1 or - // func (coll *Collection) FindOne(ctx context.Context, filter interface{}, opts ...*options.FindOneOptions) *SingleResult + // func (coll *Collection) FindOne(ctx context.Context, filter interface{}, + // opts ...*options.FindOneOptions) *SingleResult name = "FindOne" and n = 1 or - // func (coll *Collection) FindOneAndDelete(ctx context.Context, filter interface{}, ...) *SingleResult + // func (coll *Collection) FindOneAndDelete(ctx context.Context, filter interface{}, ...) + // *SingleResult name = "FindOneAndDelete" and n = 1 or - // func (coll *Collection) FindOneAndReplace(ctx context.Context, filter interface{}, replacement interface{}, ...) *SingleResult + // func (coll *Collection) FindOneAndReplace(ctx context.Context, filter interface{}, + // replacement interface{}, ...) *SingleResult name = "FindOneAndReplace" and n = 1 or - // func (coll *Collection) FindOneAndUpdate(ctx context.Context, filter interface{}, update interface{}, ...) *SingleResult + // func (coll *Collection) FindOneAndUpdate(ctx context.Context, filter interface{}, + // update interface{}, ...) *SingleResult name = "FindOneAndUpdate" and n = 1 or - // func (coll *Collection) ReplaceOne(ctx context.Context, filter interface{}, replacement interface{}, ...) (*UpdateResult, error) + // func (coll *Collection) ReplaceOne(ctx context.Context, filter interface{}, + // replacement interface{}, ...) (*UpdateResult, error) name = "ReplaceOne" and n = 1 or - // func (coll *Collection) UpdateMany(ctx context.Context, filter interface{}, update interface{}, ...) (*UpdateResult, error) + // func (coll *Collection) UpdateMany(ctx context.Context, filter interface{}, + // update interface{}, ...) (*UpdateResult, error) name = "UpdateMany" and n = 1 or - // func (coll *Collection) UpdateOne(ctx context.Context, filter interface{}, update interface{}, ...) (*UpdateResult, error) + // func (coll *Collection) UpdateOne(ctx context.Context, filter interface{}, + // update interface{}, ...) (*UpdateResult, error) name = "UpdateOne" and n = 1 or - // func (coll *Collection) Watch(ctx context.Context, pipeline interface{}, ...) (*ChangeStream, error) + // func (coll *Collection) Watch(ctx context.Context, pipeline interface{}, ...) + // (*ChangeStream, error) name = "Watch" and n = 1 or - // func (coll *Collection) Aggregate(ctx context.Context, pipeline interface{}, opts ...*options.AggregateOptions) (*Cursor, error) + // func (coll *Collection) Aggregate(ctx context.Context, pipeline interface{}, + // opts ...*options.AggregateOptions) (*Cursor, error) name = "Aggregate" and n = 1 } From cc24a8879fd7496a8d5a62d5d1991f19aa811426 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Wed, 20 May 2020 10:05:43 +0100 Subject: [PATCH 3/5] Rewrite a taint step to make more idiomatic use of the data-flow library. --- ql/src/semmle/go/frameworks/NoSQL.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ql/src/semmle/go/frameworks/NoSQL.qll b/ql/src/semmle/go/frameworks/NoSQL.qll index ba3f4f9f11b..2a7a1ccaac5 100644 --- a/ql/src/semmle/go/frameworks/NoSQL.qll +++ b/ql/src/semmle/go/frameworks/NoSQL.qll @@ -112,7 +112,7 @@ module NoSQL { predicate isAdditionalMongoTaintStep(DataFlow::Node pred, DataFlow::Node succ) { // Taint an entry if the `Value` is tainted exists(Write w, DataFlow::Node base, Field f | w.writesField(base, f, pred) | - base = succ.getASuccessor*() and + base = succ.(DataFlow::PostUpdateNode).getPreUpdateNode() and base.getType().hasQualifiedName("go.mongodb.org/mongo-driver/bson/primitive", "E") and f.getName() = "Value" ) From 267416f61f55209af832003fd8f63fe8ece33501 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Wed, 20 May 2020 10:08:49 +0100 Subject: [PATCH 4/5] Rename a predicate to clarify that it is MongoDB specific. --- ql/src/semmle/go/frameworks/NoSQL.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ql/src/semmle/go/frameworks/NoSQL.qll b/ql/src/semmle/go/frameworks/NoSQL.qll index 2a7a1ccaac5..75ab2dc3765 100644 --- a/ql/src/semmle/go/frameworks/NoSQL.qll +++ b/ql/src/semmle/go/frameworks/NoSQL.qll @@ -33,7 +33,7 @@ module NoSQL { * [go.mongodb.org/mongo-driver/mongo](https://pkg.go.dev/go.mongodb.org/mongo-driver/mongo) * interprets parameter `n` as a query. */ - private predicate collectionMethods(string name, int n) { + private predicate mongoDbCollectionMethod(string name, int n) { // func (coll *Collection) CountDocuments(ctx context.Context, filter interface{}, // opts ...*options.CountOptions) (int64, error) name = "CountDocuments" and n = 1 @@ -98,7 +98,7 @@ module NoSQL { private class MongoDbCollectionQuery extends Range { MongoDbCollectionQuery() { exists(Method meth, string methodName, int n | - collectionMethods(methodName, n) and + mongoDbCollectionMethod(methodName, n) and meth.hasQualifiedName("go.mongodb.org/mongo-driver/mongo", "Collection", methodName) and this = meth.getACall().getArgument(n) ) From 9a4bee94489b4e4f22d4442aa39b6d30f36094bd Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Wed, 20 May 2020 10:10:28 +0100 Subject: [PATCH 5/5] Add change note. --- change-notes/2020-05-20-mongodb-model.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 change-notes/2020-05-20-mongodb-model.md diff --git a/change-notes/2020-05-20-mongodb-model.md b/change-notes/2020-05-20-mongodb-model.md new file mode 100644 index 00000000000..ff0a09d37de --- /dev/null +++ b/change-notes/2020-05-20-mongodb-model.md @@ -0,0 +1,3 @@ +lgtm,codescanning +* Modeling of the `go.mongodb.org/mongo-driver/mongo` package has been added, which may lead to more + results from the security queries.