Skip to content

Commit a46787e

Browse files
henrymercerhenrymercer
authored
Merge pull request github#7351 from github/henrymercer/js-atm-heuristic-sinks-improvements
JS: Improve handling of heuristic sinks in endpoint filters
2 parents bd9b96e + f08f07e commit a46787e

File tree

5 files changed

+113
-106
lines changed

5 files changed

+113
-106
lines changed

‎javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/NosqlInjectionATM.qll

Lines changed: 59 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -20,68 +20,70 @@ module SinkEndpointFilter {
2020
* effective sink.
2121
*/
2222
string getAReasonSinkExcluded(DataFlow::Node sinkCandidate) {
23-
(
24-
result = StandardEndpointFilters::getAReasonSinkExcluded(sinkCandidate)
23+
result = StandardEndpointFilters::getAReasonSinkExcluded(sinkCandidate)
24+
or
25+
exists(DataFlow::CallNode call | sinkCandidate = call.getAnArgument() |
26+
// additional databases accesses that aren't modeled yet
27+
call.(DataFlow::MethodCallNode).getMethodName() =
28+
["create", "createCollection", "createIndexes"] and
29+
result = "matches database access call heuristic"
2530
or
26-
// Require NoSQL injection sink candidates to be direct arguments to external library calls.
27-
//
28-
// The standard endpoint filters allow sink candidates which are within object literals or
29-
// array literals, for example `req.sendFile(_, { path: ENDPOINT })`.
30-
//
31-
// However, the NoSQL injection query deals differently with these types of sinks compared to
32-
// other security queries. Other security queries such as SQL injection tend to treat
33-
// `ENDPOINT` as the ground truth sink, but the NoSQL injection query instead treats
34-
// `{ path: ENDPOINT }` as the ground truth sink and defines an additional flow step to ensure
35-
// data flows from `ENDPOINT` to the ground truth sink `{ path: ENDPOINT }`.
36-
//
37-
// Therefore for the NoSQL injection boosted query, we must explicitly ignore sink candidates
38-
// within object literals or array literals, to avoid having multiple alerts for the same
39-
// security vulnerability (one FP where the sink is `ENDPOINT` and one TP where the sink is
40-
// `{ path: ENDPOINT }`).
41-
//
42-
// We use the same reason as in the standard endpoint filters to avoid duplicate reasons for
43-
// endpoints that are neither direct nor indirect arguments to a likely external library call.
44-
not sinkCandidate = StandardEndpointFilters::getALikelyExternalLibraryCall().getAnArgument() and
45-
result = "not an argument to a likely external library call"
31+
// Remove modeled sinks
32+
CoreKnowledge::isArgumentToKnownLibrarySinkFunction(sinkCandidate) and
33+
result = "modeled sink"
4634
or
47-
exists(DataFlow::CallNode call | sinkCandidate = call.getAnArgument() |
48-
// additional databases accesses that aren't modeled yet
49-
call.(DataFlow::MethodCallNode).getMethodName() =
50-
["create", "createCollection", "createIndexes"] and
51-
result = "matches database access call heuristic"
52-
or
53-
// Remove modeled sinks
54-
CoreKnowledge::isArgumentToKnownLibrarySinkFunction(sinkCandidate) and
55-
result = "modeled sink"
56-
or
57-
// Remove common kinds of unlikely sinks
58-
CoreKnowledge::isKnownStepSrc(sinkCandidate) and
59-
result = "predecessor in a modeled flow step"
60-
or
61-
// Remove modeled database calls. Arguments to modeled calls are very likely to be modeled
62-
// as sinks if they are true positives. Therefore arguments that are not modeled as sinks
63-
// are unlikely to be true positives.
64-
call instanceof DatabaseAccess and
65-
result = "modeled database access"
66-
or
67-
// Remove calls to APIs that aren't relevant to NoSQL injection
68-
call.getReceiver().asExpr() instanceof HTTP::RequestExpr and
69-
result = "receiver is a HTTP request expression"
70-
or
71-
call.getReceiver().asExpr() instanceof HTTP::ResponseExpr and
72-
result = "receiver is a HTTP response expression"
73-
)
74-
) and
35+
// Remove common kinds of unlikely sinks
36+
CoreKnowledge::isKnownStepSrc(sinkCandidate) and
37+
result = "predecessor in a modeled flow step"
38+
or
39+
// Remove modeled database calls. Arguments to modeled calls are very likely to be modeled
40+
// as sinks if they are true positives. Therefore arguments that are not modeled as sinks
41+
// are unlikely to be true positives.
42+
call instanceof DatabaseAccess and
43+
result = "modeled database access"
44+
or
45+
// Remove calls to APIs that aren't relevant to NoSQL injection
46+
call.getReceiver().asExpr() instanceof HTTP::RequestExpr and
47+
result = "receiver is a HTTP request expression"
48+
or
49+
call.getReceiver().asExpr() instanceof HTTP::ResponseExpr and
50+
result = "receiver is a HTTP response expression"
51+
)
52+
or
53+
// Require NoSQL injection sink candidates to be (a) direct arguments to external library calls
54+
// or (b) heuristic sinks for NoSQL injection.
55+
//
56+
// ## Direct arguments to external library calls
57+
//
58+
// The `StandardEndpointFilters::flowsToArgumentOfLikelyExternalLibraryCall` endpoint filter
59+
// allows sink candidates which are within object literals or array literals, for example
60+
// `req.sendFile(_, { path: ENDPOINT })`.
61+
//
62+
// However, the NoSQL injection query deals differently with these types of sinks compared to
63+
// other security queries. Other security queries such as SQL injection tend to treat
64+
// `ENDPOINT` as the ground truth sink, but the NoSQL injection query instead treats
65+
// `{ path: ENDPOINT }` as the ground truth sink and defines an additional flow step to ensure
66+
// data flows from `ENDPOINT` to the ground truth sink `{ path: ENDPOINT }`.
67+
//
68+
// Therefore for the NoSQL injection boosted query, we must ignore sink candidates within object
69+
// literals or array literals, to avoid having multiple alerts for the same security
70+
// vulnerability (one FP where the sink is `ENDPOINT` and one TP where the sink is
71+
// `{ path: ENDPOINT }`). We accomplish this by directly testing that the sink candidate is an
72+
// argument of a likely external library call.
73+
//
74+
// ## Heuristic sinks
75+
//
76+
// We also allow heuristic sinks in addition to direct arguments to external library calls.
77+
// These are copied from the `HeuristicNosqlInjectionSink` class defined within
78+
// `codeql/javascript/ql/src/semmle/javascript/heuristics/AdditionalSinks.qll`.
79+
// We can't reuse the class because importing that file would cause us to treat these
80+
// heuristic sinks as known sinks.
81+
not sinkCandidate = StandardEndpointFilters::getALikelyExternalLibraryCall().getAnArgument() and
7582
not (
76-
// Explicitly allow the following heuristic sinks.
77-
//
78-
// These are copied from the `HeuristicNosqlInjectionSink` class defined within
79-
// `codeql/javascript/ql/src/semmle/javascript/heuristics/AdditionalSinks.qll`.
80-
// We can't reuse the class because importing that file would cause us to treat these
81-
// heuristic sinks as known sinks.
8283
isAssignedToOrConcatenatedWith(sinkCandidate, "(?i)(nosql|query)") or
8384
isArgTo(sinkCandidate, "(?i)(query)")
84-
)
85+
) and
86+
result = "not a direct argument to a likely external library call or a heuristic sink"
8587
}
8688
}
8789

