JS: Port TaintedPath

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

@@ -31,7 +31,28 @@ module TaintedPath {
   /**
    * A barrier guard for tainted-path vulnerabilities.
    */
-  abstract class BarrierGuardNode extends DataFlow::LabeledBarrierGuardNode { }
+  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() }
+
+    /**
+     * Holds if this node acts as a barrier for `label`, blocking further flow from `e` if `this` evaluates to `outcome`.
+     */
+    predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) { 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) }
+
+    override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+      this.blocksExpr(outcome, e, label)
+    }
+  }
+
+  deprecated class BarrierGuardNode = BarrierGuard;
 
   module Label {
     /**
@@ -345,10 +366,10 @@ module TaintedPath {
    *
    * This is relevant for paths that are known to be normalized.
    */
-  class StartsWithDotDotSanitizer extends BarrierGuardNode instanceof StringOps::StartsWith {
+  class StartsWithDotDotSanitizer extends BarrierGuard instanceof StringOps::StartsWith {
     StartsWithDotDotSanitizer() { isDotDotSlashPrefix(super.getSubstring()) }
 
-    override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+    override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) {
       // Sanitize in the false case for:
       //   .startsWith(".")
       //   .startsWith("..")
@@ -365,12 +386,12 @@ module TaintedPath {
   /**
    * A check of the form `whitelist.includes(x)` or equivalent, which sanitizes `x` in its "then" branch.
    */
-  class MembershipTestBarrierGuard extends BarrierGuardNode {
+  class MembershipTestBarrierGuard extends BarrierGuard {
     MembershipCandidate candidate;
 
     MembershipTestBarrierGuard() { this = candidate.getTest() }
 
-    override predicate blocks(boolean outcome, Expr e) {
+    override predicate blocksExpr(boolean outcome, Expr e) {
       candidate = e.flow() and
       candidate.getTestPolarity() = outcome
     }
@@ -380,7 +401,7 @@ module TaintedPath {
    * A check of form `x.startsWith(dir)` that sanitizes normalized absolute paths, since it is then
    * known to be in a subdirectory of `dir`.
    */
-  class StartsWithDirSanitizer extends BarrierGuardNode {
+  class StartsWithDirSanitizer extends BarrierGuard {
     StringOps::StartsWith startsWith;
 
     StartsWithDirSanitizer() {
@@ -390,7 +411,7 @@ module TaintedPath {
       not startsWith.getSubstring().getStringValue() = "/"
     }
 
-    override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+    override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) {
       outcome = startsWith.getPolarity() and
       e = startsWith.getBaseString().asExpr() and
       exists(Label::PosixPath posixPath | posixPath = label |
@@ -404,7 +425,7 @@ module TaintedPath {
    * A call to `path.isAbsolute` as a sanitizer for relative paths in true branch,
    * and a sanitizer for absolute paths in the false branch.
    */
-  class IsAbsoluteSanitizer extends BarrierGuardNode {
+  class IsAbsoluteSanitizer extends BarrierGuard {
     DataFlow::Node operand;
     boolean polarity;
     boolean negatable;
@@ -425,7 +446,7 @@ module TaintedPath {
       ) // !x.startsWith("/home") does not guarantee that x is not absolute
     }
 
-    override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+    override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) {
       e = operand.asExpr() and
       exists(Label::PosixPath posixPath | posixPath = label |
         outcome = polarity and posixPath.isRelative()
@@ -440,10 +461,10 @@ module TaintedPath {
   /**
    * An expression of form `x.includes("..")` or similar.
    */
-  class ContainsDotDotSanitizer extends BarrierGuardNode instanceof StringOps::Includes {
+  class ContainsDotDotSanitizer extends BarrierGuard instanceof StringOps::Includes {
     ContainsDotDotSanitizer() { isDotDotSlashPrefix(super.getSubstring()) }
 
-    override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+    override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) {
       e = super.getBaseString().asExpr() and
       outcome = super.getPolarity().booleanNot() and
       label.(Label::PosixPath).canContainDotDotSlash() // can still be bypassed by normalized absolute path
@@ -453,10 +474,10 @@ module TaintedPath {
   /**
    * An expression of form `x.matches(/\.\./)` or similar.
    */
-  class ContainsDotDotRegExpSanitizer extends BarrierGuardNode instanceof StringOps::RegExpTest {
+  class ContainsDotDotRegExpSanitizer extends BarrierGuard instanceof StringOps::RegExpTest {
     ContainsDotDotRegExpSanitizer() { super.getRegExp().getAMatchedString() = [".", "..", "../"] }
 
-    override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+    override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) {
       e = super.getStringOperand().asExpr() and
       outcome = super.getPolarity().booleanNot() and
       label.(Label::PosixPath).canContainDotDotSlash() // can still be bypassed by normalized absolute path
@@ -484,7 +505,7 @@ module TaintedPath {
    * }
    * ```
    */
-  class RelativePathStartsWithSanitizer extends BarrierGuardNode {
+  class RelativePathStartsWithSanitizer extends BarrierGuard {
     StringOps::StartsWith startsWith;
     DataFlow::CallNode pathCall;
     string member;
@@ -506,7 +527,7 @@ module TaintedPath {
       (not member = "relative" or isDotDotSlashPrefix(startsWith.getSubstring()))
     }
 
-    override predicate blocks(boolean outcome, Expr e) {
+    override predicate blocksExpr(boolean outcome, Expr e) {
       member = "relative" and
       e = this.maybeGetPathSuffix(pathCall.getArgument(1)).asExpr() and
       outcome = startsWith.getPolarity().booleanNot()
@@ -542,7 +563,7 @@ module TaintedPath {
    * An expression of form `isInside(x, y)` or similar, where `isInside` is
    * a library check for the relation between `x` and `y`.
    */
-  class IsInsideCheckSanitizer extends BarrierGuardNode {
+  class IsInsideCheckSanitizer extends BarrierGuard {
     DataFlow::Node checked;
     boolean onlyNormalizedAbsolutePaths;
 
@@ -558,7 +579,7 @@ module TaintedPath {
       )
     }
 
-    override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+    override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) {
       (
         onlyNormalizedAbsolutePaths = true and
         label.(Label::PosixPath).isNormalized() and
@@ -750,8 +771,6 @@ module TaintedPath {
       )
     )
     or
-    TaintTracking::promiseStep(src, dst) and srclabel = dstlabel
-    or
     TaintTracking::persistentStorageStep(src, dst) and srclabel = dstlabel
     or
     exists(DataFlow::PropRead read | read = dst |

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

@@ -8,7 +8,7 @@
  */
 
 import javascript
-import TaintedPathCustomizations::TaintedPath
+private import TaintedPathCustomizations::TaintedPath
 
 // Materialize flow labels
 private class ConcretePosixPath extends Label::PosixPath {
@@ -22,7 +22,44 @@ private class ConcreteSplitPath extends Label::SplitPath {
 /**
  * A taint-tracking configuration for reasoning about tainted-path vulnerabilities.
  */
-class Configuration extends DataFlow::Configuration {
+module TaintedPathConfig implements DataFlow::StateConfigSig {
+  class FlowState = DataFlow::FlowLabel;
+
+  predicate isSource(DataFlow::Node source, DataFlow::FlowLabel state) {
+    state = source.(Source).getAFlowLabel()
+  }
+
+  predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel state) {
+    state = sink.(Sink).getAFlowLabel()
+  }
+
+  predicate isBarrier(DataFlow::Node node, DataFlow::FlowLabel label) {
+    node instanceof Sanitizer and exists(label)
+    or
+    node = DataFlow::MakeLabeledBarrierGuard<BarrierGuard>::getABarrierNode(label)
+  }
+
+  predicate isBarrier(DataFlow::Node node) {
+    node = DataFlow::MakeBarrierGuard<BarrierGuard>::getABarrierNode()
+  }
+
+  predicate isAdditionalFlowStep(
+    DataFlow::Node node1, DataFlow::FlowLabel state1, DataFlow::Node node2,
+    DataFlow::FlowLabel state2
+  ) {
+    isAdditionalTaintedPathFlowStep(node1, node2, state1, state2)
+  }
+}
+
+/**
+ * Taint-tracking for reasoning about tainted-path vulnerabilities.
+ */
+module TaintedPathFlow = DataFlow::GlobalWithState<TaintedPathConfig>;
+
+/**
+ * DEPRECATED. Use the `TaintedPathFlow` module instead.
+ */
+deprecated class Configuration extends DataFlow::Configuration {
   Configuration() { this = "TaintedPath" }
 
   override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {

javascript/ql/src/Security/CWE-022/TaintedPath.ql

@@ -17,9 +17,9 @@
 
 import javascript
 import semmle.javascript.security.dataflow.TaintedPathQuery
-import DataFlow::PathGraph
+import DataFlow::DeduplicatePathGraph<TaintedPathFlow::PathNode, TaintedPathFlow::PathGraph>
 
-from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
-where cfg.hasFlowPath(source, sink)
+from PathNode source, PathNode sink
+where TaintedPathFlow::flowPath(source.getAnOriginalPathNode(), sink.getAnOriginalPathNode())
 select sink.getNode(), source, sink, "This path depends on a $@.", source.getNode(),
   "user-provided value"

javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/Consistency.ql

@@ -1,3 +1,9 @@
 import javascript
 import semmle.javascript.security.dataflow.TaintedPathQuery
 import testUtilities.ConsistencyChecking
+
+class TaintedPathConsistency extends ConsistencyConfiguration {
+  TaintedPathConsistency() { this = "TaintedPathConsistency" }
+
+  override DataFlow::Node getAnAlert() { TaintedPathFlow::flowTo(result) }
+}

javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/other-fs-libraries.js

@@ -70,7 +70,11 @@ http.createServer(function(req, res) {
   fs.readFileSync(path); // NOT OK
   mkdirp(path); // NOT OK
   mkdirp.sync(path); // NOT OK
+  func(path);
 });
+function func(x) {
+  fs.readFileSync(x); // NOT OK
+}
 
 const fsp = require("fs/promises");
 http.createServer(function(req, res) {

javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/sharedlib-repro.js

@@ -0,0 +1,35 @@
+const fs = require('fs');
+const express = require('express');
+const app = express();
+
+app.get('/', function (req, res) {
+    getTree(req, res, { workspaceDir: '/tmp' });
+});
+
+function getTree(req, res, options) {
+    var workspaceId = req.params.workspaceId;
+    var realfileRootPath = workspaceId; // getfileRoot(workspaceId);
+    var filePath = workspaceId; // path.join(options.workspaceDir,realfileRootPath, req.params["0"]);
+    withStatsAndETag(req.params.workspaceId, function (err, stats, etag) {});
+}
+
+function getfileRoot(workspaceId) {
+    var userId = decodeUserIdFromWorkspaceId(workspaceId);
+    return path.join(userId.substring(0,2), userId, decodeWorkspaceNameFromWorkspaceId(workspaceId));
+}
+
+function withStatsAndETag(filepath, callback) {
+	fs.readFileSync(filepath); // NOT OK
+};
+
+function decodeUserIdFromWorkspaceId(workspaceId) {
+    var index = workspaceId.lastIndexOf(SEPARATOR);
+    if (index === -1) return null;
+    return workspaceId.substring(0, index);
+}
+
+function decodeWorkspaceNameFromWorkspaceId(workspaceId) {
+    var index = workspaceId.lastIndexOf(SEPARATOR);
+    if (index === -1) return null;
+    return workspaceId.substring(index + 1);
+}

javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/tainted-promise-steps.js

@@ -0,0 +1,15 @@
+var fs = require('fs'),
+    http = require('http'),
+    url = require('url');
+
+var server = http.createServer(function(req, res) {
+  let path = url.parse(req.url, true).query.path;
+  doRead(Promise.resolve(path));
+});
+
+async function doRead(pathPromise) {
+  fs.readFileSync(await pathPromise); // NOT OK
+  pathPromise.then(path => fs.readFileSync(path)); // NO TOK
+}
+
+server.listen();