Skip to content

Commit f08f07e

Browse files
henrymercerhenrymercer
committed
JS: Improve handling of heuristic sinks in endpoint filters
Previously heuristic sinks were always included, to avoid us filtering them out due to not being an argument to an external library call. In this commit we move the argument to an external library call filtering to the query-specific endpoint filters. This lets us filter out heuristic sinks if they match one of the other endpoint filters, reducing FPs.
1 parent 322e394 commit f08f07e

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)