Merge branch 'js-team-sprint' into priv-file-polish

2025-12-16 16:53:25 +01:00 · 2020-06-18 16:40:53 +02:00
parent 9ba2c98ec0 20e96799e2
commit 1556b62007
13 changed files with 303 additions and 22 deletions
--- a/change-notes/1.25/analysis-javascript.md
+++ b/change-notes/1.25/analysis-javascript.md
@@ -37,6 +37,7 @@
 | Unsafe expansion of self-closing HTML tag (`js/unsafe-html-expansion`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities caused by unsafe expansion of self-closing HTML tags. |
 | Unsafe shell command constructed from library input (`js/shell-command-constructed-from-input`) | correctness, security, external/cwe/cwe-078, external/cwe/cwe-088 | Highlights potential command injections due to a shell command being constructed from library inputs. Results are shown on LGTM by default. |
 | Exposure of private files (`js/exposure-of-private-files`) | security, external/cwe/cwe-200 | Highlights servers that serve private files. Results are shown on LGTM by default. |
+| Storage of sensitive information in build artifact (`js/build-artifact-leak`) | security, external/cwe/cwe-312 | Highlights storage of sensitive information in build artifacts. Results are shown on LGTM by default. |
 | Improper code sanitization (`js/bad-code-sanitization`) | security, external/cwe/cwe-094, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights string concatenation where code is constructed without proper sanitization. Results are shown on LGTM by default. |

 ## Changes to existing queries
--- a/javascript/ql/src/Security/CWE-312/BuildArtifactLeak.qhelp
+++ b/javascript/ql/src/Security/CWE-312/BuildArtifactLeak.qhelp
@@ -0,0 +1,36 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+  <overview>
+    <p>
+      Sensitive information included in a build artifact can allow an attacker to access
+      the sensitive information if the artifact is published.
+    </p>
+  </overview>
+
+  <recommendation>
+    <p>
+      Only store information that is meant to be publicly available in a build artifact.
+    </p>
+  </recommendation>
+
+  <example>
+    <p>
+      The following example creates a <code>webpack</code> configuration that inserts all environment
+      variables from the host into the build artifact:
+    </p>
+    <sample src="examples/build-leak.js"/>
+    <p>
+      The environment variables might include API keys or other sensitive information, and the build-system
+      should instead insert only the environment variables that are supposed to be public. 
+    </p>
+    <p>
+      The issue has been fixed below, where only the <code>DEBUG</code> environment variable is inserted into the artifact.
+    </p>
+    <sample src="examples/build-leak-fixed.js"/>
+  </example>
+  <references>
+    <li>webpack: <a href="https://webpack.js.org/plugins/define-plugin/">DefinePlugin API</a>.</li>
+  </references>
+</qhelp>
--- a/javascript/ql/src/Security/CWE-312/BuildArtifactLeak.ql
+++ b/javascript/ql/src/Security/CWE-312/BuildArtifactLeak.ql
@@ -0,0 +1,23 @@
+/**
+ * @name Storage of sensitive information in build artifact
+ * @description Including sensitive information in a build artifact can
+ *              expose it to an attacker.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id js/build-artifact-leak
+ * @tags security
+ *       external/cwe/cwe-312
+ *       external/cwe/cwe-315
+ *       external/cwe/cwe-359
+ */
+
+import javascript
+import semmle.javascript.security.dataflow.BuildArtifactLeak::BuildArtifactLeak
+import DataFlow::PathGraph
+
+from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select sink.getNode(), source, sink,
+  "Sensitive data returned by $@ is stored in a build artifact here.", source.getNode(),
+  source.getNode().(CleartextLogging::Source).describe()
--- a/javascript/ql/src/Security/CWE-312/examples/build-leak-fixed.js
+++ b/javascript/ql/src/Security/CWE-312/examples/build-leak-fixed.js
@@ -0,0 +1,9 @@
+const webpack = require("webpack");
+
+module.exports = [{
+    plugins: [
+        new webpack.DefinePlugin({
+            'process.env': JSON.stringify({ DEBUG: process.env.DEBUG })
+        })
+    ]
+}];
--- a/javascript/ql/src/Security/CWE-312/examples/build-leak.js
+++ b/javascript/ql/src/Security/CWE-312/examples/build-leak.js
@@ -0,0 +1,9 @@
+const webpack = require("webpack");
+
+module.exports = [{
+    plugins: [
+        new webpack.DefinePlugin({
+            "process.env": JSON.stringify(process.env)
+        })
+    ]
+}];
--- a/javascript/ql/src/semmle/javascript/Arrays.qll
+++ b/javascript/ql/src/semmle/javascript/Arrays.qll
@@ -40,6 +40,11 @@ module ArrayTaintTracking {
      succ = call
    )
    or
+    // `array.reduce` with tainted value in callback
+    call.(DataFlow::MethodCallNode).getMethodName() = "reduce" and
+    pred = call.getArgument(0).(DataFlow::FunctionNode).getAReturn() and // Require the argument to be a closure to avoid spurious call/return flow
+    succ = call
+    or
    // `array.push(e)`, `array.unshift(e)`: if `e` is tainted, then so is `array`.
    exists(string name |
      name = "push" or
--- a/javascript/ql/src/semmle/javascript/security/dataflow/BuildArtifactLeak.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/BuildArtifactLeak.qll
@@ -0,0 +1,45 @@
+/**
+ * Provides a dataflow tracking configuration for reasoning about
+ * storage of sensitive information in build artifact.
+ *
+ * Note, for performance reasons: only import this file if
+ * `CleartextLogging::Configuration` is needed, otherwise
+ * `CleartextLoggingCustomizations` should be imported instead.
+ */
+
+import javascript
+
+/**
+ * Classes and predicates for storage of sensitive information in build artifact query.
+ */
+module BuildArtifactLeak {
+  import BuildArtifactLeakCustomizations::BuildArtifactLeak
+  import CleartextLoggingCustomizations::CleartextLogging as CleartextLogging
+
+  /**
+   * A taint tracking configuration for storage of sensitive information in build artifact.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() { this = "BuildArtifactLeak" }
+
+    override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel lbl) {
+      source.(CleartextLogging::Source).getLabel() = lbl
+    }
+
+    override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel lbl) {
+      sink.(Sink).getLabel() = lbl
+    }
+
+    override predicate isSanitizer(DataFlow::Node node) {
+      node instanceof CleartextLogging::Barrier
+    }
+
+    override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
+      CleartextLogging::isSanitizerEdge(pred, succ)
+    }
+
+    override predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node trg) {
+      CleartextLogging::isAdditionalTaintStep(src, trg)
+    }
+  }
+}
--- a/javascript/ql/src/semmle/javascript/security/dataflow/BuildArtifactLeakCustomizations.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/BuildArtifactLeakCustomizations.qll
@@ -0,0 +1,32 @@
+/**
+ * Provides default sinks for reasoning about storage of sensitive information
+ * in build artifact, as well as extension points for adding your own.
+ */
+
+import javascript
+private import semmle.javascript.dataflow.InferredTypes
+private import semmle.javascript.security.SensitiveActions::HeuristicNames
+
+/**
+ * Sinks for storage of sensitive information in build artifact.
+ */
+module BuildArtifactLeak {
+  /**
+   * A data flow sink for storage of sensitive information in a build artifact.
+   */
+  abstract class Sink extends DataFlow::Node {
+    /**
+     * Gets a data-flow label that leaks information for this sink.
+     */
+    DataFlow::FlowLabel getLabel() { result.isTaint() }
+  }
+
+  /**
+   * An instantiation of `webpack.DefintePlugin` that stores information in a compiled JavaScript file.
+   */
+  class WebpackDefinePluginSink extends Sink {
+    WebpackDefinePluginSink() {
+      this = DataFlow::moduleMember("webpack", "DefinePlugin").getAnInstantiation().getAnArgument()
+    }
+  }
+}
--- a/javascript/ql/src/semmle/javascript/security/dataflow/CleartextLogging.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/CleartextLogging.qll
@@ -35,23 +35,11 @@ module CleartextLogging {
    override predicate isSanitizer(DataFlow::Node node) { node instanceof Barrier }

    override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
-      succ.(DataFlow::PropRead).getBase() = pred
+      CleartextLogging::isSanitizerEdge(pred, succ)
    }

    override predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node trg) {
-      // A taint propagating data flow edge through objects: a tainted write taints the entire object.
-      exists(DataFlow::PropWrite write |
-        write.getRhs() = src and
-        trg.(DataFlow::SourceNode).flowsTo(write.getBase())
-      )
-      or
-      // Taint through the arguments object.
-      exists(DataFlow::CallNode call, Function f |
-        src = call.getAnArgument() and
-        f = call.getACallee() and
-        not call.isImprecise() and
-        trg.asExpr() = f.getArgumentsVariable().getAnAccess()
-      )
+      CleartextLogging::isAdditionalTaintStep(src, trg)
    }
  }
 }
