Merge pull request #6261 from max-schaefer/js/module-constructor

codeql-ci · web-flow · commit b14139f3a07c · 2021-07-16T00:28:30.000-07:00
Approved by asgerf
diff --git a/javascript/ql/src/semmle/javascript/ApiGraphs.qll b/javascript/ql/src/semmle/javascript/ApiGraphs.qll
@@ -617,11 +617,11 @@ module API {
     cached
     predicate use(TApiNode nd, DataFlow::Node ref) {
       exists(string m, Module mod | nd = MkModuleDef(m) and mod = importableModule(m) |
-        ref.(ModuleVarNode).getModule() = mod
+        ref = DataFlow::moduleVarNode(mod)
       )
       or
       exists(string m, Module mod | nd = MkModuleExport(m) and mod = importableModule(m) |
-        ref.(ExportsVarNode).getModule() = mod
+        ref = DataFlow::exportsVarNode(mod)
         or
         exists(DataFlow::Node base | use(MkModuleDef(m), base) |
           ref = trackUseNode(base).getAPropertyRead("exports")
@@ -742,12 +742,9 @@ module API {
       or
       // additional backwards step from `require('m')` to `exports` or `module.exports` in m
       exists(Import imp | imp.getImportedModuleNode() = trackDefNode(nd, t.continue()) |
-        result.(ExportsVarNode).getModule() = imp.getImportedModule()
+        result = DataFlow::exportsVarNode(imp.getImportedModule())
         or
-        exists(ModuleVarNode mod |
-          mod.getModule() = imp.getImportedModule() and
-          result = mod.(DataFlow::SourceNode).getAPropertyRead("exports")
-        )
+        result = DataFlow::moduleVarNode(imp.getImportedModule()).getAPropertyRead("exports")
       )
       or
       t = defStep(nd, result)
@@ -981,46 +978,3 @@ private module Label {
   /** Gets the `promisedError` edge label connecting a promise to its rejected value. */
   string promisedError() { result = "promisedError" }
 }
-
-private class NodeModuleSourcesNodes extends DataFlow::SourceNode::Range {
-  Variable v;
-
-  NodeModuleSourcesNodes() {
-    exists(NodeModule m |
-      this = DataFlow::ssaDefinitionNode(SSA::implicitInit(v)) and
-      v = [m.getModuleVariable(), m.getExportsVariable()]
-    )
-  }
-
-  Variable getVariable() { result = v }
-}
-
-/**
- * A CommonJS/AMD `module` variable.
- */
-private class ModuleVarNode extends DataFlow::Node {
-  Module m;
-
-  ModuleVarNode() {
-    this.(NodeModuleSourcesNodes).getVariable() = m.(NodeModule).getModuleVariable()
-    or
-    DataFlow::parameterNode(this, m.(AmdModule).getDefine().getModuleParameter())
-  }
-
-  Module getModule() { result = m }
-}
-
-/**
- * A CommonJS/AMD `exports` variable.
- */
-private class ExportsVarNode extends DataFlow::Node {
-  Module m;
-
-  ExportsVarNode() {
-    this.(NodeModuleSourcesNodes).getVariable() = m.(NodeModule).getExportsVariable()
-    or
-    DataFlow::parameterNode(this, m.(AmdModule).getDefine().getExportsParameter())
-  }
-
-  Module getModule() { result = m }
-}
diff --git a/javascript/ql/src/semmle/javascript/dataflow/Sources.qll b/javascript/ql/src/semmle/javascript/dataflow/Sources.qll
@@ -347,6 +347,55 @@ module SourceNode {
   }
 }
 
+private class NodeModuleSourcesNodes extends SourceNode::Range {
+  Variable v;
+
+  NodeModuleSourcesNodes() {
+    exists(NodeModule m |
+      this = DataFlow::ssaDefinitionNode(SSA::implicitInit(v)) and
+      v = [m.getModuleVariable(), m.getExportsVariable()]
+    )
+  }
+
+  Variable getVariable() { result = v }
+}
+
+/**
+ * A CommonJS/AMD `module` variable.
+ */
+private class ModuleVarNode extends DataFlow::Node {
+  Module m;
+
+  ModuleVarNode() {
+    this.(NodeModuleSourcesNodes).getVariable() = m.(NodeModule).getModuleVariable()
+    or
+    DataFlow::parameterNode(this, m.(AmdModule).getDefine().getModuleParameter())
+  }
+
+  Module getModule() { result = m }
+}
+
+/**
+ * A CommonJS/AMD `exports` variable.
+ */
+private class ExportsVarNode extends DataFlow::Node {
+  Module m;
+
+  ExportsVarNode() {
+    this.(NodeModuleSourcesNodes).getVariable() = m.(NodeModule).getExportsVariable()
+    or
+    DataFlow::parameterNode(this, m.(AmdModule).getDefine().getExportsParameter())
+  }
+
+  Module getModule() { result = m }
+}
+
+/** Gets the CommonJS/AMD `module` variable for module `m`. */
+SourceNode moduleVarNode(Module m) { result.(ModuleVarNode).getModule() = m }
+
+/** Gets the CommonJS/AMD `exports` variable for module `m`. */
+SourceNode exportsVarNode(Module m) { result.(ExportsVarNode).getModule() = m }
+
 deprecated class DefaultSourceNode extends SourceNode {
   DefaultSourceNode() { this instanceof SourceNode::DefaultRange }
 }
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/CodeInjectionCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/CodeInjectionCustomizations.qll
@@ -169,13 +169,19 @@ module CodeInjection {
   }
 
   /**
-   * The first argument to `Module.prototype._compile` from the Node.js built-in module `module`,
-   * considered as a code-injection sink.
+   * The first argument to `Module.prototype._compile`, considered as a code-injection sink.
    */
   class ModuleCompileSink extends Sink {
     ModuleCompileSink() {
+      // `require('module').prototype._compile`
       this =
         API::moduleImport("module").getInstance().getMember("_compile").getACall().getArgument(0)
+      or
+      // `module.constructor.prototype._compile`
+      exists(DataFlow::SourceNode moduleConstructor |
+        moduleConstructor = DataFlow::moduleVarNode(_).getAPropertyRead("constructor") and
+        this = moduleConstructor.getAnInstantiation().getAMethodCall("_compile").getArgument(0)
+      )
     }
   }
 
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.expected b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.expected
@@ -97,6 +97,9 @@ nodes
 | module.js:9:16:9:29 | req.query.code |
 | module.js:9:16:9:29 | req.query.code |
 | module.js:9:16:9:29 | req.query.code |
+| module.js:11:17:11:30 | req.query.code |
+| module.js:11:17:11:30 | req.query.code |
+| module.js:11:17:11:30 | req.query.code |
 | react-native.js:7:7:7:33 | tainted |
 | react-native.js:7:17:7:33 | req.param("code") |
 | react-native.js:7:17:7:33 | req.param("code") |
@@ -221,6 +224,7 @@ edges
 | express.js:19:37:19:70 | req.par ... odule") | express.js:19:37:19:70 | req.par ... odule") |
 | express.js:21:19:21:48 | req.par ... ntext") | express.js:21:19:21:48 | req.par ... ntext") |
 | module.js:9:16:9:29 | req.query.code | module.js:9:16:9:29 | req.query.code |
+| module.js:11:17:11:30 | req.query.code | module.js:11:17:11:30 | req.query.code |
 | react-native.js:7:7:7:33 | tainted | react-native.js:8:32:8:38 | tainted |
 | react-native.js:7:7:7:33 | tainted | react-native.js:8:32:8:38 | tainted |
 | react-native.js:7:7:7:33 | tainted | react-native.js:10:23:10:29 | tainted |
@@ -305,6 +309,7 @@ edges
 | express.js:19:37:19:70 | req.par ... odule") | express.js:19:37:19:70 | req.par ... odule") | express.js:19:37:19:70 | req.par ... odule") | $@ flows to here and is interpreted as code. | express.js:19:37:19:70 | req.par ... odule") | User-provided value |
 | express.js:21:19:21:48 | req.par ... ntext") | express.js:21:19:21:48 | req.par ... ntext") | express.js:21:19:21:48 | req.par ... ntext") | $@ flows to here and is interpreted as code. | express.js:21:19:21:48 | req.par ... ntext") | User-provided value |
 | module.js:9:16:9:29 | req.query.code | module.js:9:16:9:29 | req.query.code | module.js:9:16:9:29 | req.query.code | $@ flows to here and is interpreted as code. | module.js:9:16:9:29 | req.query.code | User-provided value |