‎javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/SqlInjectionATM.qll

Lines changed: 26 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -25,36 +25,38 @@ module SinkEndpointFilter {
2525
* effective sink.
2626
*/
2727
string getAReasonSinkExcluded(DataFlow::Node sinkCandidate) {
28-
(
29-
result = StandardEndpointFilters::getAReasonSinkExcluded(sinkCandidate)
28+
result = StandardEndpointFilters::getAReasonSinkExcluded(sinkCandidate)
29+
or
30+
exists(DataFlow::CallNode call | sinkCandidate = call.getAnArgument() |
31+
// prepared statements for SQL
32+
any(DataFlow::CallNode cn | cn.getCalleeName() = "prepare")
33+
.getAMethodCall("run")
34+
.getAnArgument() = sinkCandidate and
35+
result = "prepared SQL statement"
3036
or
31-
exists(DataFlow::CallNode call | sinkCandidate = call.getAnArgument() |
32-
// prepared statements for SQL
33-
any(DataFlow::CallNode cn | cn.getCalleeName() = "prepare")
34-
.getAMethodCall("run")
35-
.getAnArgument() = sinkCandidate and
36-
result = "prepared SQL statement"
37-
or
38-
sinkCandidate instanceof DataFlow::ArrayCreationNode and
39-
result = "array creation"
40-
or
41-
// UI is unrelated to SQL
42-
call.getCalleeName().regexpMatch("(?i).*(render|html).*") and
43-
result = "HTML / rendering"
44-
)
45-
) and
37+
sinkCandidate instanceof DataFlow::ArrayCreationNode and
38+
result = "array creation"
39+
or
40+
// UI is unrelated to SQL
41+
call.getCalleeName().regexpMatch("(?i).*(render|html).*") and
42+
result = "HTML / rendering"
43+
)
44+
or
45+
// Require SQL injection sink candidates to be (a) arguments to external library calls
46+
// (possibly indirectly), or (b) heuristic sinks.
47+
//
48+
// Heuristic sinks are copied from the `HeuristicSqlInjectionSink` class defined within
49+
// `codeql/javascript/ql/src/semmle/javascript/heuristics/AdditionalSinks.qll`.
50+
// We can't reuse the class because importing that file would cause us to treat these
51+
// heuristic sinks as known sinks.
52+
not StandardEndpointFilters::flowsToArgumentOfLikelyExternalLibraryCall(sinkCandidate) and
4653
not (
47-
// Explicitly allow the following heuristic sinks.
48-
//
49-
// These are copied from the `HeuristicSqlInjectionSink` class defined within
50-
// `codeql/javascript/ql/src/semmle/javascript/heuristics/AdditionalSinks.qll`.
51-
// We can't reuse the class because importing that file would cause us to treat these
52-
// heuristic sinks as known sinks.
5354
isAssignedToOrConcatenatedWith(sinkCandidate, "(?i)(sql|query)") or
5455
isArgTo(sinkCandidate, "(?i)(query)") or
5556
isConcatenatedWithString(sinkCandidate,
5657
"(?s).*(ALTER|COUNT|CREATE|DATABASE|DELETE|DISTINCT|DROP|FROM|GROUP|INSERT|INTO|LIMIT|ORDER|SELECT|TABLE|UPDATE|WHERE).*")
57-
)
58+
) and
59+
result = "not an argument to a likely external library call or a heuristic sink"
5860
}
5961
}
6062

