From 3a146514cea911df2906d717e2dc3cc6860f874b Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Thu, 13 Feb 2020 18:44:02 +0100
Subject: [PATCH 1/6] add sanitizer for relative ".." in js/path-injection

---
 .../security/dataflow/TaintedPath.qll         |  3 +-
 .../dataflow/TaintedPathCustomizations.qll    | 43 +++++++++---
 .../CWE-022/TaintedPath/TaintedPath.expected  | 67 +++++++++++++++++++
 .../CWE-022/TaintedPath/normalizedPaths.js    | 23 +++++++
 4 files changed, 126 insertions(+), 10 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPath.qll b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPath.qll
index b46f7d508f76..c72257b47c0f 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPath.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPath.qll
@@ -35,7 +35,8 @@ module TaintedPath {
       guard instanceof StartsWithDotDotSanitizer or
       guard instanceof StartsWithDirSanitizer or
       guard instanceof IsAbsoluteSanitizer or
-      guard instanceof ContainsDotDotSanitizer
+      guard instanceof ContainsDotDotSanitizer or
+      guard instanceof RelativePathContainsDotDotGuard
     }
 
     override predicate isAdditionalFlowStep(
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
index 25bb232f8fec..4c5037b56ddc 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
@@ -12,19 +12,17 @@ module TaintedPath {
    */
   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
-    }
+    DataFlow::FlowLabel getAFlowLabel() { result instanceof Label::PosixPath }
   }
 
   /**
    * A data flow sink for tainted-path vulnerabilities.
    */
   abstract class Sink extends DataFlow::Node {
+    Sink() { not this instanceof Sanitizer }
+
     /** Gets a flow label denoting the type of value for which this is a sink. */
-    DataFlow::FlowLabel getAFlowLabel() {
-      result instanceof Label::PosixPath
-    }
+    DataFlow::FlowLabel getAFlowLabel() { result instanceof Label::PosixPath }
   }
 
   /**
@@ -355,6 +353,35 @@ module TaintedPath {
     }
   }
 
+  /**
+   * A sanitizer that recognizes the following pattern:
+   *   var relative = path.relative(webroot, pathname);
+   *   if(relative.startsWith(".." + path.sep) || relative == "..") {
+   *     // pathname is unsafe
+   *   } else {
+   *     // pathname is safe
+   *   }
+   */
+  class RelativePathContainsDotDotGuard extends DataFlow::BarrierGuardNode {
+    StringOps::StartsWith startsWith;
+    DataFlow::CallNode relativeCall;
+
+    RelativePathContainsDotDotGuard() {
+      this = startsWith and
+      relativeCall = DataFlow::moduleImport("path").getAMemberCall("relative") and
+      startsWith.getBaseString().getALocalSource() = relativeCall and
+      exists(DataFlow::Node subString | subString = startsWith.getSubstring() |
+        subString.mayHaveStringValue("..")
+        or
+        subString.(StringOps::ConcatenationRoot).getFirstLeaf().mayHaveStringValue("..")
+      )
+    }
+
+    override predicate blocks(boolean outcome, Expr e) {
+      e = relativeCall.getArgument(1).asExpr() and outcome = false
+    }
+  }
+
   /**
    * A source of remote user input, considered as a flow source for
    * tainted-path vulnerabilities.
@@ -396,9 +423,7 @@ module TaintedPath {
    * A path argument to a file system access, which disallows upward navigation.
    */
   private class FsPathSinkWithoutUpwardNavigation extends FsPathSink {
-    FsPathSinkWithoutUpwardNavigation() {
-      fileSystemAccess.isUpwardNavigationRejected(this)
-    }
+    FsPathSinkWithoutUpwardNavigation() { fileSystemAccess.isUpwardNavigationRejected(this) }
 
     override DataFlow::FlowLabel getAFlowLabel() {
       // The protection is ineffective if the ../ segments have already
diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
index 7e2aa30fe1f2..650c2ed94403 100644
--- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
@@ -1259,6 +1259,34 @@ nodes
 | normalizedPaths.js:250:21:250:24 | path |
 | normalizedPaths.js:250:21:250:24 | path |
 | normalizedPaths.js:250:21:250:24 | path |
+| normalizedPaths.js:254:7:254:47 | path |
+| normalizedPaths.js:254:7:254:47 | path |
+| normalizedPaths.js:254:7:254:47 | path |
+| normalizedPaths.js:254:7:254:47 | path |
+| normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
+| normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
+| normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
+| normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
+| normalizedPaths.js:254:33:254:46 | req.query.path |
+| normalizedPaths.js:254:33:254:46 | req.query.path |
+| normalizedPaths.js:254:33:254:46 | req.query.path |
+| normalizedPaths.js:254:33:254:46 | req.query.path |
+| normalizedPaths.js:254:33:254:46 | req.query.path |
+| normalizedPaths.js:256:19:256:22 | path |
+| normalizedPaths.js:256:19:256:22 | path |
+| normalizedPaths.js:256:19:256:22 | path |
+| normalizedPaths.js:256:19:256:22 | path |
+| normalizedPaths.js:256:19:256:22 | path |
+| normalizedPaths.js:262:21:262:24 | path |
+| normalizedPaths.js:262:21:262:24 | path |
+| normalizedPaths.js:262:21:262:24 | path |
+| normalizedPaths.js:262:21:262:24 | path |
+| normalizedPaths.js:262:21:262:24 | path |
+| normalizedPaths.js:270:21:270:24 | path |
+| normalizedPaths.js:270:21:270:24 | path |
+| normalizedPaths.js:270:21:270:24 | path |
+| normalizedPaths.js:270:21:270:24 | path |
+| normalizedPaths.js:270:21:270:24 | path |
 | 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") |
@@ -3630,6 +3658,42 @@ edges
 | normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:236:14:236:47 | pathMod ... y.path) |
 | normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:236:14:236:47 | pathMod ... y.path) |
 | normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:236:14:236:47 | pathMod ... y.path) |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:256:19:256:22 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:256:19:256:22 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:256:19:256:22 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:256:19:256:22 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:256:19:256:22 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:256:19:256:22 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:256:19:256:22 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:256:19:256:22 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:262:21:262:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:262:21:262:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:262:21:262:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:262:21:262:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:262:21:262:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:262:21:262:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:262:21:262:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:262:21:262:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
