changes based on review

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(

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 == "..") {
+   * var relative = path.relative(webroot, pathname);
-   *     // pathname is unsafe
+   * if(relative.startsWith(".." + path.sep) || relative == "..") {
-   *   } else {
+   *   // pathname is unsafe
-   *     // pathname is safe
+   * } 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 |
+      isDotDotSlashPrefix(startsWith.getSubstring())
-        subString = startsWith.getSubstring() and
-        (prefix = ".." or prefix = "../")
-      |
-        subString.mayHaveStringValue(prefix)
-        or
-        subString.(StringOps::ConcatenationRoot).getFirstLeaf().mayHaveStringValue(prefix)
-      )
     }
 
     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()
     }
   }
 

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 |

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! 
+  }
 });