‎javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/StandardEndpointFilters.qll

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,6 @@ private import CoreKnowledge as CoreKnowledge
1313

1414
/** Provides a set of reasons why a given data flow node should be excluded as a sink candidate. */
1515
string getAReasonSinkExcluded(DataFlow::Node n) {
16-
not flowsToArgumentOfLikelyExternalLibraryCall(n) and
17-
result = "not an argument to a likely external library call"
18-
or
1916
isArgumentToModeledFunction(n) and result = "argument to modeled function"
2017
or
2118
isArgumentToSinklessLibrary(n) and result = "argument to sinkless library"

‎javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/TaintedPathATM.qll

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,14 +25,17 @@ module SinkEndpointFilter {
2525
* effective sink.
2626
*/
2727
string getAReasonSinkExcluded(DataFlow::Node sinkCandidate) {
28-
result = StandardEndpointFilters::getAReasonSinkExcluded(sinkCandidate) and
28+
result = StandardEndpointFilters::getAReasonSinkExcluded(sinkCandidate)
29+
or
30+
// Require path injection sink candidates to be (a) arguments to external library calls
31+
// (possibly indirectly), or (b) heuristic sinks.
32+
//
33+
// Heuristic sinks are mostly copied from the `HeuristicTaintedPathSink` class defined within
34+
// `codeql/javascript/ql/src/semmle/javascript/heuristics/AdditionalSinks.qll`.
35+
// We can't reuse the class because importing that file would cause us to treat these
36+
// heuristic sinks as known sinks.
37+
not StandardEndpointFilters::flowsToArgumentOfLikelyExternalLibraryCall(sinkCandidate) and
2938
not (
30-
// Explicitly allow the following heuristic sinks.
31-
//
32-
// These are mostly copied from the `HeuristicTaintedPathSink` class defined within
33-
// `codeql/javascript/ql/src/semmle/javascript/heuristics/AdditionalSinks.qll`.
34-
// We can't reuse the class because importing that file would cause us to treat these
35-
// heuristic sinks as known sinks.
3639
isAssignedToOrConcatenatedWith(sinkCandidate, "(?i)(file|folder|dir|absolute)")
3740
or
3841
isArgTo(sinkCandidate, "(?i)(get|read)file")
@@ -51,7 +54,8 @@ module SinkEndpointFilter {
5154
// `isAssignedToOrConcatenatedWith` predicate call above, we also allow the noisier "path"
5255
// name.
5356
isAssignedToOrConcatenatedWith(sinkCandidate, "(?i)path")
54-
)
57+
) and
58+
result = "not a direct argument to a likely external library call or a heuristic sink"
5559
}
5660
}
5761

‎javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -24,21 +24,22 @@ module SinkEndpointFilter {
2424
* effective sink.
2525
*/
2626
string getAReasonSinkExcluded(DataFlow::Node sinkCandidate) {
27-
(
28-
result = StandardEndpointFilters::getAReasonSinkExcluded(sinkCandidate)
29-
or
30-
exists(DataFlow::CallNode call | sinkCandidate = call.getAnArgument() |
31-
call.getCalleeName() = "setState"
32-
) and
33-
result = "setState calls ought to be safe in react applications"
27+
result = StandardEndpointFilters::getAReasonSinkExcluded(sinkCandidate)
28+
or
29+
exists(DataFlow::CallNode call | sinkCandidate = call.getAnArgument() |
30+
call.getCalleeName() = "setState"
3431
) and
32+
result = "setState calls ought to be safe in react applications"
33+
or
34+
// Require XSS sink candidates to be (a) arguments to external library calls (possibly
35+
// indirectly), or (b) heuristic sinks.
36+
//
37+
// Heuristic sinks are copied from the `HeuristicDomBasedXssSink` class defined within
38+
// `codeql/javascript/ql/src/semmle/javascript/heuristics/AdditionalSinks.qll`.
39+
// We can't reuse the class because importing that file would cause us to treat these
40+
// heuristic sinks as known sinks.
41+
not StandardEndpointFilters::flowsToArgumentOfLikelyExternalLibraryCall(sinkCandidate) and
3542
not (
36-
// Explicitly allow the following heuristic sinks.
37-
//
38-
// These are copied from the `HeuristicDomBasedXssSink` class defined within
39-
// `codeql/javascript/ql/src/semmle/javascript/heuristics/AdditionalSinks.qll`.
40-
// We can't reuse the class because importing that file would cause us to treat these
41-
// heuristic sinks as known sinks.
4243
isAssignedToOrConcatenatedWith(sinkCandidate, "(?i)(html|innerhtml)")
4344
or
4445
isArgTo(sinkCandidate, "(?i)(html|render)")
@@ -54,7 +55,8 @@ module SinkEndpointFilter {
5455
pw.getPropertyName().regexpMatch("(?i).*html*") and
5556
pw.getRhs() = sinkCandidate
5657
)
57-
)
58+
) and
59+
result = "not a direct argument to a likely external library call or a heuristic sink"
5860
}
5961
}
6062

0 commit comments

Comments
 (0)