recognize exported functions that use the arguments object

erik-krogh · erik-krogh · commit 2d65aa17db2c · 2021-10-27T20:35:37.000+02:00
diff --git a/javascript/ql/lib/semmle/javascript/PackageExports.qll b/javascript/ql/lib/semmle/javascript/PackageExports.qll
@@ -11,11 +11,18 @@ private import semmle.javascript.internal.CachedStages
  * Gets a parameter that is a library input to a top-level package.
  */
 cached
-DataFlow::ParameterNode getALibraryInputParameter() {
+DataFlow::SourceNode getALibraryInputParameter() {
   Stages::Taint::ref() and
   exists(int bound, DataFlow::FunctionNode func |
-    func = getAValueExportedByPackage().getABoundFunctionValue(bound) and
+    func = getAValueExportedByPackage().getABoundFunctionValue(bound)
+  |
     result = func.getParameter(any(int arg | arg >= bound))
+    or
+    exists(DataFlow::PropRead read |
+      not read.getPropertyName() = "length" and
+      result = read and
+      read.getBase() = func.getFunction().getArgumentsVariable().getAnAccess().flow()
+    )
   )
 }
 
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
@@ -50,14 +50,14 @@ module UnsafeShellCommandConstruction {
   /**
    * A parameter of an exported function, seen as a source for shell command constructed from library input.
    */
-  class ExternalInputSource extends Source, DataFlow::ParameterNode {
+  class ExternalInputSource extends Source, DataFlow::SourceNode {
     ExternalInputSource() {
       this = Exports::getALibraryInputParameter() and
       not (
         // looks to be on purpose.
-        this.getName() = ["cmd", "command"]
+        this.(DataFlow::ParameterNode).getName() = ["cmd", "command"]
         or
-        this.getName().regexpMatch(".*(Cmd|Command)$") // ends with "Cmd" or "Command"
+        this.(DataFlow::ParameterNode).getName().regexpMatch(".*(Cmd|Command)$") // ends with "Cmd" or "Command"
       )
     }
   }
diff --git a/javascript/ql/lib/semmle/javascript/security/performance/PolynomialReDoSCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/performance/PolynomialReDoSCustomizations.qll
@@ -126,7 +126,7 @@ module PolynomialReDoS {
   /**
    * A parameter of an exported function, seen as a source for polynomial-redos.
    */
-  class ExternalInputSource extends Source, DataFlow::ParameterNode {
+  class ExternalInputSource extends Source, DataFlow::SourceNode {
     ExternalInputSource() { this = Exports::getALibraryInputParameter() }
 
     override string getKind() { result = "library" }
diff --git a/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected b/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected
@@ -19,6 +19,19 @@ nodes
 | lib.js:11:35:11:38 | path |
 | lib.js:11:35:11:47 | path.slice(1) |
 | lib.js:11:35:11:47 | path.slice(1) |
+| lib.js:14:38:14:41 | path |
+| lib.js:14:38:14:41 | path |
+| lib.js:15:3:15:14 | obj[path[0]] |
+| lib.js:15:3:15:14 | obj[path[0]] |
+| lib.js:15:7:15:10 | path |
+| lib.js:15:7:15:13 | path[0] |
+| lib.js:20:7:20:25 | path |
+| lib.js:20:14:20:25 | arguments[1] |
+| lib.js:20:14:20:25 | arguments[1] |
+| lib.js:22:3:22:14 | obj[path[0]] |
+| lib.js:22:3:22:14 | obj[path[0]] |
+| lib.js:22:7:22:10 | path |
+| lib.js:22:7:22:13 | path[0] |
 | tst.js:5:9:5:38 | taint |
 | tst.js:5:17:5:38 | String( ... y.data) |
 | tst.js:5:24:5:37 | req.query.data |
@@ -66,6 +79,17 @@ edges
 | lib.js:11:35:11:38 | path | lib.js:11:35:11:47 | path.slice(1) |
 | lib.js:11:35:11:47 | path.slice(1) | lib.js:1:43:1:46 | path |
 | lib.js:11:35:11:47 | path.slice(1) | lib.js:1:43:1:46 | path |
+| lib.js:14:38:14:41 | path | lib.js:15:7:15:10 | path |
+| lib.js:14:38:14:41 | path | lib.js:15:7:15:10 | path |
+| lib.js:15:7:15:10 | path | lib.js:15:7:15:13 | path[0] |
+| lib.js:15:7:15:13 | path[0] | lib.js:15:3:15:14 | obj[path[0]] |
+| lib.js:15:7:15:13 | path[0] | lib.js:15:3:15:14 | obj[path[0]] |
+| lib.js:20:7:20:25 | path | lib.js:22:7:22:10 | path |
+| lib.js:20:14:20:25 | arguments[1] | lib.js:20:7:20:25 | path |
+| lib.js:20:14:20:25 | arguments[1] | lib.js:20:7:20:25 | path |
+| lib.js:22:7:22:10 | path | lib.js:22:7:22:13 | path[0] |
+| lib.js:22:7:22:13 | path[0] | lib.js:22:3:22:14 | obj[path[0]] |
+| lib.js:22:7:22:13 | path[0] | lib.js:22:3:22:14 | obj[path[0]] |
 | tst.js:5:9:5:38 | taint | tst.js:8:12:8:16 | taint |
 | tst.js:5:9:5:38 | taint | tst.js:9:12:9:16 | taint |
 | tst.js:5:9:5:38 | taint | tst.js:12:25:12:29 | taint |
@@ -91,6 +115,8 @@ edges
 | tst.js:33:23:33:25 | obj | tst.js:48:9:48:11 | obj |
 #select
 | lib.js:6:7:6:9 | obj | lib.js:1:43:1:46 | path | lib.js:6:7:6:9 | obj | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | lib.js:1:43:1:46 | path | here |
+| lib.js:15:3:15:14 | obj[path[0]] | lib.js:14:38:14:41 | path | lib.js:15:3:15:14 | obj[path[0]] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | lib.js:14:38:14:41 | path | here |
+| lib.js:22:3:22:14 | obj[path[0]] | lib.js:20:14:20:25 | arguments[1] | lib.js:22:3:22:14 | obj[path[0]] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | lib.js:20:14:20:25 | arguments[1] | here |
 | tst.js:8:5:8:17 | object[taint] | tst.js:5:24:5:37 | req.query.data | tst.js:8:5:8:17 | object[taint] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:5:24:5:37 | req.query.data | here |
 | tst.js:9:5:9:17 | object[taint] | tst.js:5:24:5:37 | req.query.data | tst.js:9:5:9:17 | object[taint] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:5:24:5:37 | req.query.data | here |
 | tst.js:14:5:14:32 | unsafeG ...  taint) | tst.js:5:24:5:37 | req.query.data | tst.js:14:5:14:32 | unsafeG ...  taint) | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:5:24:5:37 | req.query.data | here |
diff --git a/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/lib.js b/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/lib.js
@@ -9,4 +9,15 @@ module.exports.set = function recSet(obj, path, value) {
   }
 
   return recSet(obj[currentPath], path.slice(1), value);
+}
+
+module.exports.set2 = function (obj, path, value) {
+  obj[path[0]][path[1]] = value; // NOT OK
+}
+
+module.exports.setWithArgs = function() {
+  var obj = arguments[0];
+  var path = arguments[1];
+  var value = arguments[2];
+  obj[path[0]][path[1]] = value; // NOT OK
 }

Original file line number	Diff line number	Diff line change
`@@ -50,14 +50,14 @@ module UnsafeShellCommandConstruction {`
`50`	`50`	`/**`
`51`	`51`	`* A parameter of an exported function, seen as a source for shell command constructed from library input.`
`52`	`52`	`*/`
`53`		`- class ExternalInputSource extends Source, DataFlow::ParameterNode {`
	`53`	`+ class ExternalInputSource extends Source, DataFlow::SourceNode {`
`54`	`54`	`ExternalInputSource() {`
`55`	`55`	`this = Exports::getALibraryInputParameter() and`
`56`	`56`	`not (`
`57`	`57`	`// looks to be on purpose.`
`58`		`- this.getName() = ["cmd", "command"]`
	`58`	`+ this.(DataFlow::ParameterNode).getName() = ["cmd", "command"]`
`59`	`59`	`or`
`60`		`- this.getName().regexpMatch(".*(Cmd\|Command)$") // ends with "Cmd" or "Command"`
	`60`	`+ this.(DataFlow::ParameterNode).getName().regexpMatch(".*(Cmd\|Command)$") // ends with "Cmd" or "Command"`
`61`	`61`	`)`
`62`	`62`	`}`
`63`	`63`	`}`