From 9502869e3c2f38131c6549901181c97838d40361 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 14 Sep 2020 09:59:22 +0200 Subject: [PATCH 1/2] improve join-order for aliasPropertyPresenceStep --- .../security/dataflow/UnsafeJQueryPlugin.qll | 25 ++++++++++++++++--- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeJQueryPlugin.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeJQueryPlugin.qll index 3c71407b8c5..a2a7246b4a7 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeJQueryPlugin.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeJQueryPlugin.qll @@ -62,10 +62,27 @@ module UnsafeJQueryPlugin { * With this taint-step we regain that `foo.bar` is tainted, because `PropertyPresenceSanitizer` could remove it. */ private predicate aliasPropertyPresenceStep(DataFlow::Node src, DataFlow::Node sink) { - exists(PropertyPresenceSanitizer sanitizer, DataFlow::PropRead read | read = src | - read = sanitizer.getPropRead() and - sink = AccessPath::getAnAliasedSourceNode(read) and - read.getBasicBlock().(ReachableBasicBlock).strictlyDominates(sink.getBasicBlock()) + exists(ReachableBasicBlock srcBB, ReachableBasicBlock sinkBB | + aliasPropertyPresenceStepHelper(src, sink, srcBB, sinkBB) and + srcBB.strictlyDominates(sinkBB) + ) + } + + /** + * Holds if there is a taint-step from `src` to `sink`, and `srcBB` is the basicblock for `src` and `sinkBB` is the basicblock for `sink`. + * + * This predicate is outlined to get a better join-order. + */ + pragma[noinline] + private predicate aliasPropertyPresenceStepHelper( + DataFlow::PropRead src, DataFlow::Node sink, ReachableBasicBlock srcBB, + ReachableBasicBlock sinkBB + ) { + exists(PropertyPresenceSanitizer sanitizer | + src = sanitizer.getPropRead() and + sink = AccessPath::getAnAliasedSourceNode(src) and + srcBB = src.getBasicBlock() and + sinkBB = sink.getBasicBlock() ) } } From 6fb534f178c624a4476c2ef66f51ff68ae51e2e8 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 14 Sep 2020 09:59:48 +0200 Subject: [PATCH 2/2] fix catastrophic join order in UnsafeJQueryPlugin --- .../UnsafeJQueryPluginCustomizations.qll | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeJQueryPluginCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeJQueryPluginCustomizations.qll index 546a04e08d4..2e2fe77d21b 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeJQueryPluginCustomizations.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeJQueryPluginCustomizations.qll @@ -195,15 +195,25 @@ module UnsafeJQueryPlugin { */ predicate isLikelyIntentionalHtmlSink(DataFlow::Node sink) { exists( - JQuery::JQueryPluginMethod plugin, DataFlow::PropWrite defaultDef, string default, + JQuery::JQueryPluginMethod plugin, DataFlow::PropWrite defaultDef, DataFlow::PropRead finalRead | hasDefaultOption(plugin, defaultDef) and - defaultDef.getPropertyName() = finalRead.getPropertyName() and - defaultDef.getRhs().mayHaveStringValue(default) and - default.regexpMatch("\\s*<.*") and + defaultDef = getALikelyHTMLWrite(finalRead.getPropertyName()) and finalRead.flowsTo(sink) and sink.getTopLevel() = plugin.getTopLevel() ) } + + /** + * Gets a property-write that writes a HTML-like constant string to `prop`. + */ + pragma[noinline] + private DataFlow::PropWrite getALikelyHTMLWrite(string prop) { + exists(string default | + result.getRhs().mayHaveStringValue(default) and + default.regexpMatch("\\s*<.*") and + result.getPropertyName() = prop + ) + } }