From 2f6a3ac5b15df5dced72887462d4a43bdabf099d Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Tue, 6 Oct 2020 13:12:13 +0200 Subject: [PATCH] Remove 2020 sinks from SqlInjection.ql --- .../semmle/javascript/frameworks/NoSQL.qll | 315 +++++++++++++++++- .../lib/semmle/javascript/frameworks/SQL.qll | 279 +++++++++++++++- 2 files changed, 574 insertions(+), 20 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/NoSQL.qll b/javascript/ql/lib/semmle/javascript/frameworks/NoSQL.qll index 98ed667f71f..51dffc3e35b 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/NoSQL.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/NoSQL.qll @@ -7,7 +7,17 @@ import semmle.javascript.Promises module NoSQL { /** An expression that is interpreted as a NoSQL query. */ - abstract class Query extends Expr { } + abstract class Query extends Expr { + /** Gets an expression that is interpreted as a code operator in this query. */ + DataFlow::Node getACodeOperator() { none() } + } +} + +/** + * Gets the value of a `$where` property of an object that flows to `n`. + */ +private DataFlow::Node getADollarWherePropertyValue(DataFlow::Node n) { + result = n.getALocalSource().getAPropertyWrite("$where").getRhs() } /** @@ -24,7 +34,7 @@ private module MongoDB { */ private DataFlow::SourceNode getAMongoClient(DataFlow::TypeTracker t) { t.start() and - result = mongodb().getAPropertyRead("MongoClient") + result = mongodb().getAPropertyRead("MongoClient") or exists(DataFlow::TypeTracker t2 | result = getAMongoClient(t2).track(t2, t)) } @@ -37,7 +47,7 @@ private module MongoDB { /** Gets a data flow node that leads to a `connect` callback. */ private DataFlow::SourceNode getAMongoDbCallback(DataFlow::TypeBackTracker t) { t.start() and - result = getAMongoClient().getAMemberCall("connect").getArgument(1).getALocalSource() + result = getAMongoClient().getAMemberCall("connect").getLastArgument().getALocalSource() or exists(DataFlow::TypeBackTracker t2 | result = getAMongoDbCallback(t2).backtrack(t2, t)) } @@ -52,7 +62,13 @@ private module MongoDB { */ private DataFlow::SourceNode getAMongoDb(DataFlow::TypeTracker t) { t.start() and - result = getAMongoDbCallback().getParameter(1) + ( + exists(DataFlow::ParameterNode p | + p = result and + p = getAMongoDbCallback().getParameter(1) and + not p.getName().toLowerCase() = "client" // mongodb v3 provides a `Mongoclient` here + ) + ) or exists(DataFlow::TypeTracker t2 | result = getAMongoDb(t2).track(t2, t)) } @@ -105,22 +121,27 @@ private module MongoDB { QueryCall() { exists(string m | this = getACollection().getAMethodCall(m) | - m = "count" and queryArgIdx = 0 - or - m = "distinct" and queryArgIdx = 1 - or - m = "find" and queryArgIdx = 0 + CollectionMethodSignatures::interpretsArgumentAsQuery(m, queryArgIdx) ) } override DataFlow::Node getAQueryArgument() { result = getArgument(queryArgIdx) } } + /** Gets a data flow node that leads to a `connect` callback. */ + private DataFlow::FunctionNode getAMongoDbCallback() { + result = getAMongoDbCallback(DataFlow::TypeBackTracker::end()) + } + /** * An expression that is interpreted as a MongoDB query. */ class Query extends NoSQL::Query { Query() { this = any(QueryCall qc).getAQueryArgument().asExpr() } + + override DataFlow::Node getACodeOperator() { + result = getADollarWherePropertyValue(this.flow()) + } } } @@ -141,14 +162,250 @@ private module Mongoose { } /** - * A Mongoose collection object. + * A Mongoose function invocation. */ - class Model extends MongoDB::Collection { - Model() { this = getAMongooseInstance().getAMemberCall("model") } + private class InvokeNode extends DataFlow::InvokeNode { + /** + * Holds if this invocation returns an object of type `Query`. + */ + abstract predicate returnsQuery(); + + /** + * Holds if this invocation returns a `Query` that evaluates to one or + * more Documents (`asArray` is false if it evaluates to a single + * Document). + */ + abstract predicate returnsDocumentQuery(boolean asArray); + + /** + * Holds if this invocation interprets `arg` as a query. + */ + abstract predicate interpretsArgumentAsQuery(DataFlow::Node arg); } /** - * An expression passed to `mongoose.createConnection` to supply credentials. + * Gets an expression that may refer to a MongoDB database connection. + */ + module Model { + private class ModelInvokeNode extends InvokeNode, DataFlow::MethodCallNode { + ModelInvokeNode() { this = ref().getAMethodCall() } + + override predicate returnsQuery() { MethodSignatures::returnsQuery(getMethodName()) } + + override predicate returnsDocumentQuery(boolean asArray) { + MethodSignatures::returnsDocumentQuery(getMethodName(), asArray) + } + + override predicate interpretsArgumentAsQuery(DataFlow::Node arg) { + exists(int n | + MethodSignatures::interpretsArgumentAsQuery(this.getMethodName(), n) and + arg = this.getArgument(n) + ) + } + } + + /** + * Gets a data flow node referring to a Mongoose Model object. + */ + private DataFlow::SourceNode ref(DataFlow::TypeTracker t) { + ( + result = getAMongooseInstance().getAMemberCall("model") + or + exists(DataFlow::SourceNode conn | conn = createConnection() | + result = conn.getAMemberCall("model") or + result = conn.getAPropertyRead("models").getAPropertyRead() + ) + or + result.hasUnderlyingType("mongoose", "Model") + ) and + t.start() + or + exists(DataFlow::TypeTracker t2 | result = ref(t2).track(t2, t)) + } + + /** + * Gets a data flow node referring to a Mongoose model object. + */ + DataFlow::SourceNode ref() { result = ref(DataFlow::TypeTracker::end()) } + + /** + * Provides signatures for the Model methods. + */ + module MethodSignatures { + /** + * Holds if Model method `name` interprets parameter `n` as a query. + */ + predicate interpretsArgumentAsQuery(string name, int n) { + // implement lots of the MongoDB collection interface + MongoDB::CollectionMethodSignatures::interpretsArgumentAsQuery(name, n) + or + name = "findByIdAndUpdate" and n = 1 + or + name = "where" and n = 0 + } + + /** + * Holds if Model method `name` returns a Query. + */ + predicate returnsQuery(string name) { + name = "$where" or + name = "count" or + name = "countDocuments" or + name = "deleteMany" or + name = "deleteOne" or + name = "find" or + name = "findById" or + name = "findByIdAndDelete" or + name = "findByIdAndRemove" or + name = "findByIdAndUpdate" or + name = "findOne" or + name = "findOneAndDelete" or + name = "findOneAndRemove" or + name = "findOneAndReplace" or + name = "findOneAndUpdate" or + name = "geosearch" or + name = "replaceOne" or + name = "update" or + name = "updateMany" or + name = "updateOne" or + name = "where" + } + + /** + * Holds if Document method `name` returns a query that results in + * one or more documents, the documents are wrapped in an array + * if `asArray` is true. + */ + predicate returnsDocumentQuery(string name, boolean asArray) { + asArray = false and name = "findOne" + or + asArray = true and name = "find" + } + } + } + + /** + * Provides classes modeling the Mongoose Document class + */ + module Document { + private class DocumentInvokeNode extends InvokeNode, DataFlow::MethodCallNode { + DocumentInvokeNode() { this = ref().getAMethodCall() } + + override predicate returnsQuery() { MethodSignatures::returnsQuery(getMethodName()) } + + override predicate returnsDocumentQuery(boolean asArray) { + MethodSignatures::returnsDocumentQuery(getMethodName(), asArray) + } + + override predicate interpretsArgumentAsQuery(DataFlow::Node arg) { + exists(int n | + MethodSignatures::interpretsArgumentAsQuery(this.getMethodName(), n) and + arg = this.getArgument(n) + ) + } + } + + /** + * A Mongoose Document that is retrieved from the backing database. + */ + class RetrievedDocument extends DataFlow::SourceNode { + RetrievedDocument() { + exists(boolean asArray, DataFlow::ParameterNode param | + exists(InvokeNode call | + call.returnsDocumentQuery(asArray) and + param = call.getCallback(call.getNumArgument() - 1).getParameter(1) + ) + | + asArray = false and this = param + or + asArray = true and + exists(DataFlow::PropRead access | + // limitation: look for direct accesses + access = param.getAPropertyRead() and + not exists(access.getPropertyName()) and + this = access + ) + ) + } + } + + /** + * Gets a data flow node referring to a Mongoose Document object. + */ + private DataFlow::SourceNode ref(DataFlow::TypeTracker t) { + ( + result instanceof RetrievedDocument or + result.hasUnderlyingType("mongoose", "Document") + ) and + t.start() + or + exists(DataFlow::TypeTracker t2, DataFlow::SourceNode succ | succ = ref(t2) | + result = succ.track(t2, t) + or + result = succ.getAMethodCall(any(string name | MethodSignatures::returnsDocument(name))) and + t = t2.continue() + ) + } + + /** + * Gets a data flow node referring to a Mongoose Document object. + */ + DataFlow::SourceNode ref() { result = ref(DataFlow::TypeTracker::end()) } + + private module MethodSignatures { + /** + * Holds if Document method `name` returns a Query. + */ + predicate returnsQuery(string name) { + // Documents are subtypes of Models + Model::MethodSignatures::returnsQuery(name) or + name = "replaceOne" or + name = "update" or + name = "updateOne" + } + + /** + * Holds if Document method `name` interprets parameter `n` as a query. + */ + predicate interpretsArgumentAsQuery(string name, int n) { + // Documents are subtypes of Models + Model::MethodSignatures::interpretsArgumentAsQuery(name, n) + or + n = 0 and + ( + name = "replaceOne" or + name = "update" or + name = "updateOne" + ) + } + + /** + * Holds if Document method `name` returns a query that results in + * one or more documents, the documents are wrapped in an array + * if `asArray` is true. + */ + predicate returnsDocumentQuery(string name, boolean asArray) { + // Documents are subtypes of Models + Model::MethodSignatures::returnsDocumentQuery(name, asArray) + } + + /** + * Holds if Document method `name` returns a Document. + */ + predicate returnsDocument(string name) { + name = "depopulate" or + name = "init" or + name = "populate" or + name = "overwrite" + } + } + } + + /** + * A collection based on the type `mongodb.Collection`. + * + * Note that this also covers `mongoose` models since they are subtypes + * of `mongodb.Collection`. */ class Credentials extends CredentialsExpr { string kind; @@ -163,4 +420,36 @@ private module Mongoose { override string getCredentialsKind() { result = kind } } + + /** + * An expression that is interpreted as a (part of a) MongoDB query. + */ + class MongoDBQueryPart extends NoSQL::Query { + MongoDBQueryPart() { any(InvokeNode call).interpretsArgumentAsQuery(this.flow()) } + + override DataFlow::Node getACodeOperator() { + result = getADollarWherePropertyValue(this.flow()) + } + } + + /** + * An evaluation of a MongoDB query. + */ + class ShorthandQueryEvaluation extends DatabaseAccess { + InvokeNode invk; + + ShorthandQueryEvaluation() { + this = invk and + // shorthand for execution: provide a callback + invk.returnsQuery() and + exists(invk.getCallback(invk.getNumArgument() - 1)) + } + + override DataFlow::Node getAQueryArgument() { + // NB: the complete information is not easily accessible for deeply chained calls + invk.interpretsArgumentAsQuery(result) + } + + override string getCredentialsKind() { result = kind } + } } diff --git a/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll b/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll index 8e564889b88..7c7f5403524 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll @@ -41,6 +41,8 @@ private module MySql { private DataFlow::SourceNode pool(DataFlow::TypeTracker t) { t.start() and result = createPool() + or + exists(DataFlow::TypeTracker t2 | result = pool(t2).track(t2, t)) } /** Gets a reference to a MySQL pool. */ @@ -57,6 +59,8 @@ private module MySql { or result = pool().getAMethodCall("getConnection").getABoundCallbackParameter(0, 1) ) + or + exists(DataFlow::TypeTracker t2 | result = connection(t2).track(t2, t)) } /** Gets a reference to a MySQL connection instance. */ @@ -66,8 +70,6 @@ private module MySql { private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode { QueryCall() { this = [pool(), connection()].getAMethodCall("query") } - override DataFlow::Node getAResult() { result = this.getCallback(_).getParameter(1) } - override DataFlow::Node getAQueryArgument() { result = this.getArgument(0) } } @@ -314,6 +316,8 @@ private module Sqlite { private DataFlow::SourceNode db(DataFlow::TypeTracker t) { t.start() and result = newDb() + or + exists(DataFlow::TypeTracker t2 | result = db(t2).track(t2, t)) } /** Gets a data flow node referring to a Sqlite database instance. */ @@ -323,11 +327,6 @@ private module Sqlite { private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode { QueryCall() { this = db().getAMethodCall(["all", "each", "exec", "get", "prepare", "run"]) } - override DataFlow::Node getAResult() { - result = this.getCallback(1).getParameter(1) or - PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp()) - } - override DataFlow::Node getAQueryArgument() { result = this.getArgument(0) } } @@ -336,3 +335,269 @@ private module Sqlite { QueryString() { this = any(QueryCall qc).getAQueryArgument().asExpr() } } } + +/** + * Provides classes modelling the `mssql` package. + */ +private module MsSql { + /** Gets a reference to the `mssql` module. */ + DataFlow::SourceNode mssql() { result = DataFlow::moduleImport("mssql") } + + /** Gets a data flow node referring to a request object. */ + private DataFlow::SourceNode request(DataFlow::TypeTracker t) { + t.start() and + ( + // new require('mssql').Request() + result = mssql().getAConstructorInvocation("Request") + or + // request.input(...) + result = request().getAMethodCall("input") + ) + or + exists(DataFlow::TypeTracker t2 | result = request(t2).track(t2, t)) + } + + /** Gets a data flow node referring to a request object. */ + DataFlow::SourceNode request() { result = request(DataFlow::TypeTracker::end()) } + + /** A tagged template evaluated as a query. */ + private class QueryTemplateExpr extends DatabaseAccess, DataFlow::ValueNode { + override TaggedTemplateExpr astNode; + + QueryTemplateExpr() { mssql().getAPropertyRead("query").flowsToExpr(astNode.getTag()) } + + override DataFlow::Node getAQueryArgument() { + result = DataFlow::valueNode(astNode.getTemplate().getAnElement()) + } + } + + /** A call to a MsSql query method. */ + private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode { + QueryCall() { this = request().getAMethodCall(["query", "batch"]) } + + override DataFlow::Node getAQueryArgument() { result = getArgument(0) } + } + + /** An expression that is passed to a method that interprets it as SQL. */ + class QueryString extends SQL::SqlString { + QueryString() { + exists(DatabaseAccess dba | dba instanceof QueryTemplateExpr or dba instanceof QueryCall | + this = dba.getAQueryArgument().asExpr() + ) + } + } + + /** An element of a query template, which is automatically sanitized. */ + class QueryTemplateSanitizer extends SQL::SqlSanitizer { + QueryTemplateSanitizer() { + this = any(QueryTemplateExpr qte).getAQueryArgument().asExpr() and + input = this and + output = this + } + } + + /** An expression that is passed as user name or password when creating a client or a pool. */ + class Credentials extends CredentialsExpr { + string kind; + + Credentials() { + exists(DataFlow::InvokeNode call, string prop | + ( + call = mssql().getAMemberCall("connect") + or + call = mssql().getAConstructorInvocation("ConnectionPool") + ) and + this = call.getOptionArgument(0, prop).asExpr() and + ( + prop = "user" and kind = "user name" + or + prop = "password" and kind = prop + ) + ) + } + + override string getCredentialsKind() { result = kind } + } +} + +/** + * Provides classes modelling the `sequelize` package. + */ +private module Sequelize { + /** Gets a node referring to an instance of the `Sequelize` class. */ + private DataFlow::SourceNode sequelize(DataFlow::TypeTracker t) { + t.start() and + result = DataFlow::moduleImport("sequelize").getAnInstantiation() + or + exists(DataFlow::TypeTracker t2 | result = sequelize(t2).track(t2, t)) + } + + /** Gets a node referring to an instance of the `Sequelize` class. */ + DataFlow::SourceNode sequelize() { result = sequelize(DataFlow::TypeTracker::end()) } + + /** A call to `Sequelize.query`. */ + private class QueryCall extends DatabaseAccess, DataFlow::ValueNode { + override MethodCallExpr astNode; + + QueryCall() { this = sequelize().getAMethodCall("query") } + + override DataFlow::Node getAQueryArgument() { + result = DataFlow::valueNode(astNode.getArgument(0)) + } + } + + /** An expression that is passed to `Sequelize.query` method and hence interpreted as SQL. */ + class QueryString extends SQL::SqlString { + QueryString() { this = any(QueryCall qc).getAQueryArgument().asExpr() } + } + + /** + * An expression that is passed as user name or password when creating an instance of the + * `Sequelize` class. + */ + class Credentials extends CredentialsExpr { + string kind; + + Credentials() { + exists(NewExpr ne, string prop | + ne = sequelize().asExpr() and + ( + this = ne.getArgument(1) and prop = "username" + or + this = ne.getArgument(2) and prop = "password" + or + ne.hasOptionArgument(ne.getNumArgument() - 1, prop, this) + ) and + ( + prop = "username" and kind = "user name" + or + prop = "password" and kind = prop + ) + ) + } + + override string getCredentialsKind() { result = kind } + } +} + +/** + * Provides classes modelling the Google Cloud Spanner library. + */ +private module Spanner { + /** + * Gets a node that refers to the `Spanner` class + */ + DataFlow::SourceNode spanner() { + // older versions + result = DataFlow::moduleImport("@google-cloud/spanner") + or + // newer versions + result = DataFlow::moduleMember("@google-cloud/spanner", "Spanner") + } + + /** Gets a data flow node referring to the result of `Spanner()` or `new Spanner()`. */ + private DataFlow::SourceNode spannerNew(DataFlow::TypeTracker t) { + t.start() and + result = spanner().getAnInvocation() + or + exists(DataFlow::TypeTracker t2 | result = spannerNew(t2).track(t2, t)) + } + + /** Gets a data flow node referring to the result of `Spanner()` or `new Spanner()`. */ + DataFlow::SourceNode spannerNew() { result = spannerNew(DataFlow::TypeTracker::end()) } + + /** Gets a data flow node referring to the result of `.instance()`. */ + private DataFlow::SourceNode instance(DataFlow::TypeTracker t) { + t.start() and + result = spannerNew().getAMethodCall("instance") + or + exists(DataFlow::TypeTracker t2 | result = instance(t2).track(t2, t)) + } + + /** Gets a data flow node referring to the result of `.instance()`. */ + DataFlow::SourceNode instance() { result = instance(DataFlow::TypeTracker::end()) } + + /** Gets a node that refers to an instance of the `Database` class. */ + private DataFlow::SourceNode database(DataFlow::TypeTracker t) { + t.start() and + result = instance().getAMethodCall("database") + or + exists(DataFlow::TypeTracker t2 | result = database(t2).track(t2, t)) + } + + /** Gets a node that refers to an instance of the `Database` class. */ + DataFlow::SourceNode database() { result = database(DataFlow::TypeTracker::end()) } + + /** Gets a node that refers to an instance of the `v1.SpannerClient` class. */ + private DataFlow::SourceNode v1SpannerClient(DataFlow::TypeTracker t) { + t.start() and + result = spanner().getAPropertyRead("v1").getAPropertyRead("SpannerClient").getAnInstantiation() + or + exists(DataFlow::TypeTracker t2 | result = v1SpannerClient(t2).track(t2, t)) + } + + /** Gets a node that refers to an instance of the `v1.SpannerClient` class. */ + DataFlow::SourceNode v1SpannerClient() { result = v1SpannerClient(DataFlow::TypeTracker::end()) } + + /** Gets a node that refers to a transaction object. */ + private DataFlow::SourceNode transaction(DataFlow::TypeTracker t) { + t.start() and + result = database().getAMethodCall("runTransaction").getABoundCallbackParameter(0, 1) + or + exists(DataFlow::TypeTracker t2 | result = transaction(t2).track(t2, t)) + } + + /** Gets a node that refers to a transaction object. */ + DataFlow::SourceNode transaction() { result = transaction(DataFlow::TypeTracker::end()) } + + /** + * A call to a Spanner method that executes a SQL query. + */ + abstract class SqlExecution extends DatabaseAccess, DataFlow::InvokeNode { + /** + * Gets the position of the query argument; default is zero, which can be overridden + * by subclasses. + */ + int getQueryArgumentPosition() { result = 0 } + + override DataFlow::Node getAQueryArgument() { + result = getArgument(getQueryArgumentPosition()) or + result = getOptionArgument(getQueryArgumentPosition(), "sql") + } + } + + /** + * A SQL execution that takes the input directly in the first argument or in the `sql` option. + */ + class DatabaseRunCall extends SqlExecution { + DatabaseRunCall() { + this = database().getAMethodCall(["run", "runPartitionedUpdate", "runStream"]) + } + } + + /** + * A SQL execution that takes an array of SQL strings or { sql: string } objects. + */ + class TransactionRunCall extends SqlExecution { + TransactionRunCall() { this = transaction().getAMethodCall(["run", "runStream", "runUpdate"]) } + } + + /** + * A SQL execution that only takes the input in the `sql` option, and do not accept query strings + * directly. + */ + class ExecuteSqlCall extends SqlExecution { + ExecuteSqlCall() { + this = v1SpannerClient().getAMethodCall(["executeSql", "executeStreamingSql"]) + } + + override DataFlow::Node getAQueryArgument() { result = getOptionArgument(0, "sql") } + } + + /** + * An expression that is interpreted as a SQL string. + */ + class QueryString extends SQL::SqlString { + QueryString() { this = any(SqlExecution se).getAQueryArgument().asExpr() } + } +}