From bdfe938d02a36e56fffc1362b0c1d8eb0da489fb Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Wed, 31 Oct 2018 10:32:42 -0400
Subject: [PATCH] JavaScript: Improve `StackTraceExposure` query.

It now also flags exposure of the entire exception object (not just the `stack` property).
---
 change-notes/1.19/analysis-javascript.md      |  1 +
 .../security/dataflow/StackTraceExposure.qll  | 22 ++++++++++++++-----
 .../CWE-209/StackTraceExposure.expected       |  4 +++-
 .../test/query-tests/Security/CWE-209/tst.js  | 18 +++++++++++++++
 4 files changed, 38 insertions(+), 7 deletions(-)
 create mode 100644 javascript/ql/test/query-tests/Security/CWE-209/tst.js

diff --git a/change-notes/1.19/analysis-javascript.md b/change-notes/1.19/analysis-javascript.md
index cca00ae7da8f..118e7fa0239d 100644
--- a/change-notes/1.19/analysis-javascript.md
+++ b/change-notes/1.19/analysis-javascript.md
@@ -44,6 +44,7 @@
 | Unused import | Fewer false-positive results | This rule no longer flags imports used by the `transform-react-jsx` Babel plugin. |
 | Self assignment | Fewer false-positive results | This rule now ignores self-assignments preceded by a JSDoc comment with a `@type` tag. |
 | Client side cross-site scripting | More results | This rule now also flags HTML injection in the body of an email. |
+| Information exposure through a stack trace | More results | This rule now also flags cases where the entire exception object (including the stack trace) may be exposed. |
 
 ## Changes to QL libraries
 
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/StackTraceExposure.qll b/javascript/ql/src/semmle/javascript/security/dataflow/StackTraceExposure.qll
index 75579403dd20..e0367666a8d1 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/StackTraceExposure.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/StackTraceExposure.qll
@@ -23,22 +23,32 @@ module StackTraceExposure {
       src instanceof Source
     }
 
+    override predicate isSanitizer(DataFlow::Node nd) {
+      super.isSanitizer(nd)
+      or
+      // read of a property other than `stack`
+      nd.(DataFlow::PropRead).getPropertyName() != "stack"
+      or
+      // `toString` does not include the stack trace
+      nd.(DataFlow::MethodCallNode).getMethodName() = "toString"
+      or
+      nd = StringConcatenation::getAnOperand(_)
+    }
+
     override predicate isSink(DataFlow::Node snk) {
       snk instanceof Sink
     }
   }
 
+
   /**
    * A read of the `stack` property of an exception, viewed as a data flow
    * sink for stack trace exposure vulnerabilities.
    */
-  class DefaultSource extends Source, DataFlow::ValueNode {
+  class DefaultSource extends Source, DataFlow::Node {
     DefaultSource() {
-      // any read of the `stack` property of an exception is a source
-      exists (Parameter exc |
-        exc = any(TryStmt try).getACatchClause().getAParameter() and
-        this = DataFlow::parameterNode(exc).getAPropertyRead("stack")
-      )
+      // any exception is a source
+      this = DataFlow::parameterNode(any(TryStmt try).getACatchClause().getAParameter())
     }
   }
   
diff --git a/javascript/ql/test/query-tests/Security/CWE-209/StackTraceExposure.expected b/javascript/ql/test/query-tests/Security/CWE-209/StackTraceExposure.expected
index 8f89e63b419a..8ec5b31e6e9d 100644
--- a/javascript/ql/test/query-tests/Security/CWE-209/StackTraceExposure.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-209/StackTraceExposure.expected
@@ -1 +1,3 @@
-| node.js:11:13:11:21 | err.stack | Stack trace information from $@ may be exposed to an external user here. | node.js:11:13:11:21 | err.stack | here |
+| node.js:11:13:11:21 | err.stack | Stack trace information from $@ may be exposed to an external user here. | node.js:8:10:8:12 | err | here |
+| tst.js:7:13:7:13 | e | Stack trace information from $@ may be exposed to an external user here. | tst.js:6:12:6:12 | e | here |
+| tst.js:17:11:17:17 | e.stack | Stack trace information from $@ may be exposed to an external user here. | tst.js:6:12:6:12 | e | here |
diff --git a/javascript/ql/test/query-tests/Security/CWE-209/tst.js b/javascript/ql/test/query-tests/Security/CWE-209/tst.js
new file mode 100644
index 000000000000..b1450ce088ae
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-209/tst.js
@@ -0,0 +1,18 @@
+var http = require('http');
+
+http.createServer(function onRequest(req, res) {
+  try {
+    throw new Error();
+  } catch (e) {
+    res.end(e);                        // NOT OK
+    fail(res, e);
+    res.end(e.message);                // OK
+    res.end("Caught exception " + e);  // OK
+    res.end(e.toString());             // OK
+    res.end(`Caught exception ${e}.`); // OK
+  }
+});
+
+function fail(res, e) {
+  res.end(e.stack);                    // NOT OK
+}