JS: Port UnsafeShellCommandConstruction

javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll

@@ -46,6 +46,21 @@ module UnsafeShellCommandConstruction {
    */
   abstract class Sanitizer extends DataFlow::Node { }
 
+  /**
+   * A barrier guard for shell command constructed from library input vulnerabilities.
+   */
+  abstract class BarrierGuard extends DataFlow::Node {
+    /**
+     * Holds if this node acts as a barrier for data flow, blocking further flow from `e` if `this` evaluates to `outcome`.
+     */
+    predicate blocksExpr(boolean outcome, Expr e) { none() }
+  }
+
+  /** A subclass of `BarrierGuard` that is used for backward compatibility with the old data flow library. */
+  abstract class BarrierGuardLegacy extends BarrierGuard, TaintTracking::SanitizerGuardNode {
+    override predicate sanitizes(boolean outcome, Expr e) { this.blocksExpr(outcome, e) }
+  }
+
   /**
    * A parameter of an exported function, seen as a source for shell command constructed from library input.
    */
@@ -270,13 +285,13 @@ module UnsafeShellCommandConstruction {
    * A sanitizer that sanitizers paths that exist in the file-system.
    * For example: `x` is sanitized in `fs.existsSync(x)` or `fs.existsSync(x + "/suffix/path")`.
    */
-  class PathExistsSanitizerGuard extends TaintTracking::SanitizerGuardNode, DataFlow::CallNode {
+  class PathExistsSanitizerGuard extends BarrierGuardLegacy, DataFlow::CallNode {
     PathExistsSanitizerGuard() {
       this = DataFlow::moduleMember("path", "exist").getACall() or
       this = DataFlow::moduleMember("fs", "existsSync").getACall()
     }
 
-    override predicate sanitizes(boolean outcome, Expr e) {
+    override predicate blocksExpr(boolean outcome, Expr e) {
       outcome = true and
       (
         e = this.getArgument(0).asExpr() or
@@ -289,26 +304,26 @@ module UnsafeShellCommandConstruction {
    * A guard of the form `typeof x === "<T>"`, where `<T>` is  "number", or "boolean",
    * which sanitizes `x` in its "then" branch.
    */
-  class TypeOfSanitizer extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode {
+  class TypeOfSanitizer extends BarrierGuardLegacy, DataFlow::ValueNode {
     Expr x;
     override EqualityTest astNode;
 
     TypeOfSanitizer() { TaintTracking::isTypeofGuard(astNode, x, ["number", "boolean"]) }
 
-    override predicate sanitizes(boolean outcome, Expr e) {
+    override predicate blocksExpr(boolean outcome, Expr e) {
       outcome = astNode.getPolarity() and
       e = x
     }
   }
 
   /** A guard that checks whether `x` is a number. */
-  class NumberGuard extends TaintTracking::SanitizerGuardNode instanceof DataFlow::CallNode {
+  class NumberGuard extends BarrierGuardLegacy instanceof DataFlow::CallNode {
     Expr x;
     boolean polarity;
 
     NumberGuard() { TaintTracking::isNumberGuard(this, x, polarity) }
 
-    override predicate sanitizes(boolean outcome, Expr e) { e = x and outcome = polarity }
+    override predicate blocksExpr(boolean outcome, Expr e) { e = x and outcome = polarity }
   }
 
   private import semmle.javascript.dataflow.internal.AccessPaths

javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionQuery.qll

@@ -13,7 +13,38 @@ import UnsafeShellCommandConstructionCustomizations::UnsafeShellCommandConstruct
 /**
  * A taint-tracking configuration for reasoning about shell command constructed from library input vulnerabilities.
  */
-class Configuration extends TaintTracking::Configuration {
+module UnsafeShellCommandConstructionConfig implements DataFlow::ConfigSig {
+  // TODO: we get a FP in the test case due to SanitizingRegExpTest not being able to generate a barrier edge
+  //       for an edge into a phi node.
+  predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+  predicate isBarrier(DataFlow::Node node) {
+    node instanceof Sanitizer or
+    node = DataFlow::MakeBarrierGuard<BarrierGuard>::getABarrierNode() or
+    node = TaintTracking::AdHocWhitelistCheckSanitizer::getABarrierNode()
+  }
+
+  predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
+    none()
+    // TODO: localFieldStep is too expensive with dataflow2
+    // DataFlow::localFieldStep(pred, succ)
+  }
+
+  DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext }
+}
+
+/**
+ * Taint-tracking for reasoning about shell command constructed from library input vulnerabilities.
+ */
+module UnsafeShellCommandConstructionFlow =
+  TaintTracking::Global<UnsafeShellCommandConstructionConfig>;
+
+/**
+ * DEPRECATED. Use the `UnsafeShellCommandConstructionFlow` module instead.
+ */
+deprecated class Configuration extends TaintTracking::Configuration {
   Configuration() { this = "UnsafeShellCommandConstruction" }
 
   override predicate isSource(DataFlow::Node source) { source instanceof Source }

javascript/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.ql

@@ -15,10 +15,12 @@
 
 import javascript
 import semmle.javascript.security.dataflow.UnsafeShellCommandConstructionQuery
-import DataFlow::PathGraph
+import UnsafeShellCommandConstructionFlow::PathGraph
 
-from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, Sink sinkNode
-where cfg.hasFlowPath(source, sink) and sinkNode = sink.getNode()
+from
+  UnsafeShellCommandConstructionFlow::PathNode source,
+  UnsafeShellCommandConstructionFlow::PathNode sink, Sink sinkNode
+where UnsafeShellCommandConstructionFlow::flowPath(source, sink) and sinkNode = sink.getNode()
 select sinkNode.getAlertLocation(), source, sink,
   "This " + sinkNode.getSinkType() + " which depends on $@ is later used in a $@.",
   source.getNode(), "library input", sinkNode.getCommandExecution(), "shell command"