+| normalizedPaths.js:254:14:254:47 | pathMod ... y.path) | normalizedPaths.js:254:7:254:47 | path |
+| normalizedPaths.js:254:14:254:47 | pathMod ... y.path) | normalizedPaths.js:254:7:254:47 | path |
+| normalizedPaths.js:254:14:254:47 | pathMod ... y.path) | normalizedPaths.js:254:7:254:47 | path |
+| normalizedPaths.js:254:14:254:47 | pathMod ... y.path) | normalizedPaths.js:254:7:254:47 | path |
+| normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
+| normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
+| normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
+| normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
+| normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
+| normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
+| normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
+| normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
 | tainted-require.js:7:19:7:37 | req.param("module") | tainted-require.js:7:19:7:37 | req.param("module") |
 | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") |
 | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") |
@@ -4411,6 +4475,9 @@ edges
 | normalizedPaths.js:238:19:238:22 | path | normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:238:19:238:22 | path | This path depends on $@. | normalizedPaths.js:236:33:236:46 | req.query.path | a user-provided value |
 | normalizedPaths.js:245:21:245:24 | path | normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:245:21:245:24 | path | This path depends on $@. | normalizedPaths.js:236:33:236:46 | req.query.path | a user-provided value |
 | normalizedPaths.js:250:21:250:24 | path | normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:250:21:250:24 | path | This path depends on $@. | normalizedPaths.js:236:33:236:46 | req.query.path | a user-provided value |
+| normalizedPaths.js:256:19:256:22 | path | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:256:19:256:22 | path | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
+| normalizedPaths.js:262:21:262:24 | path | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:262:21:262:24 | path | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
+| normalizedPaths.js:270:21:270:24 | path | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:270:21:270:24 | path | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
 | 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:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | This path depends on $@. | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | a user-provided value |
 | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | This path depends on $@. | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | a user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js
index c0be777dd84e..121ac5e8e635 100644
--- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js
@@ -249,3 +249,26 @@ app.get('/resolve-path', (req, res) => {
   else
     fs.readFileSync(path); // NOT OK - wrong polarity
 });
+
+app.get('/relative-startswith', (req, res) => {
+  let path = pathModule.resolve(req.query.path);
+
+  fs.readFileSync(path); // NOT OK
+
+  var self = something();
+	
+  var relative = pathModule.relative(self.webroot, path);
+  if(relative.startsWith(".." + pathModule.sep) || relative == "..") {
+    fs.readFileSync(path); // NOT OK! 
+  } else {
+    fs.readFileSync(path); // OK! 
+  }
+
+  let newpath = pathModule.normalize(p);
+  var relativePath = path.relative(path.normalize(workspaceDir), newpath);
+  if (relativePath.indexOf('..' + pathModule.sep) === 0) {
+    fs.readFileSync(path); // NOT OK!
+  } else {
+	  fs.readFileSync(newpath); // OK! 
+  }
+});

From 9d610041281c7373c95b56d03aeaadae3d1c0c98 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Fri, 14 Feb 2020 12:31:12 +0100
Subject: [PATCH 2/6] remove redundant constructor on sink

---
 .../javascript/security/dataflow/TaintedPathCustomizations.qll  | 2 --
 1 file changed, 2 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
