Merge pull request #443 from xiemaisi/js/improve-stack-trace-exposure

Approved by asger-semmle

Author: semmle-qlci

change-notes/1.19/analysis-javascript.md

@@ -41,6 +41,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
 

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())
     }
   }
 

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 |

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
+}