github · codeql-ci · Feb 10, 2022 · May 5, 2021 · May 5, 2021 · May 5, 2021
@@ -216,6 +216,10 @@ private DataFlow::Node getAnExportFromModule(Module mod) {
   or
   result = mod.getABulkExportedNode()
   or
+  // exports saved to the global object
+  result = DataFlow::globalObjectRef().getAPropertyWrite().getRhs() and
+  result.getTopLevel() = mod
+  or
   result.analyze().getAValue() = TAbstractModuleObject(mod)
 }
 

@@ -252,6 +252,25 @@ module CodeInjection {
     }
   }
 
+  /**
+   * A system command execution of "node", where the executed code is seen as a code injection sink.
+   */
+  class NodeCallSink extends Sink {
+    NodeCallSink() {
+      exists(SystemCommandExecution s |
+        s.getACommandArgument().mayHaveStringValue("node")
+        or
+        s.getACommandArgument() =
+          DataFlow::globalVarRef("process").getAPropertyRead("argv").getAPropertyRead("0")
+      |
+        exists(DataFlow::SourceNode arr | arr = s.getArgumentList().getALocalSource() |
+          arr.getAPropertyWrite().getRhs().mayHaveStringValue("-e") and
+          this = arr.getAPropertyWrite().getRhs()
+        )
+      )
+    }
+  }
+
   /** A sink for code injection via template injection. */
   abstract private class TemplateSink extends Sink {
     override string getMessageSuffix() {
@@ -356,4 +375,9 @@ module CodeInjection {
       this = LodashUnderscore::member("template").getACall().getArgument(0)
     }
   }
+
+  /**
+   * A call to JSON.stringify() seen as a sanitizer.
+   */
+  class JSONStringifySanitizer extends Sanitizer, JsonStringifyCall { }
 }
