share code between unsafe-shell and unsafe-html queries

Author: erik-krogh

javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll

@@ -36,7 +36,7 @@ module UnsafeHtmlConstruction {
     }
 
     override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
-      classFieldStep(pred, succ)
+      DataFlow::localFieldStep(pred, succ)
     }
   }
 }

javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll

@@ -166,19 +166,6 @@ module UnsafeHtmlConstruction {
     override string describe() { result = "Markdown rendering" }
   }
 
-  /**
-   * A taint step from the write of a field in a constructor to a read of the same field in an instance method.
-   */
-  predicate classFieldStep(DataFlow::Node pred, DataFlow::Node succ) {
-    // flow-step from a property written in the constructor to a use in an instance method.
-    // "simulates" client usage of a class, and regains some flow-steps lost by `requireMatchedReturn` below.
-    exists(DataFlow::ClassNode clazz, string prop |
-      DataFlow::thisNode(clazz.getConstructor().getFunction()).getAPropertyWrite(prop).getRhs() =
-        pred and
-      DataFlow::thisNode(clazz.getAnInstanceMethod().getFunction()).getAPropertyRead(prop) = succ
-    )
-  }
-
   /**
    *  Holds if there is a path without unmatched return steps from `source` to `sink`.
    */

javascript/ql/src/semmle/javascript/security/dataflow/UnsafeShellCommandConstruction.qll

@@ -14,6 +14,7 @@ import javascript
  */
 module UnsafeShellCommandConstruction {
   import UnsafeShellCommandConstructionCustomizations::UnsafeShellCommandConstruction
+  import UnsafeHtmlConstructionCustomizations
 
   /**
    * A taint-tracking configuration for reasoning about shell command constructed from library input vulnerabilities.
@@ -35,21 +36,11 @@ module UnsafeShellCommandConstruction {
     // override to require that there is a path without unmatched return steps
     override predicate hasFlowPath(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) {
       super.hasFlowPath(source, sink) and
-      exists(DataFlow::MidPathNode mid |
-        source.getASuccessor*() = mid and
-        sink = mid.getASuccessor() and
-        mid.getPathSummary().hasReturn() = false
-      )
+      UnsafeHtmlConstruction::requireMatchedReturn(source, sink)
     }
 
     override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
-      // flow-step from a property written in the constructor to a use in an instance method.
-      // "simulates" client usage of a class, and regains some flow-steps lost by `hasFlowPath` above.
-      exists(DataFlow::ClassNode clz, string name |
-        pred =
-          DataFlow::thisNode(clz.getConstructor().getFunction()).getAPropertyWrite(name).getRhs() and
-        succ = DataFlow::thisNode(clz.getInstanceMethod(_).getFunction()).getAPropertyRead(name)
-      )
+      DataFlow::localFieldStep(pred, succ)
     }
   }
 }