index 4c5037b56ddc..00b6790c6d0e 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
@@ -19,8 +19,6 @@ module TaintedPath {
    * A data flow sink for tainted-path vulnerabilities.
    */
   abstract class Sink extends DataFlow::Node {
-    Sink() { not this instanceof Sanitizer }
-
     /** Gets a flow label denoting the type of value for which this is a sink. */
     DataFlow::FlowLabel getAFlowLabel() { result instanceof Label::PosixPath }
   }

From d765a33b8dc9e64e72303bcb1d63a72a2881b411 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Fri, 14 Feb 2020 12:36:54 +0100
Subject: [PATCH 3/6] add support for "../" prefixes in sanitizer

---
 .../dataflow/TaintedPathCustomizations.qll         |  9 ++++++---
 .../CWE-022/TaintedPath/TaintedPath.expected       | 14 ++++++++++++++
 .../CWE-022/TaintedPath/normalizedPaths.js         | 10 +++++++++-
 3 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
index 00b6790c6d0e..38b06e06f141 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
@@ -368,10 +368,13 @@ module TaintedPath {
       this = startsWith and
       relativeCall = DataFlow::moduleImport("path").getAMemberCall("relative") and
       startsWith.getBaseString().getALocalSource() = relativeCall and
-      exists(DataFlow::Node subString | subString = startsWith.getSubstring() |
-        subString.mayHaveStringValue("..")
+      exists(DataFlow::Node subString, string prefix |
+        subString = startsWith.getSubstring() and
+        (prefix = ".." or prefix = "../")
+      |
+        subString.mayHaveStringValue(prefix)
         or
-        subString.(StringOps::ConcatenationRoot).getFirstLeaf().mayHaveStringValue("..")
+        subString.(StringOps::ConcatenationRoot).getFirstLeaf().mayHaveStringValue(prefix)
       )
     }
 
diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
index 650c2ed94403..856aeb014ce8 100644
--- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
@@ -1287,6 +1287,11 @@ nodes
 | normalizedPaths.js:270:21:270:24 | path |
 | normalizedPaths.js:270:21:270:24 | path |
 | normalizedPaths.js:270:21:270:24 | path |
+| normalizedPaths.js:278:21:278:24 | path |
+| normalizedPaths.js:278:21:278:24 | path |
+| normalizedPaths.js:278:21:278:24 | path |
+| normalizedPaths.js:278:21:278:24 | path |
+| normalizedPaths.js:278:21:278:24 | path |
 | 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") |
@@ -3682,6 +3687,14 @@ edges
 | normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
 | normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
 | normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:278:21:278:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:278:21:278:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:278:21:278:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:278:21:278:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:278:21:278:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:278:21:278:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:278:21:278:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:278:21:278:24 | path |
 | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) | normalizedPaths.js:254:7:254:47 | path |
 | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) | normalizedPaths.js:254:7:254:47 | path |
 | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) | normalizedPaths.js:254:7:254:47 | path |
@@ -4478,6 +4491,7 @@ edges
 | normalizedPaths.js:256:19:256:22 | path | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:256:19:256:22 | path | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
 | normalizedPaths.js:262:21:262:24 | path | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:262:21:262:24 | path | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
 | normalizedPaths.js:270:21:270:24 | path | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:270:21:270:24 | path | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
+| normalizedPaths.js:278:21:278:24 | path | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:278:21:278:24 | path | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
 | 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:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | This path depends on $@. | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | a user-provided value |
 | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | This path depends on $@. | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | a user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js
index 121ac5e8e635..29a682b8d1e6 100644
--- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js
@@ -269,6 +269,14 @@ app.get('/relative-startswith', (req, res) => {
   if (relativePath.indexOf('..' + pathModule.sep) === 0) {
     fs.readFileSync(path); // NOT OK!
   } else {
-	  fs.readFileSync(newpath); // OK! 
+    fs.readFileSync(newpath); // OK!
+  }
+
+  let newpath = pathModule.normalize(p);
+  var relativePath = path.relative(path.normalize(workspaceDir), newpath);
+  if (relativePath.indexOf('../') === 0) {
+    fs.readFileSync(path); // NOT OK!
+  } else {
+    fs.readFileSync(newpath); // OK! 
   }
 });

From 94814fa72193e36c20db6b021ebfa6793f89c2c8 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Fri, 14 Feb 2020 13:03:04 +0100
Subject: [PATCH 4/6] fix typos in the test

---
 .../CWE-022/TaintedPath/TaintedPath.expected  | 104 +++++++++++++-----
 .../CWE-022/TaintedPath/normalizedPaths.js    |  12 +-
 2 files changed, 82 insertions(+), 34 deletions(-)

diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
index 856aeb014ce8..ffe143fc4235 100644
--- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
@@ -1282,16 +1282,40 @@ nodes
 | normalizedPaths.js:262:21:262:24 | path |
 | normalizedPaths.js:262:21:262:24 | path |
 | normalizedPaths.js:262:21:262:24 | path |
