Merge pull request #4244 from erik-krogh/badJQueryJoin

JS: Fix Bad join orders in UnsafeJQueryPlugin

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 |
+    exists(ReachableBasicBlock srcBB, ReachableBasicBlock sinkBB |
-      read = sanitizer.getPropRead() and
+      aliasPropertyPresenceStepHelper(src, sink, srcBB, sinkBB) and
-      sink = AccessPath::getAnAliasedSourceNode(read) and
+      srcBB.strictlyDominates(sinkBB)
-      read.getBasicBlock().(ReachableBasicBlock).strictlyDominates(sink.getBasicBlock())
+    )
+  }
+
+  /**
+   * 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()
     )
   }
 }

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 = getALikelyHTMLWrite(finalRead.getPropertyName()) and
-      defaultDef.getRhs().mayHaveStringValue(default) and
-      default.regexpMatch("\\s*<.*") 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
+    )
+  }
 }