From 132e0bf4b7b965ca1e59009ecf99ab20831c9dc4 Mon Sep 17 00:00:00 2001 From: Stephan Brandauer Date: Wed, 8 Dec 2021 14:30:33 +0100 Subject: [PATCH 1/4] add database accesses as additional (heuristic) remote flow sources --- .../ql/lib/semmle/javascript/Concepts.qll | 15 ++ .../lib/semmle/javascript/frameworks/Knex.qll | 34 ++- .../semmle/javascript/frameworks/NoSQL.qll | 56 ++++- .../lib/semmle/javascript/frameworks/SQL.qll | 200 +++++++++++++++ .../heuristics/AdditionalSources.qll | 15 +- .../Security/heuristics/HeuristicSink.ql | 2 +- .../heuristics/HeuristicSource.expected | 2 - .../Security/heuristics/HeuristicSource.ql | 11 +- .../Security/heuristics/sources.js | 234 +++++++++++++++++- 9 files changed, 547 insertions(+), 22 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/Concepts.qll b/javascript/ql/lib/semmle/javascript/Concepts.qll index 798747af8b8..a7c49ed0cf3 100644 --- a/javascript/ql/lib/semmle/javascript/Concepts.qll +++ b/javascript/ql/lib/semmle/javascript/Concepts.qll @@ -84,6 +84,21 @@ abstract class FileNameSource extends DataFlow::Node { } abstract class DatabaseAccess extends DataFlow::Node { /** Gets an argument to this database access that is interpreted as a query. */ abstract DataFlow::Node getAQueryArgument(); + + /** Gets a node to which a result of the access may flow. */ + DataFlow::Node getAResult() { + none() // Overridden in subclass + } + + /** + * Holds if the data returned can be a user-controlled object, + * such as a JSON object parsed from user-controlled data. + */ + predicate returnsUserControlledObject() { + // NB: Most data bases support JSON data (some via plugins), + // which is why this has a default implementation. + any() + } } /** diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Knex.qll b/javascript/ql/lib/semmle/javascript/frameworks/Knex.qll index 41d54afde82..6f2098f27c9 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/Knex.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/Knex.qll @@ -46,17 +46,35 @@ module Knex { RawKnexSqlString() { this = any(RawKnexCall call).getArgument(0).asExpr() } } - /** A call that triggers a SQL query submission. */ - private class KnexDatabaseAccess extends DatabaseAccess { - KnexDatabaseAccess() { - this = knexObject().getMember(["then", "stream", "asCallback"]).getACall() + /** A call that triggers a SQL query submission by calling then/stream/asCallback. */ + private class KnexDatabaseCallback extends DatabaseAccess, DataFlow::CallNode { + string member; + + KnexDatabaseCallback() { + member = ["then", "stream", "asCallback"] and + this = knexObject().getMember(member).getACall() + } + + override DataFlow::Node getAResult() { + member = "then" and + result = this.getCallback(0).getParameter(0) or - exists(AwaitExpr await | - this = await.flow() and - await.getOperand() = knexObject().getAUse().asExpr() - ) + member = "asCallback" and + result = this.getCallback(0).getParameter(1) } override DataFlow::Node getAQueryArgument() { none() } } + + private class KnexDatabaseAwait extends DatabaseAccess, DataFlow::ValueNode { + KnexDatabaseAwait() { + exists(AwaitExpr enclosingAwait | this = enclosingAwait.flow() | + enclosingAwait.getOperand() = knexObject().getAUse().asExpr() + ) + } + + override DataFlow::Node getAResult() { result = this } + + override DataFlow::Node getAQueryArgument() { none() } + } } diff --git a/javascript/ql/lib/semmle/javascript/frameworks/NoSQL.qll b/javascript/ql/lib/semmle/javascript/frameworks/NoSQL.qll index 7136af06889..8924bcdd46d 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/NoSQL.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/NoSQL.qll @@ -3,6 +3,7 @@ */ import javascript +import semmle.javascript.Promises module NoSQL { /** An expression that is interpreted as a NoSQL query. */ @@ -65,6 +66,10 @@ private module MongoDB { override DataFlow::Node getAQueryArgument() { result = this.getArgument(queryArgIdx) } + override DataFlow::Node getAResult() { + PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp()) + } + DataFlow::Node getACodeOperator() { result = getADollarWhereProperty(this.getParameter(queryArgIdx)) } @@ -537,12 +542,29 @@ private module Mongoose { // NB: the complete information is not easily accessible for deeply chained calls f.getQueryArgument().getARhs() = result } + + override DataFlow::Node getAResult() { + result = this.getCallback(this.getNumArgument() - 1).getParameter(1) + } } - class ExplicitQueryEvaluation extends DatabaseAccess { + class ExplicitQueryEvaluation extends DatabaseAccess, DataFlow::CallNode { + string member; + ExplicitQueryEvaluation() { // explicit execution using a Query method call - Query::getAMongooseQuery().getMember(["exec", "then", "catch"]).getACall() = this + member = ["exec", "then", "catch"] and + Query::getAMongooseQuery().getMember(member).getACall() = this + } + + private int resultParamIndex() { + member = "then" and result = 0 + or + member = "exec" and result = 1 + } + + override DataFlow::Node getAResult() { + result = this.getCallback(_).getParameter(this.resultParamIndex()) } override DataFlow::Node getAQueryArgument() { @@ -588,6 +610,10 @@ private module Minimongo { override DataFlow::Node getAQueryArgument() { result = this.getArgument(queryArgIdx) } + override DataFlow::Node getAResult() { + PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp()) + } + DataFlow::Node getACodeOperator() { result = getADollarWhereProperty(this.getParameter(queryArgIdx)) } @@ -609,7 +635,7 @@ private module Minimongo { * Provides classes modeling the MarsDB library. */ private module MarsDB { - private class MarsDBAccess extends DatabaseAccess { + private class MarsDBAccess extends DatabaseAccess, DataFlow::CallNode { string method; MarsDBAccess() { @@ -623,21 +649,29 @@ private module MarsDB { string getMethod() { result = method } + override DataFlow::Node getAResult() { + PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp()) + } + override DataFlow::Node getAQueryArgument() { none() } } /** A call to a MarsDB query method. */ - private class QueryCall extends DatabaseAccess, API::CallNode { + private class QueryCall extends MarsDBAccess, API::CallNode { int queryArgIdx; QueryCall() { exists(string m | - this.(MarsDBAccess).getMethod() = m and + this.getMethod() = m and // implements parts of the Minimongo interface Minimongo::CollectionMethodSignatures::interpretsArgumentAsQuery(m, queryArgIdx) ) } + override DataFlow::Node getAResult() { + PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp()) + } + override DataFlow::Node getAQueryArgument() { result = this.getArgument(queryArgIdx) } DataFlow::Node getACodeOperator() { @@ -744,9 +778,13 @@ private module Redis { /** * An access to a database through redis */ - class RedisDatabaseAccess extends DatabaseAccess { + class RedisDatabaseAccess extends DatabaseAccess, DataFlow::CallNode { RedisDatabaseAccess() { this = redis().getMember(_).getACall() } + override DataFlow::Node getAResult() { + PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp()) + } + override DataFlow::Node getAQueryArgument() { none() } } } @@ -768,9 +806,13 @@ private module IoRedis { /** * An access to a database through ioredis */ - class IoRedisDatabaseAccess extends DatabaseAccess { + class IoRedisDatabaseAccess extends DatabaseAccess, DataFlow::CallNode { IoRedisDatabaseAccess() { this = ioredis().getMember(_).getACall() } + override DataFlow::Node getAResult() { + PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp()) + } + override DataFlow::Node getAQueryArgument() { none() } } } diff --git a/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll b/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll index 51eb1723f3f..c8e78b7568e 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll @@ -3,6 +3,7 @@ */ import javascript +import semmle.javascript.Promises module SQL { /** A string-valued expression that is interpreted as a SQL command. */ @@ -81,6 +82,8 @@ private module MySql { ) } + override DataFlow::Node getAResult() { result = this.getCallback(_).getParameter(1) } + override DataFlow::Node getAQueryArgument() { result = this.getArgument(0) } } @@ -178,6 +181,16 @@ private module Postgres { private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode { QueryCall() { this = [client(), pool()].getMember("query").getACall() } + override DataFlow::Node getAResult() { + this.getNumArgument() = 2 and + result = this.getCallback(1).getParameter(1) + or + this.getNumArgument() = 1 and + result = this.getAMethodCall("then").getCallback(0).getParameter(0) + or + PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp()) + } + override DataFlow::Node getAQueryArgument() { result = this.getArgument(0) } } @@ -322,6 +335,10 @@ private module Postgres { ) } + override DataFlow::Node getAResult() { + PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp()) + } + override DataFlow::Node getAQueryArgument() { result = this.getADirectQueryArgument() or @@ -370,6 +387,11 @@ private module Sqlite { this = database().getMember("prepare").getACall() } + 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) } } @@ -420,6 +442,11 @@ private module MsSql { mssql().getMember("query").getAUse() = DataFlow::valueNode(astNode.getTag()) } + override DataFlow::Node getAResult() { + PromiseFlow::loadStep(this.(DataFlow::SourceNode).getALocalUse(), result, + Promises::valueProp()) + } + override DataFlow::Node getAQueryArgument() { result = DataFlow::valueNode(astNode.getTemplate().getAnElement()) } @@ -429,6 +456,12 @@ private module MsSql { private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode { QueryCall() { this = [mssql(), request()].getMember(["query", "batch"]).getACall() } + 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) } } @@ -491,6 +524,31 @@ 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() + or + result = API::Node::ofType(["sequelize", "sequelize-typescript"], ["Sequelize", "default"]) + } + + /** A call to `Sequelize.query`. */ + private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode { + QueryCall() { this = instance().getMember("query").getACall() } + + override DataFlow::Node getAResult() { + PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp()) + } + + override DataFlow::Node getAQueryArgument() { + result = this.getArgument(0) + or + result = this.getOptionArgument(0, "query") + } + } + class SequelizeSink extends ModelInput::SinkModelCsv { override predicate row(string row) { row = @@ -540,3 +598,145 @@ private module SpannerCsv { } } } + +/** + * Provides classes modeling the Google Cloud Spanner library. + */ +private module Spanner { + /** + * Gets a node that refers to the `Spanner` class + */ + API::Node spanner() { + // older versions + result = API::moduleImport("@google-cloud/spanner") + or + // newer versions + result = API::moduleImport("@google-cloud/spanner").getMember("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() + or + result = API::Node::ofType("@google-cloud/spanner", "Database") + } + + /** + * Gets a node that refers to an instance of the `v1.SpannerClient` class. + */ + API::Node v1SpannerClient() { + result = spanner().getMember("v1").getMember("SpannerClient").getInstance() + or + result = API::Node::ofType("@google-cloud/spanner", "v1.SpannerClient") + } + + /** + * Gets a node that refers to a transaction object. + */ + API::Node transaction() { + result = + database() + .getMember(["runTransaction", "runTransactionAsync"]) + .getParameter([0, 1]) + .getParameter(1) + or + result = API::Node::ofType("@google-cloud/spanner", "Transaction") + } + + /** + * Gets a node that refers to a snapshot object. + */ + API::Node snapshot() { + result = database().getMember("getSnapshot").getParameter([0, 1]).getParameter(1) + or + result = API::Node::ofType("@google-cloud/spanner", "Snapshot") + } + + /** Gets an API node referring to a `BatchTransaction` object. */ + API::Node batchTransaction() { + result = database().getMember("batchTransaction").getReturn() + or + result = database().getMember("createBatchTransaction").getReturn().getPromised() + or + result = API::Node::ofType("@google-cloud/spanner", "BatchTransaction") + } + + /** + * A call to a Spanner method that executes a SQL query. + */ + abstract class SqlExecution extends DatabaseAccess, DataFlow::InvokeNode { } + + /** + * 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() + or + this = snapshot().getMember(["run", "runStream"]).getACall() + } + + override DataFlow::Node getAResult() { + PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp()) + or + this = [database(), transaction(), snapshot()].getMember("run").getACall() and + result = this.getCallback(_).getParameter(1) + } + + override DataFlow::Node getAQueryArgument() { + result = this.getArgument(0) + or + result = this.getOptionArgument(0, "sql") + } + } + + /** + * 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 getAResult() { + none() // no results, batch update callbacks get only row counts. + } + + 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() + } + } + + /** + * A SQL execution that only takes the input in the `sql` option, and do not accept query strings + * directly. + */ + class SqlExecutionWithOption extends SqlExecution, DataFlow::CallNode { + SqlExecutionWithOption() { + this = v1SpannerClient().getMember(["executeSql", "executeStreamingSql"]).getACall() + } + + override DataFlow::Node getAResult() { + this = v1SpannerClient().getMember("executeSql").getACall() and + result = this.getCallback(_).getParameter(1) + } + + override DataFlow::Node getAQueryArgument() { result = this.getOptionArgument(0, "sql") } + } + + /** + * An expression that is interpreted as a SQL string. + */ + class QueryString extends SQL::SqlString { + QueryString() { this = any(SqlExecution se).getAQueryArgument().asExpr() } + } +} diff --git a/javascript/ql/lib/semmle/javascript/heuristics/AdditionalSources.qll b/javascript/ql/lib/semmle/javascript/heuristics/AdditionalSources.qll index 7b1e8a349d4..db1e39fdf63 100644 --- a/javascript/ql/lib/semmle/javascript/heuristics/AdditionalSources.qll +++ b/javascript/ql/lib/semmle/javascript/heuristics/AdditionalSources.qll @@ -14,7 +14,7 @@ private import semmle.javascript.security.dataflow.CommandInjectionCustomization abstract class HeuristicSource extends DataFlow::Node { } /** - * An access to a password, viewed a source of remote flow. + * An access to a password, viewed as a source of remote flow. */ private class RemoteFlowPassword extends HeuristicSource, RemoteFlowSource { RemoteFlowPassword() { isReadFrom(this, "(?is).*(password|passwd).*") } @@ -52,3 +52,16 @@ class RemoteServerResponse extends HeuristicSource, RemoteFlowSource { override string getSourceType() { result = "a response from a remote server" } } + +/** + * The data read from a database. + */ +class DatabaseAccessResultRemoteFlowSource extends HeuristicSource, RemoteFlowSource { + DatabaseAccessResultRemoteFlowSource() { exists(DatabaseAccess dba | this = dba.getAResult()) } + + override string getSourceType() { result = "Database query result" } + + override predicate isUserControlledObject() { + this.(DatabaseAccess).returnsUserControlledObject() + } +} diff --git a/javascript/ql/test/library-tests/Security/heuristics/HeuristicSink.ql b/javascript/ql/test/library-tests/Security/heuristics/HeuristicSink.ql index 6554331526f..36ef266633a 100644 --- a/javascript/ql/test/library-tests/Security/heuristics/HeuristicSink.ql +++ b/javascript/ql/test/library-tests/Security/heuristics/HeuristicSink.ql @@ -1,4 +1,4 @@ import javascript private import semmle.javascript.heuristics.AdditionalSinks -select any(HeuristicSink s) +select any(HeuristicSink s | s.getFile().getBaseName() = "sinks.js") diff --git a/javascript/ql/test/library-tests/Security/heuristics/HeuristicSource.expected b/javascript/ql/test/library-tests/Security/heuristics/HeuristicSource.expected index 52862db346b..e69de29bb2d 100644 --- a/javascript/ql/test/library-tests/Security/heuristics/HeuristicSource.expected +++ b/javascript/ql/test/library-tests/Security/heuristics/HeuristicSource.expected @@ -1,2 +0,0 @@ -| sources.js:2:5:2:12 | password | -| sources.js:3:5:3:20 | JSON.stringify() | diff --git a/javascript/ql/test/library-tests/Security/heuristics/HeuristicSource.ql b/javascript/ql/test/library-tests/Security/heuristics/HeuristicSource.ql index e605e91eae4..72d94707e6b 100644 --- a/javascript/ql/test/library-tests/Security/heuristics/HeuristicSource.ql +++ b/javascript/ql/test/library-tests/Security/heuristics/HeuristicSource.ql @@ -1,4 +1,13 @@ import javascript private import semmle.javascript.heuristics.AdditionalSources +import testUtilities.ConsistencyChecking -select any(HeuristicSource s) +class Taint extends TaintTracking::Configuration { + Taint() { this = "Taint" } + + override predicate isSource(DataFlow::Node node) { node instanceof HeuristicSource } + + override predicate isSink(DataFlow::Node node) { + node = any(DataFlow::CallNode call | call.getCalleeName() = "sink").getAnArgument() + } +} diff --git a/javascript/ql/test/library-tests/Security/heuristics/sources.js b/javascript/ql/test/library-tests/Security/heuristics/sources.js index 45b4fca2ea4..4817cc182d1 100644 --- a/javascript/ql/test/library-tests/Security/heuristics/sources.js +++ b/javascript/ql/test/library-tests/Security/heuristics/sources.js @@ -1,4 +1,234 @@ +const pg = require('pg'); +const { Stream } = require('stream'); +const pool = new pg.Pool({}); + +function sink(data) {} + (function() { - password; - JSON.stringify(); + const password = '1234'; + sink(password); // NOT OK + + const s = JSON.stringify(); + sink(s); // NOT OK +})(); + +(async function() { + const knex = require('knex'); + + const users = knex().select('*').from('users'); + users.then(function (users) { + sink(users); // NOT OK + }); + + users.asCallback(function (err, users) { + sink(users); // NOT OK + }); + + sink(await users); // NOT OK +})(); + +(function() { + const pg = require('pg'); + + const pool = new pg.Pool({}); + pool.connect(async function (err, client, done) { + client.query('SELECT * FROM users', function (err, users) { + sink(users); + }); + + const thenable = client.query('SELECT * FROM users') + thenable.then(function(users) { + sink(users); // NOT OK + }); + + const pgpromise = client.query('SELECT * FROM users'); + sink(await pgpromise); // NOT OK + }); +})(); + +(async function () { + const pgpromise = require('pg-promise')(); + const db = pgpromise('postgres://username:password@localhost:1234/database'); + const pgppromise = db.any('SELECT * FROM users'); + + pgppromise.then(function (users) { + sink(users); + }); + + sink(await pgppromise); +})(); + +(function () { + const mysql = require('mysql2'); + const conn = mysql.createConnection({}); + + conn.query( + 'SELECT * FROM `users`', + function(err, users, fields) { + sink(users); // NOT OK + } + ); + + conn.execute( + 'SELECT * FROM `users` WHERE name = ?', + ['Alice'], + function(err, users) { + sink(users); + } + ); +})(); + +(async function () { + const sqlite = require('sqlite3'); + const db = new sqlite.Database(':memory:'); + + db.all('SELECT * FROM users', function (err, users) { + sink(users); // NOT OK + }); + + const sqlitepromise = db.all('SELECT * FROM users'); + + sink(await sqlitepromise); // NOT OK +})(); + +(async function () { + const { Sequelize } = require('sequelize'); + const sequelize = new Sequelize('sqlite::memory:'); + + class User extends sequelize.Model {} + User.init({ name: sequelize.DataTypes.String }, { sequelize, modelName: 'user' }); + + sequelize.query('SELECT * FROM users').then(function (users) { + sink(users); // NOT OK + }); +})(); + +(async function () { + const sql = require('mssql'); + await sql.connect('...'); + + sql.query('SELECT * FROM users', function (err, users) { + sink(users); // NOT OK + }); + + const mssqlthenable = sql.query('SELECT * FROM users'); + + mssqlthenable.then(function (users) { + sink(users); // NOT OK + }); + + const mssqlpromise = sql.query('SELECT * FROM users'); + sink(await mssqlpromise); // NOT OK + + const uname = 'Alice'; + const mssqltaggedquery = sql.query`SELECT * FROM users where name=${uname}` + sink(await mssqltaggedquery); // NOT OK +})(); + +(async function () { + const {Spanner} = require('@google-cloud/spanner'); + const db = new Spanner({projectId: 'test'}) + .instance('instanceid') + .database('databaseid'); + + const spannerpromise = db.run({ + sql: 'SELECT * FROM users' + }); + + sink(await spannerpromise); // NOT OK + + db.run({ + sql: 'SELECT * FROM users' + }, function (err, rows, stats, meta) { + sink(rows); // NOT OK + }); + + const client = new Spanner.v1.SpannerClient({}); + client.executeSql('SELECT * FROM users', {}, function (err, users) { + sink(users); // NOT OK + }); + + db.runTransaction(function(err, txn) { + txn.run('SELECT * FROM users', function (err, users) { + sink(users); // NOT OK + }); + }); + + db.getSnapshot(function (err, txn) { + txn.run('SELECT * FROM users', function (err, users) { + sink(users); // NOT OK + }); + txn.end(); + }); +})(); + +(function () { + const { MongoClient } = require('mongodb'); + + MongoClient.connect('mongodb://localhost:1234', async function (err, db) { + const collection = db.collection('users'); + const users = await collection.find({}); + sink(users); // NOT OK + }); +})(); + +(async function () { + const mongoose = require('mongoose'); + await mongoose.connect('mongodb://localhost:1234'); + + const User = mongoose.model('User', { + name: { + type: String, + unique: true + } + }); + + User.find({ name: 'Alice' }, function (err, alice) { + sink(alice); // NOT OK + }); + + User.find({ name: 'Bob' }).exec(function (err, bob) { + sink(bob); // NOT OK + }); + + const promise = User.find({ name: 'Claire' }); + promise.then(c => sink(c)); // NOT OK +})(); + +(async function () { + const minimongo = require('minimongo'); + const LocalDb = minimongo.MemoryDb; + const db = new LocalDb(); + const doc = db.users; + + const users = await doc.find({}); + sink(users); // NOT OK +})(); + +(async function () { + const { Collection } = require('marsdb'); + + const doc = new Collection('users'); + + const users = await doc.find({}); + + sink(users); // NOT OK +})(); + +(async function () { + const redis = require("redis"); + const client = redis.createClient(); + + const alice = await client.get('alice'); + + sink(alice); // NOT OK +})(); + +(async function () { + const Redis = require('ioredis'); + const redis = new Redis(); + + const bob = await redis.get('bob'); + + sink(bob); // NOT OK })(); From 09a28c428c25714b1b10cd4312c5e989152833e6 Mon Sep 17 00:00:00 2001 From: Stephan Brandauer Date: Wed, 12 Jan 2022 17:07:16 +0100 Subject: [PATCH 2/4] base implementation of Spanner model on models-as-data --- .../ql/lib/semmle/javascript/Concepts.qll | 10 -- .../lib/semmle/javascript/frameworks/SQL.qll | 153 ++---------------- .../frameworks/data/internal/Shared.qll | 2 +- .../heuristics/AdditionalSources.qll | 16 +- .../Security/heuristics/sources.js | 14 +- 5 files changed, 40 insertions(+), 155 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/Concepts.qll b/javascript/ql/lib/semmle/javascript/Concepts.qll index a7c49ed0cf3..e3c3f0d2357 100644 --- a/javascript/ql/lib/semmle/javascript/Concepts.qll +++ b/javascript/ql/lib/semmle/javascript/Concepts.qll @@ -89,16 +89,6 @@ abstract class DatabaseAccess extends DataFlow::Node { DataFlow::Node getAResult() { none() // Overridden in subclass } - - /** - * Holds if the data returned can be a user-controlled object, - * such as a JSON object parsed from user-controlled data. - */ - predicate returnsUserControlledObject() { - // NB: Most data bases support JSON data (some via plugins), - // which is why this has a default implementation. - any() - } } /** diff --git a/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll b/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll index c8e78b7568e..b4d8b20778a 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll @@ -574,7 +574,10 @@ private module SpannerCsv { "@google-cloud/spanner;;@google-cloud/spanner;;Member[Spanner]", "@google-cloud/spanner;Database;@google-cloud/spanner;;ReturnValue.Member[instance].ReturnValue.Member[database].ReturnValue", "@google-cloud/spanner;v1.SpannerClient;@google-cloud/spanner;;Member[v1].Member[SpannerClient].Instance", - "@google-cloud/spanner;Transaction;@google-cloud/spanner;Database;Member[runTransaction,runTransactionAsync].Argument[0..1].Parameter[1]", + "@google-cloud/spanner;Transaction;@google-cloud/spanner;Database;Member[runTransaction,runTransactionAsync,getTransaction].Argument[0..1].Parameter[1]", + "@google-cloud/spanner;Transaction;@google-cloud/spanner;Database;Member[getTransaction].ReturnValue.Awaited", + "@google-cloud/spanner;Snapshot;@google-cloud/spanner;Database;Member[getSnapshot].Argument[0..1].Parameter[1]", + "@google-cloud/spanner;Snapshot;@google-cloud/spanner;Database;Member[getSnapshot].ReturnValue.Awaited", "@google-cloud/spanner;BatchTransaction;@google-cloud/spanner;Database;Member[batchTransaction].ReturnValue", "@google-cloud/spanner;BatchTransaction;@google-cloud/spanner;Database;Member[createBatchTransaction].ReturnValue.Awaited", "@google-cloud/spanner;~SqlExecutorDirect;@google-cloud/spanner;Database;Member[run,runPartitionedUpdate,runStream]", @@ -597,146 +600,22 @@ private module SpannerCsv { ] } } -} -/** - * Provides classes modeling the Google Cloud Spanner library. - */ -private module Spanner { - /** - * Gets a node that refers to the `Spanner` class - */ - API::Node spanner() { - // older versions - result = API::moduleImport("@google-cloud/spanner") - or - // newer versions - result = API::moduleImport("@google-cloud/spanner").getMember("Spanner") - } + class SpannerSources extends ModelInput::SourceModelCsv { + string spannerClass() { result = ["v1.SpannerClient", "Database", "Transaction", "Snapshot",] } - /** - * Gets a node that refers to an instance of the `Database` class. - */ - API::Node database() { - result = - spanner().getReturn().getMember("instance").getReturn().getMember("database").getReturn() - or - result = API::Node::ofType("@google-cloud/spanner", "Database") - } - - /** - * Gets a node that refers to an instance of the `v1.SpannerClient` class. - */ - API::Node v1SpannerClient() { - result = spanner().getMember("v1").getMember("SpannerClient").getInstance() - or - result = API::Node::ofType("@google-cloud/spanner", "v1.SpannerClient") - } - - /** - * Gets a node that refers to a transaction object. - */ - API::Node transaction() { - result = - database() - .getMember(["runTransaction", "runTransactionAsync"]) - .getParameter([0, 1]) - .getParameter(1) - or - result = API::Node::ofType("@google-cloud/spanner", "Transaction") - } - - /** - * Gets a node that refers to a snapshot object. - */ - API::Node snapshot() { - result = database().getMember("getSnapshot").getParameter([0, 1]).getParameter(1) - or - result = API::Node::ofType("@google-cloud/spanner", "Snapshot") - } - - /** Gets an API node referring to a `BatchTransaction` object. */ - API::Node batchTransaction() { - result = database().getMember("batchTransaction").getReturn() - or - result = database().getMember("createBatchTransaction").getReturn().getPromised() - or - result = API::Node::ofType("@google-cloud/spanner", "BatchTransaction") - } - - /** - * A call to a Spanner method that executes a SQL query. - */ - abstract class SqlExecution extends DatabaseAccess, DataFlow::InvokeNode { } - - /** - * 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() - or - this = snapshot().getMember(["run", "runStream"]).getACall() + string resultPath() { + result = + [ + "Member[executeSql].Argument[0..].Parameter[1]", + "Member[executeSql].ReturnValue.Awaited.Member[0]", "Member[run].ReturnValue.Awaited", + "Member[run].Argument[0..].Parameter[1]", + ] } - override DataFlow::Node getAResult() { - PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp()) - or - this = [database(), transaction(), snapshot()].getMember("run").getACall() and - result = this.getCallback(_).getParameter(1) + override predicate row(string row) { + row = + "@google-cloud/spanner;" + spannerClass() + ";" + resultPath() + ";database-access-result" } - - override DataFlow::Node getAQueryArgument() { - result = this.getArgument(0) - or - result = this.getOptionArgument(0, "sql") - } - } - - /** - * 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 getAResult() { - none() // no results, batch update callbacks get only row counts. - } - - 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() - } - } - - /** - * A SQL execution that only takes the input in the `sql` option, and do not accept query strings - * directly. - */ - class SqlExecutionWithOption extends SqlExecution, DataFlow::CallNode { - SqlExecutionWithOption() { - this = v1SpannerClient().getMember(["executeSql", "executeStreamingSql"]).getACall() - } - - override DataFlow::Node getAResult() { - this = v1SpannerClient().getMember("executeSql").getACall() and - result = this.getCallback(_).getParameter(1) - } - - override DataFlow::Node getAQueryArgument() { result = this.getOptionArgument(0, "sql") } - } - - /** - * An expression that is interpreted as a SQL string. - */ - class QueryString extends SQL::SqlString { - QueryString() { this = any(SqlExecution se).getAQueryArgument().asExpr() } } } diff --git a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/Shared.qll b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/Shared.qll index fc712ca99dc..ccc53ff9a2f 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/Shared.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/Shared.qll @@ -31,7 +31,7 @@ * - Instance: the value returned by a constructor call * - Awaited: the value from a resolved promise/future-like object * - WithArity[n]: match a call with the given arity. May be a range of form `x..y` (inclusive) and/or a comma-separated list. - * - Other langauge-specific tokens mentioned in `ModelsAsData.qll`. + * - Other language-specific tokens mentioned in `ModelsAsData.qll`. * 4. The `input` and `output` columns specify how data enters and leaves the element selected by the * first `(package, type, path)` tuple. Both strings are `.`-separated access paths * of the same syntax as the `path` column. diff --git a/javascript/ql/lib/semmle/javascript/heuristics/AdditionalSources.qll b/javascript/ql/lib/semmle/javascript/heuristics/AdditionalSources.qll index db1e39fdf63..1a1c0c76e0d 100644 --- a/javascript/ql/lib/semmle/javascript/heuristics/AdditionalSources.qll +++ b/javascript/ql/lib/semmle/javascript/heuristics/AdditionalSources.qll @@ -61,7 +61,21 @@ class DatabaseAccessResultRemoteFlowSource extends HeuristicSource, RemoteFlowSo override string getSourceType() { result = "Database query result" } + override predicate isUserControlledObject() { any() } +} + +/** + * A remote flow source originating from a database access. + */ +private class RemoteFlowSourceFromDBAccess extends RemoteFlowSource, HeuristicSource { + RemoteFlowSourceFromDBAccess() { + this = ModelOutput::getASourceNode("database-access-result").getAUse() + } + + override string getSourceType() { result = "Database access" } + override predicate isUserControlledObject() { - this.(DatabaseAccess).returnsUserControlledObject() + // NB. supported databases all might return JSON. + any() } } diff --git a/javascript/ql/test/library-tests/Security/heuristics/sources.js b/javascript/ql/test/library-tests/Security/heuristics/sources.js index 4817cc182d1..95f459520bd 100644 --- a/javascript/ql/test/library-tests/Security/heuristics/sources.js +++ b/javascript/ql/test/library-tests/Security/heuristics/sources.js @@ -1,9 +1,3 @@ -const pg = require('pg'); -const { Stream } = require('stream'); -const pool = new pg.Pool({}); - -function sink(data) {} - (function() { const password = '1234'; sink(password); // NOT OK @@ -130,6 +124,13 @@ function sink(data) {} const db = new Spanner({projectId: 'test'}) .instance('instanceid') .database('databaseid'); + + db.executeSql('SELECT * FROM users', {}, function (err, users) { + sink(users); // NOT OK + }); + + const [users] = (await db.executeSql('SELECT * FROM users', {})); + sink(users); // NOT OK const spannerpromise = db.run({ sql: 'SELECT * FROM users' @@ -152,6 +153,7 @@ function sink(data) {} txn.run('SELECT * FROM users', function (err, users) { sink(users); // NOT OK }); + txn.commit(function () {}); }); db.getSnapshot(function (err, txn) { From 63aaf24063eb09b45b8a771493237e6afec8f7a0 Mon Sep 17 00:00:00 2001 From: Stephan Brandauer Date: Thu, 13 Jan 2022 09:39:52 +0100 Subject: [PATCH 3/4] base implementation of Sequelize model on models-as-data --- .../lib/semmle/javascript/frameworks/SQL.qll | 39 +++++-------------- 1 file changed, 10 insertions(+), 29 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll b/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll index b4d8b20778a..0a468d656a7 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll @@ -435,7 +435,7 @@ private module MsSql { API::Node pool() { result = mssqlClass("ConnectionPool") } /** A tagged template evaluated as a query. */ - private class QueryTemplateExpr extends DatabaseAccess, DataFlow::ValueNode { + private class QueryTemplateExpr extends DatabaseAccess, DataFlow::ValueNode, DataFlow::SourceNode { override TaggedTemplateExpr astNode; QueryTemplateExpr() { @@ -443,8 +443,7 @@ private module MsSql { } override DataFlow::Node getAResult() { - PromiseFlow::loadStep(this.(DataFlow::SourceNode).getALocalUse(), result, - Promises::valueProp()) + PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp()) } override DataFlow::Node getAQueryArgument() { @@ -524,31 +523,6 @@ 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() - or - result = API::Node::ofType(["sequelize", "sequelize-typescript"], ["Sequelize", "default"]) - } - - /** A call to `Sequelize.query`. */ - private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode { - QueryCall() { this = instance().getMember("query").getACall() } - - override DataFlow::Node getAResult() { - PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp()) - } - - override DataFlow::Node getAQueryArgument() { - result = this.getArgument(0) - or - result = this.getOptionArgument(0, "query") - } - } - class SequelizeSink extends ModelInput::SinkModelCsv { override predicate row(string row) { row = @@ -563,6 +537,12 @@ private module Sequelize { ] } } + + class SequelizeSource extends ModelInput::SourceModelCsv { + override predicate row(string row) { + row = "sequelize;Sequelize;Member[query].ReturnValue.Awaited;database-access-result" + } + } } private module SpannerCsv { @@ -615,7 +595,8 @@ private module SpannerCsv { override predicate row(string row) { row = - "@google-cloud/spanner;" + spannerClass() + ";" + resultPath() + ";database-access-result" + "@google-cloud/spanner;" + this.spannerClass() + ";" + this.resultPath() + + ";database-access-result" } } } From 93507a2d71df5c0a40b9c9ab7baf607d0ce14e4d Mon Sep 17 00:00:00 2001 From: Stephan Brandauer Date: Thu, 13 Jan 2022 10:53:58 +0100 Subject: [PATCH 4/4] combine two implementations for database-accesses as remote flow sources --- .../javascript/heuristics/AdditionalSources.qll | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/heuristics/AdditionalSources.qll b/javascript/ql/lib/semmle/javascript/heuristics/AdditionalSources.qll index 1a1c0c76e0d..6585fb7133b 100644 --- a/javascript/ql/lib/semmle/javascript/heuristics/AdditionalSources.qll +++ b/javascript/ql/lib/semmle/javascript/heuristics/AdditionalSources.qll @@ -53,23 +53,13 @@ class RemoteServerResponse extends HeuristicSource, RemoteFlowSource { override string getSourceType() { result = "a response from a remote server" } } -/** - * The data read from a database. - */ -class DatabaseAccessResultRemoteFlowSource extends HeuristicSource, RemoteFlowSource { - DatabaseAccessResultRemoteFlowSource() { exists(DatabaseAccess dba | this = dba.getAResult()) } - - override string getSourceType() { result = "Database query result" } - - override predicate isUserControlledObject() { any() } -} - /** * A remote flow source originating from a database access. */ private class RemoteFlowSourceFromDBAccess extends RemoteFlowSource, HeuristicSource { RemoteFlowSourceFromDBAccess() { - this = ModelOutput::getASourceNode("database-access-result").getAUse() + this = ModelOutput::getASourceNode("database-access-result").getAUse() or + exists(DatabaseAccess dba | this = dba.getAResult()) } override string getSourceType() { result = "Database access" }