From 68584e549e53e68fc5ac9de86e502c1d5b293426 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 11 Jul 2023 12:57:33 +0200 Subject: [PATCH] JS: Replace isOptionallySanitizedEdge with a node --- .../adaptivethreatmodeling/XssATM.qll | 7 ++----- .../adaptivethreatmodeling/XssThroughDomATM.qll | 7 ++----- .../dataflow/DomBasedXssCustomizations.qll | 17 ++++++++++++++++- .../security/dataflow/DomBasedXssQuery.qll | 10 +++------- .../dataflow/UnsafeHtmlConstructionQuery.qll | 6 ++---- .../security/dataflow/XssThroughDomQuery.qll | 7 ++----- 6 files changed, 27 insertions(+), 27 deletions(-) diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll index 5daac270292..eff490b638d 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll @@ -23,7 +23,8 @@ class DomBasedXssAtmConfig extends AtmConfig { override predicate isSanitizer(DataFlow::Node node) { super.isSanitizer(node) or - node instanceof DomBasedXss::Sanitizer + node instanceof DomBasedXss::Sanitizer or + DomBasedXss::isOptionallySanitizedNode(node) } override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) { @@ -31,10 +32,6 @@ class DomBasedXssAtmConfig extends AtmConfig { guard instanceof QuoteGuard or guard instanceof ContainsHtmlGuard } - - override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) { - DomBasedXss::isOptionallySanitizedEdge(pred, succ) - } } private import semmle.javascript.security.dataflow.Xss::Shared as Shared diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssThroughDomATM.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssThroughDomATM.qll index e188da15a7e..0eeba5d23ad 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssThroughDomATM.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssThroughDomATM.qll @@ -23,7 +23,8 @@ class XssThroughDomAtmConfig extends AtmConfig { override predicate isSanitizer(DataFlow::Node node) { super.isSanitizer(node) or - node instanceof DomBasedXss::Sanitizer + node instanceof DomBasedXss::Sanitizer or + DomBasedXss::isOptionallySanitizedNode(node) } override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) { @@ -34,10 +35,6 @@ class XssThroughDomAtmConfig extends AtmConfig { guard instanceof QuoteGuard or guard instanceof ContainsHtmlGuard } - - override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) { - DomBasedXss::isOptionallySanitizedEdge(pred, succ) - } } /** diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssCustomizations.qll index f372f62cbd7..f62fff2d886 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssCustomizations.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssCustomizations.qll @@ -290,9 +290,13 @@ module DomBasedXss { private class HtmlSanitizerAsSanitizer extends Sanitizer instanceof HtmlSanitizerCall { } /** + * DEPRECATED. Use `isOptionallySanitizedNode` instead. + * * Holds if there exists two dataflow edges to `succ`, where one edges is sanitized, and the other edge starts with `pred`. */ - predicate isOptionallySanitizedEdge(DataFlow::Node pred, DataFlow::Node succ) { + deprecated predicate isOptionallySanitizedEdge = isOptionallySanitizedEdgeInternal/2; + + private predicate isOptionallySanitizedEdgeInternal(DataFlow::Node pred, DataFlow::Node succ) { exists(HtmlSanitizerCall sanitizer | // sanitized = sanitize ? sanitizer(source) : source; exists(ConditionalExpr branch, Variable var, VarAccess access | @@ -319,6 +323,17 @@ module DomBasedXss { ) } + /** + * Holds if `node` should be considered optionally sanitized as it occurs in a branch + * that controls whether sanitization is enabled. + * + * For example, in `sanitized = sanitize ? sanitizer(source) : source`, the right-hand `source` expression + * is considered an optionally sanitized node. + */ + predicate isOptionallySanitizedNode(DataFlow::Node node) { + isOptionallySanitizedEdgeInternal(_, node) + } + /** A source of remote user input, considered as a flow source for DOM-based XSS. */ class RemoteFlowSourceAsSource extends Source instanceof RemoteFlowSource { } diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll index fa64b7300b0..cc4fc0c47ea 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll @@ -86,13 +86,9 @@ class Configuration extends TaintTracking::Configuration { // we assume that `.join()` calls have a prefix, and thus block the prefix label. node = any(DataFlow::MethodCallNode call | call.getMethodName() = "join") and lbl = prefixLabel() - } - - override predicate isSanitizerEdge( - DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel label - ) { - isOptionallySanitizedEdge(pred, succ) and - label = [DataFlow::FlowLabel::taint(), prefixLabel(), TaintedUrlSuffix::label()] + or + isOptionallySanitizedNode(node) and + lbl = [DataFlow::FlowLabel::taint(), prefixLabel(), TaintedUrlSuffix::label()] } override predicate isAdditionalFlowStep( diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll index fd0c3ee76aa..ed655e60412 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll @@ -31,10 +31,8 @@ class Configration extends TaintTracking::Configuration { node instanceof DomBasedXss::Sanitizer or node instanceof UnsafeJQueryPlugin::Sanitizer - } - - override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) { - DomBasedXss::isOptionallySanitizedEdge(pred, succ) + or + DomBasedXss::isOptionallySanitizedNode(node) } // override to require that there is a path without unmatched return steps diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll index 70c1f65d5da..cc75078fd67 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll @@ -20,7 +20,8 @@ class Configuration extends TaintTracking::Configuration { override predicate isSanitizer(DataFlow::Node node) { super.isSanitizer(node) or - node instanceof DomBasedXss::Sanitizer + node instanceof DomBasedXss::Sanitizer or + DomBasedXss::isOptionallySanitizedNode(node) } override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) { @@ -32,10 +33,6 @@ class Configuration extends TaintTracking::Configuration { guard instanceof ContainsHtmlGuard } - override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) { - DomBasedXss::isOptionallySanitizedEdge(pred, succ) - } - override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { succ = DataFlow::globalVarRef("URL").getAMemberCall("createObjectURL") and pred = succ.(DataFlow::InvokeNode).getArgument(0)