hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-30 19:26:02 +02:00
Code Issues Packages Projects Releases Wiki Activity

JS: Improve handling of heuristic sinks in endpoint filters

Browse Source 
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.
This commit is contained in:
Henry Mercer
2021-12-09 15:00:54 +00:00
parent 322e39446d
commit f08f07e19e
5 changed files with 113 additions and 106 deletions
Download Patch File Download Diff File Expand all files Collapse all files

116
javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/NosqlInjectionATM.qll
View File

@@ -20,68 +20,70 @@ module SinkEndpointFilter {
* effective sink.
*/
string getAReasonSinkExcluded(DataFlow::Node sinkCandidate) {
(
result = StandardEndpointFilters::getAReasonSinkExcluded(sinkCandidate)
result = StandardEndpointFilters::getAReasonSinkExcluded(sinkCandidate)
or
exists(DataFlow::CallNode call | sinkCandidate = call.getAnArgument() |
// additional databases accesses that aren't modeled yet
call.(DataFlow::MethodCallNode).getMethodName() =
["create", "createCollection", "createIndexes"] and
result = "matches database access call heuristic"
or
// Require NoSQL injection sink candidates to be direct arguments to external library calls.
//
// The standard endpoint filters allow sink candidates which are within object literals or
// array literals, for example `req.sendFile(_, { path: ENDPOINT })`.
//
// However, the NoSQL injection query deals differently with these types of sinks compared to
// other security queries. Other security queries such as SQL injection tend to treat
// `ENDPOINT` as the ground truth sink, but the NoSQL injection query instead treats
// `{ path: ENDPOINT }` as the ground truth sink and defines an additional flow step to ensure
// data flows from `ENDPOINT` to the ground truth sink `{ path: ENDPOINT }`.
//
// Therefore for the NoSQL injection boosted query, we must explicitly ignore sink candidates
// within object literals or array literals, to avoid having multiple alerts for the same
// security vulnerability (one FP where the sink is `ENDPOINT` and one TP where the sink is
// `{ path: ENDPOINT }`).
//
// We use the same reason as in the standard endpoint filters to avoid duplicate reasons for
// endpoints that are neither direct nor indirect arguments to a likely external library call.
not sinkCandidate = StandardEndpointFilters::getALikelyExternalLibraryCall().getAnArgument() and
result = "not an argument to a likely external library call"
// Remove modeled sinks
CoreKnowledge::isArgumentToKnownLibrarySinkFunction(sinkCandidate) and
result = "modeled sink"
or
exists(DataFlow::CallNode call | sinkCandidate = call.getAnArgument() |
// additional databases accesses that aren't modeled yet
call.(DataFlow::MethodCallNode).getMethodName() =
["create", "createCollection", "createIndexes"] and
result = "matches database access call heuristic"
or
// Remove modeled sinks
CoreKnowledge::isArgumentToKnownLibrarySinkFunction(sinkCandidate) and
result = "modeled sink"
or
// Remove common kinds of unlikely sinks
CoreKnowledge::isKnownStepSrc(sinkCandidate) and
result = "predecessor in a modeled flow step"
or
// Remove modeled database calls. Arguments to modeled calls are very likely to be modeled
// as sinks if they are true positives. Therefore arguments that are not modeled as sinks
// are unlikely to be true positives.
call instanceof DatabaseAccess and
result = "modeled database access"
or
// Remove calls to APIs that aren't relevant to NoSQL injection
call.getReceiver().asExpr() instanceof HTTP::RequestExpr and
result = "receiver is a HTTP request expression"
or
call.getReceiver().asExpr() instanceof HTTP::ResponseExpr and
result = "receiver is a HTTP response expression"
)
) and
// Remove common kinds of unlikely sinks
CoreKnowledge::isKnownStepSrc(sinkCandidate) and
result = "predecessor in a modeled flow step"
or
// Remove modeled database calls. Arguments to modeled calls are very likely to be modeled
// as sinks if they are true positives. Therefore arguments that are not modeled as sinks
// are unlikely to be true positives.
call instanceof DatabaseAccess and
result = "modeled database access"
or
// Remove calls to APIs that aren't relevant to NoSQL injection
call.getReceiver().asExpr() instanceof HTTP::RequestExpr and
result = "receiver is a HTTP request expression"
or
call.getReceiver().asExpr() instanceof HTTP::ResponseExpr and
result = "receiver is a HTTP response expression"
)
or
// Require NoSQL injection sink candidates to be (a) direct arguments to external library calls
// or (b) heuristic sinks for NoSQL injection.
//
// ## Direct arguments to external library calls
//
// The `StandardEndpointFilters::flowsToArgumentOfLikelyExternalLibraryCall` endpoint filter
// allows sink candidates which are within object literals or array literals, for example
// `req.sendFile(_, { path: ENDPOINT })`.
//
// However, the NoSQL injection query deals differently with these types of sinks compared to
// other security queries. Other security queries such as SQL injection tend to treat
// `ENDPOINT` as the ground truth sink, but the NoSQL injection query instead treats
// `{ path: ENDPOINT }` as the ground truth sink and defines an additional flow step to ensure
// data flows from `ENDPOINT` to the ground truth sink `{ path: ENDPOINT }`.
//
// Therefore for the NoSQL injection boosted query, we must ignore sink candidates within object
// literals or array literals, to avoid having multiple alerts for the same security
// vulnerability (one FP where the sink is `ENDPOINT` and one TP where the sink is
// `{ path: ENDPOINT }`). We accomplish this by directly testing that the sink candidate is an
// argument of a likely external library call.
//
// ## Heuristic sinks
//
// We also allow heuristic sinks in addition to direct arguments to external library calls.
// These are copied from the `HeuristicNosqlInjectionSink` class defined within
// `codeql/javascript/ql/src/semmle/javascript/heuristics/AdditionalSinks.qll`.
// We can't reuse the class because importing that file would cause us to treat these
// heuristic sinks as known sinks.
not sinkCandidate = StandardEndpointFilters::getALikelyExternalLibraryCall().getAnArgument() and
not (
// Explicitly allow the following heuristic sinks.
//
// These are copied from the `HeuristicNosqlInjectionSink` class defined within
// `codeql/javascript/ql/src/semmle/javascript/heuristics/AdditionalSinks.qll`.
// We can't reuse the class because importing that file would cause us to treat these
// heuristic sinks as known sinks.
isAssignedToOrConcatenatedWith(sinkCandidate, "(?i)(nosql|query)") or
isArgTo(sinkCandidate, "(?i)(query)")
)
) and
result = "not a direct argument to a likely external library call or a heuristic sink"
}
}