@@ -0,0 +1,47 @@
+/**
+ * Provides a taint-tracking configuration for reasoning about code
+ * constructed from libary input vulnerabilities.
+ *
+ * Note, for performance reasons: only import this file if
+ * `UnsafeCodeConstruction::Configuration` is needed, otherwise
+ * `UnsafeCodeConstructionCustomizations` should be imported instead.
+ */
+
+import javascript
+
+/**
+ * Classes and predicates for the code constructed from library input query.
+ */
+module UnsafeCodeConstruction {
+  private import semmle.javascript.security.dataflow.CodeInjectionCustomizations::CodeInjection as CodeInjection
+  import UnsafeCodeConstructionCustomizations::UnsafeCodeConstruction
+
+  /**
+   * A taint-tracking configuration for reasoning about unsafe code constructed from library input.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() { this = "UnsafeCodeConstruction" }
+
+    override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+    override predicate isSanitizer(DataFlow::Node node) {
+      super.isSanitizer(node) or
+      node instanceof CodeInjection::Sanitizer
+    }
+
+    override predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node trg) {
+      // HTML sanitizers are insufficient protection against code injection
+      src = trg.(HtmlSanitizerCall).getInput()
+      or
+      DataFlow::localFieldStep(src, trg)
+    }
+
+    // override to require that there is a path without unmatched return steps
+    override predicate hasFlowPath(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) {
+      super.hasFlowPath(source, sink) and
+      DataFlow::hasPathWithoutUnmatchedReturn(source, sink)
+    }
+  }
+}
@@ -0,0 +1,63 @@
+/**
+ * Provides default sources, sinks and sanitizers for reasoning about code
+ * constructed from libary input vulnerabilities, as well as extension points for
+ * adding your own.
+ */
+
+import javascript
+
+/**
+ * Module containing sources, sinks, and sanitizers for code constructed from library input.
+ */
+module UnsafeCodeConstruction {
+  private import semmle.javascript.security.dataflow.CodeInjectionCustomizations::CodeInjection as CodeInjection
+  private import semmle.javascript.PackageExports as Exports
+
+  /**
+   * A source for code constructed from library input vulnerabilities.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A parameter of an exported function, seen as a source.
+   */
+  class ExternalInputSource extends Source, DataFlow::ParameterNode {
+    ExternalInputSource() {
+      this = Exports::getALibraryInputParameter() and
+      // permit parameters that clearly are intended to contain executable code.
+      not this.getName() = "code"
+    }
+  }
+
+  /**
+   * A sink for unsafe code constructed from library input vulnerabilities.
+   */
+  abstract class Sink extends DataFlow::Node {
+    /**
+     * Gets the node where the unsafe code is executed.
+     */
+    abstract DataFlow::Node getCodeSink();
+  }
+
+  /**
+   * Gets a node that is later executed as code in `codeSink`.
+   */
+  private DataFlow::Node isExecutedAsCode(DataFlow::TypeBackTracker t, CodeInjection::Sink codeSink) {
+    t.start() and result = codeSink
+    or
+    exists(DataFlow::TypeBackTracker t2 | t2 = t.smallstep(result, isExecutedAsCode(t, codeSink)))
+  }
+
+  /**
+   * A string concatenation leaf that is later executed as code.
+   */
+  class StringConcatExecutedAsCode extends Sink, StringOps::ConcatenationLeaf {
+    CodeInjection::Sink codeSink;
+
+    StringConcatExecutedAsCode() {
+      this.getRoot() = isExecutedAsCode(DataFlow::TypeBackTracker::end(), codeSink)
+    }
+
+    override DataFlow::Node getCodeSink() { result = codeSink }
+  }
+}
@@ -0,0 +1,55 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+When a library function dynamically constructs code in a potentially unsafe way, then 
+it's important to document to clients of the library that the function should only be
+used with trusted inputs.
+
+If the function is not documented as being potentially unsafe, then a client may
+incorrectly use inputs containing unsafe code fragments, and thereby leave the
+client vulnerable to code-injection attacks.
+</p>
+
+</overview>
+
+<recommendation>
+<p>
+Properly document library functions that construct code from unsanitized 
+inputs, or avoid constructing code in the first place.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example shows two methods implemented using `eval`: a simple
+deserialization routine and a getter method.
+If untrusted inputs are used with these methods,
+then an attacker might be able to execute arbitrary code on the system. 
+</p>
+
+<sample src="examples/UnsafeCodeConstruction.js" />
+
+<p>
+To avoid this problem, either properly document that the function is potentially
+unsafe, or use an alternative solution such as `JSON.parse` or another library, like in the examples below, 
+that does not allow arbitrary code to be executed.
+</p>
+
+<sample src="examples/UnsafeCodeConstructionSafe.js" />
+
+</example>
+
+<references>
+<li>
+OWASP:
+<a href="https://www.owasp.org/index.php/Code_Injection">Code Injection</a>.
+</li>
+<li>
+Wikipedia: <a href="https://en.wikipedia.org/wiki/Code_injection">Code Injection</a>.
+</li>
+</references>
+</qhelp>
@@ -0,0 +1,22 @@
+/**
+ * @name Unsafe code constructed from libary input
+ * @description Using externally controlled strings to construct code may allow a malicious
+ *              user to execute arbitrary code.
+ * @kind path-problem
+ * @problem.severity warning
+ * @precision medium
+ * @id js/unsafe-code-construction
+ * @tags security
+ *       external/cwe/cwe-094
+ *       external/cwe/cwe-079
+ *       external/cwe/cwe-116
+ */
+
+import javascript
+import DataFlow::PathGraph
+import semmle.javascript.security.dataflow.UnsafeCodeConstruction::UnsafeCodeConstruction
+
+from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "$@ flows to here and is later $@.", source.getNode(),
+  "Library input", sink.getNode().(Sink).getCodeSink(), "interpreted as code"
@@ -0,0 +1,7 @@
+export function unsafeDeserialize(value) {
+  return eval(`(${value})`);
+}
+
+export function unsafeGetter(obj, path) {
+    return eval(`obj.${path}`);
+}
@@ -0,0 +1,8 @@
+export function safeDeserialize(value) {
+  return JSON.parse(value);
+}
+
+const _ = require("lodash");
+export function safeGetter(object, path) {
+  return _.get(object, path);
+}
@@ -0,0 +1,5 @@
+---
+category: newQuery
+---
+* A new query, `js/unsafe-code-construction`, has been added to the query suite, highlighting libraries that may leave clients vulnerable to arbitary code execution.
+  The query is not run by default.
@@ -15,8 +15,6 @@ nodes
 | app.js:53:30:53:58 | req.que ... tedCode |
 | app.js:54:33:54:64 | req.que ... CodeRaw |
 | app.js:54:33:54:64 | req.que ... CodeRaw |
-| app.js:55:37:55:72 | req.que ... JsonRaw |
-| app.js:55:37:55:72 | req.que ... JsonRaw |
 | app.js:56:25:56:48 | req.que ... shSink1 |
 | app.js:56:25:56:48 | req.que ... shSink1 |
 | app.js:58:35:58:68 | req.que ... rString |
@@ -64,11 +62,6 @@ nodes
 | views/njk_sinks.njk:14:42:14:76 | {{ dataInGeneratedCodeRaw \| safe }} |
 | views/njk_sinks.njk:14:45:14:66 | dataInG ... CodeRaw |
 | views/njk_sinks.njk:14:45:14:73 | dataInG ...  \| safe |
-| views/njk_sinks.njk:15:46:15:91 | {{ dataInGeneratedCodeJsonRaw \| json \| safe }} |
-| views/njk_sinks.njk:15:46:15:91 | {{ dataInGeneratedCodeJsonRaw \| json \| safe }} |
-| views/njk_sinks.njk:15:49:15:74 | dataInG ... JsonRaw |
-| views/njk_sinks.njk:15:49:15:81 | dataInG ...  \| json |
-| views/njk_sinks.njk:15:49:15:88 | dataInG ...  \| safe |
 | views/njk_sinks.njk:17:19:17:38 | {{ backslashSink1 }} |
 | views/njk_sinks.njk:17:19:17:38 | {{ backslashSink1 }} |
 | views/njk_sinks.njk:17:22:17:35 | backslashSink1 |
@@ -96,8 +89,6 @@ edges
 | app.js:53:30:53:58 | req.que ... tedCode | views/njk_sinks.njk:13:42:13:60 | dataInGeneratedCode |
 | app.js:54:33:54:64 | req.que ... CodeRaw | views/njk_sinks.njk:14:45:14:66 | dataInG ... CodeRaw |
 | app.js:54:33:54:64 | req.que ... CodeRaw | views/njk_sinks.njk:14:45:14:66 | dataInG ... CodeRaw |
-| app.js:55:37:55:72 | req.que ... JsonRaw | views/njk_sinks.njk:15:49:15:74 | dataInG ... JsonRaw |
-| app.js:55:37:55:72 | req.que ... JsonRaw | views/njk_sinks.njk:15:49:15:74 | dataInG ... JsonRaw |
 | app.js:56:25:56:48 | req.que ... shSink1 | views/njk_sinks.njk:17:22:17:35 | backslashSink1 |
 | app.js:56:25:56:48 | req.que ... shSink1 | views/njk_sinks.njk:17:22:17:35 | backslashSink1 |
 | app.js:58:35:58:68 | req.que ... rString | views/njk_sinks.njk:22:42:22:65 | dataInE ... rString |
@@ -137,10 +128,6 @@ edges
 | views/njk_sinks.njk:14:45:14:66 | dataInG ... CodeRaw | views/njk_sinks.njk:14:45:14:73 | dataInG ...  \| safe |
 | views/njk_sinks.njk:14:45:14:73 | dataInG ...  \| safe | views/njk_sinks.njk:14:42:14:76 | {{ dataInGeneratedCodeRaw \| safe }} |
 | views/njk_sinks.njk:14:45:14:73 | dataInG ...  \| safe | views/njk_sinks.njk:14:42:14:76 | {{ dataInGeneratedCodeRaw \| safe }} |
-| views/njk_sinks.njk:15:49:15:74 | dataInG ... JsonRaw | views/njk_sinks.njk:15:49:15:81 | dataInG ...  \| json |
-| views/njk_sinks.njk:15:49:15:81 | dataInG ...  \| json | views/njk_sinks.njk:15:49:15:88 | dataInG ...  \| safe |
-| views/njk_sinks.njk:15:49:15:88 | dataInG ...  \| safe | views/njk_sinks.njk:15:46:15:91 | {{ dataInGeneratedCodeJsonRaw \| json \| safe }} |
-| views/njk_sinks.njk:15:49:15:88 | dataInG ...  \| safe | views/njk_sinks.njk:15:46:15:91 | {{ dataInGeneratedCodeJsonRaw \| json \| safe }} |
 | views/njk_sinks.njk:17:22:17:35 | backslashSink1 | views/njk_sinks.njk:17:19:17:38 | {{ backslashSink1 }} |
 | views/njk_sinks.njk:17:22:17:35 | backslashSink1 | views/njk_sinks.njk:17:19:17:38 | {{ backslashSink1 }} |
 | views/njk_sinks.njk:22:42:22:65 | dataInE ... rString | views/njk_sinks.njk:22:39:22:68 | {{ dataInEventHandlerString }} |
@@ -161,7 +148,6 @@ edges
 | views/hbs_sinks.hbs:33:39:33:68 | {{ dataInEventHandlerString }} | app.js:38:35:38:68 | req.que ... rString | views/hbs_sinks.hbs:33:39:33:68 | {{ dataInEventHandlerString }} | $@ flows to here and is interpreted as code. | app.js:38:35:38:68 | req.que ... rString | User-provided value |
 | views/njk_sinks.njk:13:39:13:63 | {{ dataInGeneratedCode }} | app.js:53:30:53:58 | req.que ... tedCode | views/njk_sinks.njk:13:39:13:63 | {{ dataInGeneratedCode }} | $@ flows to here and is interpreted as code. | app.js:53:30:53:58 | req.que ... tedCode | User-provided value |
 | views/njk_sinks.njk:14:42:14:76 | {{ dataInGeneratedCodeRaw \| safe }} | app.js:54:33:54:64 | req.que ... CodeRaw | views/njk_sinks.njk:14:42:14:76 | {{ dataInGeneratedCodeRaw \| safe }} | $@ flows to here and is interpreted as code. | app.js:54:33:54:64 | req.que ... CodeRaw | User-provided value |
-| views/njk_sinks.njk:15:46:15:91 | {{ dataInGeneratedCodeJsonRaw \| json \| safe }} | app.js:55:37:55:72 | req.que ... JsonRaw | views/njk_sinks.njk:15:46:15:91 | {{ dataInGeneratedCodeJsonRaw \| json \| safe }} | $@ flows to here and is interpreted as code. | app.js:55:37:55:72 | req.que ... JsonRaw | User-provided value |
 | views/njk_sinks.njk:17:19:17:38 | {{ backslashSink1 }} | app.js:56:25:56:48 | req.que ... shSink1 | views/njk_sinks.njk:17:19:17:38 | {{ backslashSink1 }} | $@ flows to here and is interpreted as code. | app.js:56:25:56:48 | req.que ... shSink1 | User-provided value |
 | views/njk_sinks.njk:22:39:22:68 | {{ dataInEventHandlerString }} | app.js:58:35:58:68 | req.que ... rString | views/njk_sinks.njk:22:39:22:68 | {{ dataInEventHandlerString }} | $@ flows to here and is interpreted as code. | app.js:58:35:58:68 | req.que ... rString | User-provided value |
 | views/njk_sinks.njk:23:39:23:78 | {{ dataInEventHandlerStringRaw \| safe }} | app.js:59:38:59:74 | req.que ... ringRaw | views/njk_sinks.njk:23:39:23:78 | {{ dataInEventHandlerStringRaw \| safe }} | $@ flows to here and is interpreted as code. | app.js:59:38:59:74 | req.que ... ringRaw | User-provided value |