Merge pull request github#7870 from erik-krogh/nodeReExport

Approved by esbena

Author: codeql-ci

javascript/ql/lib/semmle/javascript/NodeJS.qll

@@ -2,6 +2,7 @@
 
 import javascript
 private import NodeModuleResolutionImpl
+private import semmle.javascript.DynamicPropertyAccess as DynamicPropertyAccess
 
 /**
  * A Node.js module.
@@ -90,6 +91,18 @@ class NodeModule extends Module {
             .getAnExportedValue(name)
     )
     or
+    // var imp = require('./imp');
+    // for (var name in imp){
+    //   module.exports[name] = imp[name];
+    // }
+    exists(DynamicPropertyAccess::EnumeratedPropName read, Import imp, DataFlow::PropWrite write |
+      read.getSourceObject().getALocalSource().asExpr() = imp and
+      getASourceProp(read) = write.getRhs() and
+      write.getBase() = this.getAModuleExportsNode() and
+      write.getPropertyNameExpr().flow().getImmediatePredecessor*() = read and
+      result = imp.getImportedModule().getAnExportedValue(name)
+    )
+    or
     // an externs definition (where appropriate)
     exists(PropAccess pacc | result = DataFlow::valueNode(pacc) |
       pacc.getBase() = this.getAModuleExportsNode().asExpr() and
@@ -150,6 +163,21 @@ class NodeModule extends Module {
   }
 }
 
+// An copy of `DynamicPropertyAccess::EnumeratedPropName::getASourceProp` that doesn't use the callgraph.
+// This avoids making the module-imports recursive with the callgraph.
+private DataFlow::SourceNode getASourceProp(DynamicPropertyAccess::EnumeratedPropName prop) {
+  exists(DataFlow::Node base, DataFlow::Node key |
+    exists(DynamicPropertyAccess::DynamicPropRead read |
+      not read.hasDominatingAssignment() and
+      base = read.getBase() and
+      key = read.getPropertyNameNode() and
+      result = read
+    ) and
+    prop.getASourceObjectRef().flowsTo(base) and
+    key.getImmediatePredecessor*() = prop
+  )
+}
+
 /**
  * Gets an expression that syntactically could be a alias for `module.exports`.
  * This predicate exists to reduce the size of `getAModuleExportsNode`,
@@ -158,7 +186,7 @@ class NodeModule extends Module {
 pragma[noinline]
 private DataFlow::Node getAModuleExportsCandidate() {
   // A bit of manual magic
-  result = any(DataFlow::PropWrite w | exists(w.getPropertyName())).getBase()
+  result = any(DataFlow::PropWrite w).getBase()
   or
   result = DataFlow::valueNode(any(PropAccess p | exists(p.getPropertyName())).getBase())
   or

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

@@ -1,4 +1,8 @@
 nodes
+| lib/isImported.js:5:49:5:52 | name |
+| lib/isImported.js:5:49:5:52 | name |
+| lib/isImported.js:6:22:6:25 | name |
+| lib/isImported.js:6:22:6:25 | name |
 | lib/lib2.js:3:28:3:31 | name |
 | lib/lib2.js:3:28:3:31 | name |
 | lib/lib2.js:4:22:4:25 | name |
@@ -271,6 +275,10 @@ nodes
 | lib/subLib/index.js:8:22:8:25 | name |
 | lib/subLib/index.js:8:22:8:25 | name |
 edges
+| lib/isImported.js:5:49:5:52 | name | lib/isImported.js:6:22:6:25 | name |
+| lib/isImported.js:5:49:5:52 | name | lib/isImported.js:6:22:6:25 | name |
+| lib/isImported.js:5:49:5:52 | name | lib/isImported.js:6:22:6:25 | name |
+| lib/isImported.js:5:49:5:52 | name | lib/isImported.js:6:22:6:25 | name |
 | 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 |
 | lib/lib2.js:3:28:3:31 | name | lib/lib2.js:4:22:4:25 | name |
@@ -587,6 +595,7 @@ edges
 | lib/subLib/index.js:7:32:7:35 | name | lib/subLib/index.js:8:22:8:25 | name |
 | lib/subLib/index.js:7:32:7:35 | name | lib/subLib/index.js:8:22:8:25 | name |
 #select
+| lib/isImported.js:6:10:6:25 | "rm -rf " + name | lib/isImported.js:5:49:5:52 | name | lib/isImported.js:6:22:6:25 | name | $@ based on $@ is later used in $@. | lib/isImported.js:6:10:6:25 | "rm -rf " + name | String concatenation | lib/isImported.js:5:49:5:52 | name | library input | lib/isImported.js:6:2:6:26 | cp.exec ... + name) | shell command |
 | 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 $@ is later used in $@. | lib/lib2.js:4:10:4:25 | "rm -rf " + name | String concatenation | lib/lib2.js:3:28:3:31 | name | library input | 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 $@ is later used in $@. | lib/lib2.js:8:10:8:25 | "rm -rf " + name | String concatenation | lib/lib2.js:7:32:7:35 | name | library input | lib/lib2.js:8:2:8:26 | cp.exec ... + name) | shell command |
 | lib/lib.js:4:10:4:25 | "rm -rf " + name | lib/lib.js:3:28:3:31 | name | lib/lib.js:4:22:4:25 | name | $@ based on $@ is later used in $@. | lib/lib.js:4:10:4:25 | "rm -rf " + name | String concatenation | lib/lib.js:3:28:3:31 | name | library input | lib/lib.js:4:2:4:26 | cp.exec ... + name) | shell command |

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

@@ -0,0 +1,7 @@
+// is imported from lib.js
+
+const cp = require("child_process");
+
+module.exports.thisMethodIsImported = function (name) {
+	cp.exec("rm -rf " + name); // NOT OK
+}

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

@@ -499,4 +499,9 @@ module.exports.myCommand = function (myCommand) {
 		MyThing.cp.exec("rm -rf " + name); // NOT OK
 	}
 });
-  
+  
+
+var imp = require('./isImported');
+for (var name in imp){
+  module.exports[name] = imp[name];
+}