From fb137c868a54e85bf9eed89a1dbfc948eaf32293 Mon Sep 17 00:00:00 2001 From: tiferet Date: Thu, 17 Nov 2022 17:00:33 -0800 Subject: [PATCH] `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 bce5a3172d6..2e051f1fd12 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 f03631bfdcf..5de89d30c5e 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 14821404e6d..d51e99060bd 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 302907820da..646748b29bc 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 2cc848a7ea9..5869be2c325 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 444f682304d..1b5c235107e 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 697928d74b0..98668ba01fd 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 9439fda8ab2..04724557843 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 d8de88e3454..deae494c226 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 a1d06d2881d..b77685e966c 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)) }