From da88b22aacd6a3cc2bb8ef7a80deaefafefcf668 Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Tue, 20 Oct 2020 09:51:04 +0200 Subject: [PATCH] Remove additional SQL sinks --- benjamin-button.md | 10 + .../lib/semmle/javascript/frameworks/SQL.qll | 271 ------------------ 2 files changed, 10 insertions(+), 271 deletions(-) diff --git a/benjamin-button.md b/benjamin-button.md index 3d63edddca0..c009f7d1f43 100644 --- a/benjamin-button.md +++ b/benjamin-button.md @@ -33,6 +33,16 @@ Sinks added between 2020-01-01 and 2020-10-06 have been removed. Found by lookin - the commit titles of https://github.com/github/codeql/commits/main/javascript/ql/test/query-tests/Security/CWE-089 - the PR titles of https://github.com/github/codeql/pulls?page=2&q=is%3Apr+label%3AJS+is%3Aclosed+sink +Sinks added between 2018-08-02 and 2020-01-01 have been removed. Found by looking at: + +- the commit titles of https://github.com/github/codeql/commits/main/javascript/ql/test/query-tests/Security/CWE-089 +- the PR titles of https://github.com/github/codeql/pulls?page=2&q=is%3Apr+label%3AJS+is%3Aclosed+sink +- the PR titles of https://github.com/github/codeql/pulls?page=2&q=is%3Apr+label%3AJS+is%3Aclosed+sql + +TypeTracking in SQL.qll (added before the open-sourcing squash) + +The model of `mssql` and `sequelize` (added before the open-sourcing squash) + ## PseudoProperties Pseudo-properties (`$name$`) used in type-tracking and global dataflow configurations have been disabled. diff --git a/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll b/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll index 1da76a5ed10..a7c257cdf98 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll @@ -41,8 +41,6 @@ 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. */ @@ -59,8 +57,6 @@ 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. */ @@ -320,8 +316,6 @@ 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. */ @@ -344,268 +338,3 @@ private module Sqlite { QueryString() { this = any(QueryCall qc).getAQueryArgument().asExpr() } } } - -/** - * Provides classes modeling 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, DataFlow::SourceNode { - override TaggedTemplateExpr astNode; - - QueryTemplateExpr() { mssql().getAPropertyRead("query").flowsToExpr(astNode.getTag()) } - - override DataFlow::Node getAResult() { - PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp()) - } - - 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 getAResult() { - result = this.getCallback(1).getParameter(1) - or - PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp()) - } - - override DataFlow::Node getAQueryArgument() { result = this.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 modeling 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 - ) - ) - } - } -} - -/** - * 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"]) - } - } -}