JS: Restrict sendfile sink

Author: asger-semmle

javascript/ql/src/semmle/javascript/Concepts.qll

@@ -36,6 +36,14 @@ abstract class FileSystemAccess extends DataFlow::Node {
    * sanitization to prevent the path arguments from traversing outside the root folder.
    */
   DataFlow::Node getRootPathArgument() { none() }
+
+  /**
+   * Holds if this file system access will reject paths containing path traversal
+   * segments (`../`).
+   *
+   * `argument` should refer to the relevant path argument or root path argument.
+   */
+  predicate isPathTraversalRejected(DataFlow::Node argument) { none() }
 }
 
 /**

javascript/ql/src/semmle/javascript/frameworks/Express.qll

@@ -839,6 +839,10 @@ module Express {
     override DataFlow::Node getRootPathArgument() {
       result = this.(DataFlow::CallNode).getOptionArgument(1, "root")
     }
+
+    override predicate isPathTraversalRejected(DataFlow::Node argument) {
+      argument = getAPathArgument()
+    }
   }
 
   /**

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

@@ -19,13 +19,11 @@ module TaintedPath {
     Configuration() { this = "TaintedPath" }
 
     override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
-      source instanceof Source and
-      label instanceof Label::PosixPath
+      label = source.(Source).getAFlowLabel()
     }
 
     override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
-      sink instanceof Sink and
-      label instanceof Label::PosixPath
+      label = sink.(Sink).getAFlowLabel()
     }
 
     override predicate isBarrier(DataFlow::Node node) {

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

@@ -10,12 +10,22 @@ module TaintedPath {
   /**
    * A data flow source for tainted-path vulnerabilities.
    */
-  abstract class Source extends DataFlow::Node { }
+  abstract class Source extends DataFlow::Node {
+    /** Gets a flow label denoting the type of value for which this is a source. */
+    DataFlow::FlowLabel getAFlowLabel() {
+      result instanceof Label::PosixPath
+    }
+  }
 
   /**
    * A data flow sink for tainted-path vulnerabilities.
    */
-  abstract class Sink extends DataFlow::Node { }
+  abstract class Sink extends DataFlow::Node {
+    /** Gets a flow label denoting the type of value for which this is a sink. */
+    DataFlow::FlowLabel getAFlowLabel() {
+      result instanceof Label::PosixPath
+    }
+  }
 
   /**
    * A sanitizer for tainted-path vulnerabilities.
@@ -369,17 +379,36 @@ module TaintedPath {
    * A path argument to a file system access.
    */
   class FsPathSink extends Sink, DataFlow::ValueNode {
+    FileSystemAccess fileSystemAccess;
+
     FsPathSink() {
-      exists(FileSystemAccess fs |
-        this = fs.getAPathArgument() and
-        not exists(fs.getRootPathArgument())
+      (
+        this = fileSystemAccess.getAPathArgument() and
+        not exists(fileSystemAccess.getRootPathArgument())
         or
-        this = fs.getRootPathArgument()
+        this = fileSystemAccess.getRootPathArgument()
       ) and
       not this = any(ResolvingPathCall call).getInput()
     }
   }
 
+  /**
+   * A path argument to a file system access, which disallows path traversal.
+   */
+  private class FsPathSinkWithoutPathTraversal extends FsPathSink {
+    FsPathSinkWithoutPathTraversal() {
+      fileSystemAccess.isPathTraversalRejected(this)
+    }
+
+    override DataFlow::FlowLabel getAFlowLabel() {
+      // The protection is ineffective if the ../ segments have already
+      // cancelled out against the intended root dir using path.join or similar.
+      // Only flag normalized paths, as this corresponds to the output
+      // of a normalizing call that had a malicious input.
+      result.(Label::PosixPath).isNormalized()
+    }
+  }
+
   /**
    * A path argument to the Express `res.render` method.
    */