Merge pull request #958 from esben-semmle/js/improve-tainted-path

JS: add taint steps for fs.realpath and fs.realpathSync

Author: Max Schaefer

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

@@ -267,6 +267,31 @@ module NodeJSLib {
     }
   }
 
+  /**
+   * A call to a fs-module method that preserves taint.
+   */
+  private class FsFlowTarget extends TaintTracking::AdditionalTaintStep {
+    DataFlow::Node tainted;
+
+    FsFlowTarget() {
+      exists(DataFlow::CallNode call, string methodName |
+        call = DataFlow::moduleMember("fs", methodName).getACall()
+        |
+        methodName = "realpathSync" and
+        tainted = call.getArgument(0) and
+        this = call
+        or
+        methodName = "realpath" and
+        tainted = call.getArgument(0) and
+        this = call.getCallback(1).getParameter(1)
+      )
+    }
+
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      pred = tainted and succ = this
+    }
+  }
+
   /**
    * A model of taint propagation through `new Buffer` and `Buffer.from`.
    */

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

@@ -67,6 +67,16 @@ nodes
 | TaintedPath.js:102:30:102:31 | ev |
 | TaintedPath.js:103:24:103:25 | ev |
 | TaintedPath.js:103:24:103:30 | ev.data |
+| TaintedPath.js:107:6:107:47 | path |
+| TaintedPath.js:107:13:107:36 | url.par ... , true) |
+| TaintedPath.js:107:13:107:42 | url.par ... ).query |
+| TaintedPath.js:107:13:107:47 | url.par ... ry.path |
+| TaintedPath.js:107:23:107:29 | req.url |
+| TaintedPath.js:109:28:109:48 | fs.real ... c(path) |
+| TaintedPath.js:109:44:109:47 | path |
+| TaintedPath.js:110:14:110:17 | path |
+| TaintedPath.js:111:32:111:39 | realpath |
+| TaintedPath.js:112:45:112:52 | realpath |
 | tainted-array-steps.js:9:7:9:48 | path |
 | tainted-array-steps.js:9:14:9:37 | url.par ... , true) |
 | tainted-array-steps.js:9:14:9:43 | url.par ... ).query |
@@ -150,6 +160,15 @@ edges
 | TaintedPath.js:102:30:102:31 | ev | TaintedPath.js:103:24:103:25 | ev |
 | TaintedPath.js:103:24:103:25 | ev | TaintedPath.js:103:24:103:30 | ev.data |
 | TaintedPath.js:103:24:103:30 | ev.data | TaintedPath.js:78:26:78:45 | Cookie.get("unsafe") |
+| TaintedPath.js:107:6:107:47 | path | TaintedPath.js:109:44:109:47 | path |
+| TaintedPath.js:107:6:107:47 | path | TaintedPath.js:110:14:110:17 | path |
+| TaintedPath.js:107:13:107:36 | url.par ... , true) | TaintedPath.js:107:13:107:42 | url.par ... ).query |
+| TaintedPath.js:107:13:107:42 | url.par ... ).query | TaintedPath.js:107:13:107:47 | url.par ... ry.path |
+| TaintedPath.js:107:13:107:47 | url.par ... ry.path | TaintedPath.js:107:6:107:47 | path |
+| TaintedPath.js:107:23:107:29 | req.url | TaintedPath.js:107:13:107:36 | url.par ... , true) |
+| TaintedPath.js:109:44:109:47 | path | TaintedPath.js:109:28:109:48 | fs.real ... c(path) |
+| TaintedPath.js:110:14:110:17 | path | TaintedPath.js:111:32:111:39 | realpath |
+| TaintedPath.js:111:32:111:39 | realpath | TaintedPath.js:112:45:112:52 | realpath |
 | tainted-array-steps.js:9:7:9:48 | path | tainted-array-steps.js:11:40:11:43 | path |
 | tainted-array-steps.js:9:7:9:48 | path | tainted-array-steps.js:13:26:13:29 | path |
 | tainted-array-steps.js:9:14:9:37 | url.par ... , true) | tainted-array-steps.js:9:14:9:43 | url.par ... ).query |
@@ -190,6 +209,8 @@ edges
 | TaintedPath.js:85:31:85:74 | require ... ).query | TaintedPath.js:85:61:85:67 | req.url | TaintedPath.js:85:31:85:74 | require ... ).query | This path depends on $@. | TaintedPath.js:85:61:85:67 | req.url | a user-provided value |
 | TaintedPath.js:86:31:86:73 | require ... ).query | TaintedPath.js:86:60:86:66 | req.url | TaintedPath.js:86:31:86:73 | require ... ).query | This path depends on $@. | TaintedPath.js:86:60:86:66 | req.url | a user-provided value |
 | TaintedPath.js:94:48:94:60 | req.params[0] | TaintedPath.js:94:48:94:60 | req.params[0] | TaintedPath.js:94:48:94:60 | req.params[0] | This path depends on $@. | TaintedPath.js:94:48:94:60 | req.params[0] | a user-provided value |
+| TaintedPath.js:109:28:109:48 | fs.real ... c(path) | TaintedPath.js:107:23:107:29 | req.url | TaintedPath.js:109:28:109:48 | fs.real ... c(path) | This path depends on $@. | TaintedPath.js:107:23:107:29 | req.url | a user-provided value |
+| TaintedPath.js:112:45:112:52 | realpath | TaintedPath.js:107:23:107:29 | req.url | TaintedPath.js:112:45:112:52 | realpath | This path depends on $@. | TaintedPath.js:107:23:107:29 | req.url | a user-provided value |
 | tainted-array-steps.js:11:29:11:54 | ['publi ... in('/') | tainted-array-steps.js:9:24:9:30 | req.url | tainted-array-steps.js:11:29:11:54 | ['publi ... in('/') | This path depends on $@. | tainted-array-steps.js:9:24:9:30 | req.url | a user-provided value |
 | tainted-array-steps.js:15:29:15:43 | parts.join('/') | tainted-array-steps.js:9:24:9:30 | req.url | tainted-array-steps.js:15:29:15:43 | parts.join('/') | This path depends on $@. | tainted-array-steps.js:9:24:9:30 | req.url | 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 |

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

@@ -102,3 +102,15 @@ var server = http.createServer(function(req, res) {
 addEventListener('message', (ev) => {
   Cookie.set("unsafe", ev.data);
 });
+
+var server = http.createServer(function(req, res) {
+	let path = url.parse(req.url, true).query.path;
+
+	res.write(fs.readFileSync(fs.realpathSync(path)));
+	fs.realpath(path,
+	                function(err, realpath){
+		                res.write(fs.readFileSync(realpath));
+	                }
+	               );
+
+});