Merge pull request #360 from smowton/smowton/feature/stack-trace-exposure

Add stack-trace exposure query

Author: smowton

change-notes/2020-10-01-stack-trace-exposure.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* A new query `go/stack-trace-exposure` has been added. The query flags exposure of a stack trace to a remote party.

ql/src/Security/CWE-209/StackTraceExposure.go

@@ -0,0 +1,18 @@
+package example
+
+import (
+	"log"
+	"net/http"
+	"runtime"
+)
+
+func handlePanic(w http.ResponseWriter, r *http.Request) {
+	buf := make([]byte, 2<<16)
+	buf = buf[:runtime.Stack(buf, true)]
+	// BAD: printing a stack trace back to the response
+	w.Write(buf)
+	// GOOD: logging the response to the server and sending
+	// a more generic message.
+	log.Printf("Panic: %s", buf)
+	w.Write([]byte("An unexpected runtime error occurred"))
+}

ql/src/Security/CWE-209/StackTraceExposure.qhelp

@@ -0,0 +1,38 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Software developers often add stack traces to error messages, as a
+debugging aid. Whenever that error message occurs for an end user, the
+developer can use the stack trace to help identify how to fix the
+problem. In particular, stack traces can tell the developer more about
+the sequence of events that led to a failure, as opposed to merely the
+final state of the software when the error occurred.</p>
+
+<p>Unfortunately, the same information can be useful to an attacker.
+The sequence of class names in a stack trace can reveal the structure
+of the application as well as any internal components it relies on.</p>
+</overview>
+
+<recommendation>
+<p>Send the user a more generic error message that reveals less information.
+Either suppress the stack trace entirely, or log it only on the server.</p>
+</recommendation>
+
+<example>
+<p>In the following example, a panic is handled in two different
+ways. In the first version, labeled BAD, a detailed stack trace is
+written to a user-facing HTTP response object, which may disclose
+sensitive information. In the second version, the error message is
+logged only on the server. That way, the developers can still access
+and use the error log, but remote users will not see the
+information.</p>
+
+<sample src="StackTraceExposure.go" />
+</example>
+
+<references>
+<li>OWASP: <a href="https://owasp.org/www-community/Improper_Error_Handling">Improper Error Handling</a>.</li>
+</references>
+</qhelp>

ql/src/Security/CWE-209/StackTraceExposure.ql

@@ -0,0 +1,76 @@
+/**
+ * @name Information exposure through a stack trace
+ * @description Information from a stack trace propagates to an external user.
+ *              Stack traces can unintentionally reveal implementation details
+ *              that are useful to an attacker for developing a subsequent exploit.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id go/stack-trace-exposure
+ * @tags security
+ *       external/cwe/cwe-209
+ *       external/cwe/cwe-497
+ */
+
+import go
+import semmle.go.security.InsecureFeatureFlag::InsecureFeatureFlag
+import DataFlow::PathGraph
+
+/**
+ * A flag indicating the program is in debug or development mode, or that stack
+ * dumps have been specifically enabled.
+ */
+class DebugModeFlag extends FlagKind {
+  DebugModeFlag() { this = "debugMode" }
+
+  bindingset[result]
+  override string getAFlagName() {
+    result.regexpMatch("(?i).*(trace|debug|devel|(enable|disable|print)stack).*")
+  }
+}
+
+/**
+ * The function `runtime.Stack`, which emits a stack trace.
+ */
+class StackFunction extends Function {
+  StackFunction() { this.hasQualifiedName("runtime", "Stack") }
+}
+
+/**
+ * The function `runtime/debug.Stack`, which emits a stack trace.
+ */
+class DebugStackFunction extends Function {
+  DebugStackFunction() { this.hasQualifiedName("runtime/debug", "Stack") }
+}
+
+/**
+ * A taint-tracking configuration that looks for stack traces being written to
+ * an HTTP response body without an intervening debug- or development-mode conditional.
+ */
+class StackTraceExposureConfig extends TaintTracking::Configuration {
+  StackTraceExposureConfig() { this = "StackTraceExposureConfig" }
+
+  override predicate isSource(DataFlow::Node node) {
+    node.(DataFlow::PostUpdateNode).getPreUpdateNode() =
+      any(StackFunction f).getACall().getArgument(0) or
+    node = any(DebugStackFunction f).getACall().getResult()
+  }
+
+  override predicate isSink(DataFlow::Node node) { node instanceof HTTP::ResponseBody }
+
+  override predicate isSanitizer(DataFlow::Node node) {
+    // Sanitise everything controlled by an is-debug-mode check.
+    // Imprecision: I don't try to guess which arm of a branch is intended
+    // to mean debug mode, and which is production mode.
+    exists(ControlFlow::ConditionGuardNode cgn |
+      cgn.ensures(any(DebugModeFlag f).getAFlag().getANode(), _)
+    |
+      cgn.dominates(node.getBasicBlock())
+    )
+  }
+}
+
+from StackTraceExposureConfig cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select source.getNode(), source, sink, "This stack trace is exposed to a remote user $@.",
+  sink.getNode(), "here"

ql/test/query-tests/Security/CWE-209/StackTraceExposure.expected

@@ -0,0 +1,7 @@
+edges
+| test.go:14:2:14:4 | definition of buf : slice type | test.go:17:10:17:12 | buf |
+nodes
+| test.go:14:2:14:4 | definition of buf : slice type | semmle.label | definition of buf : slice type |
+| test.go:17:10:17:12 | buf | semmle.label | buf |
+#select
+| test.go:14:2:14:4 | definition of buf | test.go:14:2:14:4 | definition of buf : slice type | test.go:17:10:17:12 | buf | This stack trace is exposed to a remote user $@. | test.go:17:10:17:12 | buf | here |

ql/test/query-tests/Security/CWE-209/StackTraceExposure.qlref

@@ -0,0 +1 @@
+Security/CWE-209/StackTraceExposure.ql

ql/test/query-tests/Security/CWE-209/test.go

@@ -0,0 +1,41 @@
+package test
+
+import (
+	"log"
+	"net/http"
+	"runtime"
+)
+
+var debug, development, trace, enableStackTrace, disableStackTrace, printStackTrace bool
+
+var logger log.Logger
+
+func handlePanic(w http.ResponseWriter, r *http.Request) {
+	buf := make([]byte, 2<<16)
+	buf = buf[:runtime.Stack(buf, true)]
+	// BAD: printing a stack trace back to the response
+	w.Write(buf)
+	// GOOD: logging the response to the server and sending
+	// a more generic message.
+	logger.Printf("Panic: %s", buf)
+	w.Write([]byte("An unexpected runtime error occurred"))
+	// GOOD: guarding remote stack dumps with tests that suggest an opt-in debug mode:
+	if debug {
+		w.Write(buf)
+	}
+	if development {
+		w.Write(buf)
+	}
+	if trace {
+		w.Write(buf)
+	}
+	if enableStackTrace {
+		w.Write(buf)
+	}
+	if !disableStackTrace {
+		w.Write(buf) // Note our analysis doesn't actually check this branch goes the right way
+	}
+	if printStackTrace {
+		w.Write(buf)
+	}
+}