From 0eb7ac94cc6af1a522374e2fc7e6949d19c8fc33 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 30 Sep 2020 17:29:53 +0100 Subject: [PATCH] Add stack-trace exposure query This is a port of `java/stack-trace-exposure`, and does the same job: warn that a stack dump is written to an HTTP response. --- .../2020-10-01-stack-trace-exposure.md | 2 + ql/src/Security/CWE-209/StackTraceExposure.go | 18 +++++ .../Security/CWE-209/StackTraceExposure.qhelp | 38 ++++++++++ ql/src/Security/CWE-209/StackTraceExposure.ql | 76 +++++++++++++++++++ .../CWE-209/StackTraceExposure.expected | 7 ++ .../Security/CWE-209/StackTraceExposure.qlref | 1 + ql/test/query-tests/Security/CWE-209/test.go | 41 ++++++++++ 7 files changed, 183 insertions(+) create mode 100644 change-notes/2020-10-01-stack-trace-exposure.md create mode 100644 ql/src/Security/CWE-209/StackTraceExposure.go create mode 100644 ql/src/Security/CWE-209/StackTraceExposure.qhelp create mode 100644 ql/src/Security/CWE-209/StackTraceExposure.ql create mode 100644 ql/test/query-tests/Security/CWE-209/StackTraceExposure.expected create mode 100644 ql/test/query-tests/Security/CWE-209/StackTraceExposure.qlref create mode 100644 ql/test/query-tests/Security/CWE-209/test.go diff --git a/change-notes/2020-10-01-stack-trace-exposure.md b/change-notes/2020-10-01-stack-trace-exposure.md new file mode 100644 index 00000000000..35437a0598f --- /dev/null +++ b/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. diff --git a/ql/src/Security/CWE-209/StackTraceExposure.go b/ql/src/Security/CWE-209/StackTraceExposure.go new file mode 100644 index 00000000000..cb6520609a9 --- /dev/null +++ b/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")) +} diff --git a/ql/src/Security/CWE-209/StackTraceExposure.qhelp b/ql/src/Security/CWE-209/StackTraceExposure.qhelp new file mode 100644 index 00000000000..bd787ac619e --- /dev/null +++ b/ql/src/Security/CWE-209/StackTraceExposure.qhelp @@ -0,0 +1,38 @@ + + + +

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.

+ +

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.

+ + + +

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.

+ + + +

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.

+ + + + + +
  • OWASP: Improper Error Handling.
    • +     +     diff --git a/ql/src/Security/CWE-209/StackTraceExposure.ql b/ql/src/Security/CWE-209/StackTraceExposure.ql new file mode 100644 index 00000000000..c8f4c4b45a6 --- /dev/null +++ b/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" diff --git a/ql/test/query-tests/Security/CWE-209/StackTraceExposure.expected b/ql/test/query-tests/Security/CWE-209/StackTraceExposure.expected new file mode 100644 index 00000000000..583fd709000 --- /dev/null +++ b/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 | diff --git a/ql/test/query-tests/Security/CWE-209/StackTraceExposure.qlref b/ql/test/query-tests/Security/CWE-209/StackTraceExposure.qlref new file mode 100644 index 00000000000..18cf2d49a1a --- /dev/null +++ b/ql/test/query-tests/Security/CWE-209/StackTraceExposure.qlref @@ -0,0 +1 @@ +Security/CWE-209/StackTraceExposure.ql diff --git a/ql/test/query-tests/Security/CWE-209/test.go b/ql/test/query-tests/Security/CWE-209/test.go new file mode 100644 index 00000000000..2ad35680048 --- /dev/null +++ b/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) + } +}