Skip to content

Commit 40ad88b

Browse files
author
Stephan Brandauer
authored
Merge pull request #7474 from kaeluka/db-reads-as-taint-sources
JS: DB reads as taint sources
2 parents 8583a4f + 93507a2 commit 40ad88b

File tree

10 files changed

+405
-24
lines changed

10 files changed

+405
-24
lines changed

‎javascript/ql/lib/semmle/javascript/Concepts.qll

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,11 @@ abstract class FileNameSource extends DataFlow::Node { }
8484
abstract class DatabaseAccess extends DataFlow::Node {
8585
/** Gets an argument to this database access that is interpreted as a query. */
8686
abstract DataFlow::Node getAQueryArgument();
87+
88+
/** Gets a node to which a result of the access may flow. */
89+
DataFlow::Node getAResult() {
90+
none() // Overridden in subclass
91+
}
8792
}
8893

8994
/**

‎javascript/ql/lib/semmle/javascript/frameworks/Knex.qll

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -46,17 +46,35 @@ module Knex {
4646
RawKnexSqlString() { this = any(RawKnexCall call).getArgument(0).asExpr() }
4747
}
4848

49-
/** A call that triggers a SQL query submission. */
50-
private class KnexDatabaseAccess extends DatabaseAccess {
51-
KnexDatabaseAccess() {
52-
this = knexObject().getMember(["then", "stream", "asCallback"]).getACall()
49+
/** A call that triggers a SQL query submission by calling then/stream/asCallback. */
50+
private class KnexDatabaseCallback extends DatabaseAccess, DataFlow::CallNode {
51+
string member;
52+
53+
KnexDatabaseCallback() {
54+
member = ["then", "stream", "asCallback"] and
55+
this = knexObject().getMember(member).getACall()
56+
}
57+
58+
override DataFlow::Node getAResult() {
59+
member = "then" and
60+
result = this.getCallback(0).getParameter(0)
5361
or
54-
exists(AwaitExpr await |
55-
this = await.flow() and
56-
await.getOperand() = knexObject().getAUse().asExpr()
62+
member = "asCallback" and
63+
result = this.getCallback(0).getParameter(1)
64+
}
65+
66+
override DataFlow::Node getAQueryArgument() { none() }
67+
}
68+
69+
private class KnexDatabaseAwait extends DatabaseAccess, DataFlow::ValueNode {
70+
KnexDatabaseAwait() {
71+
exists(AwaitExpr enclosingAwait | this = enclosingAwait.flow() |
72+
enclosingAwait.getOperand() = knexObject().getAUse().asExpr()
5773
)
5874
}
5975

76+
override DataFlow::Node getAResult() { result = this }
77+
6078
override DataFlow::Node getAQueryArgument() { none() }
6179
}
6280
}

‎javascript/ql/lib/semmle/javascript/frameworks/NoSQL.qll

Lines changed: 49 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
*/
44

55
import javascript
6+
import semmle.javascript.Promises
67

78
module NoSQL {
89
/** An expression that is interpreted as a NoSQL query. */
@@ -65,6 +66,10 @@ private module MongoDB {
6566

6667
override DataFlow::Node getAQueryArgument() { result = this.getArgument(queryArgIdx) }
6768

69+
override DataFlow::Node getAResult() {
70+
PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
71+
}
72+
6873
DataFlow::Node getACodeOperator() {
6974
result = getADollarWhereProperty(this.getParameter(queryArgIdx))
7075
}
@@ -537,12 +542,29 @@ private module Mongoose {
537542
// NB: the complete information is not easily accessible for deeply chained calls
538543
f.getQueryArgument().getARhs() = result
539544
}
545+
546+
override DataFlow::Node getAResult() {
547+
result = this.getCallback(this.getNumArgument() - 1).getParameter(1)
548+
}
540549
}
541550

542-
class ExplicitQueryEvaluation extends DatabaseAccess {
551+
class ExplicitQueryEvaluation extends DatabaseAccess, DataFlow::CallNode {
552+
string member;
553+
543554
ExplicitQueryEvaluation() {
544555
// explicit execution using a Query method call
545-
Query::getAMongooseQuery().getMember(["exec", "then", "catch"]).getACall() = this
556+
member = ["exec", "then", "catch"] and
557+
Query::getAMongooseQuery().getMember(member).getACall() = this
558+
}
559+
560+
private int resultParamIndex() {
561+
member = "then" and result = 0
562+
or
563+
member = "exec" and result = 1
564+
}
565+
566+
override DataFlow::Node getAResult() {
567+
result = this.getCallback(_).getParameter(this.resultParamIndex())
546568
}
547569

548570
override DataFlow::Node getAQueryArgument() {
@@ -588,6 +610,10 @@ private module Minimongo {
588610

589611
override DataFlow::Node getAQueryArgument() { result = this.getArgument(queryArgIdx) }
590612

613+
override DataFlow::Node getAResult() {
614+
PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
615+
}
616+
591617
DataFlow::Node getACodeOperator() {
592618
result = getADollarWhereProperty(this.getParameter(queryArgIdx))
593619
}
@@ -609,7 +635,7 @@ private module Minimongo {
609635
* Provides classes modeling the MarsDB library.
610636
*/
611637
private module MarsDB {
612-
private class MarsDBAccess extends DatabaseAccess {
638+
private class MarsDBAccess extends DatabaseAccess, DataFlow::CallNode {
613639
string method;
614640

615641
MarsDBAccess() {
@@ -623,21 +649,29 @@ private module MarsDB {
623649

624650
string getMethod() { result = method }
625651

652+
override DataFlow::Node getAResult() {
653+
PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
654+
}
655+
626656
override DataFlow::Node getAQueryArgument() { none() }
627657
}
628658

629659
/** A call to a MarsDB query method. */
630-
private class QueryCall extends DatabaseAccess, API::CallNode {
660+
private class QueryCall extends MarsDBAccess, API::CallNode {
631661
int queryArgIdx;
632662

633663
QueryCall() {
634664
exists(string m |
635-
this.(MarsDBAccess).getMethod() = m and
665+
this.getMethod() = m and
636666
// implements parts of the Minimongo interface
637667
Minimongo::CollectionMethodSignatures::interpretsArgumentAsQuery(m, queryArgIdx)
638668
)
639669
}
640670

671+
override DataFlow::Node getAResult() {
672+
PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
673+
}
674+
641675
override DataFlow::Node getAQueryArgument() { result = this.getArgument(queryArgIdx) }
642676

643677
DataFlow::Node getACodeOperator() {
@@ -744,9 +778,13 @@ private module Redis {
744778
/**
745779
* An access to a database through redis
746780
*/
747-
class RedisDatabaseAccess extends DatabaseAccess {
781+
class RedisDatabaseAccess extends DatabaseAccess, DataFlow::CallNode {
748782
RedisDatabaseAccess() { this = redis().getMember(_).getACall() }
749783

784+
override DataFlow::Node getAResult() {
785+
PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
786+
}
787+
750788
override DataFlow::Node getAQueryArgument() { none() }
751789
}
752790
}
@@ -768,9 +806,13 @@ private module IoRedis {
768806
/**
769807
* An access to a database through ioredis
770808
*/
771-
class IoRedisDatabaseAccess extends DatabaseAccess {
809+
class IoRedisDatabaseAccess extends DatabaseAccess, DataFlow::CallNode {
772810
IoRedisDatabaseAccess() { this = ioredis().getMember(_).getACall() }
773811

812+
override DataFlow::Node getAResult() {
813+
PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
814+
}
815+
774816
override DataFlow::Node getAQueryArgument() { none() }
775817
}
776818
}

‎javascript/ql/lib/semmle/javascript/frameworks/SQL.qll

Lines changed: 62 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
*/
44

55
import javascript
6+
import semmle.javascript.Promises
67

78
module SQL {
89
/** A string-valued expression that is interpreted as a SQL command. */
@@ -81,6 +82,8 @@ private module MySql {
8182
)
8283
}
8384

85+
override DataFlow::Node getAResult() { result = this.getCallback(_).getParameter(1) }
86+
8487
override DataFlow::Node getAQueryArgument() { result = this.getArgument(0) }
8588
}
8689

@@ -178,6 +181,16 @@ private module Postgres {
178181
private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode {
179182
QueryCall() { this = [client(), pool()].getMember("query").getACall() }
180183

184+
override DataFlow::Node getAResult() {
185+
this.getNumArgument() = 2 and
186+
result = this.getCallback(1).getParameter(1)
187+
or
188+
this.getNumArgument() = 1 and
189+
result = this.getAMethodCall("then").getCallback(0).getParameter(0)
190+
or
191+
PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
192+
}
193+
181194
override DataFlow::Node getAQueryArgument() { result = this.getArgument(0) }
182195
}
183196

@@ -322,6 +335,10 @@ private module Postgres {
322335
)
323336
}
324337

338+
override DataFlow::Node getAResult() {
339+
PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
340+
}
341+
325342
override DataFlow::Node getAQueryArgument() {
326343
result = this.getADirectQueryArgument()
327344
or
@@ -370,6 +387,11 @@ private module Sqlite {
370387
this = database().getMember("prepare").getACall()
371388
}
372389

390+
override DataFlow::Node getAResult() {
391+
result = this.getCallback(1).getParameter(1) or
392+
PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
393+
}
394+
373395
override DataFlow::Node getAQueryArgument() { result = this.getArgument(0) }
374396
}
375397

@@ -413,13 +435,17 @@ private module MsSql {
413435
API::Node pool() { result = mssqlClass("ConnectionPool") }
414436

415437
/** A tagged template evaluated as a query. */
416-
private class QueryTemplateExpr extends DatabaseAccess, DataFlow::ValueNode {
438+
private class QueryTemplateExpr extends DatabaseAccess, DataFlow::ValueNode, DataFlow::SourceNode {
417439
override TaggedTemplateExpr astNode;
418440

419441
QueryTemplateExpr() {
420442
mssql().getMember("query").getAUse() = DataFlow::valueNode(astNode.getTag())
421443
}
422444

445+
override DataFlow::Node getAResult() {
446+
PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
447+
}
448+
423449
override DataFlow::Node getAQueryArgument() {
424450
result = DataFlow::valueNode(astNode.getTemplate().getAnElement())
425451
}
@@ -429,6 +455,12 @@ private module MsSql {
429455
private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode {
430456
QueryCall() { this = [mssql(), request()].getMember(["query", "batch"]).getACall() }
431457

458+
override DataFlow::Node getAResult() {
459+
result = this.getCallback(1).getParameter(1)
460+
or
461+
PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
462+
}
463+
432464
override DataFlow::Node getAQueryArgument() { result = this.getArgument(0) }
433465
}
434466

@@ -505,6 +537,12 @@ private module Sequelize {
505537
]
506538
}
507539
}
540+
541+
class SequelizeSource extends ModelInput::SourceModelCsv {
542+
override predicate row(string row) {
543+
row = "sequelize;Sequelize;Member[query].ReturnValue.Awaited;database-access-result"
544+
}
545+
}
508546
}
509547

510548
private module SpannerCsv {
@@ -516,7 +554,10 @@ private module SpannerCsv {
516554
"@google-cloud/spanner;;@google-cloud/spanner;;Member[Spanner]",
517555
"@google-cloud/spanner;Database;@google-cloud/spanner;;ReturnValue.Member[instance].ReturnValue.Member[database].ReturnValue",
518556
"@google-cloud/spanner;v1.SpannerClient;@google-cloud/spanner;;Member[v1].Member[SpannerClient].Instance",
519-
"@google-cloud/spanner;Transaction;@google-cloud/spanner;Database;Member[runTransaction,runTransactionAsync].Argument[0..1].Parameter[1]",
557+
"@google-cloud/spanner;Transaction;@google-cloud/spanner;Database;Member[runTransaction,runTransactionAsync,getTransaction].Argument[0..1].Parameter[1]",
558+
"@google-cloud/spanner;Transaction;@google-cloud/spanner;Database;Member[getTransaction].ReturnValue.Awaited",
559+
"@google-cloud/spanner;Snapshot;@google-cloud/spanner;Database;Member[getSnapshot].Argument[0..1].Parameter[1]",
560+
"@google-cloud/spanner;Snapshot;@google-cloud/spanner;Database;Member[getSnapshot].ReturnValue.Awaited",
520561
"@google-cloud/spanner;BatchTransaction;@google-cloud/spanner;Database;Member[batchTransaction].ReturnValue",
521562
"@google-cloud/spanner;BatchTransaction;@google-cloud/spanner;Database;Member[createBatchTransaction].ReturnValue.Awaited",
522563
"@google-cloud/spanner;~SqlExecutorDirect;@google-cloud/spanner;Database;Member[run,runPartitionedUpdate,runStream]",
@@ -539,4 +580,23 @@ private module SpannerCsv {
539580
]
540581
}
541582
}
583+
584+
class SpannerSources extends ModelInput::SourceModelCsv {
585+
string spannerClass() { result = ["v1.SpannerClient", "Database", "Transaction", "Snapshot",] }
586+
587+
string resultPath() {
588+
result =
589+
[
590+
"Member[executeSql].Argument[0..].Parameter[1]",
591+
"Member[executeSql].ReturnValue.Awaited.Member[0]", "Member[run].ReturnValue.Awaited",
592+
"Member[run].Argument[0..].Parameter[1]",
593+
]
594+
}
595+
596+
override predicate row(string row) {
597+
row =
598+
"@google-cloud/spanner;" + this.spannerClass() + ";" + this.resultPath() +
599+
";database-access-result"
600+
}
601+
}
542602
}

‎javascript/ql/lib/semmle/javascript/frameworks/data/internal/Shared.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@
3131
* - Instance: the value returned by a constructor call
3232
* - Awaited: the value from a resolved promise/future-like object
3333
* - WithArity[n]: match a call with the given arity. May be a range of form `x..y` (inclusive) and/or a comma-separated list.
34-
* - Other langauge-specific tokens mentioned in `ModelsAsData.qll`.
34+
* - Other language-specific tokens mentioned in `ModelsAsData.qll`.
3535
* 4. The `input` and `output` columns specify how data enters and leaves the element selected by the
3636
* first `(package, type, path)` tuple. Both strings are `.`-separated access paths
3737
* of the same syntax as the `path` column.

‎javascript/ql/lib/semmle/javascript/heuristics/AdditionalSources.qll

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ private import semmle.javascript.security.dataflow.CommandInjectionCustomization
1414
abstract class HeuristicSource extends DataFlow::Node { }
1515

1616
/**
17-
* An access to a password, viewed a source of remote flow.
17+
* An access to a password, viewed as a source of remote flow.
1818
*/
1919
private class RemoteFlowPassword extends HeuristicSource, RemoteFlowSource {
2020
RemoteFlowPassword() { isReadFrom(this, "(?is).*(password|passwd).*") }
@@ -52,3 +52,20 @@ class RemoteServerResponse extends HeuristicSource, RemoteFlowSource {
5252

5353
override string getSourceType() { result = "a response from a remote server" }
5454
}
55+
56+
/**
57+
* A remote flow source originating from a database access.
58+
*/
59+
private class RemoteFlowSourceFromDBAccess extends RemoteFlowSource, HeuristicSource {
60+
RemoteFlowSourceFromDBAccess() {
61+
this = ModelOutput::getASourceNode("database-access-result").getAUse() or
62+
exists(DatabaseAccess dba | this = dba.getAResult())
63+
}
64+
65+
override string getSourceType() { result = "Database access" }
66+
67+
override predicate isUserControlledObject() {
68+
// NB. supported databases all might return JSON.
69+
any()
70+
}
71+
}

‎javascript/ql/test/library-tests/Security/heuristics/HeuristicSink.ql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
11
import javascript
22
private import semmle.javascript.heuristics.AdditionalSinks
33

4-
select any(HeuristicSink s)
4+
select any(HeuristicSink s | s.getFile().getBaseName() = "sinks.js")

‎javascript/ql/test/library-tests/Security/heuristics/HeuristicSource.expected

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +0,0 @@
1-
| sources.js:2:5:2:12 | password |
2-
| sources.js:3:5:3:20 | JSON.stringify() |

‎javascript/ql/test/library-tests/Security/heuristics/HeuristicSource.ql

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,13 @@
11
import javascript
22
private import semmle.javascript.heuristics.AdditionalSources
3+
import testUtilities.ConsistencyChecking
34

4-
select any(HeuristicSource s)
5+
class Taint extends TaintTracking::Configuration {
6+
Taint() { this = "Taint" }
7+
8+
override predicate isSource(DataFlow::Node node) { node instanceof HeuristicSource }
9+
10+
override predicate isSink(DataFlow::Node node) {
11+
node = any(DataFlow::CallNode call | call.getCalleeName() = "sink").getAnArgument()
12+
}
13+
}

0 commit comments

Comments
 (0)