Merge pull request github#6472 from erik-krogh/apiPromise

Approved by asgerf

Author: codeql-ci

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. */

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,15 +724,15 @@ 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) }
 
     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() }
   }
 
   /**

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 |

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