From c769ef44cd10518cf24abbcd85d5750d2b818115 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 | 647 ++++++------------ .../lib/semmle/javascript/frameworks/SQL.qll | 371 +++++----- 2 files changed, 356 insertions(+), 662 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/NoSQL.qll b/javascript/ql/lib/semmle/javascript/frameworks/NoSQL.qll index 7136af06889..cb03e7a57d5 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/NoSQL.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/NoSQL.qll @@ -13,10 +13,10 @@ module NoSQL { } /** - * Gets a value that has been assigned to the "$where" property of an object that flows to `queryArg`. + * Gets the value of a `$where` property of an object that flows to `n`. */ -private DataFlow::Node getADollarWhereProperty(API::Node queryArg) { - result = queryArg.getMember("$where").getARhs() +private DataFlow::Node getADollarWherePropertyValue(DataFlow::Node n) { + result = n.getALocalSource().getAPropertyWrite("$where").getRhs() } /** @@ -24,61 +24,107 @@ private DataFlow::Node getADollarWhereProperty(API::Node queryArg) { */ private module MongoDB { /** - * Gets an access to `mongodb.MongoClient` or a database. - * - * In Mongo version 2.x, a client and a database handle were the same concept, but in 3.x - * they were separated. To handle everything with a single model, we treat them as the same here. + * Gets an import of MongoDB. */ - private API::Node getAMongoClientOrDatabase() { - result = API::moduleImport("mongodb").getMember("MongoClient") + DataFlow::ModuleImportNode mongodb() { result.getPath() = "mongodb" } + + /** + * Gets an access to `mongodb.MongoClient`. + */ + private DataFlow::SourceNode getAMongoClient(DataFlow::TypeTracker t) { + t.start() and + result = mongodb().getAPropertyRead("MongoClient") or - result = getAMongoClientOrDatabase().getMember("db").getReturn() - or - result = getAMongoClientOrDatabase().getMember("connect").getLastParameter().getParameter(1) + exists(DataFlow::TypeTracker t2 | result = getAMongoClient(t2).track(t2, t)) } - /** Gets a data flow node referring to a MongoDB collection. */ - private API::Node getACollection() { - // A collection resulting from calling `Db.collection(...)`. - exists(API::Node collection | - collection = getAMongoClientOrDatabase().getMember("collection").getReturn() - | - result = collection - or - result = collection.getParameter(1).getParameter(0) + /** + * Gets an access to `mongodb.MongoClient`. + */ + DataFlow::SourceNode getAMongoClient() { result = getAMongoClient(DataFlow::TypeTracker::end()) } + + /** 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").getLastArgument().getALocalSource() + or + exists(DataFlow::TypeBackTracker t2 | result = getAMongoDbCallback(t2).backtrack(t2, t)) + } + + /** Gets a data flow node that leads to a `connect` callback. */ + private DataFlow::FunctionNode getAMongoDbCallback() { + result = getAMongoDbCallback(DataFlow::TypeBackTracker::end()) + } + + /** + * Gets an expression that may refer to a MongoDB database connection. + */ + private DataFlow::SourceNode getAMongoDb(DataFlow::TypeTracker t) { + t.start() and + ( + exists(DataFlow::ParameterNode p | + p = result and + p = getAMongoDbCallback().getParameter(1) and + not p.getName().toLowerCase() = "client" // mongodb v3 provides a `Mongoclient` here + ) ) or - // note that this also covers `mongoose` models since they are subtypes of `mongodb.Collection` - result = API::Node::ofType("mongodb", "Collection") + exists(DataFlow::TypeTracker t2 | result = getAMongoDb(t2).track(t2, t)) } - /** A call to a MongoDB query method. */ - private class QueryCall extends DatabaseAccess, API::CallNode { - int queryArgIdx; + /** + * Gets an expression that may refer to a MongoDB database connection. + */ + DataFlow::SourceNode getAMongoDb() { result = getAMongoDb(DataFlow::TypeTracker::end()) } - QueryCall() { - exists(string method | - CollectionMethodSignatures::interpretsArgumentAsQuery(method, queryArgIdx) and - this = getACollection().getMember(method).getACall() - ) - } + /** + * A data flow node that may hold a MongoDB collection. + */ + abstract class Collection extends DataFlow::SourceNode { } - override DataFlow::Node getAQueryArgument() { result = this.getArgument(queryArgIdx) } - - DataFlow::Node getACodeOperator() { - result = getADollarWhereProperty(this.getParameter(queryArgIdx)) + /** + * A collection resulting from calling `Db.collection(...)`. + */ + private class CollectionFromDb extends Collection { + CollectionFromDb() { + this = getAMongoDb().getAMethodCall("collection") + or + this = getAMongoDb().getAMethodCall("collection").getCallback(1).getParameter(0) } } /** - * An expression that is interpreted as a MongoDB query. + * A collection based on the type `mongodb.Collection`. + * + * Note that this also covers `mongoose` models since they are subtypes + * of `mongodb.Collection`. */ - class Query extends NoSQL::Query { - QueryCall qc; + private class CollectionFromType extends Collection { + CollectionFromType() { hasUnderlyingType("mongodb", "Collection") } + } - Query() { this = qc.getAQueryArgument().asExpr() } + /** Gets a data flow node referring to a MongoDB collection. */ + private DataFlow::SourceNode getACollection(DataFlow::TypeTracker t) { + t.start() and + result instanceof Collection + or + exists(DataFlow::TypeTracker t2 | result = getACollection(t2).track(t2, t)) + } - override DataFlow::Node getACodeOperator() { result = qc.getACodeOperator() } + /** Gets a data flow node referring to a MongoDB collection. */ + DataFlow::SourceNode getACollection() { result = getACollection(DataFlow::TypeTracker::end()) } + + /** A call to a MongoDB query method. */ + private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode { + int queryArgIdx; + + QueryCall() { + exists(string m | this = getACollection().getAMethodCall(m) | + CollectionMethodSignatures::interpretsArgumentAsQuery(m, queryArgIdx) + ) + } + + override DataFlow::Node getAQueryArgument() { result = getArgument(queryArgIdx) } } /** @@ -138,6 +184,17 @@ private module MongoDB { ) } } + + /** + * 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()) + } + } } /** @@ -147,74 +204,82 @@ private module Mongoose { /** * Gets an import of Mongoose. */ - API::Node getAMongooseInstance() { result = API::moduleImport("mongoose") } + DataFlow::ModuleImportNode getAMongooseInstance() { result.getPath() = "mongoose" } /** - * Gets a reference to `mongoose.createConnection`. + * Gets a call to `mongoose.createConnection`. */ - API::Node createConnection() { result = getAMongooseInstance().getMember("createConnection") } + DataFlow::CallNode createConnection() { + result = getAMongooseInstance().getAMemberCall("createConnection") + } /** - * A Mongoose function. + * A Mongoose function invocation. */ - abstract private class MongooseFunction extends API::Node { + private class InvokeNode extends DataFlow::InvokeNode { /** - * Gets the API-graph node for the result from this function (if the function returns a `Query`). + * Holds if this invocation returns an object of type `Query`. */ - abstract API::Node getQueryReturn(); + abstract predicate returnsQuery(); /** - * Holds if this function returns a `Query` that evaluates to one or + * 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); /** - * Gets an argument that this function interprets as a query. + * Holds if this invocation interprets `arg` as a query. */ - abstract API::Node getQueryArgument(); + abstract predicate interpretsArgumentAsQuery(DataFlow::Node arg); } /** * Provides classes modeling the Mongoose Model class */ module Model { - private class ModelFunction extends MongooseFunction { - string methodName; + private class ModelInvokeNode extends InvokeNode, DataFlow::MethodCallNode { + ModelInvokeNode() { this = ref().getAMethodCall() } - ModelFunction() { this = getModelObject().getMember(methodName) } - - override API::Node getQueryReturn() { - MethodSignatures::returnsQuery(methodName) and result = this.getReturn() - } + override predicate returnsQuery() { MethodSignatures::returnsQuery(getMethodName()) } override predicate returnsDocumentQuery(boolean asArray) { - MethodSignatures::returnsDocumentQuery(methodName, asArray) + MethodSignatures::returnsDocumentQuery(getMethodName(), asArray) } - override API::Node getQueryArgument() { + override predicate interpretsArgumentAsQuery(DataFlow::Node arg) { exists(int n | - MethodSignatures::interpretsArgumentAsQuery(methodName, n) and - result = this.getParameter(n) + MethodSignatures::interpretsArgumentAsQuery(this.getMethodName(), n) and + arg = this.getArgument(n) ) } } /** - * Gets a API-graph node referring to a Mongoose Model object. + * Gets a data flow node referring to a Mongoose Model object. */ - private API::Node getModelObject() { - result = getAMongooseInstance().getMember("model").getReturn() + 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(API::Node conn | conn = createConnection().getReturn() | - result = conn.getMember("model").getReturn() or - result = conn.getMember("models").getAMember() - ) - or - result = API::Node::ofType("mongoose", "Model") + 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. */ @@ -226,30 +291,36 @@ private module Mongoose { // implement lots of the MongoDB collection interface MongoDB::CollectionMethodSignatures::interpretsArgumentAsQuery(name, n) or - name = "find" + ["ById", "One"] + "AndUpdate" and n = 1 + name = "findByIdAndUpdate" and n = 1 or - name in ["delete" + ["Many", "One"], "geoSearch", "remove", "replaceOne", "where"] and - n = 0 - or - name in [ - "find" + ["", "ById", "One"], - "find" + ["ById", "One"] + "And" + ["Delete", "Remove", "Update"], - "update" + ["", "Many", "One"] - ] and - n = 0 + name = "where" and n = 0 } /** * Holds if Model method `name` returns a Query. */ predicate returnsQuery(string name) { - name = - [ - "$where", "count", "findOne", "findOneAndDelete", "findOneAndRemove", - "findOneAndReplace", "findOneAndUpdate", "geosearch", "remove", "replaceOne", "update", - "updateMany", "countDocuments", "updateOne", "where", "deleteMany", "deleteOne", "find", - "findById", "findByIdAndDelete", "findByIdAndRemove", "findByIdAndUpdate" - ] + 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" } /** @@ -265,128 +336,23 @@ private module Mongoose { } } - /** - * Provides classes modeling the Mongoose Query class - */ - module Query { - private class QueryFunction extends MongooseFunction { - string methodName; - - QueryFunction() { this = getAMongooseQuery().getMember(methodName) } - - override API::Node getQueryReturn() { - MethodSignatures::returnsQuery(methodName) and result = this.getReturn() - } - - override predicate returnsDocumentQuery(boolean asArray) { - MethodSignatures::returnsDocumentQuery(methodName, asArray) - } - - override API::Node getQueryArgument() { - exists(int n | - MethodSignatures::interpretsArgumentAsQuery(methodName, n) and - result = this.getParameter(n) - ) - } - } - - private class NewQueryFunction extends MongooseFunction { - NewQueryFunction() { this = getAMongooseInstance().getMember("Query") } - - override API::Node getQueryReturn() { result = this.getInstance() } - - override predicate returnsDocumentQuery(boolean asArray) { none() } - - override API::Node getQueryArgument() { result = this.getParameter(2) } - } - - /** - * Gets a data flow node referring to a Mongoose query object. - */ - API::Node getAMongooseQuery() { - result = any(MongooseFunction f).getQueryReturn() - or - result = API::Node::ofType("mongoose", "Query") - or - result = - getAMongooseQuery() - .getMember(any(string name | MethodSignatures::returnsQuery(name))) - .getReturn() - } - - /** - * Provides signatures for the Query methods. - */ - module MethodSignatures { - /** - * Holds if Query method `name` interprets parameter `n` as a query. - */ - predicate interpretsArgumentAsQuery(string name, int n) { - n = 0 and - name = - [ - "and", "count", "findOneAndReplace", "findOneAndUpdate", "merge", "nor", "or", "remove", - "replaceOne", "setQuery", "setUpdate", "update", "countDocuments", "updateMany", - "updateOne", "where", "deleteMany", "deleteOne", "elemMatch", "find", "findOne", - "findOneAndDelete", "findOneAndRemove" - ] - or - n = 1 and - name = ["distinct", "findOneAndUpdate", "update", "updateMany", "updateOne"] - } - - /** - * Holds if Query method `name` returns a Query. - */ - predicate returnsQuery(string name) { - name = - [ - "$where", "J", "comment", "count", "countDocuments", "distinct", "elemMatch", "equals", - "error", "estimatedDocumentCount", "exists", "explain", "all", "find", "findById", - "findOne", "findOneAndRemove", "findOneAndUpdate", "geometry", "get", "gt", "gte", - "hint", "and", "in", "intersects", "lean", "limit", "lt", "lte", "map", "map", - "maxDistance", "maxTimeMS", "batchsize", "maxscan", "mod", "ne", "near", "nearSphere", - "nin", "or", "orFail", "polygon", "populate", "box", "read", "readConcern", "regexp", - "remove", "select", "session", "set", "setOptions", "setQuery", "setUpdate", "center", - "size", "skip", "slaveOk", "slice", "snapshot", "sort", "update", "w", "where", - "within", "centerSphere", "wtimeout", "circle", "collation" - ] - } - - /** - * Holds if Query 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 DocumentFunction extends MongooseFunction { - string methodName; + private class DocumentInvokeNode extends InvokeNode, DataFlow::MethodCallNode { + DocumentInvokeNode() { this = ref().getAMethodCall() } - DocumentFunction() { this = getAMongooseDocument().getMember(methodName) } - - override API::Node getQueryReturn() { - MethodSignatures::returnsQuery(methodName) and result = this.getReturn() - } + override predicate returnsQuery() { MethodSignatures::returnsQuery(getMethodName()) } override predicate returnsDocumentQuery(boolean asArray) { - MethodSignatures::returnsDocumentQuery(methodName, asArray) + MethodSignatures::returnsDocumentQuery(getMethodName(), asArray) } - override API::Node getQueryArgument() { + override predicate interpretsArgumentAsQuery(DataFlow::Node arg) { exists(int n | - MethodSignatures::interpretsArgumentAsQuery(methodName, n) and - result = this.getParameter(n) + MethodSignatures::interpretsArgumentAsQuery(this.getMethodName(), n) and + arg = this.getArgument(n) ) } } @@ -394,33 +360,23 @@ private module Mongoose { /** * A Mongoose Document that is retrieved from the backing database. */ - class RetrievedDocument extends API::Node { + class RetrievedDocument extends DataFlow::SourceNode { RetrievedDocument() { - exists(boolean asArray, API::Node param | - exists(MongooseFunction func | - func.returnsDocumentQuery(asArray) and - param = func.getLastParameter().getParameter(1) - ) - or - exists(API::Node f | - f = Query::getAMongooseQuery().getMember("then") and - param = f.getParameter(0).getParameter(0) - or - f = Query::getAMongooseQuery().getMember("exec") and - param = f.getParameter(0).getParameter(1) - | - exists(DataFlow::MethodCallNode pred | - // limitation: look at the previous method call - Query::MethodSignatures::returnsDocumentQuery(pred.getMethodName(), asArray) and - pred.getAMethodCall() = f.getACall() - ) + 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 - // limitation: look for direct accesses - this = param.getUnknownMember() + exists(DataFlow::PropRead access | + // limitation: look for direct accesses + access = param.getAPropertyRead() and + not exists(access.getPropertyName()) and + this = access + ) ) } } @@ -428,17 +384,26 @@ private module Mongoose { /** * Gets a data flow node referring to a Mongoose Document object. */ - private API::Node getAMongooseDocument() { - result instanceof RetrievedDocument + private DataFlow::SourceNode ref(DataFlow::TypeTracker t) { + ( + result instanceof RetrievedDocument or + result.hasUnderlyingType("mongoose", "Document") + ) and + t.start() or - result = API::Node::ofType("mongoose", "Document") - or - result = - getAMongooseDocument() - .getMember(any(string name | MethodSignatures::returnsDocument(name))) - .getReturn() + 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. @@ -495,9 +460,7 @@ private module Mongoose { string kind; Credentials() { - exists(string prop | - this = createConnection().getParameter(3).getMember(prop).getARhs().asExpr() - | + exists(string prop | this = createConnection().getOptionArgument(3, prop).asExpr() | prop = "user" and kind = "user name" or prop = "pass" and kind = "password" @@ -511,233 +474,29 @@ private module Mongoose { * An expression that is interpreted as a (part of a) MongoDB query. */ class MongoDBQueryPart extends NoSQL::Query { - MongooseFunction f; - - MongoDBQueryPart() { this = f.getQueryArgument().getARhs().asExpr() } + MongoDBQueryPart() { any(InvokeNode call).interpretsArgumentAsQuery(this.flow()) } override DataFlow::Node getACodeOperator() { - result = getADollarWhereProperty(f.getQueryArgument()) + result = getADollarWherePropertyValue(this.flow()) } } /** * An evaluation of a MongoDB query. */ - class ShorthandQueryEvaluation extends DatabaseAccess, DataFlow::InvokeNode { - MongooseFunction f; + class ShorthandQueryEvaluation extends DatabaseAccess { + InvokeNode invk; ShorthandQueryEvaluation() { - this = f.getACall() and + this = invk and // shorthand for execution: provide a callback - exists(f.getQueryReturn()) and - exists(this.getCallback(this.getNumArgument() - 1)) + 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 - f.getQueryArgument().getARhs() = result - } - } - - class ExplicitQueryEvaluation extends DatabaseAccess { - ExplicitQueryEvaluation() { - // explicit execution using a Query method call - Query::getAMongooseQuery().getMember(["exec", "then", "catch"]).getACall() = this - } - - override DataFlow::Node getAQueryArgument() { - // NB: the complete information is not easily accessible for deeply chained calls - none() - } - } -} - -/** - * Provides classes modeling the Minimongo library. - */ -private module Minimongo { - /** - * Provides signatures for the Collection methods. - */ - module CollectionMethodSignatures { - /** - * Holds if Collection method `name` interprets parameter `n` as a query. - */ - predicate interpretsArgumentAsQuery(string m, int queryArgIdx) { - // implements most of the MongoDB interface - MongoDB::CollectionMethodSignatures::interpretsArgumentAsQuery(m, queryArgIdx) - } - } - - /** A call to a Minimongo query method. */ - private class QueryCall extends DatabaseAccess, API::CallNode { - int queryArgIdx; - - QueryCall() { - exists(string m | - this = - API::moduleImport("minimongo") - .getAMember() - .getReturn() - .getAMember() - .getMember(m) - .getACall() and - CollectionMethodSignatures::interpretsArgumentAsQuery(m, queryArgIdx) - ) - } - - override DataFlow::Node getAQueryArgument() { result = this.getArgument(queryArgIdx) } - - DataFlow::Node getACodeOperator() { - result = getADollarWhereProperty(this.getParameter(queryArgIdx)) - } - } - - /** - * An expression that is interpreted as a Minimongo query. - */ - class Query extends NoSQL::Query { - QueryCall qc; - - Query() { this = qc.getAQueryArgument().asExpr() } - - override DataFlow::Node getACodeOperator() { result = qc.getACodeOperator() } - } -} - -/** - * Provides classes modeling the MarsDB library. - */ -private module MarsDB { - private class MarsDBAccess extends DatabaseAccess { - string method; - - MarsDBAccess() { - this = - API::moduleImport("marsdb") - .getMember("Collection") - .getInstance() - .getMember(method) - .getACall() - } - - string getMethod() { result = method } - - override DataFlow::Node getAQueryArgument() { none() } - } - - /** A call to a MarsDB query method. */ - private class QueryCall extends DatabaseAccess, API::CallNode { - int queryArgIdx; - - QueryCall() { - exists(string m | - this.(MarsDBAccess).getMethod() = m and - // implements parts of the Minimongo interface - Minimongo::CollectionMethodSignatures::interpretsArgumentAsQuery(m, queryArgIdx) - ) - } - - override DataFlow::Node getAQueryArgument() { result = this.getArgument(queryArgIdx) } - - DataFlow::Node getACodeOperator() { - result = getADollarWhereProperty(this.getParameter(queryArgIdx)) - } - } - - /** - * An expression that is interpreted as a MarsDB query. - */ - class Query extends NoSQL::Query { - QueryCall qc; - - Query() { this = qc.getAQueryArgument().asExpr() } - - override DataFlow::Node getACodeOperator() { result = qc.getACodeOperator() } - } -} - -/** - * Provides classes modeling the `Node Redis` library. - * - * Redis is an in-memory key-value store and not a database, - * but `Node Redis` can be exploited similarly to a NoSQL database by giving a method an array as argument instead of a string. - * As an example the below two invocations of `client.set` are equivalent: - * - * ``` - * const redis = require("redis"); - * const client = redis.createClient(); - * client.set("key", "value"); - * client.set(["key", "value"]); - * ``` - * - * ioredis is a very similar library. However, ioredis does not support array arguments in the same way, and is therefore not vulnerable to the same kind of type confusion. - */ -private module Redis { - /** - * Gets a `Node Redis` client. - */ - private API::Node client() { - result = API::moduleImport("redis").getMember("createClient").getReturn() - or - result = API::moduleImport("redis").getMember("RedisClient").getInstance() - or - result = client().getMember("duplicate").getReturn() - or - result = client().getMember("duplicate").getLastParameter().getParameter(1) - } - - /** - * Gets a (possibly chained) reference to a batch operation object. - * These have the same API as a redis client, except the calls are chained, and the sequence is terminated with a `.exec` call. - */ - private API::Node multi() { - result = client().getMember(["multi", "batch"]).getReturn() - or - result = multi().getAMember().getReturn() - } - - /** - * Gets a `Node Redis` client instance. Either a client created using `createClient()`, or a batch operation object. - */ - private API::Node redis() { result = [client(), multi()] } - - /** - * Provides signatures for the query methods from Node Redis. - */ - module QuerySignatures { - /** - * Holds if `method` interprets parameter `argIndex` as a key, and a later parameter determines a value/field. - * Thereby the method is vulnerable if parameter `argIndex` is unexpectedly an array instead of a string, as an attacker can control arguments to Redis that the attacker was not supposed to control. - * - * Only setters and similar methods are included. - * For getter-like methods it is not generally possible to gain access "outside" of where you are supposed to have access, - * it is at most possible to get a Redis call to return more results than expected (e.g. by adding more members to [`geohash`](https://redis.io/commands/geohash)). - */ - predicate argumentIsAmbiguousKey(string method, int argIndex) { - method = - [ - "set", "publish", "append", "bitfield", "decrby", "getset", "hincrby", "hincrbyfloat", - "hset", "hsetnx", "incrby", "incrbyfloat", "linsert", "lpush", "lpushx", "lset", "ltrim", - "rename", "renamenx", "rpushx", "setbit", "setex", "smove", "zincrby", "zinterstore", - "hdel", "lpush", "pfadd", "rpush", "sadd", "sdiffstore", "srem" - ] and - argIndex = 0 - or - method = ["bitop", "hmset", "mset", "msetnx", "geoadd"] and - argIndex in [0 .. any(DataFlow::InvokeNode invk).getNumArgument() - 1] - } - } - - /** - * An expression that is interpreted as a key in a Node Redis call. - */ - class RedisKeyArgument extends NoSQL::Query { - RedisKeyArgument() { - exists(string method, int argIndex | - QuerySignatures::argumentIsAmbiguousKey(method, argIndex) and - this = redis().getMember(method).getParameter(argIndex).getARhs().asExpr() - ) + invk.interpretsArgumentAsQuery(result) } } diff --git a/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll b/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll index 37f7637c3fd..e876067f845 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll @@ -28,54 +28,42 @@ module SQL { * Provides classes modeling the (API compatible) `mysql` and `mysql2` packages. */ private module MySql { - private string moduleName() { result = ["mysql", "mysql2", "mysql2/promise"] } + private DataFlow::SourceNode mysql() { result = DataFlow::moduleImport(["mysql", "mysql2"]) } - /** Gets the package name `mysql` or `mysql2`. */ - API::Node mysql() { result = API::moduleImport(moduleName()) } + private DataFlow::CallNode createPool() { result = mysql().getAMemberCall("createPool") } - /** Gets a reference to `mysql.createConnection`. */ - API::Node createConnection() { - result = mysql().getMember(["createConnection", "createConnectionPromise"]) + /** Gets a reference to a MySQL pool. */ + 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 `mysql.createPool`. */ - API::Node createPool() { result = mysql().getMember(["createPool", "createPoolCluster"]) } + /** Gets a reference to a MySQL pool. */ + private DataFlow::SourceNode pool() { result = pool(DataFlow::TypeTracker::end()) } - /** Gets a node that contains a MySQL pool created using `mysql.createPool()`. */ - API::Node pool() { - result = createPool().getReturn() - or - result = pool().getMember("on").getReturn() - or - result = API::Node::ofType(moduleName(), ["Pool", "PoolCluster"]) - } + /** Gets a call to `mysql.createConnection`. */ + DataFlow::CallNode createConnection() { result = mysql().getAMemberCall("createConnection") } - /** Gets a data flow node that contains a freshly created MySQL connection instance. */ - API::Node connection() { - result = createConnection().getReturn() - or - result = createConnection().getReturn().getPromised() - or - result = pool().getMember("getConnection").getParameter(0).getParameter(1) - or - result = pool().getMember("getConnection").getPromised() - or - exists(API::CallNode call | - call = pool().getMember("on").getACall() and - call.getArgument(0).getStringValue() = ["connection", "acquire", "release"] and - result = call.getParameter(1).getParameter(0) + /** Gets a reference to a MySQL connection instance. */ + private DataFlow::SourceNode connection(DataFlow::TypeTracker t) { + t.start() and + ( + result = createConnection() + or + result = pool().getAMethodCall("getConnection").getABoundCallbackParameter(0, 1) ) or - result = API::Node::ofType(moduleName(), ["Connection", "PoolConnection"]) + exists(DataFlow::TypeTracker t2 | result = connection(t2).track(t2, t)) } + /** Gets a reference to a MySQL connection instance. */ + DataFlow::SourceNode connection() { result = connection(DataFlow::TypeTracker::end()) } + /** A call to the MySql `query` method. */ private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode { - QueryCall() { - exists(API::Node recv | recv = pool() or recv = connection() | - this = recv.getMember(["query", "execute"]).getACall() - ) - } + QueryCall() { this = [pool(), connection()].getAMethodCall("query") } override DataFlow::Node getAQueryArgument() { result = this.getArgument(0) } } @@ -88,7 +76,7 @@ private module MySql { /** A call to the `escape` or `escapeId` method that performs SQL sanitization. */ class EscapingSanitizer extends SQL::SqlSanitizer, MethodCallExpr { EscapingSanitizer() { - this = [mysql(), pool(), connection()].getMember(["escape", "escapeId"]).getACall().asExpr() and + this = [mysql(), pool(), connection()].getAMethodCall(["escape", "escapeId"]).asExpr() and input = this.getArgument(0) and output = this } @@ -99,9 +87,8 @@ private module MySql { string kind; Credentials() { - exists(API::Node callee, string prop | - callee in [createConnection(), createPool()] and - this = callee.getParameter(0).getMember(prop).getARhs().asExpr() and + exists(string prop | + this = [createConnection(), createPool()].getOptionArgument(0, prop).asExpr() and ( prop = "user" and kind = "user name" or @@ -118,61 +105,23 @@ private module MySql { * Provides classes modeling the PostgreSQL packages, such as `pg` and `pg-promise`. */ private module Postgres { - API::Node pg() { - result = API::moduleImport("pg") - or - result = pgpMain().getMember("pg") - } - - /** Gets a reference to the `Client` constructor in the `pg` package, for example `require('pg').Client`. */ - API::Node newClient() { result = pg().getMember("Client") } - - /** Gets a freshly created Postgres client instance. */ - API::Node client() { - result = newClient().getInstance() - or - // pool.connect(function(err, client) { ... }) - result = pool().getMember("connect").getParameter(0).getParameter(1) - or - // await pool.connect() - result = pool().getMember("connect").getReturn().getPromised() - or - result = pgpConnection().getMember("client") - or - exists(API::CallNode call | - call = pool().getMember("on").getACall() and - call.getArgument(0).getStringValue() = ["connect", "acquire"] and - result = call.getParameter(1).getParameter(0) - ) - or - result = client().getMember("on").getReturn() - or - result = API::Node::ofType("pg", ["Client", "PoolClient"]) - } - - /** Gets a constructor that when invoked constructs a new connection pool. */ - API::Node newPool() { + /** Gets an expression that constructs a new connection pool. */ + DataFlow::InvokeNode newPool() { // new require('pg').Pool() - result = pg().getMember("Pool") + result = DataFlow::moduleImport("pg").getAConstructorInvocation("Pool") or // new require('pg-pool') - result = API::moduleImport("pg-pool") + result = DataFlow::moduleImport("pg-pool").getAnInstantiation() } - /** Gets an API node that refers to a connection pool. */ - API::Node pool() { - result = newPool().getInstance() - or - result = pgpDatabase().getMember("$pool") - or - result = pool().getMember("on").getReturn() - or - result = API::Node::ofType("pg", "Pool") + /** Gets a creation of a Postgres client. */ + DataFlow::InvokeNode newClient() { + result = DataFlow::moduleImport("pg").getAConstructorInvocation("Client") } /** A call to the Postgres `query` method. */ private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode { - QueryCall() { this = [client(), pool()].getMember("query").getACall() } + QueryCall() { this = [newClient(), newPool()].getAMethodCall("query") } override DataFlow::Node getAQueryArgument() { result = this.getArgument(0) } } @@ -191,11 +140,7 @@ private module Postgres { string kind; Credentials() { - exists(string prop | - this = [newClient(), newPool()].getParameter(0).getMember(prop).getARhs().asExpr() - or - this = pgPromise().getParameter(0).getMember(prop).getARhs().asExpr() - | + exists(string prop | this = [newClient(), newPool()].getOptionArgument(0, prop).asExpr() | prop = "user" and kind = "user name" or prop = "password" and kind = prop @@ -336,35 +281,32 @@ private module Postgres { */ private module Sqlite { /** Gets a reference to the `sqlite3` module. */ - API::Node sqlite() { - result = API::moduleImport("sqlite3") + DataFlow::SourceNode sqlite() { + result = DataFlow::moduleImport("sqlite3") or - result = sqlite().getMember("verbose").getReturn() + result = sqlite().getAMemberCall("verbose") } - /** Gets an expression that constructs or returns a Sqlite database instance. */ - API::Node database() { + /** Gets an expression that constructs a Sqlite database instance. */ + DataFlow::SourceNode newDb() { // new require('sqlite3').Database() - result = sqlite().getMember("Database").getInstance() - or - // chained call - result = getAChainingQueryCall() - or - result = API::Node::ofType("sqlite3", "Database") + result = sqlite().getAConstructorInvocation("Database") } - /** A call to a query method on a Sqlite database instance that returns the same instance. */ - private API::Node getAChainingQueryCall() { - result = database().getMember(["all", "each", "exec", "get", "run"]).getReturn() + /** Gets a data flow node referring to a Sqlite database instance. */ + 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. */ + DataFlow::SourceNode db() { result = db(DataFlow::TypeTracker::end()) } + /** A call to a Sqlite query method. */ private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode { - QueryCall() { - this = getAChainingQueryCall().getAnImmediateUse() - or - this = database().getMember("prepare").getACall() - } + QueryCall() { this = db().getAMethodCall(["all", "each", "exec", "get", "prepare", "run"]) } override DataFlow::Node getAQueryArgument() { result = this.getArgument(0) } } @@ -380,41 +322,30 @@ private module Sqlite { */ private module MsSql { /** Gets a reference to the `mssql` module. */ - API::Node mssql() { result = API::moduleImport("mssql") } + DataFlow::SourceNode mssql() { result = DataFlow::moduleImport("mssql") } - /** Gets a node referring to an instance of the given class. */ - API::Node mssqlClass(string name) { - result = mssql().getMember(name).getInstance() + /** 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 - result = API::Node::ofType("mssql", name) + exists(DataFlow::TypeTracker t2 | result = request(t2).track(t2, t)) } - /** Gets an API node referring to a Request object. */ - API::Node request() { - result = mssqlClass("Request") - or - result = request().getMember(["input", "replaceInput", "output", "replaceOutput"]).getReturn() - or - result = [transaction(), pool()].getMember("request").getReturn() - } - - /** Gets an API node referring to a Transaction object. */ - API::Node transaction() { - result = mssqlClass("Transaction") - or - result = pool().getMember("transaction").getReturn() - } - - /** Gets a API node referring to a ConnectionPool object. */ - API::Node pool() { result = mssqlClass("ConnectionPool") } + /** 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().getMember("query").getAUse() = DataFlow::valueNode(astNode.getTag()) - } + QueryTemplateExpr() { mssql().getAPropertyRead("query").flowsToExpr(astNode.getTag()) } override DataFlow::Node getAQueryArgument() { result = DataFlow::valueNode(astNode.getTemplate().getAnElement()) @@ -423,7 +354,7 @@ private module MsSql { /** A call to a MsSql query method. */ private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode { - QueryCall() { this = [mssql(), request()].getMember(["query", "batch"]).getACall() } + QueryCall() { this = request().getAMethodCall(["query", "batch"]) } override DataFlow::Node getAQueryArgument() { result = this.getArgument(0) } } @@ -451,13 +382,13 @@ private module MsSql { string kind; Credentials() { - exists(API::Node callee, string prop | + exists(DataFlow::InvokeNode call, string prop | ( - callee = mssql().getMember("connect") + call = mssql().getAMemberCall("connect") or - callee = mssql().getMember("ConnectionPool") + call = mssql().getAConstructorInvocation("ConnectionPool") ) and - this = callee.getParameter(0).getMember(prop).getARhs().asExpr() and + this = call.getOptionArgument(0, prop).asExpr() and ( prop = "user" and kind = "user name" or @@ -474,34 +405,31 @@ private module MsSql { * Provides classes modeling the `sequelize` package. */ private module Sequelize { - /** Gets an import of the `sequelize` module or one that re-exports it. */ - API::Node sequelize() { result = API::moduleImport(["sequelize", "sequelize-typescript"]) } - - /** Gets an expression that creates an instance of the `Sequelize` class. */ - API::Node instance() { - result = [sequelize(), sequelize().getMember("Sequelize")].getInstance() + /** 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 - result = API::Node::ofType(["sequelize", "sequelize-typescript"], ["Sequelize", "default"]) + 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::MethodCallNode { - QueryCall() { this = instance().getMember("query").getACall() } + private class QueryCall extends DatabaseAccess, DataFlow::ValueNode { + override MethodCallExpr astNode; + + QueryCall() { this = sequelize().getAMethodCall("query") } override DataFlow::Node getAQueryArgument() { - result = this.getArgument(0) - or - result = this.getOptionArgument(0, "query") + 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() - or - this = sequelize().getMember(["literal", "asIs"]).getParameter(0).getARhs().asExpr() - } + QueryString() { this = any(QueryCall qc).getAQueryArgument().asExpr() } } /** @@ -513,7 +441,7 @@ private module Sequelize { Credentials() { exists(NewExpr ne, string prop | - ne = sequelize().getAnInstantiation().asExpr() and + ne = sequelize().asExpr() and ( this = ne.getArgument(1) and prop = "username" or @@ -540,101 +468,108 @@ private module Spanner { /** * Gets a node that refers to the `Spanner` class */ - API::Node spanner() { + DataFlow::SourceNode spanner() { // older versions - result = API::moduleImport("@google-cloud/spanner") + result = DataFlow::moduleImport("@google-cloud/spanner") or // newer versions - result = API::moduleImport("@google-cloud/spanner").getMember("Spanner") + result = DataFlow::moduleMember("@google-cloud/spanner", "Spanner") } - /** - * Gets a node that refers to an instance of the `Database` class. - */ - API::Node database() { - result = - spanner().getReturn().getMember("instance").getReturn().getMember("database").getReturn() + /** 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 - result = API::Node::ofType("@google-cloud/spanner", "Database") + exists(DataFlow::TypeTracker t2 | result = spannerNew(t2).track(t2, t)) } - /** - * Gets a node that refers to an instance of the `v1.SpannerClient` class. - */ - API::Node v1SpannerClient() { - result = spanner().getMember("v1").getMember("SpannerClient").getInstance() + /** 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 - result = API::Node::ofType("@google-cloud/spanner", "v1.SpannerClient") + exists(DataFlow::TypeTracker t2 | result = instance(t2).track(t2, t)) } - /** - * Gets a node that refers to a transaction object. - */ - API::Node transaction() { - result = - database() - .getMember(["runTransaction", "runTransactionAsync"]) - .getParameter([0, 1]) - .getParameter(1) + /** 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 - result = API::Node::ofType("@google-cloud/spanner", "Transaction") + exists(DataFlow::TypeTracker t2 | result = database(t2).track(t2, t)) } - /** Gets an API node referring to a `BatchTransaction` object. */ - API::Node batchTransaction() { - result = database().getMember("batchTransaction").getReturn() + /** 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 - result = database().getMember("createBatchTransaction").getReturn().getPromised() - or - result = API::Node::ofType("@google-cloud/spanner", "BatchTransaction") + 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 { } + 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 SqlExecutionDirect extends SqlExecution { - SqlExecutionDirect() { - this = database().getMember(["run", "runPartitionedUpdate", "runStream"]).getACall() - or - this = transaction().getMember(["run", "runStream", "runUpdate"]).getACall() - or - this = batchTransaction().getMember("createQueryPartitions").getACall() - } - - override DataFlow::Node getAQueryArgument() { - result = this.getArgument(0) - or - result = this.getOptionArgument(0, "sql") + 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 SqlExecutionBatch extends SqlExecution, API::CallNode { - SqlExecutionBatch() { this = transaction().getMember("batchUpdate").getACall() } - - override DataFlow::Node getAQueryArgument() { - // just use the whole array as the query argument, as arrays becomes tainted if one of the elements - // are tainted - result = this.getArgument(0) - or - result = this.getParameter(0).getUnknownMember().getMember("sql").getARhs() - } + 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 SqlExecutionWithOption extends SqlExecution { - SqlExecutionWithOption() { - this = v1SpannerClient().getMember(["executeSql", "executeStreamingSql"]).getACall() + class ExecuteSqlCall extends SqlExecution { + ExecuteSqlCall() { + this = v1SpannerClient().getAMethodCall(["executeSql", "executeStreamingSql"]) } override DataFlow::Node getAQueryArgument() { result = this.getOptionArgument(0, "sql") }