--- a/javascript/ql/src/semmle/javascript/security/dataflow/CleartextLoggingCustomizations.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/CleartextLoggingCustomizations.qll
@@ -176,17 +176,50 @@ module CleartextLogging {

    override string describe() { result = "process environment" }

-    override DataFlow::FlowLabel getLabel() {
-      result.isTaint() or
-      result instanceof PartiallySensitiveMap
-    }
+    override DataFlow::FlowLabel getLabel() { result.isTaint() }
  }

  /**
-   * A flow label describing a map that might contain sensitive information in some properties.
-   * Property reads on such maps where the property name is fixed is unlikely to leak sensitive information.
+   * Holds if the edge `pred` -> `succ` should be sanitized for clear-text logging of sensitive information.
   */
-  class PartiallySensitiveMap extends DataFlow::FlowLabel {
-    PartiallySensitiveMap() { this = "PartiallySensitiveMap" }
+  predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
+    succ.(DataFlow::PropRead).getBase() = pred
+  }
+
+  /**
+   * Holds if the edge `src` -> `trg` is an additional taint-step for clear-text logging of sensitive information.
+   */
+  predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node trg) {
+    // A taint propagating data flow edge through objects: a tainted write taints the entire object.
+    exists(DataFlow::PropWrite write |
+      write.getRhs() = src and
+      trg.(DataFlow::SourceNode).flowsTo(write.getBase())
+    )
+    or
+    // A property-copy step,
+    // dst[x] = src[x]
+    // dst[x] = JSON.stringify(src[x])
+    exists(DataFlow::PropWrite write, DataFlow::PropRead read |
+      read = write.getRhs()
+      or
+      exists(DataFlow::MethodCallNode stringify |
+        stringify = write.getRhs() and
+        stringify = DataFlow::globalVarRef("JSON").getAMethodCall("stringify") and
+        stringify.getArgument(0) = read
+      )
+    |
+      not exists(write.getPropertyName()) and
+      not exists(read.getPropertyName()) and
+      src = read.getBase() and
+      trg = write.getBase().getALocalSource()
+    )
+    or
+    // Taint through the arguments object.
+    exists(DataFlow::CallNode call, Function f |
+      src = call.getAnArgument() and
+      f = call.getACallee() and
+      not call.isImprecise() and
+      trg.asExpr() = f.getArgumentsVariable().getAnAccess()
+    )
  }
 }