-| normalizedPaths.js:270:21:270:24 | path |
-| normalizedPaths.js:270:21:270:24 | path |
-| normalizedPaths.js:270:21:270:24 | path |
-| normalizedPaths.js:270:21:270:24 | path |
-| normalizedPaths.js:270:21:270:24 | path |
-| normalizedPaths.js:278:21:278:24 | path |
-| normalizedPaths.js:278:21:278:24 | path |
-| normalizedPaths.js:278:21:278:24 | path |
-| normalizedPaths.js:278:21:278:24 | path |
-| normalizedPaths.js:278:21:278:24 | path |
+| normalizedPaths.js:267:7:267:42 | newpath |
+| normalizedPaths.js:267:7:267:42 | newpath |
+| normalizedPaths.js:267:7:267:42 | newpath |
+| normalizedPaths.js:267:7:267:42 | newpath |
+| normalizedPaths.js:267:17:267:42 | pathMod ... e(path) |
+| normalizedPaths.js:267:17:267:42 | pathMod ... e(path) |
+| normalizedPaths.js:267:17:267:42 | pathMod ... e(path) |
+| normalizedPaths.js:267:17:267:42 | pathMod ... e(path) |
+| normalizedPaths.js:267:38:267:41 | path |
+| normalizedPaths.js:267:38:267:41 | path |
+| normalizedPaths.js:267:38:267:41 | path |
+| normalizedPaths.js:267:38:267:41 | path |
+| normalizedPaths.js:270:21:270:27 | newpath |
+| normalizedPaths.js:270:21:270:27 | newpath |
+| normalizedPaths.js:270:21:270:27 | newpath |
+| normalizedPaths.js:270:21:270:27 | newpath |
+| normalizedPaths.js:270:21:270:27 | newpath |
+| normalizedPaths.js:275:7:275:42 | newpath |
+| normalizedPaths.js:275:7:275:42 | newpath |
+| normalizedPaths.js:275:7:275:42 | newpath |
+| normalizedPaths.js:275:7:275:42 | newpath |
+| normalizedPaths.js:275:17:275:42 | pathMod ... e(path) |
+| normalizedPaths.js:275:17:275:42 | pathMod ... e(path) |
+| normalizedPaths.js:275:17:275:42 | pathMod ... e(path) |
+| normalizedPaths.js:275:17:275:42 | pathMod ... e(path) |
+| normalizedPaths.js:275:38:275:41 | path |
+| normalizedPaths.js:275:38:275:41 | path |
+| normalizedPaths.js:275:38:275:41 | path |
+| normalizedPaths.js:275:38:275:41 | path |
+| normalizedPaths.js:278:21:278:27 | newpath |
+| normalizedPaths.js:278:21:278:27 | newpath |
+| normalizedPaths.js:278:21:278:27 | newpath |
+| normalizedPaths.js:278:21:278:27 | newpath |
+| normalizedPaths.js:278:21:278:27 | newpath |
 | 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") |
@@ -3679,22 +3703,14 @@ edges
 | normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:262:21:262:24 | path |
 | normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:262:21:262:24 | path |
 | normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:262:21:262:24 | path |
-| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
-| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
-| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
-| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
-| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
-| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
-| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
-| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
-| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:278:21:278:24 | path |
-| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:278:21:278:24 | path |
-| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:278:21:278:24 | path |
-| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:278:21:278:24 | path |
-| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:278:21:278:24 | path |
-| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:278:21:278:24 | path |
-| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:278:21:278:24 | path |
-| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:278:21:278:24 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:267:38:267:41 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:267:38:267:41 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:267:38:267:41 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:267:38:267:41 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:275:38:275:41 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:275:38:275:41 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:275:38:275:41 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:275:38:275:41 | path |
 | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) | normalizedPaths.js:254:7:254:47 | path |
 | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) | normalizedPaths.js:254:7:254:47 | path |
 | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) | normalizedPaths.js:254:7:254:47 | path |
@@ -3707,6 +3723,38 @@ edges
 | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
 | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
 | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
