Merge pull request #2202 from asger-semmle/express-sendfile

Approved by esbena

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

@@ -39,6 +39,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 upward navigation
+   * segments (`../`).
+   *
+   * `argument` should refer to the relevant path argument or root path argument.
+   */
+  predicate isUpwardNavigationRejected(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 isUpwardNavigationRejected(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 upward navigation.
+   */
+  private class FsPathSinkWithoutUpwardNavigation extends FsPathSink {
+    FsPathSinkWithoutUpwardNavigation() {
+      fileSystemAccess.isUpwardNavigationRejected(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.
    */

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

@@ -1 +1 @@
-| normalizedPaths.js:208:35:208:60 | // OK - ...  anyway | Spurious alert |
+| normalizedPaths.js:208:38:208:63 | // OK - ...  anyway | Spurious alert |

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

@@ -10,21 +10,21 @@ let app = express();
 app.get('/basic', (req, res) => {
   let path = req.query.path;
 
-  res.sendFile(path); // NOT OK
-  res.sendFile('./' + path); // NOT OK
-  res.sendFile(path + '/index.html'); // NOT OK
-  res.sendFile(pathModule.join(path, 'index.html')); // NOT OK
-  res.sendFile(pathModule.join('/home/user/www', path)); // NOT OK
+  fs.readFileSync(path); // NOT OK
+  fs.readFileSync('./' + path); // NOT OK
+  fs.readFileSync(path + '/index.html'); // NOT OK
+  fs.readFileSync(pathModule.join(path, 'index.html')); // NOT OK
+  fs.readFileSync(pathModule.join('/home/user/www', path)); // NOT OK
 });
 
 app.get('/normalize', (req, res) => {
   let path = pathModule.normalize(req.query.path);
 
-  res.sendFile(path); // NOT OK
-  res.sendFile('./' + path); // NOT OK
-  res.sendFile(path + '/index.html'); // NOT OK
-  res.sendFile(pathModule.join(path, 'index.html')); // NOT OK
-  res.sendFile(pathModule.join('/home/user/www', path)); // NOT OK
+  fs.readFileSync(path); // NOT OK
+  fs.readFileSync('./' + path); // NOT OK
+  fs.readFileSync(path + '/index.html'); // NOT OK
+  fs.readFileSync(pathModule.join(path, 'index.html')); // NOT OK
+  fs.readFileSync(pathModule.join('/home/user/www', path)); // NOT OK
 });
 
 app.get('/normalize-notAbsolute', (req, res) => {
@@ -33,21 +33,21 @@ app.get('/normalize-notAbsolute', (req, res) => {
   if (pathModule.isAbsolute(path))
     return;
    
-  res.sendFile(path); // NOT OK
+  fs.readFileSync(path); // NOT OK
 
   if (!path.startsWith("."))
-    res.sendFile(path); // OK
+    fs.readFileSync(path); // OK
   else
-    res.sendFile(path); // NOT OK - wrong polarity
+    fs.readFileSync(path); // NOT OK - wrong polarity
   
   if (!path.startsWith(".."))
-    res.sendFile(path); // OK
+    fs.readFileSync(path); // OK
   
   if (!path.startsWith("../"))
-    res.sendFile(path); // OK
+    fs.readFileSync(path); // OK
 
   if (!path.startsWith(".." + pathModule.sep))
-    res.sendFile(path); // OK
+    fs.readFileSync(path); // OK
 });
 
 app.get('/normalize-noInitialDotDot', (req, res) => {
@@ -56,16 +56,16 @@ app.get('/normalize-noInitialDotDot', (req, res) => {
   if (path.startsWith(".."))
     return;
 
-  res.sendFile(path); // NOT OK - could be absolute
+  fs.readFileSync(path); // NOT OK - could be absolute
 
-  res.sendFile("./" + path); // OK - coerced to relative
+  fs.readFileSync("./" + path); // OK - coerced to relative
 
-  res.sendFile(path + "/index.html"); // NOT OK - not coerced
+  fs.readFileSync(path + "/index.html"); // NOT OK - not coerced
 
   if (!pathModule.isAbsolute(path))
-    res.sendFile(path); // OK
+    fs.readFileSync(path); // OK
   else
-    res.sendFile(path); // NOT OK
+    fs.readFileSync(path); // NOT OK
 });
 
 app.get('/prepend-normalize', (req, res) => {
@@ -73,9 +73,9 @@ app.get('/prepend-normalize', (req, res) => {
   let path = pathModule.normalize('./' + req.query.path);
 
   if (!path.startsWith(".."))
-    res.sendFile(path); // OK
+    fs.readFileSync(path); // OK
   else
-     res.sendFile(path); // NOT OK
+     fs.readFileSync(path); // NOT OK
 });
 
 app.get('/absolute', (req, res) => {
@@ -107,53 +107,53 @@ app.get('/combined-check', (req, res) => {
   
   // Combined absoluteness and folder check in one startsWith call
   if (path.startsWith("/home/user/www"))
-    res.sendFile(path); // OK
+    fs.readFileSync(path); // OK
 
   if (path[0] !== "/" && path[0] !== ".")
-    res.sendFile(path); // OK
+    fs.readFileSync(path); // OK
 });
 
 app.get('/realpath', (req, res) => {
   let path = fs.realpathSync(req.query.path);
 
-  res.sendFile(path); // NOT OK
-  res.sendFile(pathModule.join(path, 'index.html')); // NOT OK
+  fs.readFileSync(path); // NOT OK
+  fs.readFileSync(pathModule.join(path, 'index.html')); // NOT OK
 
   if (path.startsWith("/home/user/www"))
-    res.sendFile(path); // OK - both absolute and normalized before check
+    fs.readFileSync(path); // OK - both absolute and normalized before check
     
-  res.sendFile(pathModule.join('.', path)); // OK - normalized and coerced to relative
-  res.sendFile(pathModule.join('/home/user/www', path)); // OK
+  fs.readFileSync(pathModule.join('.', path)); // OK - normalized and coerced to relative
+  fs.readFileSync(pathModule.join('/home/user/www', path)); // OK
 });
 
 app.get('/coerce-relative', (req, res) => {
   let path = pathModule.join('.', req.query.path);
 
   if (!path.startsWith('..'))
-    res.sendFile(path); // OK
+    fs.readFileSync(path); // OK
   else
-    res.sendFile(path); // NOT OK
+    fs.readFileSync(path); // NOT OK
 });
 
 app.get('/coerce-absolute', (req, res) => {
   let path = pathModule.join('/home/user/www', req.query.path);
 
   if (path.startsWith('/home/user/www'))
-    res.sendFile(path); // OK
+    fs.readFileSync(path); // OK
   else
-    res.sendFile(path); // NOT OK
+    fs.readFileSync(path); // NOT OK
 });
 
 app.get('/concat-after-normalization', (req, res) => {
   let path = 'foo/' + pathModule.normalize(req.query.path);
 
   if (!path.startsWith('..'))
-    res.sendFile(path); // NOT OK - prefixing foo/ invalidates check
+    fs.readFileSync(path); // NOT OK - prefixing foo/ invalidates check
   else
-    res.sendFile(path); // NOT OK
+    fs.readFileSync(path); // NOT OK
 
   if (!path.includes('..'))
-    res.sendFile(path); // OK
+    fs.readFileSync(path); // OK
 });
 
 app.get('/noDotDot', (req, res) => {
@@ -162,12 +162,12 @@ app.get('/noDotDot', (req, res) => {
   if (path.includes('..'))
     return;
 
-  res.sendFile(path); // NOT OK - can still be absolute
+  fs.readFileSync(path); // NOT OK - can still be absolute
 
   if (!pathModule.isAbsolute(path))
-    res.sendFile(path); // OK
+    fs.readFileSync(path); // OK
   else
-    res.sendFile(path); // NOT OK
+    fs.readFileSync(path); // NOT OK
 });
 
 app.get('/join-regression', (req, res) => {
@@ -181,53 +181,53 @@ app.get('/join-regression', (req, res) => {
   if (path.startsWith('/x')) {path;} else {path;}
   if (path.startsWith('.')) {path;} else {path;}
 
-  res.sendFile(path); // NOT OK
+  fs.readFileSync(path); // NOT OK
 
   if (pathModule.isAbsolute(path))
-    res.sendFile(path); // NOT OK
+    fs.readFileSync(path); // NOT OK
   else
-    res.sendFile(path); // NOT OK
+    fs.readFileSync(path); // NOT OK
 
   if (path.includes('..'))
-    res.sendFile(path); // NOT OK
+    fs.readFileSync(path); // NOT OK
   else
-    res.sendFile(path); // NOT OK
+    fs.readFileSync(path); // NOT OK
 
   if (!path.includes('..') && !pathModule.isAbsolute(path))
-    res.sendFile(path); // OK
+    fs.readFileSync(path); // OK
   else
-    res.sendFile(path); // NOT OK
+    fs.readFileSync(path); // NOT OK
 
   let normalizedPath = pathModule.normalize(path);
   if (normalizedPath.startsWith('/home/user/www'))
-    res.sendFile(normalizedPath); // OK
+    fs.readFileSync(normalizedPath); // OK
   else
-    res.sendFile(normalizedPath); // NOT OK
+    fs.readFileSync(normalizedPath); // NOT OK
 
   if (normalizedPath.startsWith('/home/user/www') || normalizedPath.startsWith('/home/user/public'))
-    res.sendFile(normalizedPath); // OK - but flagged anyway
+    fs.readFileSync(normalizedPath); // OK - but flagged anyway
   else
-    res.sendFile(normalizedPath); // NOT OK
+    fs.readFileSync(normalizedPath); // NOT OK
 });
 
 app.get('/decode-after-normalization', (req, res) => {
   let path = pathModule.normalize(req.query.path);
   
   if (!pathModule.isAbsolute(path) && !path.startsWith('..'))
-    res.sendFile(path); // OK
+    fs.readFileSync(path); // OK
 
   path = decodeURIComponent(path);
 
   if (!pathModule.isAbsolute(path) && !path.startsWith('..'))
-    res.sendFile(path); // NOT OK - not normalized
+    fs.readFileSync(path); // NOT OK - not normalized
 });
 
 app.get('/replace', (req, res) => {
   let path = pathModule.normalize(req.query.path).replace(/%20/g, ' ');
   if (!pathModule.isAbsolute(path)) {
-    res.sendFile(path); // NOT OK
+    fs.readFileSync(path); // NOT OK
 
     path = path.replace(/\.\./g, '');
-    res.sendFile(path); // OK
+    fs.readFileSync(path); // OK
   }
 });

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

@@ -1,8 +1,9 @@
 var express = require('express');
+let path = require('path');
 
 var app = express();
 
-app.get('/some/path', function(req, res) {
+app.get('/some/path/:x', function(req, res) {
   // BAD: sending a file based on un-sanitized query parameters
   res.sendFile(req.param("gimme"));
   // BAD: same as above
@@ -15,4 +16,13 @@ app.get('/some/path', function(req, res) {
 
   // BAD: doesn't help if user controls root
   res.sendFile(req.param("file"), { root: req.param("dir") });
+
+  let homeDir = path.resolve('.');
+  res.sendFile(homeDir + '/data/' + req.params.x); // OK: sendFile disallows ../
+  res.sendfile('data/' + req.params.x); // OK: sendfile disallows ../
+
+  res.sendFile(path.resolve('data', req.params.x)); // NOT OK
+  res.sendfile(path.join('data', req.params.x)); // NOT OK
+
+  res.sendFile(homeDir + path.join('data', req.params.x)); // kinda OK - can only escape from 'data/'
 });