--- a/javascript/ql/test/query-tests/Security/CWE-312/BuildArtifactLeak.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-312/BuildArtifactLeak.expected
@@ -0,0 +1,57 @@
+nodes
+| build-leaks.js:4:39:6:1 | { // NO ... .env)\\n} |
+| build-leaks.js:4:39:6:1 | { // NO ... .env)\\n} |
+| build-leaks.js:5:20:5:46 | JSON.st ... ss.env) |
+| build-leaks.js:5:35:5:45 | process.env |
+| build-leaks.js:5:35:5:45 | process.env |
+| build-leaks.js:13:11:19:10 | raw |
+| build-leaks.js:13:17:19:10 | Object. ...      }) |
+| build-leaks.js:14:18:14:20 | env |
+| build-leaks.js:15:24:15:34 | process.env |
+| build-leaks.js:15:24:15:34 | process.env |
+| build-leaks.js:16:20:16:22 | env |
+| build-leaks.js:21:11:26:5 | stringifed |
+| build-leaks.js:21:24:26:5 | {\\n      ... )\\n    } |
+| build-leaks.js:22:24:25:14 | Object. ...  }, {}) |
+| build-leaks.js:22:49:22:51 | env |
+| build-leaks.js:23:39:23:41 | raw |
+| build-leaks.js:24:20:24:22 | env |
+| build-leaks.js:30:22:30:31 | stringifed |
+| build-leaks.js:34:26:34:57 | getEnv( ... ngified |
+| build-leaks.js:34:26:34:57 | getEnv( ... ngified |
+| build-leaks.js:40:9:40:60 | pw |
+| build-leaks.js:40:14:40:60 | url.par ... assword |
+| build-leaks.js:40:14:40:60 | url.par ... assword |
+| build-leaks.js:41:43:41:86 | { "proc ... y(pw) } |
+| build-leaks.js:41:43:41:86 | { "proc ... y(pw) } |
+| build-leaks.js:41:67:41:84 | JSON.stringify(pw) |
+| build-leaks.js:41:82:41:83 | pw |
+edges
+| build-leaks.js:5:20:5:46 | JSON.st ... ss.env) | build-leaks.js:4:39:6:1 | { // NO ... .env)\\n} |
+| build-leaks.js:5:20:5:46 | JSON.st ... ss.env) | build-leaks.js:4:39:6:1 | { // NO ... .env)\\n} |
+| build-leaks.js:5:35:5:45 | process.env | build-leaks.js:5:20:5:46 | JSON.st ... ss.env) |
+| build-leaks.js:5:35:5:45 | process.env | build-leaks.js:5:20:5:46 | JSON.st ... ss.env) |
+| build-leaks.js:13:11:19:10 | raw | build-leaks.js:23:39:23:41 | raw |
+| build-leaks.js:13:17:19:10 | Object. ...      }) | build-leaks.js:13:11:19:10 | raw |
+| build-leaks.js:14:18:14:20 | env | build-leaks.js:16:20:16:22 | env |
+| build-leaks.js:15:24:15:34 | process.env | build-leaks.js:14:18:14:20 | env |
+| build-leaks.js:15:24:15:34 | process.env | build-leaks.js:14:18:14:20 | env |
+| build-leaks.js:16:20:16:22 | env | build-leaks.js:13:17:19:10 | Object. ...      }) |
+| build-leaks.js:21:11:26:5 | stringifed | build-leaks.js:30:22:30:31 | stringifed |
+| build-leaks.js:21:24:26:5 | {\\n      ... )\\n    } | build-leaks.js:21:11:26:5 | stringifed |
+| build-leaks.js:22:24:25:14 | Object. ...  }, {}) | build-leaks.js:21:24:26:5 | {\\n      ... )\\n    } |
+| build-leaks.js:22:49:22:51 | env | build-leaks.js:24:20:24:22 | env |
+| build-leaks.js:23:39:23:41 | raw | build-leaks.js:22:49:22:51 | env |
+| build-leaks.js:24:20:24:22 | env | build-leaks.js:22:24:25:14 | Object. ...  }, {}) |
+| build-leaks.js:30:22:30:31 | stringifed | build-leaks.js:34:26:34:57 | getEnv( ... ngified |
+| build-leaks.js:30:22:30:31 | stringifed | build-leaks.js:34:26:34:57 | getEnv( ... ngified |
+| build-leaks.js:40:9:40:60 | pw | build-leaks.js:41:82:41:83 | pw |
+| build-leaks.js:40:14:40:60 | url.par ... assword | build-leaks.js:40:9:40:60 | pw |
+| build-leaks.js:40:14:40:60 | url.par ... assword | build-leaks.js:40:9:40:60 | pw |
+| build-leaks.js:41:67:41:84 | JSON.stringify(pw) | build-leaks.js:41:43:41:86 | { "proc ... y(pw) } |
+| build-leaks.js:41:67:41:84 | JSON.stringify(pw) | build-leaks.js:41:43:41:86 | { "proc ... y(pw) } |
+| build-leaks.js:41:82:41:83 | pw | build-leaks.js:41:67:41:84 | JSON.stringify(pw) |
+#select
+| build-leaks.js:4:39:6:1 | { // NO ... .env)\\n} | build-leaks.js:5:35:5:45 | process.env | build-leaks.js:4:39:6:1 | { // NO ... .env)\\n} | Sensitive data returned by $@ is stored in a build artifact here. | build-leaks.js:5:35:5:45 | process.env | process environment |
+| build-leaks.js:34:26:34:57 | getEnv( ... ngified | build-leaks.js:15:24:15:34 | process.env | build-leaks.js:34:26:34:57 | getEnv( ... ngified | Sensitive data returned by $@ is stored in a build artifact here. | build-leaks.js:15:24:15:34 | process.env | process environment |
+| build-leaks.js:41:43:41:86 | { "proc ... y(pw) } | build-leaks.js:40:14:40:60 | url.par ... assword | build-leaks.js:41:43:41:86 | { "proc ... y(pw) } | Sensitive data returned by $@ is stored in a build artifact here. | build-leaks.js:40:14:40:60 | url.par ... assword | an access to current_password |
--- a/javascript/ql/test/query-tests/Security/CWE-312/BuildArtifactLeak.qlref
+++ b/javascript/ql/test/query-tests/Security/CWE-312/BuildArtifactLeak.qlref
@@ -0,0 +1 @@
+Security/CWE-312/BuildArtifactLeak.ql
--- a/javascript/ql/test/query-tests/Security/CWE-312/build-leaks.js
+++ b/javascript/ql/test/query-tests/Security/CWE-312/build-leaks.js
@@ -0,0 +1,42 @@
+const webpack = require("webpack");
+
+
+var plugin = new webpack.DefinePlugin({ // NOT OK
+    "process.env": JSON.stringify(process.env)
+});
+
+// OK
+new webpack.DefinePlugin({ 'process.env': JSON.stringify({ DEBUG: process.env.DEBUG }) })
+
+
+function getEnv(env) {
+    const raw = Object.keys(process.env)
+        .reduce((env, key) => {
+            env[key] = process.env[key]
+            return env
+        }, {
+            NODE_ENV: process.env.NODE_ENV || env || 'development'
+        })
+
+    const stringifed = {
+        'process.env': Object.keys(raw).reduce((env, key) => {
+            env[key] = JSON.stringify(raw[key])
+            return env
+        }, {})
+    }
+
+    return {
+        raw: raw,
+        stringified: stringifed
+    }
+}
+
+new webpack.DefinePlugin(getEnv('production').stringified); // NOT OK
+
+var https = require('https');
+var url = require('url');
+
+var server = https.createServer(function (req, res) {
+    let pw = url.parse(req.url, true).query.current_password;
+    var plugin = new webpack.DefinePlugin({ "process.env.secret": JSON.stringify(pw) }); // NOT OK
+});