+| normalizedPaths.js:267:7:267:42 | newpath | normalizedPaths.js:270:21:270:27 | newpath |
+| normalizedPaths.js:267:7:267:42 | newpath | normalizedPaths.js:270:21:270:27 | newpath |
+| normalizedPaths.js:267:7:267:42 | newpath | normalizedPaths.js:270:21:270:27 | newpath |
+| normalizedPaths.js:267:7:267:42 | newpath | normalizedPaths.js:270:21:270:27 | newpath |
+| normalizedPaths.js:267:7:267:42 | newpath | normalizedPaths.js:270:21:270:27 | newpath |
+| normalizedPaths.js:267:7:267:42 | newpath | normalizedPaths.js:270:21:270:27 | newpath |
+| normalizedPaths.js:267:7:267:42 | newpath | normalizedPaths.js:270:21:270:27 | newpath |
+| normalizedPaths.js:267:7:267:42 | newpath | normalizedPaths.js:270:21:270:27 | newpath |
+| normalizedPaths.js:267:17:267:42 | pathMod ... e(path) | normalizedPaths.js:267:7:267:42 | newpath |
+| normalizedPaths.js:267:17:267:42 | pathMod ... e(path) | normalizedPaths.js:267:7:267:42 | newpath |
+| normalizedPaths.js:267:17:267:42 | pathMod ... e(path) | normalizedPaths.js:267:7:267:42 | newpath |
+| normalizedPaths.js:267:17:267:42 | pathMod ... e(path) | normalizedPaths.js:267:7:267:42 | newpath |
+| normalizedPaths.js:267:38:267:41 | path | normalizedPaths.js:267:17:267:42 | pathMod ... e(path) |
+| normalizedPaths.js:267:38:267:41 | path | normalizedPaths.js:267:17:267:42 | pathMod ... e(path) |
+| normalizedPaths.js:267:38:267:41 | path | normalizedPaths.js:267:17:267:42 | pathMod ... e(path) |
+| normalizedPaths.js:267:38:267:41 | path | normalizedPaths.js:267:17:267:42 | pathMod ... e(path) |
+| normalizedPaths.js:275:7:275:42 | newpath | normalizedPaths.js:278:21:278:27 | newpath |
+| normalizedPaths.js:275:7:275:42 | newpath | normalizedPaths.js:278:21:278:27 | newpath |
+| normalizedPaths.js:275:7:275:42 | newpath | normalizedPaths.js:278:21:278:27 | newpath |
+| normalizedPaths.js:275:7:275:42 | newpath | normalizedPaths.js:278:21:278:27 | newpath |
+| normalizedPaths.js:275:7:275:42 | newpath | normalizedPaths.js:278:21:278:27 | newpath |
+| normalizedPaths.js:275:7:275:42 | newpath | normalizedPaths.js:278:21:278:27 | newpath |
+| normalizedPaths.js:275:7:275:42 | newpath | normalizedPaths.js:278:21:278:27 | newpath |
+| normalizedPaths.js:275:7:275:42 | newpath | normalizedPaths.js:278:21:278:27 | newpath |
+| normalizedPaths.js:275:17:275:42 | pathMod ... e(path) | normalizedPaths.js:275:7:275:42 | newpath |
+| normalizedPaths.js:275:17:275:42 | pathMod ... e(path) | normalizedPaths.js:275:7:275:42 | newpath |
+| normalizedPaths.js:275:17:275:42 | pathMod ... e(path) | normalizedPaths.js:275:7:275:42 | newpath |
+| normalizedPaths.js:275:17:275:42 | pathMod ... e(path) | normalizedPaths.js:275:7:275:42 | newpath |
+| normalizedPaths.js:275:38:275:41 | path | normalizedPaths.js:275:17:275:42 | pathMod ... e(path) |
+| normalizedPaths.js:275:38:275:41 | path | normalizedPaths.js:275:17:275:42 | pathMod ... e(path) |
+| normalizedPaths.js:275:38:275:41 | path | normalizedPaths.js:275:17:275:42 | pathMod ... e(path) |
+| normalizedPaths.js:275:38:275:41 | path | normalizedPaths.js:275:17:275:42 | pathMod ... e(path) |
 | tainted-require.js:7:19:7:37 | req.param("module") | tainted-require.js:7:19:7:37 | req.param("module") |
 | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") |
 | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") |
@@ -4490,8 +4538,8 @@ edges
 | normalizedPaths.js:250:21:250:24 | path | normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:250:21:250:24 | path | This path depends on $@. | normalizedPaths.js:236:33:236:46 | req.query.path | a user-provided value |
 | normalizedPaths.js:256:19:256:22 | path | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:256:19:256:22 | path | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
 | normalizedPaths.js:262:21:262:24 | path | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:262:21:262:24 | path | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
-| normalizedPaths.js:270:21:270:24 | path | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:270:21:270:24 | path | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
-| normalizedPaths.js:278:21:278:24 | path | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:278:21:278:24 | path | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
+| normalizedPaths.js:270:21:270:27 | newpath | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:270:21:270:27 | newpath | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
+| normalizedPaths.js:278:21:278:27 | newpath | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:278:21:278:27 | newpath | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
 | 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:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | This path depends on $@. | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | a user-provided value |
 | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | This path depends on $@. | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | a user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js
