Make NodeJSLib use moduleMember for ES6-compatibility

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

@@ -125,6 +125,13 @@ module HTTP {
     }
   }
 
+  /**
+   * Gets the string `http` or `https`.
+   */
+  string httpOrHttps() {
+    result = "http" or result = "https"
+  }
+
   /**
    * An expression whose value is sent as (part of) the body of an HTTP response.
    */

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

@@ -16,14 +16,19 @@ module NodeJSLib {
     result = DataFlow::moduleImport("process")
   }
 
+  /**
+   * Gets a reference to a member of the 'process' object.
+   */
+  private DataFlow::SourceNode processMember(string member) {
+    result = process().getAPropertyRead(member) or
+    result = DataFlow::moduleMember("process", member)
+  }
+
   /**
    * Holds if `call` is an invocation of `http.createServer` or `https.createServer`.
    */
   predicate isCreateServer(CallExpr call) {
-    exists (string name |
-      name = "http" or name = "https" |
-      call = DataFlow::moduleMember(name, "createServer").getAnInvocation().asExpr()
-    )
+    call = DataFlow::moduleMember(HTTP::httpOrHttps(), "createServer").getAnInvocation().asExpr()
   }
 
   /**
@@ -234,30 +239,27 @@ module NodeJSLib {
   /**
    * A call to a path-module method that preserves taint.
    */
-  private class PathFlowTarget extends TaintTracking::AdditionalTaintStep, DataFlow::ValueNode {
-    override CallExpr astNode;
-    Expr tainted;
+  private class PathFlowTarget extends TaintTracking::AdditionalTaintStep, DataFlow::CallNode {
+    DataFlow::Node tainted;
 
     PathFlowTarget() {
-      exists (DataFlow::ModuleImportNode pathModule, string methodName |
-        pathModule.getPath() = "path" and
-        this = pathModule.getAMemberCall(methodName) |
+      exists (string methodName | this = DataFlow::moduleMember("path", methodName).getACall() |
         // getters
-        (methodName = "basename" and tainted = astNode.getArgument(0)) or
-        (methodName = "dirname" and tainted = astNode.getArgument(0)) or
-        (methodName = "extname" and tainted = astNode.getArgument(0)) or
+        (methodName = "basename" and tainted = getArgument(0)) or
+        (methodName = "dirname" and tainted = getArgument(0)) or
+        (methodName = "extname" and tainted = getArgument(0)) or
 
         // transformers
-        (methodName = "join" and tainted = astNode.getAnArgument()) or
-        (methodName = "normalize" and tainted = astNode.getArgument(0)) or
-        (methodName = "relative" and tainted = astNode.getArgument([0..1])) or
-        (methodName = "resolve" and tainted = astNode.getAnArgument()) or
-        (methodName = "toNamespacedPath" and tainted = astNode.getArgument(0))
+        (methodName = "join" and tainted = getAnArgument()) or
+        (methodName = "normalize" and tainted = getArgument(0)) or
+        (methodName = "relative" and tainted = getArgument([0..1])) or
+        (methodName = "resolve" and tainted = getAnArgument()) or
+        (methodName = "toNamespacedPath" and tainted = getArgument(0))
       )
     }
 
     override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-      pred.asExpr() = tainted and succ = this
+      pred = tainted and succ = this
     }
 
   }
@@ -294,13 +296,7 @@ module NodeJSLib {
   class Credentials extends CredentialsExpr {
 
     Credentials() {
-      exists (CallExpr call |
-        exists (DataFlow::ModuleImportNode http |
-          http.getPath() = "http" or http.getPath() = "https" |
-          call = http.getAMemberCall("request").asExpr()
-        ) and
-        call.hasOptionArgument(0, "auth", this)
-      )
+      this = DataFlow::moduleMember(HTTP::httpOrHttps(), "request").getACall().getOptionArgument(0, "auth").asExpr()
     }
 
     override string getCredentialsKind() {
@@ -318,7 +314,7 @@ module NodeJSLib {
     ProcessTermination() {
       this = DataFlow::moduleImport("exit").getAnInvocation()
       or
-      this = process().getAMemberCall("exit")
+      this = processMember("exit").getACall()
     }
 
   }
@@ -344,19 +340,18 @@ module NodeJSLib {
   /**
    * A call to a method from module `fs` or `graceful-fs`.
    */
-  private class NodeJSFileSystemAccess extends FileSystemAccess, DataFlow::ValueNode {
-    override MethodCallExpr astNode;
+  private class NodeJSFileSystemAccess extends FileSystemAccess, DataFlow::CallNode {
+    string methodName;
 
     NodeJSFileSystemAccess() {
-      exists (DataFlow::ModuleImportNode fs |
-        fs.getPath() = "fs" or fs.getPath() = "graceful-fs" |
-        this = fs.getAMemberCall(_)
+      exists (string moduleName | this = DataFlow::moduleMember(moduleName, methodName).getACall() |
+        moduleName = "fs" or moduleName = "graceful-fs"
       )
     }
 
     override DataFlow::Node getAPathArgument() {
-      exists (int i | fsFileParam(astNode.getMethodName(), i) |
-        result = DataFlow::valueNode(astNode.getArgument(i))
+      exists (int i | fsFileParam(methodName, i) |
+        result = getArgument(i)
       )
     }
   }

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

@@ -0,0 +1,11 @@
+import { readFileSync } from 'fs';
+import { createServer } from 'http';
+import { parse } from 'url';
+import { join } from 'path';
+
+var server = createServer(function(req, res) {
+  let path = parse(req.url, true).query.path;
+
+  // BAD: This could read any file on the file system
+  res.write(readFileSync(join("public", path)));
+});

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

@@ -1,3 +1,4 @@
+| TaintedPath-es6.js:10:26:10:45 | join("public", path) | This path depends on $@. | TaintedPath-es6.js:7:20:7:26 | req.url | a user-provided value |
 | TaintedPath.js:12:29:12:32 | path | This path depends on $@. | TaintedPath.js:9:24:9:30 | req.url | a user-provided value |
 | TaintedPath.js:15:29:15:48 | "/home/user/" + path | This path depends on $@. | TaintedPath.js:9:24:9:30 | req.url | a user-provided value |
 | TaintedPath.js:19:33:19:36 | path | This path depends on $@. | TaintedPath.js:9:24:9:30 | req.url | a user-provided value |