From 5402f047bfe4e249b7bd2159363f2077575b1a61 Mon Sep 17 00:00:00 2001 From: tiferet Date: Thu, 17 Nov 2022 11:59:36 -0800 Subject: [PATCH 1/8] Delete `CoreKnowledge`. All remaining functionality in `CoreKnowledge` is only being used in `EndpointCharacteristics`, so it can be moved there as a small set of helper predicates. --- .../EndpointCharacteristics.qll | 48 +------------------ 1 file changed, 1 insertion(+), 47 deletions(-) diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointCharacteristics.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointCharacteristics.qll index 74a5ca6256de..263685889c1e 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointCharacteristics.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointCharacteristics.qll @@ -9,6 +9,7 @@ private import semmle.javascript.security.dataflow.NosqlInjectionCustomizations private import semmle.javascript.security.dataflow.TaintedPathCustomizations private import semmle.javascript.heuristics.SyntacticHeuristics as SyntacticHeuristics private import semmle.javascript.filters.ClassifyFiles as ClassifyFiles +private import StandardEndpointFilters as StandardEndpointFilters private import semmle.javascript.security.dataflow.XxeCustomizations private import semmle.javascript.security.dataflow.RemotePropertyInjectionCustomizations private import semmle.javascript.security.dataflow.TypeConfusionThroughParameterTamperingCustomizations @@ -154,53 +155,6 @@ private predicate isKnownStepSrc(DataFlow::Node n) { DataFlow::SharedFlowStep::step(n, _, _, _) } -/** - * Holds if the data flow node is a (possibly indirect) argument of a likely external library call. - * - * This includes direct arguments of likely external library calls as well as nested object - * literals within those calls. - */ -private predicate flowsToArgumentOfLikelyExternalLibraryCall(DataFlow::Node n) { - n = getACallWithoutCallee().getAnArgument() - or - exists(DataFlow::SourceNode src | flowsToArgumentOfLikelyExternalLibraryCall(src) | - n = src.getAPropertyWrite().getRhs() - ) - or - exists(DataFlow::ArrayCreationNode arr | flowsToArgumentOfLikelyExternalLibraryCall(arr) | - n = arr.getAnElement() - ) -} - -/** - * Get calls for which we do not have the callee (i.e. the definition of the called function). This - * acts as a heuristic for identifying calls to external library functions. - */ -private DataFlow::CallNode getACallWithoutCallee() { - forall(Function callee | callee = result.getACallee() | callee.getTopLevel().isExterns()) and - not exists(DataFlow::ParameterNode param, DataFlow::FunctionNode callback | - param.flowsTo(result.getCalleeNode()) and - callback = getACallback(param, DataFlow::TypeBackTracker::end()) - ) -} - -/** - * Gets a node that flows to callback-parameter `p`. - */ -private DataFlow::SourceNode getACallback(DataFlow::ParameterNode p, DataFlow::TypeBackTracker t) { - t.start() and - result = p and - any(DataFlow::FunctionNode f).getLastParameter() = p and - exists(p.getACall()) - or - exists(DataFlow::TypeBackTracker t2 | result = getACallback(p, t2).backtrack(t2, t)) -} - -/** - * Get calls which are likely to be to external non-built-in libraries. - */ -DataFlow::CallNode getALikelyExternalLibraryCall() { result = getACallWithoutCallee() } - /* * Characteristics that are indicative of a sink. * NOTE: Initially each sink type has only one characteristic, which is that it's a sink of this type in the standard From 05a943c9b533afbe4cf71a6e09527daf17c340f7 Mon Sep 17 00:00:00 2001 From: tiferet Date: Thu, 17 Nov 2022 12:12:03 -0800 Subject: [PATCH 2/8] Delete `StandardEndpointFilters`. All remaining functionality in `StandardEndpointFilters` is only being used in `EndpointCharacteristics`, so it can be moved there as a small set of helper predicates. --- .../EndpointCharacteristics.qll | 48 ++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointCharacteristics.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointCharacteristics.qll index 263685889c1e..74a5ca6256de 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointCharacteristics.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointCharacteristics.qll @@ -9,7 +9,6 @@ private import semmle.javascript.security.dataflow.NosqlInjectionCustomizations private import semmle.javascript.security.dataflow.TaintedPathCustomizations private import semmle.javascript.heuristics.SyntacticHeuristics as SyntacticHeuristics private import semmle.javascript.filters.ClassifyFiles as ClassifyFiles -private import StandardEndpointFilters as StandardEndpointFilters private import semmle.javascript.security.dataflow.XxeCustomizations private import semmle.javascript.security.dataflow.RemotePropertyInjectionCustomizations private import semmle.javascript.security.dataflow.TypeConfusionThroughParameterTamperingCustomizations @@ -155,6 +154,53 @@ private predicate isKnownStepSrc(DataFlow::Node n) { DataFlow::SharedFlowStep::step(n, _, _, _) } +/** + * Holds if the data flow node is a (possibly indirect) argument of a likely external library call. + * + * This includes direct arguments of likely external library calls as well as nested object + * literals within those calls. + */ +private predicate flowsToArgumentOfLikelyExternalLibraryCall(DataFlow::Node n) { + n = getACallWithoutCallee().getAnArgument() + or + exists(DataFlow::SourceNode src | flowsToArgumentOfLikelyExternalLibraryCall(src) | + n = src.getAPropertyWrite().getRhs() + ) + or + exists(DataFlow::ArrayCreationNode arr | flowsToArgumentOfLikelyExternalLibraryCall(arr) | + n = arr.getAnElement() + ) +} + +/** + * Get calls for which we do not have the callee (i.e. the definition of the called function). This + * acts as a heuristic for identifying calls to external library functions. + */ +private DataFlow::CallNode getACallWithoutCallee() { + forall(Function callee | callee = result.getACallee() | callee.getTopLevel().isExterns()) and + not exists(DataFlow::ParameterNode param, DataFlow::FunctionNode callback | + param.flowsTo(result.getCalleeNode()) and + callback = getACallback(param, DataFlow::TypeBackTracker::end()) + ) +} + +/** + * Gets a node that flows to callback-parameter `p`. + */ +private DataFlow::SourceNode getACallback(DataFlow::ParameterNode p, DataFlow::TypeBackTracker t) { + t.start() and + result = p and + any(DataFlow::FunctionNode f).getLastParameter() = p and + exists(p.getACall()) + or + exists(DataFlow::TypeBackTracker t2 | result = getACallback(p, t2).backtrack(t2, t)) +} + +/** + * Get calls which are likely to be to external non-built-in libraries. + */ +DataFlow::CallNode getALikelyExternalLibraryCall() { result = getACallWithoutCallee() } + /* * Characteristics that are indicative of a sink. * NOTE: Initially each sink type has only one characteristic, which is that it's a sink of this type in the standard From 50291c7b7c1a759654d281665206710f51f1d747 Mon Sep 17 00:00:00 2001 From: tiferet Date: Thu, 17 Nov 2022 17:00:33 -0800 Subject: [PATCH 3/8] `AtmConfig` inherits from `TaintTracking::Configuration`. That way the specific configs which inherit from `AtmConfig` also inherit from `TaintTracking::Configuration`. This removes the need for two separate config classes for each query. --- .../adaptivethreatmodeling/ATMConfig.qll | 2 +- .../NosqlInjectionATM.qll | 96 +++++++++---------- .../SqlInjectionATM.qll | 24 ++--- .../adaptivethreatmodeling/TaintedPathATM.qll | 26 ++--- .../adaptivethreatmodeling/XssATM.qll | 25 ++--- .../modelbuilding/DebugResultInclusion.ql | 8 +- .../extraction/ExtractEndpointMapping.ql | 8 +- .../endpoint_large_scale/EndpointFeatures.ql | 8 +- .../FilteredTruePositives.ql | 8 +- ...ql_endpoint_filter_ignores_modeled_apis.ql | 2 +- 10 files changed, 90 insertions(+), 117 deletions(-) diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll index ed9ca5ce1c1a..4106269d7595 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll @@ -29,7 +29,7 @@ import EndpointCharacteristics as EndpointCharacteristics * `isAdditionalFlowStep` with a more generalised definition of additional edges. See * `NosqlInjectionATM.qll` for an example of doing this. */ -abstract class AtmConfig extends string { +abstract class AtmConfig extends JS::TaintTracking::Configuration { bindingset[this] AtmConfig() { any() } diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/NosqlInjectionATM.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/NosqlInjectionATM.qll index f03631bfdcff..5de89d30c5ec 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/NosqlInjectionATM.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/NosqlInjectionATM.qll @@ -1,6 +1,7 @@ /** * For internal use only. * + * A taint-tracking configuration for reasoning about NoSQL injection vulnerabilities. * Defines shared code used by the NoSQL injection boosted query. */ @@ -9,61 +10,20 @@ private import semmle.javascript.heuristics.SyntacticHeuristics private import semmle.javascript.security.dataflow.NosqlInjectionCustomizations import AdaptiveThreatModeling -class NosqlInjectionAtmConfig extends AtmConfig { - NosqlInjectionAtmConfig() { this = "NosqlInjectionATMConfig" } +class Configuration extends AtmConfig { + Configuration() { this = "NosqlInjectionATMConfig" } override predicate isKnownSource(DataFlow::Node source) { source instanceof NosqlInjection::Source or TaintedObject::isSource(source, _) } override EndpointType getASinkEndpointType() { result instanceof NosqlInjectionSinkType } -} - -/** DEPRECATED: Alias for NosqlInjectionAtmConfig */ -deprecated class NosqlInjectionATMConfig = NosqlInjectionAtmConfig; -/** Holds if src -> trg is an additional flow step in the non-boosted NoSql injection security query. */ -predicate isBaseAdditionalFlowStep( - DataFlow::Node src, DataFlow::Node trg, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl -) { - TaintedObject::step(src, trg, inlbl, outlbl) - or - // additional flow step to track taint through NoSQL query objects - inlbl = TaintedObject::label() and - outlbl = TaintedObject::label() and - exists(NoSql::Query query, DataFlow::SourceNode queryObj | - queryObj.flowsTo(query) and - queryObj.flowsTo(trg) and - src = queryObj.getAPropertyWrite().getRhs() - ) -} - -/** - * Gets a value that is (transitively) written to `query`, where `query` is a NoSQL sink. - * - * This predicate allows us to propagate data flow through property writes and array constructors - * within a query object, enabling the security query to pick up NoSQL injection vulnerabilities - * involving more complex queries. - */ -DataFlow::Node getASubexpressionWithinQuery(DataFlow::Node query) { - any(NosqlInjectionAtmConfig cfg).isEffectiveSink(query) and - exists(DataFlow::SourceNode receiver | - receiver = [getASubexpressionWithinQuery(query), query].getALocalSource() - | - result = - [receiver.getAPropertyWrite().getRhs(), receiver.(DataFlow::ArrayCreationNode).getAnElement()] - ) -} - -/** - * A taint-tracking configuration for reasoning about NoSQL injection vulnerabilities. - * - * This is largely a copy of the taint tracking configuration for the standard NoSQL injection - * query, except additional ATM sinks have been added and the additional flow step has been - * generalised to cover the sinks predicted by ATM. - */ -class Configuration extends TaintTracking::Configuration { - Configuration() { this = "NosqlInjectionATM" } + /* + * This is largely a copy of the taint tracking configuration for the standard NoSQL injection + * query, except additional ATM sinks have been added and the additional flow step has been + * generalised to cover the sinks predicted by ATM. + */ override predicate isSource(DataFlow::Node source) { source instanceof NosqlInjection::Source } @@ -75,7 +35,7 @@ class Configuration extends TaintTracking::Configuration { sink.(NosqlInjection::Sink).getAFlowLabel() = label or // Allow effective sinks to have any taint label - any(NosqlInjectionAtmConfig cfg).isEffectiveSink(sink) + isEffectiveSink(sink) } override predicate isSanitizer(DataFlow::Node node) { @@ -94,7 +54,43 @@ class Configuration extends TaintTracking::Configuration { isBaseAdditionalFlowStep(src, trg, inlbl, outlbl) or // relaxed version of previous step to track taint through unmodeled NoSQL query objects - any(NosqlInjectionAtmConfig cfg).isEffectiveSink(trg) and + isEffectiveSink(trg) and src = getASubexpressionWithinQuery(trg) } + + /** Holds if src -> trg is an additional flow step in the non-boosted NoSql injection security query. */ + private predicate isBaseAdditionalFlowStep( + DataFlow::Node src, DataFlow::Node trg, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl + ) { + TaintedObject::step(src, trg, inlbl, outlbl) + or + // additional flow step to track taint through NoSQL query objects + inlbl = TaintedObject::label() and + outlbl = TaintedObject::label() and + exists(NoSql::Query query, DataFlow::SourceNode queryObj | + queryObj.flowsTo(query) and + queryObj.flowsTo(trg) and + src = queryObj.getAPropertyWrite().getRhs() + ) + } + + /** + * Gets a value that is (transitively) written to `query`, where `query` is a NoSQL sink. + * + * This predicate allows us to propagate data flow through property writes and array constructors + * within a query object, enabling the security query to pick up NoSQL injection vulnerabilities + * involving more complex queries. + */ + private DataFlow::Node getASubexpressionWithinQuery(DataFlow::Node query) { + isEffectiveSink(query) and + exists(DataFlow::SourceNode receiver | + receiver = [getASubexpressionWithinQuery(query), query].getALocalSource() + | + result = + [ + receiver.getAPropertyWrite().getRhs(), + receiver.(DataFlow::ArrayCreationNode).getAnElement() + ] + ) + } } diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/SqlInjectionATM.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/SqlInjectionATM.qll index 14821404e6d0..d51e99060bd3 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/SqlInjectionATM.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/SqlInjectionATM.qll @@ -1,6 +1,7 @@ /** * For internal use only. * + * A taint-tracking configuration for reasoning about SQL injection vulnerabilities. * Defines shared code used by the SQL injection boosted query. */ @@ -8,30 +9,21 @@ import semmle.javascript.heuristics.SyntacticHeuristics import semmle.javascript.security.dataflow.SqlInjectionCustomizations import AdaptiveThreatModeling -class SqlInjectionAtmConfig extends AtmConfig { - SqlInjectionAtmConfig() { this = "SqlInjectionATMConfig" } +class Configuration extends AtmConfig { + Configuration() { this = "SqlInjectionATMConfig" } override predicate isKnownSource(DataFlow::Node source) { source instanceof SqlInjection::Source } override EndpointType getASinkEndpointType() { result instanceof SqlInjectionSinkType } -} - -/** DEPRECATED: Alias for SqlInjectionAtmConfig */ -deprecated class SqlInjectionATMConfig = SqlInjectionAtmConfig; - -/** - * A taint-tracking configuration for reasoning about SQL injection vulnerabilities. - * - * This is largely a copy of the taint tracking configuration for the standard SQL injection - * query, except additional sinks have been added using the sink endpoint filter. - */ -class Configuration extends TaintTracking::Configuration { - Configuration() { this = "SqlInjectionATM" } + /** + * This is largely a copy of the taint tracking configuration for the standard SQL injection + * query, except additional sinks have been added using the sink endpoint filter. + */ override predicate isSource(DataFlow::Node source) { source instanceof SqlInjection::Source } override predicate isSink(DataFlow::Node sink) { - sink instanceof SqlInjection::Sink or any(SqlInjectionAtmConfig cfg).isEffectiveSink(sink) + sink instanceof SqlInjection::Sink or isEffectiveSink(sink) } override predicate isSanitizer(DataFlow::Node node) { diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/TaintedPathATM.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/TaintedPathATM.qll index 302907820da7..646748b29bc6 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/TaintedPathATM.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/TaintedPathATM.qll @@ -1,6 +1,7 @@ /** * For internal use only. * + * A taint-tracking configuration for reasoning about path injection vulnerabilities. * Defines shared code used by the path injection boosted query. */ @@ -8,33 +9,24 @@ import semmle.javascript.heuristics.SyntacticHeuristics import semmle.javascript.security.dataflow.TaintedPathCustomizations import AdaptiveThreatModeling -class TaintedPathAtmConfig extends AtmConfig { - TaintedPathAtmConfig() { this = "TaintedPathATMConfig" } +class Configuration extends AtmConfig { + Configuration() { this = "TaintedPathATMConfig" } override predicate isKnownSource(DataFlow::Node source) { source instanceof TaintedPath::Source } override EndpointType getASinkEndpointType() { result instanceof TaintedPathSinkType } -} - -/** DEPRECATED: Alias for TaintedPathAtmConfig */ -deprecated class TaintedPathATMConfig = TaintedPathAtmConfig; - -/** - * A taint-tracking configuration for reasoning about path injection vulnerabilities. - * - * This is largely a copy of the taint tracking configuration for the standard path injection - * query, except additional ATM sinks have been added to the `isSink` predicate. - */ -class Configuration extends TaintTracking::Configuration { - Configuration() { this = "TaintedPathATM" } + /** + * This is largely a copy of the taint tracking configuration for the standard path injection + * query, except additional ATM sinks have been added to the `isSink` predicate. + */ override predicate isSource(DataFlow::Node source) { source instanceof TaintedPath::Source } override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) { label = sink.(TaintedPath::Sink).getAFlowLabel() or // Allow effective sinks to have any taint label - any(TaintedPathAtmConfig cfg).isEffectiveSink(sink) + isEffectiveSink(sink) } override predicate isSanitizer(DataFlow::Node node) { node instanceof TaintedPath::Sanitizer } @@ -60,7 +52,7 @@ class Configuration extends TaintTracking::Configuration { * of barrier guards, we port the barrier guards for the boosted query from the standard library to * sanitizer guards here. */ -class BarrierGuardNodeAsSanitizerGuardNode extends TaintTracking::LabeledSanitizerGuardNode { +private class BarrierGuardNodeAsSanitizerGuardNode extends TaintTracking::LabeledSanitizerGuardNode { BarrierGuardNodeAsSanitizerGuardNode() { this instanceof TaintedPath::BarrierGuardNode } override predicate sanitizes(boolean outcome, Expr e) { diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll index 2cc848a7ea9f..5869be2c3251 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll @@ -8,31 +8,24 @@ private import semmle.javascript.heuristics.SyntacticHeuristics private import semmle.javascript.security.dataflow.DomBasedXssCustomizations import AdaptiveThreatModeling -class DomBasedXssAtmConfig extends AtmConfig { - DomBasedXssAtmConfig() { this = "DomBasedXssATMConfig" } +class Configuration extends AtmConfig { + Configuration() { this = "DomBasedXssATMConfig" } override predicate isKnownSource(DataFlow::Node source) { source instanceof DomBasedXss::Source } override EndpointType getASinkEndpointType() { result instanceof XssSinkType } -} - -/** DEPRECATED: Alias for DomBasedXssAtmConfig */ -deprecated class DomBasedXssATMConfig = DomBasedXssAtmConfig; - -/** - * A taint-tracking configuration for reasoning about XSS vulnerabilities. - * - * This is largely a copy of the taint tracking configuration for the standard XSSThroughDom query, - * except additional ATM sinks have been added to the `isSink` predicate. - */ -class Configuration extends TaintTracking::Configuration { - Configuration() { this = "DomBasedXssATMConfiguration" } + /** + * A taint-tracking configuration for reasoning about XSS vulnerabilities. + * + * This is largely a copy of the taint tracking configuration for the standard XSSThroughDom query, + * except additional ATM sinks have been added to the `isSink` predicate. + */ override predicate isSource(DataFlow::Node source) { source instanceof DomBasedXss::Source } override predicate isSink(DataFlow::Node sink) { sink instanceof DomBasedXss::Sink or - any(DomBasedXssAtmConfig cfg).isEffectiveSink(sink) + isEffectiveSink(sink) } override predicate isSanitizer(DataFlow::Node node) { diff --git a/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/DebugResultInclusion.ql b/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/DebugResultInclusion.ql index 444f682304da..1b5c235107e9 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/DebugResultInclusion.ql +++ b/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/DebugResultInclusion.ql @@ -19,16 +19,16 @@ private import experimental.adaptivethreatmodeling.XssATM as XssAtm string getAReasonSinkExcluded(DataFlow::Node sinkCandidate, Query query) { query instanceof NosqlInjectionQuery and - result = any(NosqlInjectionAtm::NosqlInjectionAtmConfig cfg).getAReasonSinkExcluded(sinkCandidate) + result = any(NosqlInjectionAtm::Configuration cfg).getAReasonSinkExcluded(sinkCandidate) or query instanceof SqlInjectionQuery and - result = any(SqlInjectionAtm::SqlInjectionAtmConfig cfg).getAReasonSinkExcluded(sinkCandidate) + result = any(SqlInjectionAtm::Configuration cfg).getAReasonSinkExcluded(sinkCandidate) or query instanceof TaintedPathQuery and - result = any(TaintedPathAtm::TaintedPathAtmConfig cfg).getAReasonSinkExcluded(sinkCandidate) + result = any(TaintedPathAtm::Configuration cfg).getAReasonSinkExcluded(sinkCandidate) or query instanceof XssQuery and - result = any(XssAtm::DomBasedXssAtmConfig cfg).getAReasonSinkExcluded(sinkCandidate) + result = any(XssAtm::Configuration cfg).getAReasonSinkExcluded(sinkCandidate) } pragma[inline] diff --git a/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointMapping.ql b/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointMapping.ql index 697928d74b03..98668ba01fd7 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointMapping.ql +++ b/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointMapping.ql @@ -14,15 +14,15 @@ from string queryName, AtmConfig c, EndpointType e where ( queryName = "SqlInjection" and - c instanceof SqlInjectionAtm::SqlInjectionAtmConfig + c instanceof SqlInjectionAtm::Configuration or queryName = "NosqlInjection" and - c instanceof NosqlInjectionAtm::NosqlInjectionAtmConfig + c instanceof NosqlInjectionAtm::Configuration or queryName = "TaintedPath" and - c instanceof TaintedPathAtm::TaintedPathAtmConfig + c instanceof TaintedPathAtm::Configuration or - queryName = "Xss" and c instanceof XssAtm::DomBasedXssAtmConfig + queryName = "Xss" and c instanceof XssAtm::Configuration ) and e = c.getASinkEndpointType() select queryName, e.getEncoding() as label diff --git a/javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/EndpointFeatures.ql b/javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/EndpointFeatures.ql index 9439fda8ab2b..047245578433 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/EndpointFeatures.ql +++ b/javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/EndpointFeatures.ql @@ -17,10 +17,10 @@ private import experimental.adaptivethreatmodeling.EndpointCharacteristics as En query predicate tokenFeatures(DataFlow::Node endpoint, string featureName, string featureValue) { ( - not exists(any(NosqlInjectionAtm::NosqlInjectionAtmConfig cfg).getAReasonSinkExcluded(endpoint)) or - not exists(any(SqlInjectionAtm::SqlInjectionAtmConfig cfg).getAReasonSinkExcluded(endpoint)) or - not exists(any(TaintedPathAtm::TaintedPathAtmConfig cfg).getAReasonSinkExcluded(endpoint)) or - not exists(any(XssAtm::DomBasedXssAtmConfig cfg).getAReasonSinkExcluded(endpoint)) or + not exists(any(NosqlInjectionAtm::Configuration cfg).getAReasonSinkExcluded(endpoint)) or + not exists(any(SqlInjectionAtm::Configuration cfg).getAReasonSinkExcluded(endpoint)) or + not exists(any(TaintedPathAtm::Configuration cfg).getAReasonSinkExcluded(endpoint)) or + not exists(any(XssAtm::Configuration cfg).getAReasonSinkExcluded(endpoint)) or any(EndpointCharacteristics::IsArgumentToModeledFunctionCharacteristic characteristic) .getEndpoints(endpoint) ) and diff --git a/javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/FilteredTruePositives.ql b/javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/FilteredTruePositives.ql index d8de88e3454c..deae494c2265 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/FilteredTruePositives.ql +++ b/javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/FilteredTruePositives.ql @@ -23,24 +23,24 @@ import experimental.adaptivethreatmodeling.XssATM as XssAtm query predicate nosqlFilteredTruePositives(DataFlow::Node endpoint, string reason) { endpoint instanceof NosqlInjection::Sink and - reason = any(NosqlInjectionAtm::NosqlInjectionAtmConfig cfg).getAReasonSinkExcluded(endpoint) and + reason = any(NosqlInjectionAtm::Configuration cfg).getAReasonSinkExcluded(endpoint) and not reason = ["argument to modeled function", "modeled sink", "modeled database access"] } query predicate sqlFilteredTruePositives(DataFlow::Node endpoint, string reason) { endpoint instanceof SqlInjection::Sink and - reason = any(SqlInjectionAtm::SqlInjectionAtmConfig cfg).getAReasonSinkExcluded(endpoint) and + reason = any(SqlInjectionAtm::Configuration cfg).getAReasonSinkExcluded(endpoint) and reason != "argument to modeled function" } query predicate taintedPathFilteredTruePositives(DataFlow::Node endpoint, string reason) { endpoint instanceof TaintedPath::Sink and - reason = any(TaintedPathAtm::TaintedPathAtmConfig cfg).getAReasonSinkExcluded(endpoint) and + reason = any(TaintedPathAtm::Configuration cfg).getAReasonSinkExcluded(endpoint) and reason != "argument to modeled function" } query predicate xssFilteredTruePositives(DataFlow::Node endpoint, string reason) { endpoint instanceof DomBasedXss::Sink and - reason = any(XssAtm::DomBasedXssAtmConfig cfg).getAReasonSinkExcluded(endpoint) and + reason = any(XssAtm::Configuration cfg).getAReasonSinkExcluded(endpoint) and reason != "argument to modeled function" } diff --git a/javascript/ql/experimental/adaptivethreatmodeling/test/modeled_apis/nosql_endpoint_filter_ignores_modeled_apis.ql b/javascript/ql/experimental/adaptivethreatmodeling/test/modeled_apis/nosql_endpoint_filter_ignores_modeled_apis.ql index a1d06d2881d0..b77685e966c6 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/test/modeled_apis/nosql_endpoint_filter_ignores_modeled_apis.ql +++ b/javascript/ql/experimental/adaptivethreatmodeling/test/modeled_apis/nosql_endpoint_filter_ignores_modeled_apis.ql @@ -2,5 +2,5 @@ import javascript import experimental.adaptivethreatmodeling.NosqlInjectionATM as NosqlInjectionAtm query predicate effectiveSinks(DataFlow::Node node) { - not exists(any(NosqlInjectionAtm::NosqlInjectionAtmConfig cfg).getAReasonSinkExcluded(node)) + not exists(any(NosqlInjectionAtm::Configuration cfg).getAReasonSinkExcluded(node)) } From cd24ec88d6e57b00e0ba1e5e9110fbd451274ae1 Mon Sep 17 00:00:00 2001 From: tiferet Date: Thu, 17 Nov 2022 17:16:37 -0800 Subject: [PATCH 4/8] Move the definition of `isSource` to the base class: A long as we're not boosting sources, `isSource` is identical to `isKnownSource`. --- .../lib/experimental/adaptivethreatmodeling/ATMConfig.qll | 6 ++++++ .../adaptivethreatmodeling/NosqlInjectionATM.qll | 2 -- .../experimental/adaptivethreatmodeling/SqlInjectionATM.qll | 3 +-- .../experimental/adaptivethreatmodeling/TaintedPathATM.qll | 3 +-- .../lib/experimental/adaptivethreatmodeling/XssATM.qll | 6 ++---- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll index 4106269d7595..17ec9cf55cd0 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll @@ -33,6 +33,12 @@ abstract class AtmConfig extends JS::TaintTracking::Configuration { bindingset[this] AtmConfig() { any() } + /** + * Holds if `source` is a relevant taint source. When sources are not boosted, `isSource` is equivalent to + * `isKnownSource` (i.e there are no "effective" sources to be classified by an ML model). + */ + override predicate isSource(JS::DataFlow::Node source) { this.isKnownSource(source) } + /** * EXPERIMENTAL. This API may change in the future. * diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/NosqlInjectionATM.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/NosqlInjectionATM.qll index 5de89d30c5ec..6724b5794b8b 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/NosqlInjectionATM.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/NosqlInjectionATM.qll @@ -25,8 +25,6 @@ class Configuration extends AtmConfig { * generalised to cover the sinks predicted by ATM. */ - override predicate isSource(DataFlow::Node source) { source instanceof NosqlInjection::Source } - override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) { TaintedObject::isSource(source, label) } diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/SqlInjectionATM.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/SqlInjectionATM.qll index d51e99060bd3..cfca6bdebce2 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/SqlInjectionATM.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/SqlInjectionATM.qll @@ -16,11 +16,10 @@ class Configuration extends AtmConfig { override EndpointType getASinkEndpointType() { result instanceof SqlInjectionSinkType } - /** + /* * This is largely a copy of the taint tracking configuration for the standard SQL injection * query, except additional sinks have been added using the sink endpoint filter. */ - override predicate isSource(DataFlow::Node source) { source instanceof SqlInjection::Source } override predicate isSink(DataFlow::Node sink) { sink instanceof SqlInjection::Sink or isEffectiveSink(sink) diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/TaintedPathATM.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/TaintedPathATM.qll index 646748b29bc6..a48eadc2532b 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/TaintedPathATM.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/TaintedPathATM.qll @@ -16,11 +16,10 @@ class Configuration extends AtmConfig { override EndpointType getASinkEndpointType() { result instanceof TaintedPathSinkType } - /** + /* * This is largely a copy of the taint tracking configuration for the standard path injection * query, except additional ATM sinks have been added to the `isSink` predicate. */ - override predicate isSource(DataFlow::Node source) { source instanceof TaintedPath::Source } override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) { label = sink.(TaintedPath::Sink).getAFlowLabel() diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll index 5869be2c3251..cd3240e8cfcd 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll @@ -1,6 +1,7 @@ /** * For internal use only. * + * A taint-tracking configuration for reasoning about XSS vulnerabilities. * Defines shared code used by the XSS boosted query. */ @@ -15,13 +16,10 @@ class Configuration extends AtmConfig { override EndpointType getASinkEndpointType() { result instanceof XssSinkType } - /** - * A taint-tracking configuration for reasoning about XSS vulnerabilities. - * + /* * This is largely a copy of the taint tracking configuration for the standard XSSThroughDom query, * except additional ATM sinks have been added to the `isSink` predicate. */ - override predicate isSource(DataFlow::Node source) { source instanceof DomBasedXss::Source } override predicate isSink(DataFlow::Node sink) { sink instanceof DomBasedXss::Sink or From a710b723d12f98aa6ef4de7cf9edf48d1239ae9a Mon Sep 17 00:00:00 2001 From: tiferet Date: Thu, 17 Nov 2022 17:57:57 -0800 Subject: [PATCH 5/8] Move the definition of `isSink` to the base class: Holds if `sink` is a known taint sink or an "effective" sink. --- .../lib/experimental/adaptivethreatmodeling/ATMConfig.qll | 7 +++++++ .../adaptivethreatmodeling/SqlInjectionATM.qll | 4 ---- .../lib/experimental/adaptivethreatmodeling/XssATM.qll | 5 ----- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll index 17ec9cf55cd0..a2a629115175 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll @@ -39,6 +39,13 @@ abstract class AtmConfig extends JS::TaintTracking::Configuration { */ override predicate isSource(JS::DataFlow::Node source) { this.isKnownSource(source) } + /** + * Holds if `sink` is a known taint sink or an "effective" sink (a candidate to be classified by an ML model). + */ + override predicate isSink(JS::DataFlow::Node sink) { + this.isKnownSink(sink) or this.isEffectiveSink(sink) + } + /** * EXPERIMENTAL. This API may change in the future. * diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/SqlInjectionATM.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/SqlInjectionATM.qll index cfca6bdebce2..3dd9b595327e 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/SqlInjectionATM.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/SqlInjectionATM.qll @@ -21,10 +21,6 @@ class Configuration extends AtmConfig { * query, except additional sinks have been added using the sink endpoint filter. */ - override predicate isSink(DataFlow::Node sink) { - sink instanceof SqlInjection::Sink or isEffectiveSink(sink) - } - override predicate isSanitizer(DataFlow::Node node) { super.isSanitizer(node) or node instanceof SqlInjection::Sanitizer diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll index cd3240e8cfcd..43d8375b8a51 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll @@ -21,11 +21,6 @@ class Configuration extends AtmConfig { * except additional ATM sinks have been added to the `isSink` predicate. */ - override predicate isSink(DataFlow::Node sink) { - sink instanceof DomBasedXss::Sink or - isEffectiveSink(sink) - } - override predicate isSanitizer(DataFlow::Node node) { super.isSanitizer(node) or node instanceof DomBasedXss::Sanitizer From 75cd7a9ebce3692248fd486811ba188c13c840f4 Mon Sep 17 00:00:00 2001 From: tiferet Date: Fri, 18 Nov 2022 11:47:58 -0800 Subject: [PATCH 6/8] Remove code duplication in query .ql files: Define the query for finding ATM alerts in the base class `AtmConfig`, and call it from each query's .ql file. --- .../adaptivethreatmodeling/ATMConfig.qll | 12 ++++++++++++ .../adaptivethreatmodeling/src/NosqlInjectionATM.ql | 7 ++----- .../adaptivethreatmodeling/src/SqlInjectionATM.ql | 7 ++----- .../adaptivethreatmodeling/src/TaintedPathATM.ql | 7 ++----- .../adaptivethreatmodeling/src/XssATM.ql | 7 ++----- 5 files changed, 20 insertions(+), 20 deletions(-) diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll index a2a629115175..d28cef259c32 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll @@ -7,6 +7,7 @@ private import javascript as JS import EndpointTypes import EndpointCharacteristics as EndpointCharacteristics +import AdaptiveThreatModeling::ATM::ResultsInfo as AtmResultsInfo /** * EXPERIMENTAL. This API may change in the future. @@ -140,6 +141,17 @@ abstract class AtmConfig extends JS::TaintTracking::Configuration { * A cut-off value of 1 produces all alerts including those that are likely false-positives. */ float getScoreCutoff() { result = 0.0 } + + /** + * Holds if there's an ATM alert (a flow path from `source` to `sink` with ML-determined likelihood `score`) according + * to this ML-boosted configuration, whereas the unboosted base query is unlikely to report an alert for this source + * and sink. + */ + predicate getAlerts(JS::DataFlow::PathNode source, JS::DataFlow::PathNode sink, float score) { + this.hasFlowPath(source, sink) and + not AtmResultsInfo::isFlowLikelyInBaseQuery(source.getNode(), sink.getNode()) and + score = AtmResultsInfo::getScoreForFlow(source.getNode(), sink.getNode()) + } } /** DEPRECATED: Alias for AtmConfig */ diff --git a/javascript/ql/experimental/adaptivethreatmodeling/src/NosqlInjectionATM.ql b/javascript/ql/experimental/adaptivethreatmodeling/src/NosqlInjectionATM.ql index e35653fb96ac..8ec315c3d9c4 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/src/NosqlInjectionATM.ql +++ b/javascript/ql/experimental/adaptivethreatmodeling/src/NosqlInjectionATM.ql @@ -17,11 +17,8 @@ import ATM::ResultsInfo import DataFlow::PathGraph import experimental.adaptivethreatmodeling.NosqlInjectionATM -from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, float score -where - cfg.hasFlowPath(source, sink) and - not isFlowLikelyInBaseQuery(source.getNode(), sink.getNode()) and - score = getScoreForFlow(source.getNode(), sink.getNode()) +from AtmConfig cfg, DataFlow::PathNode source, DataFlow::PathNode sink, float score +where cfg.getAlerts(source, sink, score) select sink.getNode(), source, sink, "(Experimental) This may be a database query that depends on $@. Identified using machine learning.", source.getNode(), "a user-provided value", score diff --git a/javascript/ql/experimental/adaptivethreatmodeling/src/SqlInjectionATM.ql b/javascript/ql/experimental/adaptivethreatmodeling/src/SqlInjectionATM.ql index b58dd9f46094..169215657070 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/src/SqlInjectionATM.ql +++ b/javascript/ql/experimental/adaptivethreatmodeling/src/SqlInjectionATM.ql @@ -17,11 +17,8 @@ import experimental.adaptivethreatmodeling.SqlInjectionATM import ATM::ResultsInfo import DataFlow::PathGraph -from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, float score -where - cfg.hasFlowPath(source, sink) and - not isFlowLikelyInBaseQuery(source.getNode(), sink.getNode()) and - score = getScoreForFlow(source.getNode(), sink.getNode()) +from AtmConfig cfg, DataFlow::PathNode source, DataFlow::PathNode sink, float score +where cfg.getAlerts(source, sink, score) select sink.getNode(), source, sink, "(Experimental) This may be a database query that depends on $@. Identified using machine learning.", source.getNode(), "a user-provided value", score diff --git a/javascript/ql/experimental/adaptivethreatmodeling/src/TaintedPathATM.ql b/javascript/ql/experimental/adaptivethreatmodeling/src/TaintedPathATM.ql index 7e637687d75d..e4e4db0bf321 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/src/TaintedPathATM.ql +++ b/javascript/ql/experimental/adaptivethreatmodeling/src/TaintedPathATM.ql @@ -21,11 +21,8 @@ import ATM::ResultsInfo import DataFlow::PathGraph import experimental.adaptivethreatmodeling.TaintedPathATM -from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, float score -where - cfg.hasFlowPath(source, sink) and - not isFlowLikelyInBaseQuery(source.getNode(), sink.getNode()) and - score = getScoreForFlow(source.getNode(), sink.getNode()) +from AtmConfig cfg, DataFlow::PathNode source, DataFlow::PathNode sink, float score +where cfg.getAlerts(source, sink, score) select sink.getNode(), source, sink, "(Experimental) This may be a path that depends on $@. Identified using machine learning.", source.getNode(), "a user-provided value", score diff --git a/javascript/ql/experimental/adaptivethreatmodeling/src/XssATM.ql b/javascript/ql/experimental/adaptivethreatmodeling/src/XssATM.ql index d0e98c1cd54e..deac86922ab6 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/src/XssATM.ql +++ b/javascript/ql/experimental/adaptivethreatmodeling/src/XssATM.ql @@ -18,11 +18,8 @@ import ATM::ResultsInfo import DataFlow::PathGraph import experimental.adaptivethreatmodeling.XssATM -from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, float score -where - cfg.hasFlowPath(source, sink) and - not isFlowLikelyInBaseQuery(source.getNode(), sink.getNode()) and - score = getScoreForFlow(source.getNode(), sink.getNode()) +from AtmConfig cfg, DataFlow::PathNode source, DataFlow::PathNode sink, float score +where cfg.getAlerts(source, sink, score) select sink.getNode(), source, sink, "(Experimental) This may be a cross-site scripting vulnerability due to $@. Identified using machine learning.", source.getNode(), "a user-provided value", score From 6f807e9d43b94c5b0cd183766ca8b6e98d949371 Mon Sep 17 00:00:00 2001 From: tiferet Date: Tue, 29 Nov 2022 13:02:06 -0800 Subject: [PATCH 7/8] Doc suggestion from code review --- .../lib/experimental/adaptivethreatmodeling/ATMConfig.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll index d28cef259c32..9f8c066f4244 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll @@ -144,8 +144,8 @@ abstract class AtmConfig extends JS::TaintTracking::Configuration { /** * Holds if there's an ATM alert (a flow path from `source` to `sink` with ML-determined likelihood `score`) according - * to this ML-boosted configuration, whereas the unboosted base query is unlikely to report an alert for this source - * and sink. + * to this ML-boosted configuration, whereas the unboosted base query does not contain this source and sink + * combination. */ predicate getAlerts(JS::DataFlow::PathNode source, JS::DataFlow::PathNode sink, float score) { this.hasFlowPath(source, sink) and From c5184d37e7c8b3cfafaf5dcafab850589315bfac Mon Sep 17 00:00:00 2001 From: tiferet Date: Tue, 29 Nov 2022 15:46:05 -0800 Subject: [PATCH 8/8] Suggestion from code review: Name the query configuration e.g. `NosqlInjectionATMConfig` rather than `Configuration`. --- .../adaptivethreatmodeling/NosqlInjectionATM.qll | 4 ++-- .../adaptivethreatmodeling/SqlInjectionATM.qll | 4 ++-- .../adaptivethreatmodeling/TaintedPathATM.qll | 4 ++-- .../lib/experimental/adaptivethreatmodeling/XssATM.qll | 4 ++-- .../modelbuilding/DebugResultInclusion.ql | 8 ++++---- .../extraction/ExtractEndpointDataTraining.qll | 9 +++++---- .../modelbuilding/extraction/ExtractEndpointMapping.ql | 8 ++++---- .../test/endpoint_large_scale/EndpointFeatures.ql | 8 ++++---- .../test/endpoint_large_scale/FilteredTruePositives.ql | 8 ++++---- .../nosql_endpoint_filter_ignores_modeled_apis.ql | 2 +- 10 files changed, 30 insertions(+), 29 deletions(-) diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/NosqlInjectionATM.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/NosqlInjectionATM.qll index 6724b5794b8b..e6d602280a47 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/NosqlInjectionATM.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/NosqlInjectionATM.qll @@ -10,8 +10,8 @@ private import semmle.javascript.heuristics.SyntacticHeuristics private import semmle.javascript.security.dataflow.NosqlInjectionCustomizations import AdaptiveThreatModeling -class Configuration extends AtmConfig { - Configuration() { this = "NosqlInjectionATMConfig" } +class NosqlInjectionAtmConfig extends AtmConfig { + NosqlInjectionAtmConfig() { this = "NosqlInjectionAtmConfig" } override predicate isKnownSource(DataFlow::Node source) { source instanceof NosqlInjection::Source or TaintedObject::isSource(source, _) diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/SqlInjectionATM.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/SqlInjectionATM.qll index 3dd9b595327e..917e79f401e4 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/SqlInjectionATM.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/SqlInjectionATM.qll @@ -9,8 +9,8 @@ import semmle.javascript.heuristics.SyntacticHeuristics import semmle.javascript.security.dataflow.SqlInjectionCustomizations import AdaptiveThreatModeling -class Configuration extends AtmConfig { - Configuration() { this = "SqlInjectionATMConfig" } +class SqlInjectionAtmConfig extends AtmConfig { + SqlInjectionAtmConfig() { this = "SqlInjectionAtmConfig" } override predicate isKnownSource(DataFlow::Node source) { source instanceof SqlInjection::Source } diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/TaintedPathATM.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/TaintedPathATM.qll index a48eadc2532b..55b295c72ba0 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/TaintedPathATM.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/TaintedPathATM.qll @@ -9,8 +9,8 @@ import semmle.javascript.heuristics.SyntacticHeuristics import semmle.javascript.security.dataflow.TaintedPathCustomizations import AdaptiveThreatModeling -class Configuration extends AtmConfig { - Configuration() { this = "TaintedPathATMConfig" } +class TaintedPathAtmConfig extends AtmConfig { + TaintedPathAtmConfig() { this = "TaintedPathAtmConfig" } override predicate isKnownSource(DataFlow::Node source) { source instanceof TaintedPath::Source } diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll index 43d8375b8a51..d28b669bf498 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll @@ -9,8 +9,8 @@ private import semmle.javascript.heuristics.SyntacticHeuristics private import semmle.javascript.security.dataflow.DomBasedXssCustomizations import AdaptiveThreatModeling -class Configuration extends AtmConfig { - Configuration() { this = "DomBasedXssATMConfig" } +class DomBasedXssAtmConfig extends AtmConfig { + DomBasedXssAtmConfig() { this = "DomBasedXssAtmConfig" } override predicate isKnownSource(DataFlow::Node source) { source instanceof DomBasedXss::Source } diff --git a/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/DebugResultInclusion.ql b/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/DebugResultInclusion.ql index 1b5c235107e9..444f682304da 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/DebugResultInclusion.ql +++ b/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/DebugResultInclusion.ql @@ -19,16 +19,16 @@ private import experimental.adaptivethreatmodeling.XssATM as XssAtm string getAReasonSinkExcluded(DataFlow::Node sinkCandidate, Query query) { query instanceof NosqlInjectionQuery and - result = any(NosqlInjectionAtm::Configuration cfg).getAReasonSinkExcluded(sinkCandidate) + result = any(NosqlInjectionAtm::NosqlInjectionAtmConfig cfg).getAReasonSinkExcluded(sinkCandidate) or query instanceof SqlInjectionQuery and - result = any(SqlInjectionAtm::Configuration cfg).getAReasonSinkExcluded(sinkCandidate) + result = any(SqlInjectionAtm::SqlInjectionAtmConfig cfg).getAReasonSinkExcluded(sinkCandidate) or query instanceof TaintedPathQuery and - result = any(TaintedPathAtm::Configuration cfg).getAReasonSinkExcluded(sinkCandidate) + result = any(TaintedPathAtm::TaintedPathAtmConfig cfg).getAReasonSinkExcluded(sinkCandidate) or query instanceof XssQuery and - result = any(XssAtm::Configuration cfg).getAReasonSinkExcluded(sinkCandidate) + result = any(XssAtm::DomBasedXssAtmConfig cfg).getAReasonSinkExcluded(sinkCandidate) } pragma[inline] diff --git a/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointDataTraining.qll b/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointDataTraining.qll index 73628c744cfe..325809e085fd 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointDataTraining.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointDataTraining.qll @@ -206,13 +206,14 @@ query predicate reformattedTrainingEndpoints( * TODO: Delete this once we are no longer surfacing `hasFlowFromSource`. */ DataFlow::Configuration getDataFlowCfg(Query query) { - query instanceof NosqlInjectionQuery and result instanceof NosqlInjectionAtm::Configuration + query instanceof NosqlInjectionQuery and + result instanceof NosqlInjectionAtm::NosqlInjectionAtmConfig or - query instanceof SqlInjectionQuery and result instanceof SqlInjectionAtm::Configuration + query instanceof SqlInjectionQuery and result instanceof SqlInjectionAtm::SqlInjectionAtmConfig or - query instanceof TaintedPathQuery and result instanceof TaintedPathAtm::Configuration + query instanceof TaintedPathQuery and result instanceof TaintedPathAtm::TaintedPathAtmConfig or - query instanceof XssQuery and result instanceof XssAtm::Configuration + query instanceof XssQuery and result instanceof XssAtm::DomBasedXssAtmConfig } // TODO: Delete this once we are no longer surfacing `hasFlowFromSource`. diff --git a/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointMapping.ql b/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointMapping.ql index 98668ba01fd7..697928d74b03 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointMapping.ql +++ b/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointMapping.ql @@ -14,15 +14,15 @@ from string queryName, AtmConfig c, EndpointType e where ( queryName = "SqlInjection" and - c instanceof SqlInjectionAtm::Configuration + c instanceof SqlInjectionAtm::SqlInjectionAtmConfig or queryName = "NosqlInjection" and - c instanceof NosqlInjectionAtm::Configuration + c instanceof NosqlInjectionAtm::NosqlInjectionAtmConfig or queryName = "TaintedPath" and - c instanceof TaintedPathAtm::Configuration + c instanceof TaintedPathAtm::TaintedPathAtmConfig or - queryName = "Xss" and c instanceof XssAtm::Configuration + queryName = "Xss" and c instanceof XssAtm::DomBasedXssAtmConfig ) and e = c.getASinkEndpointType() select queryName, e.getEncoding() as label diff --git a/javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/EndpointFeatures.ql b/javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/EndpointFeatures.ql index 047245578433..9439fda8ab2b 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/EndpointFeatures.ql +++ b/javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/EndpointFeatures.ql @@ -17,10 +17,10 @@ private import experimental.adaptivethreatmodeling.EndpointCharacteristics as En query predicate tokenFeatures(DataFlow::Node endpoint, string featureName, string featureValue) { ( - not exists(any(NosqlInjectionAtm::Configuration cfg).getAReasonSinkExcluded(endpoint)) or - not exists(any(SqlInjectionAtm::Configuration cfg).getAReasonSinkExcluded(endpoint)) or - not exists(any(TaintedPathAtm::Configuration cfg).getAReasonSinkExcluded(endpoint)) or - not exists(any(XssAtm::Configuration cfg).getAReasonSinkExcluded(endpoint)) or + not exists(any(NosqlInjectionAtm::NosqlInjectionAtmConfig cfg).getAReasonSinkExcluded(endpoint)) or + not exists(any(SqlInjectionAtm::SqlInjectionAtmConfig cfg).getAReasonSinkExcluded(endpoint)) or + not exists(any(TaintedPathAtm::TaintedPathAtmConfig cfg).getAReasonSinkExcluded(endpoint)) or + not exists(any(XssAtm::DomBasedXssAtmConfig cfg).getAReasonSinkExcluded(endpoint)) or any(EndpointCharacteristics::IsArgumentToModeledFunctionCharacteristic characteristic) .getEndpoints(endpoint) ) and diff --git a/javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/FilteredTruePositives.ql b/javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/FilteredTruePositives.ql index deae494c2265..d8de88e3454c 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/FilteredTruePositives.ql +++ b/javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/FilteredTruePositives.ql @@ -23,24 +23,24 @@ import experimental.adaptivethreatmodeling.XssATM as XssAtm query predicate nosqlFilteredTruePositives(DataFlow::Node endpoint, string reason) { endpoint instanceof NosqlInjection::Sink and - reason = any(NosqlInjectionAtm::Configuration cfg).getAReasonSinkExcluded(endpoint) and + reason = any(NosqlInjectionAtm::NosqlInjectionAtmConfig cfg).getAReasonSinkExcluded(endpoint) and not reason = ["argument to modeled function", "modeled sink", "modeled database access"] } query predicate sqlFilteredTruePositives(DataFlow::Node endpoint, string reason) { endpoint instanceof SqlInjection::Sink and - reason = any(SqlInjectionAtm::Configuration cfg).getAReasonSinkExcluded(endpoint) and + reason = any(SqlInjectionAtm::SqlInjectionAtmConfig cfg).getAReasonSinkExcluded(endpoint) and reason != "argument to modeled function" } query predicate taintedPathFilteredTruePositives(DataFlow::Node endpoint, string reason) { endpoint instanceof TaintedPath::Sink and - reason = any(TaintedPathAtm::Configuration cfg).getAReasonSinkExcluded(endpoint) and + reason = any(TaintedPathAtm::TaintedPathAtmConfig cfg).getAReasonSinkExcluded(endpoint) and reason != "argument to modeled function" } query predicate xssFilteredTruePositives(DataFlow::Node endpoint, string reason) { endpoint instanceof DomBasedXss::Sink and - reason = any(XssAtm::Configuration cfg).getAReasonSinkExcluded(endpoint) and + reason = any(XssAtm::DomBasedXssAtmConfig cfg).getAReasonSinkExcluded(endpoint) and reason != "argument to modeled function" } diff --git a/javascript/ql/experimental/adaptivethreatmodeling/test/modeled_apis/nosql_endpoint_filter_ignores_modeled_apis.ql b/javascript/ql/experimental/adaptivethreatmodeling/test/modeled_apis/nosql_endpoint_filter_ignores_modeled_apis.ql index b77685e966c6..a1d06d2881d0 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/test/modeled_apis/nosql_endpoint_filter_ignores_modeled_apis.ql +++ b/javascript/ql/experimental/adaptivethreatmodeling/test/modeled_apis/nosql_endpoint_filter_ignores_modeled_apis.ql @@ -2,5 +2,5 @@ import javascript import experimental.adaptivethreatmodeling.NosqlInjectionATM as NosqlInjectionAtm query predicate effectiveSinks(DataFlow::Node node) { - not exists(any(NosqlInjectionAtm::Configuration cfg).getAReasonSinkExcluded(node)) + not exists(any(NosqlInjectionAtm::NosqlInjectionAtmConfig cfg).getAReasonSinkExcluded(node)) }