index 29a682b8d1e6..6248f41e66bc 100644
--- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js
@@ -264,18 +264,18 @@ app.get('/relative-startswith', (req, res) => {
     fs.readFileSync(path); // OK! 
   }
 
-  let newpath = pathModule.normalize(p);
-  var relativePath = path.relative(path.normalize(workspaceDir), newpath);
+  let newpath = pathModule.normalize(path);
+  var relativePath = pathModule.relative(pathModule.normalize(workspaceDir), newpath);
   if (relativePath.indexOf('..' + pathModule.sep) === 0) {
-    fs.readFileSync(path); // NOT OK!
+    fs.readFileSync(newpath); // NOT OK!
   } else {
     fs.readFileSync(newpath); // OK!
   }
 
-  let newpath = pathModule.normalize(p);
-  var relativePath = path.relative(path.normalize(workspaceDir), newpath);
+  let newpath = pathModule.normalize(path);
+  var relativePath = pathModule.relative(pathModule.normalize(workspaceDir), newpath);
   if (relativePath.indexOf('../') === 0) {
-    fs.readFileSync(path); // NOT OK!
+    fs.readFileSync(newpath); // NOT OK!
   } else {
     fs.readFileSync(newpath); // OK! 
   }

From a6d644bac09fbc30df4c09aaf6a3cb8f72662eb0 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Fri, 14 Feb 2020 13:10:35 +0100
Subject: [PATCH 5/6] add support for path.normalize(path.realtive(...))

---
 .../dataflow/TaintedPathCustomizations.qll    | 11 +++++-
 .../CWE-022/TaintedPath/TaintedPath.expected  | 38 +++++++++++++++++++
 .../CWE-022/TaintedPath/normalizedPaths.js    |  8 ++++
 3 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
index 38b06e06f141..fc8ac5b1a84e 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
@@ -367,7 +367,16 @@ module TaintedPath {
     RelativePathContainsDotDotGuard() {
       this = startsWith and
       relativeCall = DataFlow::moduleImport("path").getAMemberCall("relative") and
-      startsWith.getBaseString().getALocalSource() = relativeCall and
+      (
+        startsWith.getBaseString().getALocalSource() = relativeCall
+        or
+        startsWith
+            .getBaseString()
+            .getALocalSource()
+            .(NormalizingPathCall)
+            .getInput()
+            .getALocalSource() = relativeCall
+      ) and
       exists(DataFlow::Node subString, string prefix |
         subString = startsWith.getSubstring() and
         (prefix = ".." or prefix = "../")
diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
index ffe143fc4235..ab0869ca2573 100644
--- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
@@ -1316,6 +1316,23 @@ nodes
 | normalizedPaths.js:278:21:278:27 | newpath |
 | normalizedPaths.js:278:21:278:27 | newpath |
 | normalizedPaths.js:278:21:278:27 | newpath |
+| normalizedPaths.js:283:7:283:42 | newpath |
+| normalizedPaths.js:283:7:283:42 | newpath |
+| normalizedPaths.js:283:7:283:42 | newpath |
+| normalizedPaths.js:283:7:283:42 | newpath |
+| normalizedPaths.js:283:17:283:42 | pathMod ... e(path) |
+| normalizedPaths.js:283:17:283:42 | pathMod ... e(path) |
+| normalizedPaths.js:283:17:283:42 | pathMod ... e(path) |
+| normalizedPaths.js:283:17:283:42 | pathMod ... e(path) |
+| normalizedPaths.js:283:38:283:41 | path |
+| normalizedPaths.js:283:38:283:41 | path |
+| normalizedPaths.js:283:38:283:41 | path |
+| normalizedPaths.js:283:38:283:41 | path |
+| normalizedPaths.js:286:21:286:27 | newpath |
+| normalizedPaths.js:286:21:286:27 | newpath |
+| normalizedPaths.js:286:21:286:27 | newpath |
+| normalizedPaths.js:286:21:286:27 | newpath |
+| normalizedPaths.js:286:21:286:27 | newpath |
 | 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") |
@@ -3711,6 +3728,10 @@ edges
 | normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:275:38:275:41 | path |
 | normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:275:38:275:41 | path |
 | normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:275:38:275:41 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:283:38:283:41 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:283:38:283:41 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:283:38:283:41 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:283:38:283:41 | path |
 | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) | normalizedPaths.js:254:7:254:47 | path |
 | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) | normalizedPaths.js:254:7:254:47 | path |
 | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) | normalizedPaths.js:254:7:254:47 | path |
@@ -3755,6 +3776,22 @@ edges
 | normalizedPaths.js:275:38:275:41 | path | normalizedPaths.js:275:17:275:42 | pathMod ... e(path) |
 | normalizedPaths.js:275:38:275:41 | path | normalizedPaths.js:275:17:275:42 | pathMod ... e(path) |
 | normalizedPaths.js:275:38:275:41 | path | normalizedPaths.js:275:17:275:42 | pathMod ... e(path) |
+| normalizedPaths.js:283:7:283:42 | newpath | normalizedPaths.js:286:21:286:27 | newpath |
+| normalizedPaths.js:283:7:283:42 | newpath | normalizedPaths.js:286:21:286:27 | newpath |
+| normalizedPaths.js:283:7:283:42 | newpath | normalizedPaths.js:286:21:286:27 | newpath |
+| normalizedPaths.js:283:7:283:42 | newpath | normalizedPaths.js:286:21:286:27 | newpath |
+| normalizedPaths.js:283:7:283:42 | newpath | normalizedPaths.js:286:21:286:27 | newpath |
+| normalizedPaths.js:283:7:283:42 | newpath | normalizedPaths.js:286:21:286:27 | newpath |
+| normalizedPaths.js:283:7:283:42 | newpath | normalizedPaths.js:286:21:286:27 | newpath |
+| normalizedPaths.js:283:7:283:42 | newpath | normalizedPaths.js:286:21:286:27 | newpath |
+| normalizedPaths.js:283:17:283:42 | pathMod ... e(path) | normalizedPaths.js:283:7:283:42 | newpath |
+| normalizedPaths.js:283:17:283:42 | pathMod ... e(path) | normalizedPaths.js:283:7:283:42 | newpath |
+| normalizedPaths.js:283:17:283:42 | pathMod ... e(path) | normalizedPaths.js:283:7:283:42 | newpath |
+| normalizedPaths.js:283:17:283:42 | pathMod ... e(path) | normalizedPaths.js:283:7:283:42 | newpath |
+| normalizedPaths.js:283:38:283:41 | path | normalizedPaths.js:283:17:283:42 | pathMod ... e(path) |
+| normalizedPaths.js:283:38:283:41 | path | normalizedPaths.js:283:17:283:42 | pathMod ... e(path) |
+| normalizedPaths.js:283:38:283:41 | path | normalizedPaths.js:283:17:283:42 | pathMod ... e(path) |
+| normalizedPaths.js:283:38:283:41 | path | normalizedPaths.js:283:17:283:42 | pathMod ... e(path) |
 | tainted-require.js:7:19:7:37 | req.param("module") | tainted-require.js:7:19:7:37 | req.param("module") |
 | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") |
 | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") |
@@ -4540,6 +4577,7 @@ edges
 | normalizedPaths.js:262:21:262:24 | path | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:262:21:262:24 | path | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
 | normalizedPaths.js:270:21:270:27 | newpath | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:270:21:270:27 | newpath | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
 | normalizedPaths.js:278:21:278:27 | newpath | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:278:21:278:27 | newpath | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
+| normalizedPaths.js:286:21:286:27 | newpath | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:286:21:286:27 | newpath | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
 | 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:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | This path depends on $@. | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | a user-provided value |
 | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | This path depends on $@. | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | a user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js
index 6248f41e66bc..c718d9075c68 100644
--- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js
@@ -279,4 +279,12 @@ app.get('/relative-startswith', (req, res) => {
   } else {
     fs.readFileSync(newpath); // OK! 
   }
+
+  let newpath = pathModule.normalize(path);
+  var relativePath = pathModule.relative(pathModule.normalize(workspaceDir), newpath);
+  if (pathModule.normalize(relativePath).indexOf('../') === 0) {
+    fs.readFileSync(newpath); // NOT OK!
+  } else {
+    fs.readFileSync(newpath); // OK! 
+  }
 });

