add a getMaybePromisifiedCall method in API graphs, and use it to model child_process

2026-05-01 11:45:14 +02:00 · 2021-08-24 13:27:32 +02:00
parent abdf993e47
commit c664d7cfb3
4 changed files with 64 additions and 21 deletions
--- a/javascript/ql/src/semmle/javascript/ApiGraphs.qll
+++ b/javascript/ql/src/semmle/javascript/ApiGraphs.qll
@@ -57,6 +57,16 @@ module API {
     */
    CallNode getACall() { result = getReturn().getAnImmediateUse() }

+    /**
+     * Gets a call to the function represented by this API component,
+     * or a promisified version of the function.
+     */
+    CallNode getMaybePromisifiedCall() {
+      result = getACall()
+      or
+      result = Impl::getAPromisifiedInvocation(this, _, _)
+    }
+
    /**
     * Gets a `new` call to the function represented by this API component.
     */
@@ -573,10 +583,10 @@ module API {
          ref = pred.getAnInvocation()
          or
          lbl = Label::promised() and
-          PromiseFlow::loadStep(pred, ref, Promises::valueProp())
+          PromiseFlow::loadStep(pred.getALocalUse(), ref, Promises::valueProp())
          or
          lbl = Label::promisedError() and
-          PromiseFlow::loadStep(pred, ref, Promises::errorProp())
+          PromiseFlow::loadStep(pred.getALocalUse(), ref, Promises::errorProp())
        )
        or
        exists(DataFlow::Node def, DataFlow::FunctionNode fn |
@@ -855,10 +865,9 @@ module API {
        succ = MkAsyncFuncResult(f)
      )
      or
-      exists(DataFlow::SourceNode src, int bound, DataFlow::InvokeNode call |
-        use(pred, src) and
+      exists(int bound, DataFlow::InvokeNode call |
        lbl = Label::parameter(bound + call.getNumArgument()) and
-        succ = MkSyntheticCallbackArg(src, bound, call)
+        call = getAPromisifiedInvocation(pred, bound, succ)
      )
    }

@@ -870,6 +879,18 @@ module API {
    /** Gets the shortest distance from the root to `nd` in the API graph. */
    cached
    int distanceFromRoot(TApiNode nd) = shortestDistances(MkRoot/0, edge/2)(_, nd, result)
+
+    /**
+     * Gets a call to a promisified function represented by `callee` where
+     * `bound` arguments have been bound.
+     */
+    cached
+    DataFlow::InvokeNode getAPromisifiedInvocation(TApiNode callee, int bound, TApiNode succ) {
+      exists(DataFlow::SourceNode src |
+        Impl::use(callee, src) and
+        succ = Impl::MkSyntheticCallbackArg(src, bound, result)
+      )
+    }
  }

  import Label as EdgeLabel
@@ -888,7 +909,8 @@ module API {

    InvokeNode() {
      this = callee.getReturn().getAnImmediateUse() or
-      this = callee.getInstance().getAnImmediateUse()
+      this = callee.getInstance().getAnImmediateUse() or
+      this = Impl::getAPromisifiedInvocation(callee, _, _)
    }

    /** Gets the API node for the `i`th parameter of this invocation. */
--- a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
@@ -398,7 +398,7 @@ module NodeJSLib {
    override CallExpr astNode;

    ProcessTermination() {
-      this = DataFlow::moduleImport("exit").getAnInvocation()
+      this = API::moduleImport("exit").getAnInvocation()
      or
      this = processMember("exit").getACall()
    }
@@ -679,13 +679,14 @@ module NodeJSLib {
  /**
   * A call to a method from module `child_process`.
   */
-  private class ChildProcessMethodCall extends SystemCommandExecution, DataFlow::CallNode {
+  private class ChildProcessMethodCall extends SystemCommandExecution, API::CallNode {
    string methodName;

    ChildProcessMethodCall() {
-      this = maybePromisified(DataFlow::moduleMember("child_process", methodName)).getACall()
-      or
-      this = DataFlow::moduleMember("mz/child_process", methodName).getACall()
+      this =
+        API::moduleImport(["mz/child_process", "child_process"])
+            .getMember(methodName)
+            .getMaybePromisifiedCall()
    }

    private DataFlow::Node getACommandArgument(boolean shell) {
@@ -707,7 +708,7 @@ module NodeJSLib {
        )
      ) and
      // all of the above methods take the command as their first argument
-      result = getArgument(0)
+      result = getParameter(0).getARhs()
    }

    override DataFlow::Node getACommandArgument() { result = getACommandArgument(_) }
@@ -723,7 +724,7 @@ module NodeJSLib {
        methodName = "spawnSync"
      ) and
      // all of the above methods take the argument list as their second argument
-      result = getArgument(1)
+      result = getParameter(1).getARhs()
    }

    override predicate isSync() { "Sync" = methodName.suffix(methodName.length() - 4) }
@@ -731,7 +732,7 @@ module NodeJSLib {
    override DataFlow::Node getOptionsArg() {
      not result.getALocalSource() instanceof DataFlow::FunctionNode and // looks like callback
      not result.getALocalSource() instanceof DataFlow::ArrayCreationNode and // looks like argumentlist
-      not result = getArgument(0) and
+      not result = getParameter(0).getARhs() and
      // fork/spawn and all sync methos always has options as the last argument
      if
        methodName.regexpMatch("fork.*") or
@@ -740,7 +741,7 @@ module NodeJSLib {
      then result = getLastArgument()
      else
        // the rest (exec/execFile) has the options argument as their second last.
-        result = getArgument(this.getNumArgument() - 2)
+        result = getParameter(this.getNumArgument() - 2).getARhs()
    }
  }

@@ -1027,9 +1028,9 @@ module NodeJSLib {
  /**
   * Gets an import of the NodeJS EventEmitter.
   */
-  private DataFlow::SourceNode getAnEventEmitterImport() {
-    result = DataFlow::moduleImport("events") or
-    result = DataFlow::moduleMember("events", "EventEmitter")
+  private API::Node getAnEventEmitterImport() {
+    result = API::moduleImport("events") or
+    result = API::moduleImport("events").getMember("EventEmitter")
  }

  /**
@@ -1051,7 +1052,7 @@ module NodeJSLib {
   */
  private class EventEmitterSubClass extends DataFlow::ClassNode {
    EventEmitterSubClass() {
-      this.getASuperClassNode().getALocalSource() = getAnEventEmitterImport() or
+      this.getASuperClassNode() = getAnEventEmitterImport().getAUse() or
      this.getADirectSuperClass() instanceof EventEmitterSubClass
    }
  }
@@ -1186,7 +1187,7 @@ module NodeJSLib {
   * An instantiation of the `respjs` library, which is an EventEmitter.
   */
  private class RespJS extends NodeJSEventEmitter {
-    RespJS() { this = DataFlow::moduleImport("respjs").getAnInstantiation() }
+    RespJS() { this = API::moduleImport("respjs").getAnInstantiation() }
  }

  /**
--- a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected
@@ -246,6 +246,10 @@ nodes
 | lib/lib.js:482:40:482:43 | name |
 | lib/lib.js:483:30:483:33 | name |
 | lib/lib.js:483:30:483:33 | name |
+| lib/lib.js:498:45:498:48 | name |
+| lib/lib.js:498:45:498:48 | name |
+| lib/lib.js:499:31:499:34 | name |
+| lib/lib.js:499:31:499:34 | name |
 | lib/subLib2/compiled-file.ts:3:26:3:29 | name |
 | lib/subLib2/compiled-file.ts:3:26:3:29 | name |
 | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
@@ -558,6 +562,10 @@ edges
 | lib/lib.js:482:40:482:43 | name | lib/lib.js:483:30:483:33 | name |
 | lib/lib.js:482:40:482:43 | name | lib/lib.js:483:30:483:33 | name |
 | lib/lib.js:482:40:482:43 | name | lib/lib.js:483:30:483:33 | name |
+| lib/lib.js:498:45:498:48 | name | lib/lib.js:499:31:499:34 | name |
+| lib/lib.js:498:45:498:48 | name | lib/lib.js:499:31:499:34 | name |
+| lib/lib.js:498:45:498:48 | name | lib/lib.js:499:31:499:34 | name |
+| lib/lib.js:498:45:498:48 | name | lib/lib.js:499:31:499:34 | name |
 | lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
 | lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
 | lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
@@ -648,6 +656,7 @@ edges
 | lib/lib.js:447:13:447:28 | "rm -rf " + name | lib/lib.js:446:20:446:23 | name | lib/lib.js:447:25:447:28 | name | $@ based on $@ is later used in $@. | lib/lib.js:447:13:447:28 | "rm -rf " + name | String concatenation | lib/lib.js:446:20:446:23 | name | library input | lib/lib.js:447:3:447:29 | asyncEx ... + name) | shell command |
 | lib/lib.js:478:27:478:46 | config.installedPath | lib/lib.js:477:33:477:38 | config | lib/lib.js:478:27:478:46 | config.installedPath | $@ based on $@ is later used in $@. | lib/lib.js:478:27:478:46 | config.installedPath | Path concatenation | lib/lib.js:477:33:477:38 | config | library input | lib/lib.js:479:12:479:20 | exec(cmd) | shell command |
 | lib/lib.js:483:13:483:33 | ' my na ...  + name | lib/lib.js:482:40:482:43 | name | lib/lib.js:483:30:483:33 | name | $@ based on $@ is later used in $@. | lib/lib.js:483:13:483:33 | ' my na ...  + name | String concatenation | lib/lib.js:482:40:482:43 | name | library input | lib/lib.js:485:2:485:20 | cp.exec(cmd + args) | shell command |
+| lib/lib.js:499:19:499:34 | "rm -rf " + name | lib/lib.js:498:45:498:48 | name | lib/lib.js:499:31:499:34 | name | $@ based on $@ is later used in $@. | lib/lib.js:499:19:499:34 | "rm -rf " + name | String concatenation | lib/lib.js:498:45:498:48 | name | library input | lib/lib.js:499:3:499:35 | MyThing ... + name) | shell command |
 | lib/subLib2/compiled-file.ts:4:13:4:28 | "rm -rf " + name | lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name | $@ based on $@ is later used in $@. | lib/subLib2/compiled-file.ts:4:13:4:28 | "rm -rf " + name | String concatenation | lib/subLib2/compiled-file.ts:3:26:3:29 | name | library input | lib/subLib2/compiled-file.ts:4:5:4:29 | cp.exec ... + name) | shell command |
 | lib/subLib2/special-file.js:4:10:4:25 | "rm -rf " + name | lib/subLib2/special-file.js:3:28:3:31 | name | lib/subLib2/special-file.js:4:22:4:25 | name | $@ based on $@ is later used in $@. | lib/subLib2/special-file.js:4:10:4:25 | "rm -rf " + name | String concatenation | lib/subLib2/special-file.js:3:28:3:31 | name | library input | lib/subLib2/special-file.js:4:2:4:26 | cp.exec ... + name) | shell command |
 | lib/subLib3/my-file.ts:4:10:4:25 | "rm -rf " + name | lib/subLib3/my-file.ts:3:28:3:31 | name | lib/subLib3/my-file.ts:4:22:4:25 | name | $@ based on $@ is later used in $@. | lib/subLib3/my-file.ts:4:10:4:25 | "rm -rf " + name | String concatenation | lib/subLib3/my-file.ts:3:28:3:31 | name | library input | lib/subLib3/my-file.ts:4:2:4:26 | cp.exec ... + name) | shell command |
--- a/javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js
+++ b/javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js
@@ -488,4 +488,15 @@ module.exports.splitConcat = function (name) {
 module.exports.myCommand = function (myCommand) {
 	let cmd = `cd ${cwd} ; ${myCommand}`; // OK - the parameter name suggests that it is purposely a shell command.
 	cp.exec(cmd);
-}
+}
+
+(function () {
+	var MyThing = {
+		cp: require('child_process')
+	};
+
+	module.exports.myIndirectThing = function (name) {
+		MyThing.cp.exec("rm -rf " + name); // NOT OK
+	}
+});
+