+| module.js:11:17:11:30 | req.query.code | module.js:11:17:11:30 | req.query.code | module.js:11:17:11:30 | req.query.code | $@ flows to here and is interpreted as code. | module.js:11:17:11:30 | req.query.code | User-provided value |
 | react-native.js:8:32:8:38 | tainted | react-native.js:7:17:7:33 | req.param("code") | react-native.js:8:32:8:38 | tainted | $@ flows to here and is interpreted as code. | react-native.js:7:17:7:33 | req.param("code") | User-provided value |
 | react-native.js:10:23:10:29 | tainted | react-native.js:7:17:7:33 | req.param("code") | react-native.js:10:23:10:29 | tainted | $@ flows to here and is interpreted as code. | react-native.js:7:17:7:33 | req.param("code") | User-provided value |
 | react.js:10:56:10:77 | documen ... on.hash | react.js:10:56:10:77 | documen ... on.hash | react.js:10:56:10:77 | documen ... on.hash | $@ flows to here and is interpreted as code. | react.js:10:56:10:77 | documen ... on.hash | User-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/HeuristicSourceCodeInjection.expected b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/HeuristicSourceCodeInjection.expected
@@ -101,6 +101,9 @@ nodes
 | module.js:9:16:9:29 | req.query.code |
 | module.js:9:16:9:29 | req.query.code |
 | module.js:9:16:9:29 | req.query.code |
+| module.js:11:17:11:30 | req.query.code |
+| module.js:11:17:11:30 | req.query.code |
+| module.js:11:17:11:30 | req.query.code |
 | react-native.js:7:7:7:33 | tainted |
 | react-native.js:7:17:7:33 | req.param("code") |
 | react-native.js:7:17:7:33 | req.param("code") |
@@ -229,6 +232,7 @@ edges
 | express.js:19:37:19:70 | req.par ... odule") | express.js:19:37:19:70 | req.par ... odule") |
 | express.js:21:19:21:48 | req.par ... ntext") | express.js:21:19:21:48 | req.par ... ntext") |
 | module.js:9:16:9:29 | req.query.code | module.js:9:16:9:29 | req.query.code |
+| module.js:11:17:11:30 | req.query.code | module.js:11:17:11:30 | req.query.code |
 | react-native.js:7:7:7:33 | tainted | react-native.js:8:32:8:38 | tainted |
 | react-native.js:7:7:7:33 | tainted | react-native.js:8:32:8:38 | tainted |
 | react-native.js:7:7:7:33 | tainted | react-native.js:10:23:10:29 | tainted |
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/module.js b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/module.js
@@ -7,4 +7,6 @@ app.get('/some/path', function (req, res) {
     let filename = req.query.filename;
     var m = new Module(filename, module.parent);
     m._compile(req.query.code, filename); // NOT OK
+    var m2 = new module.constructor;
+    m2._compile(req.query.code, filename); // NOT OK
 });
