Merge pull request #4868 from erik-krogh/boundShell

Approved by esbena

Author: codeql-ci

javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll

@@ -628,7 +628,11 @@ module DataFlow {
     override string getPropertyName() { result = astNode.getArgument(1).getStringValue() }
 
     override Node getRhs() {
-      result = astNode.getArgument(2).(ObjectExpr).getPropertyByName("value").getInit().flow()
+      exists(ObjectExpr obj | obj = astNode.getArgument(2) |
+        result = obj.getPropertyByName("value").getInit().flow()
+        or
+        result = obj.getPropertyByName("get").getInit().flow().(DataFlow::FunctionNode).getAReturn()
+      )
     }
 
     override ControlFlowNode getWriteNode() { result = astNode }

javascript/ql/src/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll

@@ -51,10 +51,12 @@ module UnsafeShellCommandConstruction {
    */
   class ExternalInputSource extends Source, DataFlow::ParameterNode {
     ExternalInputSource() {
-      this =
-        Exports::getAValueExportedBy(Exports::getTopmostPackageJSON())
-            .getAFunctionValue()
-            .getAParameter() and
+      exists(int bound, DataFlow::FunctionNode func |
+        func =
+          Exports::getAValueExportedBy(Exports::getTopmostPackageJSON())
+              .getABoundFunctionValue(bound) and
+        this = func.getParameter(any(int arg | arg >= bound))
+      ) and
       not this.getName() = ["cmd", "command"] // looks to be on purpose.
     }
   }

javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected

@@ -191,6 +191,10 @@ nodes
 | lib/lib.js:340:22:340:26 | id(n) |
 | lib/lib.js:340:22:340:26 | id(n) |
 | lib/lib.js:340:25:340:25 | n |
+| lib/lib.js:343:29:343:34 | unsafe |
+| lib/lib.js:343:29:343:34 | unsafe |
+| lib/lib.js:345:22:345:27 | unsafe |
+| lib/lib.js:345:22:345:27 | unsafe |
 edges
 | lib/lib2.js:3:28:3:31 | name | lib/lib2.js:4:22:4:25 | name |
 | lib/lib2.js:3:28:3:31 | name | lib/lib2.js:4:22:4:25 | name |
@@ -417,6 +421,10 @@ edges
 | lib/lib.js:339:39:339:39 | n | lib/lib.js:340:25:340:25 | n |
 | lib/lib.js:340:25:340:25 | n | lib/lib.js:340:22:340:26 | id(n) |
 | lib/lib.js:340:25:340:25 | n | lib/lib.js:340:22:340:26 | id(n) |
+| lib/lib.js:343:29:343:34 | unsafe | lib/lib.js:345:22:345:27 | unsafe |
+| lib/lib.js:343:29:343:34 | unsafe | lib/lib.js:345:22:345:27 | unsafe |
+| lib/lib.js:343:29:343:34 | unsafe | lib/lib.js:345:22:345:27 | unsafe |
+| lib/lib.js:343:29:343:34 | unsafe | lib/lib.js:345:22:345:27 | unsafe |
 #select
 | lib/lib2.js:4:10:4:25 | "rm -rf " + name | lib/lib2.js:3:28:3:31 | name | lib/lib2.js:4:22:4:25 | name | $@ based on library input is later used in $@. | lib/lib2.js:4:10:4:25 | "rm -rf " + name | String concatenation | lib/lib2.js:4:2:4:26 | cp.exec ... + name) | shell command |
 | lib/lib2.js:8:10:8:25 | "rm -rf " + name | lib/lib2.js:7:32:7:35 | name | lib/lib2.js:8:22:8:25 | name | $@ based on library input is later used in $@. | lib/lib2.js:8:10:8:25 | "rm -rf " + name | String concatenation | lib/lib2.js:8:2:8:26 | cp.exec ... + name) | shell command |
@@ -472,3 +480,4 @@ edges
 | lib/lib.js:320:11:320:26 | "rm -rf " + name | lib/lib.js:314:40:314:43 | name | lib/lib.js:320:23:320:26 | name | $@ based on library input is later used in $@. | lib/lib.js:320:11:320:26 | "rm -rf " + name | String concatenation | lib/lib.js:320:3:320:27 | cp.exec ... + name) | shell command |
 | lib/lib.js:325:12:325:51 | "MyWind ... " + arg | lib/lib.js:324:40:324:42 | arg | lib/lib.js:325:49:325:51 | arg | $@ based on library input is later used in $@. | lib/lib.js:325:12:325:51 | "MyWind ... " + arg | String concatenation | lib/lib.js:326:2:326:13 | cp.exec(cmd) | shell command |
 | lib/lib.js:340:10:340:26 | "rm -rf " + id(n) | lib/lib.js:339:39:339:39 | n | lib/lib.js:340:22:340:26 | id(n) | $@ based on library input is later used in $@. | lib/lib.js:340:10:340:26 | "rm -rf " + id(n) | String concatenation | lib/lib.js:340:2:340:27 | cp.exec ...  id(n)) | shell command |
+| lib/lib.js:345:10:345:27 | "rm -rf " + unsafe | lib/lib.js:343:29:343:34 | unsafe | lib/lib.js:345:22:345:27 | unsafe | $@ based on library input is later used in $@. | lib/lib.js:345:10:345:27 | "rm -rf " + unsafe | String concatenation | lib/lib.js:345:2:345:28 | cp.exec ... unsafe) | shell command |

javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js

@@ -339,3 +339,14 @@ module.exports.unproblematic = function() {
 module.exports.problematic = function(n) {
 	cp.exec("rm -rf " + id(n)); // NOT OK
 };
+
+function boundProblem(safe, unsafe) {
+	cp.exec("rm -rf " + safe); // OK
+	cp.exec("rm -rf " + unsafe); // NOT OK
+}
+
+Object.defineProperty(module.exports, "boundProblem", {
+	get: function () {
+		return boundProblem.bind(this, "safe");
+	}
+});