Merge pull request #691 from asger-semmle/sendfile-root

JS: Recognize 'root' option in Express res.sendFile

Author: Esben Sparre Andreasen

change-notes/1.20/analysis-javascript.md

@@ -29,5 +29,6 @@
 | Uncontrolled data used in network request  | More results                 | This rule now recognizes host values that are vulnerable to injection. |
 | Unused parameter                           | Fewer false-positive results | This rule no longer flags parameters with leading underscore. |
 | Unused variable, import, function or class | Fewer false-positive results | This rule now flags fewer variables that are implictly used by JSX elements, and no longer flags variables with leading underscore. |
+| Uncontrolled data used in path expression | Fewer false-positive results | This rule now recognizes the Express `root` option, which prevents path traversal. |
 
 ## Changes to QL libraries

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

@@ -28,7 +28,15 @@ abstract class FileSystemAccess extends DataFlow::Node {
 
   /** Gets an argument to this file system access that is interpreted as a path. */
   abstract DataFlow::Node getAPathArgument();
-  
+
+  /**
+   * Gets an argument to this file system access that is interpreted as a root folder
+   * in which the path arguments are constrained.
+   *
+   * In other words, if a root argument is provided, the underlying file access does its own
+   * sanitization to prevent the path arguments from traversing outside the root folder.
+   */
+  DataFlow::Node getRootPathArgument() { none() }
 }
 
 /**

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

@@ -866,6 +866,10 @@ module Express {
     override DataFlow::Node getAPathArgument() {
       result = DataFlow::valueNode(astNode.getArgument(0))
     }
+
+    override DataFlow::Node getRootPathArgument() {
+      result = this.(DataFlow::CallNode).getOptionArgument(1, "root")
+    }
   }
 
   /**

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

@@ -71,7 +71,11 @@ module TaintedPath {
    */
   class FsPathSink extends Sink, DataFlow::ValueNode {
     FsPathSink() {
-      this = any(FileSystemAccess fs).getAPathArgument()
+      exists (FileSystemAccess fs |
+        this = fs.getAPathArgument() and
+        not exists(fs.getRootPathArgument())
+        or
+        this = fs.getRootPathArgument())
     }
   }
 

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

@@ -87,6 +87,7 @@ nodes
 | tainted-require.js:7:19:7:37 | req.param("module") |
 | tainted-sendFile.js:7:16:7:33 | req.param("gimme") |
 | tainted-sendFile.js:9:16:9:33 | req.param("gimme") |
+| tainted-sendFile.js:17:43:17:58 | req.param("dir") |
 | views.js:1:43:1:55 | req.params[0] |
 edges
 | TaintedPath-es6.js:7:7:7:44 | path | TaintedPath-es6.js:10:41:10:44 | path |
@@ -194,4 +195,5 @@ edges
 | tainted-require.js:7:19:7:37 | req.param("module") | tainted-require.js:7:19:7:37 | req.param("module") | tainted-require.js:7:19:7:37 | req.param("module") | This path depends on $@. | tainted-require.js:7:19:7:37 | req.param("module") | a user-provided value |
 | tainted-sendFile.js:7:16:7:33 | req.param("gimme") | tainted-sendFile.js:7:16:7:33 | req.param("gimme") | tainted-sendFile.js:7:16:7:33 | req.param("gimme") | This path depends on $@. | tainted-sendFile.js:7:16:7:33 | req.param("gimme") | a user-provided value |
 | tainted-sendFile.js:9:16:9:33 | req.param("gimme") | tainted-sendFile.js:9:16:9:33 | req.param("gimme") | tainted-sendFile.js:9:16:9:33 | req.param("gimme") | This path depends on $@. | tainted-sendFile.js:9:16:9:33 | req.param("gimme") | a user-provided value |
+| tainted-sendFile.js:17:43:17:58 | req.param("dir") | tainted-sendFile.js:17:43:17:58 | req.param("dir") | tainted-sendFile.js:17:43:17:58 | req.param("dir") | This path depends on $@. | tainted-sendFile.js:17:43:17:58 | req.param("dir") | a user-provided value |
 | views.js:1:43:1:55 | req.params[0] | views.js:1:43:1:55 | req.params[0] | views.js:1:43:1:55 | req.params[0] | This path depends on $@. | views.js:1:43:1:55 | req.params[0] | a user-provided value |

javascript/ql/test/query-tests/Security/CWE-022/tainted-sendFile.js

@@ -7,4 +7,12 @@ app.get('/some/path', function(req, res) {
   res.sendFile(req.param("gimme"));
   // BAD: same as above
   res.sendfile(req.param("gimme"));
+
+  // GOOD: ensures files cannot be accessed outside of root folder
+  res.sendFile(req.param("gimme"), { root: process.cwd() });
+  // GOOD: ensures files cannot be accessed outside of root folder
+  res.sendfile(req.param("gimme"), { root: process.cwd() });
+
+  // BAD: doesn't help if user controls root
+  res.sendFile(req.param("file"), { root: req.param("dir") });
 });