From 2885d48ad01cee677019f0f3e616f30ba237bc34 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 17 Feb 2020 14:44:10 +0100
Subject: [PATCH 6/6] changes based on review

---
 .../security/dataflow/TaintedPath.qll         |  2 +-
 .../dataflow/TaintedPathCustomizations.qll    | 31 +++++++--------
 .../CWE-022/TaintedPath/TaintedPath.expected  | 38 +++++++++++++++++++
 .../CWE-022/TaintedPath/normalizedPaths.js    |  8 ++++
 4 files changed, 60 insertions(+), 19 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPath.qll b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPath.qll
index c72257b47c0f..e57d3ccb3aa2 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPath.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPath.qll
@@ -36,7 +36,7 @@ module TaintedPath {
       guard instanceof StartsWithDirSanitizer or
       guard instanceof IsAbsoluteSanitizer or
       guard instanceof ContainsDotDotSanitizer or
-      guard instanceof RelativePathContainsDotDotGuard
+      guard instanceof RelativePathStartsWithDotDotSanitizer
     }
 
     override predicate isAdditionalFlowStep(
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
index fc8ac5b1a84e..a27080da74d5 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
@@ -353,20 +353,22 @@ module TaintedPath {
 
   /**
    * A sanitizer that recognizes the following pattern:
-   *   var relative = path.relative(webroot, pathname);
-   *   if(relative.startsWith(".." + path.sep) || relative == "..") {
-   *     // pathname is unsafe
-   *   } else {
-   *     // pathname is safe
-   *   }
+   * ```
+   * var relative = path.relative(webroot, pathname);
+   * if(relative.startsWith(".." + path.sep) || relative == "..") {
+   *   // pathname is unsafe
+   * } else {
+   *   // pathname is safe
+   * }
+   * ```
    */
-  class RelativePathContainsDotDotGuard extends DataFlow::BarrierGuardNode {
+  class RelativePathStartsWithDotDotSanitizer extends DataFlow::BarrierGuardNode {
     StringOps::StartsWith startsWith;
     DataFlow::CallNode relativeCall;
 
-    RelativePathContainsDotDotGuard() {
+    RelativePathStartsWithDotDotSanitizer() {
       this = startsWith and
-      relativeCall = DataFlow::moduleImport("path").getAMemberCall("relative") and
+      relativeCall = NodeJSLib::Path::moduleMember("relative").getACall() and
       (
         startsWith.getBaseString().getALocalSource() = relativeCall
         or
@@ -377,18 +379,11 @@ module TaintedPath {
             .getInput()
             .getALocalSource() = relativeCall
       ) and
-      exists(DataFlow::Node subString, string prefix |
-        subString = startsWith.getSubstring() and
-        (prefix = ".." or prefix = "../")
-      |
-        subString.mayHaveStringValue(prefix)
-        or
-        subString.(StringOps::ConcatenationRoot).getFirstLeaf().mayHaveStringValue(prefix)
-      )
+      isDotDotSlashPrefix(startsWith.getSubstring())
     }
 
     override predicate blocks(boolean outcome, Expr e) {
-      e = relativeCall.getArgument(1).asExpr() and outcome = false
+      e = relativeCall.getArgument(1).asExpr() and outcome = startsWith.getPolarity().booleanNot()
     }
   }
 
diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
index ab0869ca2573..81bc0310f5ef 100644
--- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
@@ -1333,6 +1333,23 @@ nodes
 | normalizedPaths.js:286:21:286:27 | newpath |
 | normalizedPaths.js:286:21:286:27 | newpath |
 | normalizedPaths.js:286:21:286:27 | newpath |
+| normalizedPaths.js:291:7:291:42 | newpath |
+| normalizedPaths.js:291:7:291:42 | newpath |
+| normalizedPaths.js:291:7:291:42 | newpath |
+| normalizedPaths.js:291:7:291:42 | newpath |
+| normalizedPaths.js:291:17:291:42 | pathMod ... e(path) |
+| normalizedPaths.js:291:17:291:42 | pathMod ... e(path) |
+| normalizedPaths.js:291:17:291:42 | pathMod ... e(path) |
+| normalizedPaths.js:291:17:291:42 | pathMod ... e(path) |
+| normalizedPaths.js:291:38:291:41 | path |
+| normalizedPaths.js:291:38:291:41 | path |
+| normalizedPaths.js:291:38:291:41 | path |
+| normalizedPaths.js:291:38:291:41 | path |
+| normalizedPaths.js:296:21:296:27 | newpath |
+| normalizedPaths.js:296:21:296:27 | newpath |
+| normalizedPaths.js:296:21:296:27 | newpath |
+| normalizedPaths.js:296:21:296:27 | newpath |
+| normalizedPaths.js:296:21:296:27 | newpath |
 | 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") |
@@ -3732,6 +3749,10 @@ edges
 | normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:283:38:283:41 | path |
 | normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:283:38:283:41 | path |
 | normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:283:38:283:41 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:291:38:291:41 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:291:38:291:41 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:291:38:291:41 | path |
+| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:291:38:291:41 | path |
 | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) | normalizedPaths.js:254:7:254:47 | path |
 | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) | normalizedPaths.js:254:7:254:47 | path |
 | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) | normalizedPaths.js:254:7:254:47 | path |
@@ -3792,6 +3813,22 @@ edges
 | normalizedPaths.js:283:38:283:41 | path | normalizedPaths.js:283:17:283:42 | pathMod ... e(path) |
 | normalizedPaths.js:283:38:283:41 | path | normalizedPaths.js:283:17:283:42 | pathMod ... e(path) |
 | normalizedPaths.js:283:38:283:41 | path | normalizedPaths.js:283:17:283:42 | pathMod ... e(path) |
+| normalizedPaths.js:291:7:291:42 | newpath | normalizedPaths.js:296:21:296:27 | newpath |
+| normalizedPaths.js:291:7:291:42 | newpath | normalizedPaths.js:296:21:296:27 | newpath |
+| normalizedPaths.js:291:7:291:42 | newpath | normalizedPaths.js:296:21:296:27 | newpath |
+| normalizedPaths.js:291:7:291:42 | newpath | normalizedPaths.js:296:21:296:27 | newpath |
+| normalizedPaths.js:291:7:291:42 | newpath | normalizedPaths.js:296:21:296:27 | newpath |
+| normalizedPaths.js:291:7:291:42 | newpath | normalizedPaths.js:296:21:296:27 | newpath |
+| normalizedPaths.js:291:7:291:42 | newpath | normalizedPaths.js:296:21:296:27 | newpath |
+| normalizedPaths.js:291:7:291:42 | newpath | normalizedPaths.js:296:21:296:27 | newpath |
+| normalizedPaths.js:291:17:291:42 | pathMod ... e(path) | normalizedPaths.js:291:7:291:42 | newpath |
+| normalizedPaths.js:291:17:291:42 | pathMod ... e(path) | normalizedPaths.js:291:7:291:42 | newpath |
+| normalizedPaths.js:291:17:291:42 | pathMod ... e(path) | normalizedPaths.js:291:7:291:42 | newpath |
+| normalizedPaths.js:291:17:291:42 | pathMod ... e(path) | normalizedPaths.js:291:7:291:42 | newpath |
+| normalizedPaths.js:291:38:291:41 | path | normalizedPaths.js:291:17:291:42 | pathMod ... e(path) |
+| normalizedPaths.js:291:38:291:41 | path | normalizedPaths.js:291:17:291:42 | pathMod ... e(path) |
+| normalizedPaths.js:291:38:291:41 | path | normalizedPaths.js:291:17:291:42 | pathMod ... e(path) |
+| normalizedPaths.js:291:38:291:41 | path | normalizedPaths.js:291:17:291:42 | pathMod ... e(path) |
 | tainted-require.js:7:19:7:37 | req.param("module") | tainted-require.js:7:19:7:37 | req.param("module") |
 | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") |
 | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") |
@@ -4578,6 +4615,7 @@ edges
 | normalizedPaths.js:270:21:270:27 | newpath | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:270:21:270:27 | newpath | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
 | normalizedPaths.js:278:21:278:27 | newpath | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:278:21:278:27 | newpath | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
 | normalizedPaths.js:286:21:286:27 | newpath | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:286:21:286:27 | newpath | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
+| normalizedPaths.js:296:21:296:27 | newpath | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:296:21:296:27 | newpath | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
 | 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:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | This path depends on $@. | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | a user-provided value |
 | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | This path depends on $@. | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | a user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js
index c718d9075c68..98f71a7f0f33 100644
--- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js
@@ -287,4 +287,12 @@ app.get('/relative-startswith', (req, res) => {
   } else {
     fs.readFileSync(newpath); // OK! 
   }
+
+  let newpath = pathModule.normalize(path);
+  var relativePath = pathModule.relative(pathModule.normalize(workspaceDir), newpath);
+  if (pathModule.normalize(relativePath).indexOf('../')) {
+    fs.readFileSync(newpath); // OK!
+  } else {
+    fs.readFileSync(newpath); // NOT OK! 
+  }
 });