diff --git a/javascript/ql/lib/semmle/javascript/Concepts.qll b/javascript/ql/lib/semmle/javascript/Concepts.qll index 798747af8b8..e3c3f0d2357 100644 --- a/javascript/ql/lib/semmle/javascript/Concepts.qll +++ b/javascript/ql/lib/semmle/javascript/Concepts.qll @@ -84,6 +84,11 @@ 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 + } } /** 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..0a468d656a7 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) } } @@ -413,13 +435,17 @@ 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() { mssql().getMember("query").getAUse() = DataFlow::valueNode(astNode.getTag()) } + override DataFlow::Node getAResult() { + PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp()) + } + override DataFlow::Node getAQueryArgument() { result = DataFlow::valueNode(astNode.getTemplate().getAnElement()) } @@ -429,6 +455,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) } } @@ -505,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 { @@ -516,7 +554,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]", @@ -539,4 +580,23 @@ private module SpannerCsv { ] } } + + class SpannerSources extends ModelInput::SourceModelCsv { + string spannerClass() { result = ["v1.SpannerClient", "Database", "Transaction", "Snapshot",] } + + 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 predicate row(string row) { + row = + "@google-cloud/spanner;" + this.spannerClass() + ";" + this.resultPath() + + ";database-access-result" + } + } } 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 7b1e8a349d4..6585fb7133b 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,20 @@ class RemoteServerResponse extends HeuristicSource, RemoteFlowSource { override string getSourceType() { result = "a response from a remote server" } } + +/** + * A remote flow source originating from a database access. + */ +private class RemoteFlowSourceFromDBAccess extends RemoteFlowSource, HeuristicSource { + RemoteFlowSourceFromDBAccess() { + this = ModelOutput::getASourceNode("database-access-result").getAUse() or + exists(DatabaseAccess dba | this = dba.getAResult()) + } + + override string getSourceType() { result = "Database access" } + + override predicate isUserControlledObject() { + // NB. supported databases all might return JSON. + any() + } +} 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..95f459520bd 100644 --- a/javascript/ql/test/library-tests/Security/heuristics/sources.js +++ b/javascript/ql/test/library-tests/Security/heuristics/sources.js @@ -1,4 +1,236 @@ (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'); + + 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' + }); + + 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 + }); + txn.commit(function () {}); + }); + + 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 })();