Merge pull request github#13700 from asgerf/js/path-join-spread

JS: Recognize 'fs/promises' alias and handle spread arguments in path.join()

Author: asgerf

javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll

@@ -554,7 +554,11 @@ module NodeJSLib {
       t.start()
       or
       t.start() and
-      result = DataFlow::moduleMember("fs", "promises")
+      (
+        result = DataFlow::moduleMember("fs", "promises")
+        or
+        result = DataFlow::moduleImport("fs/promises")
+      )
       or
       exists(DataFlow::TypeTracker t2, DataFlow::SourceNode pred | pred = fsModule(t2) |
         result = pred.track(t2, t)

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

@@ -847,6 +847,22 @@ module TaintedPath {
       dst = call and
       srclabel = dstlabel
     )
+    or
+    exists(DataFlow::CallNode join |
+      // path.join() with spread argument
+      join = NodeJSLib::Path::moduleMember("join").getACall() and
+      src = join.getASpreadArgument() and
+      dst = join and
+      (
+        srclabel.(Label::PosixPath).canContainDotDotSlash()
+        or
+        srclabel instanceof Label::SplitPath
+      ) and
+      dstlabel.(Label::PosixPath).isNormalized() and
+      if isRelative(join.getArgument(0).getStringValue())
+      then dstlabel.(Label::PosixPath).isRelative()
+      else dstlabel.(Label::PosixPath).isAbsolute()
+    )
   }
 
   /**

javascript/ql/src/change-notes/2023-07-10-path-join-spread.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* The `fs/promises` package is now recognised as an alias for `require('fs').promises`.
+* The `js/path-injection` query can now track taint through calls to `path.join()` with a spread argument, such as `path.join(baseDir, ...args)`.

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

@@ -45,4 +45,12 @@ var fs = {};
  */
 fs.readFileSync = function(filename, encoding) {};
 
+/**
+ * @param {string} filename
+ * @param {string} encoding
+ * @param {(function(NodeJS.ErrnoException, string): void)} callback
+ * @return {void}
+ */
+fs.readFile = function(filename, encoding, callback) {};
+
 module.exports = fs;

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

@@ -32,17 +32,17 @@ app.get('/normalize-notAbsolute', (req, res) => {
 
   if (pathModule.isAbsolute(path))
     return;
-   
+
   fs.readFileSync(path); // NOT OK
 
   if (!path.startsWith("."))
     fs.readFileSync(path); // OK
   else
     fs.readFileSync(path); // NOT OK - wrong polarity
-  
+
   if (!path.startsWith(".."))
     fs.readFileSync(path); // OK
-  
+
   if (!path.startsWith("../"))
     fs.readFileSync(path); // OK
 
@@ -52,7 +52,7 @@ app.get('/normalize-notAbsolute', (req, res) => {
 
 app.get('/normalize-noInitialDotDot', (req, res) => {
   let path = pathModule.normalize(req.query.path);
-  
+
   if (path.startsWith(".."))
     return;
 
@@ -80,7 +80,7 @@ app.get('/prepend-normalize', (req, res) => {
 
 app.get('/absolute', (req, res) => {
   let path = req.query.path;
-  
+
   if (!pathModule.isAbsolute(path))
     return;
 
@@ -92,10 +92,10 @@ app.get('/absolute', (req, res) => {
 
 app.get('/normalized-absolute', (req, res) => {
   let path = pathModule.normalize(req.query.path);
-  
+
   if (!pathModule.isAbsolute(path))
     return;
-  
+
   res.write(fs.readFileSync(path)); // NOT OK
 
   if (path.startsWith('/home/user/www'))
@@ -104,7 +104,7 @@ app.get('/normalized-absolute', (req, res) => {
 
 app.get('/combined-check', (req, res) => {
   let path = pathModule.normalize(req.query.path);
-  
+
   // Combined absoluteness and folder check in one startsWith call
   if (path.startsWith("/home/user/www"))
     fs.readFileSync(path); // OK
@@ -121,7 +121,7 @@ app.get('/realpath', (req, res) => {
 
   if (path.startsWith("/home/user/www"))
     fs.readFileSync(path); // OK - both absolute and normalized before check
-    
+
   fs.readFileSync(pathModule.join('.', path)); // OK - normalized and coerced to relative
   fs.readFileSync(pathModule.join('/home/user/www', path)); // OK
 });
@@ -212,7 +212,7 @@ app.get('/join-regression', (req, res) => {
 
 app.get('/decode-after-normalization', (req, res) => {
   let path = pathModule.normalize(req.query.path);
-  
+
   if (!pathModule.isAbsolute(path) && !path.startsWith('..'))
     fs.readFileSync(path); // OK
 
@@ -238,7 +238,7 @@ app.get('/resolve-path', (req, res) => {
   fs.readFileSync(path); // NOT OK
 
   var self = something();
-	
+
   if (path.substring(0, self.dir.length) === self.dir)
     fs.readFileSync(path); // OK
   else
@@ -256,12 +256,12 @@ app.get('/relative-startswith', (req, res) => {
   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! 
+    fs.readFileSync(path); // NOT OK!
   } else {
-    fs.readFileSync(path); // OK! 
+    fs.readFileSync(path); // OK!
   }
 
   let newpath = pathModule.normalize(path);
@@ -277,23 +277,23 @@ app.get('/relative-startswith', (req, res) => {
   if (relativePath.indexOf('../') === 0) {
     fs.readFileSync(newpath); // NOT OK!
   } else {
-    fs.readFileSync(newpath); // OK! 
+    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! 
+    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! 
+    fs.readFileSync(newpath); // NOT OK!
   }
 });
 
@@ -340,7 +340,7 @@ app.get('/yet-another-prefix', (req, res) => {
 
 	fs.readFileSync(path); // NOT OK
 
-	var abs = pathModule.resolve(path); 
+	var abs = pathModule.resolve(path);
 
 	if (abs.indexOf(root) !== 0) {
 		fs.readFileSync(path); // NOT OK
@@ -402,3 +402,8 @@ app.get('/dotdot-regexp', (req, res) => {
     fs.readFileSync(path); // OK
   }
 });
+
+app.get('/join-spread', (req, res) => {
+  fs.readFileSync(pathModule.join('foo', ...req.query.x.split('/'))); // NOT OK
+  fs.readFileSync(pathModule.join(...req.query.x.split('/'))); // NOT OK
+});

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

@@ -71,3 +71,10 @@ http.createServer(function(req, res) {
   mkdirp(path); // NOT OK
   mkdirp.sync(path); // NOT OK
 });
+
+const fsp = require("fs/promises");
+http.createServer(function(req, res) {
+  var path = url.parse(req.url, true).query.path;
+
+  fsp.readFile(path); // NOT OK
+});