50
javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/SqlInjectionATM.qll
View File

@@ -25,36 +25,38 @@ module SinkEndpointFilter {
* effective sink.
*/
string getAReasonSinkExcluded(DataFlow::Node sinkCandidate) {
(
result = StandardEndpointFilters::getAReasonSinkExcluded(sinkCandidate)
result = StandardEndpointFilters::getAReasonSinkExcluded(sinkCandidate)
or
exists(DataFlow::CallNode call | sinkCandidate = call.getAnArgument() |
// prepared statements for SQL
any(DataFlow::CallNode cn | cn.getCalleeName() = "prepare")
.getAMethodCall("run")
.getAnArgument() = sinkCandidate and
result = "prepared SQL statement"
or
exists(DataFlow::CallNode call | sinkCandidate = call.getAnArgument() |
// prepared statements for SQL
any(DataFlow::CallNode cn | cn.getCalleeName() = "prepare")
.getAMethodCall("run")
.getAnArgument() = sinkCandidate and
result = "prepared SQL statement"
or
sinkCandidate instanceof DataFlow::ArrayCreationNode and
result = "array creation"
or
// UI is unrelated to SQL
call.getCalleeName().regexpMatch("(?i).*(render|html).*") and
result = "HTML / rendering"
)
) and
sinkCandidate instanceof DataFlow::ArrayCreationNode and
result = "array creation"
or
// UI is unrelated to SQL
call.getCalleeName().regexpMatch("(?i).*(render|html).*") and
result = "HTML / rendering"
)
or
// Require SQL injection sink candidates to be (a) arguments to external library calls
// (possibly indirectly), or (b) heuristic sinks.
//
// Heuristic sinks are copied from the `HeuristicSqlInjectionSink` class defined within
// `codeql/javascript/ql/src/semmle/javascript/heuristics/AdditionalSinks.qll`.
// We can't reuse the class because importing that file would cause us to treat these
// heuristic sinks as known sinks.
not StandardEndpointFilters::flowsToArgumentOfLikelyExternalLibraryCall(sinkCandidate) and
not (
// Explicitly allow the following heuristic sinks.
//
// These are copied from the `HeuristicSqlInjectionSink` class defined within
// `codeql/javascript/ql/src/semmle/javascript/heuristics/AdditionalSinks.qll`.
// We can't reuse the class because importing that file would cause us to treat these
// heuristic sinks as known sinks.
isAssignedToOrConcatenatedWith(sinkCandidate, "(?i)(sql|query)") or
isArgTo(sinkCandidate, "(?i)(query)") or
isConcatenatedWithString(sinkCandidate,
"(?s).*(ALTER|COUNT|CREATE|DATABASE|DELETE|DISTINCT|DROP|FROM|GROUP|INSERT|INTO|LIMIT|ORDER|SELECT|TABLE|UPDATE|WHERE).*")
)
) and
result = "not an argument to a likely external library call or a heuristic sink"
}
}

