Skip to content

Commit 8a94cab

Browse files
nickrolfenickrolfe
authored
Merge pull request #11250 from github/nickrolfe/stack-trace-exposure
Ruby: add stack-trace exposure query
2 parents 7456f37 + 50b10be commit 8a94cab

File tree

9 files changed

+208
-0
lines changed

9 files changed

+208
-0
lines changed

‎ruby/ql/lib/codeql/ruby/security/StackTraceExposureCustomizations.qll

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for detecting stack trace
3+
* exposure vulnerabilities, as well as extension points for adding your own.
4+
*/
5+
6+
private import codeql.ruby.AST
7+
private import codeql.ruby.Concepts
8+
private import codeql.ruby.DataFlow
9+
private import codeql.ruby.controlflow.CfgNodes
10+
private import codeql.ruby.frameworks.core.Kernel
11+
12+
/**
13+
* Provides default sources, sinks and sanitizers for detecting stack trace
14+
* exposure vulnerabilities, as well as extension points for adding your own.
15+
*/
16+
module StackTraceExposure {
17+
/** A data flow source for stack trace exposure vulnerabilities. */
18+
abstract class Source extends DataFlow::Node { }
19+
20+
/** A data flow sink for stack trace exposure vulnerabilities. */
21+
abstract class Sink extends DataFlow::Node { }
22+
23+
/** A data flow sanitizer for stack trace exposure vulnerabilities. */
24+
abstract class Sanitizer extends DataFlow::Node { }
25+
26+
/**
27+
* A call to `backtrace` or `backtrace_locations` on a `rescue` variable,
28+
* considered as a flow source.
29+
*/
30+
class BacktraceCall extends Source, DataFlow::CallNode {
31+
BacktraceCall() {
32+
exists(DataFlow::LocalSourceNode varAccess |
33+
varAccess.asExpr().(ExprNodes::VariableReadAccessCfgNode).getExpr().getVariable() =
34+
any(RescueClause rc).getVariableExpr().(VariableAccess).getVariable() and
35+
varAccess.flowsTo(this.getReceiver())
36+
) and
37+
this.getMethodName() = ["backtrace", "backtrace_locations"]
38+
}
39+
}
40+
41+
/**
42+
* A call to `Kernel#caller`, considered as a flow source.
43+
*/
44+
class KernelCallerCall extends Source, Kernel::KernelMethodCall {
45+
KernelCallerCall() { this.getMethodName() = "caller" }
46+
}
47+
48+
/**
49+
* The body of an HTTP response that will be returned from a server,
50+
* considered as a flow sink.
51+
*/
52+
class ServerHttpResponseBodyAsSink extends Sink {
53+
ServerHttpResponseBodyAsSink() { this = any(Http::Server::HttpResponse response).getBody() }
54+
}
55+
}

‎ruby/ql/lib/codeql/ruby/security/StackTraceExposureQuery.qll

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
/**
2+
* Provides a taint-tracking configuration for detecting stack-trace exposure
3+
* vulnerabilities.
4+
*
5+
* Note, for performance reasons: only import this file if
6+
* `StackTraceExposure::Configuration` is needed; otherwise,
7+
* `StackTraceExposureCustomizations` should be imported instead.
8+
*/
9+
10+
private import codeql.ruby.DataFlow
11+
private import codeql.ruby.TaintTracking
12+
private import StackTraceExposureCustomizations::StackTraceExposure
13+
14+
/**
15+
* A taint-tracking configuration for detecting "stack trace exposure" vulnerabilities.
16+
*/
17+
class Configuration extends TaintTracking::Configuration {
18+
Configuration() { this = "StackTraceExposure" }
19+
20+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
21+
22+
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
23+
24+
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
25+
}

‎ruby/ql/src/change-notes/2022-11-14-stack-trace-exposure.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* Added a new query, `rb/stack-trace-exposure`, to detect exposure of stack-traces to users via HTTP responses.

‎ruby/ql/src/queries/security/cwe-209/StackTraceExposure.qhelp

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
7+
<p>
8+
Software developers often add stack traces to error messages, as a debugging
9+
aid. Whenever that error message occurs for an end user, the developer can use
10+
the stack trace to help identify how to fix the problem. In particular, stack
11+
traces can tell the developer more about the sequence of events that led to a
12+
failure, as opposed to merely the final state of the software when the error
13+
occurred.
14+
</p>
15+
16+
<p>
17+
Unfortunately, the same information can be useful to an attacker. The sequence
18+
of class or method names in a stack trace can reveal the structure of the
19+
application as well as any internal components it relies on. Furthermore, the
20+
error message at the top of a stack trace can include information such as
21+
server-side file names and SQL code that the application relies on, allowing an
22+
attacker to fine-tune a subsequent injection attack.
23+
</p>
24+
</overview>
25+
26+
<recommendation>
27+
<p>
28+
Send the user a more generic error message that reveals less information.
29+
Either suppress the stack trace entirely, or log it only on the server.
30+
</p>
31+
</recommendation>
32+
33+
<example>
34+
<p>
35+
In the following example, an exception is handled in two different ways. In the
36+
first version, labeled BAD, the exception is exposed to the remote user by
37+
rendering it as an HTTP response. As such, the user is able to see a detailed
38+
stack trace, which may contain sensitive information. In the second version, the
39+
error message is logged only on the server, and a generic error message is
40+
displayed to the user. That way, the developers can still access and use the
41+
error log, but remote users will not see the information. </p>
42+
43+
<sample src="examples/StackTraceExposure.rb" />
44+
</example>
45+
46+
<references>
47+
<li>OWASP: <a href="https://owasp.org/www-community/Improper_Error_Handling">Improper Error Handling</a>.</li>
48+
</references>
49+
</qhelp>