3
javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/StandardEndpointFilters.qll
View File

@@ -13,9 +13,6 @@ private import CoreKnowledge as CoreKnowledge
/** Provides a set of reasons why a given data flow node should be excluded as a sink candidate. */
string getAReasonSinkExcluded(DataFlow::Node n) {
not flowsToArgumentOfLikelyExternalLibraryCall(n) and
result = "not an argument to a likely external library call"
or
isArgumentToModeledFunction(n) and result = "argument to modeled function"
or
isArgumentToSinklessLibrary(n) and result = "argument to sinkless library"

20
javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/TaintedPathATM.qll
View File

@@ -25,14 +25,17 @@ module SinkEndpointFilter {
* effective sink.
*/
string getAReasonSinkExcluded(DataFlow::Node sinkCandidate) {
result = StandardEndpointFilters::getAReasonSinkExcluded(sinkCandidate) and
result = StandardEndpointFilters::getAReasonSinkExcluded(sinkCandidate)
or
// Require path injection sink candidates to be (a) arguments to external library calls
// (possibly indirectly), or (b) heuristic sinks.
//
// Heuristic sinks are mostly copied from the `HeuristicTaintedPathSink` class defined within
// `codeql/javascript/ql/src/semmle/javascript/heuristics/AdditionalSinks.qll`.
// We can't reuse the class because importing that file would cause us to treat these
// heuristic sinks as known sinks.
not StandardEndpointFilters::flowsToArgumentOfLikelyExternalLibraryCall(sinkCandidate) and
not (
// Explicitly allow the following heuristic sinks.
//
// These are mostly copied from the `HeuristicTaintedPathSink` class defined within
// `codeql/javascript/ql/src/semmle/javascript/heuristics/AdditionalSinks.qll`.
// We can't reuse the class because importing that file would cause us to treat these
// heuristic sinks as known sinks.
isAssignedToOrConcatenatedWith(sinkCandidate, "(?i)(file|folder|dir|absolute)")
or
isArgTo(sinkCandidate, "(?i)(get|read)file")
@@ -51,7 +54,8 @@ module SinkEndpointFilter {
// `isAssignedToOrConcatenatedWith` predicate call above, we also allow the noisier "path"
// name.
isAssignedToOrConcatenatedWith(sinkCandidate, "(?i)path")
)
) and
result = "not a direct argument to a likely external library call or a heuristic sink"
}
}

30
javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll
View File

@@ -24,21 +24,22 @@ module SinkEndpointFilter {
* effective sink.
*/
string getAReasonSinkExcluded(DataFlow::Node sinkCandidate) {
(
result = StandardEndpointFilters::getAReasonSinkExcluded(sinkCandidate)
or
exists(DataFlow::CallNode call | sinkCandidate = call.getAnArgument() |
call.getCalleeName() = "setState"
) and
result = "setState calls ought to be safe in react applications"
result = StandardEndpointFilters::getAReasonSinkExcluded(sinkCandidate)
or
exists(DataFlow::CallNode call | sinkCandidate = call.getAnArgument() |
call.getCalleeName() = "setState"
) and
result = "setState calls ought to be safe in react applications"
or
// Require XSS sink candidates to be (a) arguments to external library calls (possibly
// indirectly), or (b) heuristic sinks.
//
// Heuristic sinks are copied from the `HeuristicDomBasedXssSink` class defined within
// `codeql/javascript/ql/src/semmle/javascript/heuristics/AdditionalSinks.qll`.
// We can't reuse the class because importing that file would cause us to treat these
// heuristic sinks as known sinks.
not StandardEndpointFilters::flowsToArgumentOfLikelyExternalLibraryCall(sinkCandidate) and
not (
// Explicitly allow the following heuristic sinks.
//
// These are copied from the `HeuristicDomBasedXssSink` class defined within
// `codeql/javascript/ql/src/semmle/javascript/heuristics/AdditionalSinks.qll`.
// We can't reuse the class because importing that file would cause us to treat these
// heuristic sinks as known sinks.
isAssignedToOrConcatenatedWith(sinkCandidate, "(?i)(html|innerhtml)")
or
isArgTo(sinkCandidate, "(?i)(html|render)")
@@ -54,7 +55,8 @@ module SinkEndpointFilter {
pw.getPropertyName().regexpMatch("(?i).*html*") and
pw.getRhs() = sinkCandidate
)
)
) and
result = "not a direct argument to a likely external library call or a heuristic sink"
}
}