‎ruby/ql/src/queries/security/cwe-209/StackTraceExposure.ql

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/**
2+
* @name Information exposure through an exception
3+
* @description Leaking information about an exception, such as messages and stack traces, to an
4+
* external user can expose implementation details that are useful to an attacker for
5+
* developing a subsequent exploit.
6+
* @kind path-problem
7+
* @problem.severity error
8+
* @security-severity 5.4
9+
* @precision high
10+
* @id rb/stack-trace-exposure
11+
* @tags security
12+
* external/cwe/cwe-209
13+
* external/cwe/cwe-497
14+
*/
15+
16+
import codeql.ruby.DataFlow
17+
import codeql.ruby.security.StackTraceExposureQuery
18+
import DataFlow::PathGraph
19+
20+
from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
21+
where config.hasFlowPath(source, sink)
22+
select sink.getNode(), source, sink, "$@ can be exposed to an external user.", source.getNode(),
23+
"Error information"

‎ruby/ql/src/queries/security/cwe-209/examples/StackTraceExposure.rb

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
class UsersController < ApplicationController
2+
3+
def update_bad(id)
4+
do_computation()
5+
rescue => e
6+
# BAD
7+
render body: e.backtrace, content_type: "text/plain"
8+
end
9+
10+
def update_good(id)
11+
do_computation()
12+
rescue => e
13+
# GOOD
14+
logger.error e.backtrace
15+
render body: "Computation failed", content_type: "text/plain"
16+
end
17+
18+
end

‎ruby/ql/test/query-tests/security/cwe-209/StackTraceExposure.expected

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
edges
2+
| StackTraceExposure.rb:11:10:11:17 | call to caller : | StackTraceExposure.rb:12:18:12:19 | bt |
3+
nodes
4+
| StackTraceExposure.rb:6:18:6:28 | call to backtrace | semmle.label | call to backtrace |
5+
| StackTraceExposure.rb:11:10:11:17 | call to caller : | semmle.label | call to caller : |
6+
| StackTraceExposure.rb:12:18:12:19 | bt | semmle.label | bt |
7+
| StackTraceExposure.rb:18:18:18:28 | call to backtrace | semmle.label | call to backtrace |
8+
subpaths
9+
#select
10+
| StackTraceExposure.rb:6:18:6:28 | call to backtrace | StackTraceExposure.rb:6:18:6:28 | call to backtrace | StackTraceExposure.rb:6:18:6:28 | call to backtrace | $@ can be exposed to an external user. | StackTraceExposure.rb:6:18:6:28 | call to backtrace | Error information |
11+
| StackTraceExposure.rb:12:18:12:19 | bt | StackTraceExposure.rb:11:10:11:17 | call to caller : | StackTraceExposure.rb:12:18:12:19 | bt | $@ can be exposed to an external user. | StackTraceExposure.rb:11:10:11:17 | call to caller | Error information |
12+
| StackTraceExposure.rb:18:18:18:28 | call to backtrace | StackTraceExposure.rb:18:18:18:28 | call to backtrace | StackTraceExposure.rb:18:18:18:28 | call to backtrace | $@ can be exposed to an external user. | StackTraceExposure.rb:18:18:18:28 | call to backtrace | Error information |

‎ruby/ql/test/query-tests/security/cwe-209/StackTraceExposure.qlref

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
queries/security/cwe-209/StackTraceExposure.ql

‎ruby/ql/test/query-tests/security/cwe-209/StackTraceExposure.rb

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
class FooController < ApplicationController
2+
3+
def show
4+
something_that_might_fail()
5+
rescue => e
6+
render body: e.backtrace, content_type: "text/plain"
7+
end
8+
9+
10+
def show2
11+
bt = caller()
12+
render body: bt, content_type: "text/plain"
13+
end
14+
15+
def show3
16+
not_a_method()
17+
rescue NoMethodError => e
18+
render body: e.backtrace, content_type: "text/plain"
19+
end
20+
21+
end

0 commit comments

Comments
 (0)