Skip to content

Commit ed0c85e

Browse files
hmachmac
committed
Ruby: Model ActionView helper XSS sinks
1 parent a8197b2 commit ed0c85e

File tree

6 files changed

+145
-0
lines changed

6 files changed

+145
-0
lines changed

‎ruby/ql/lib/codeql/ruby/frameworks/ActionView.qll‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,4 +216,116 @@ class FileSystemResolverAccess extends DataFlow::CallNode, FileSystemAccess::Ran
216216

217217
override DataFlow::Node getAPathArgument() { result = this.getArgument(0) }
218218
}
219+
219220
// TODO: model flow in/out of template files properly,
221+
//
222+
/**
223+
* Action view helper methods which are XSS sinks.
224+
*/
225+
module ActionViewHelpers {
226+
/**
227+
* Calls to ActionView helpers which render their argument without escaping.
228+
* These arguments should be treated as XSS sinks.
229+
* In the documentation for classes in this module, the vulnerable argument is
230+
* named `x`.
231+
*/
232+
abstract class RawHelperCall extends MethodCall {
233+
abstract Expr getRawArgument();
234+
}
235+
236+
/**
237+
* `ActionView::Helpers::TextHelper#simple_format`.
238+
*
239+
* `simple_format(x, y, sanitize: false)`.
240+
*/
241+
private class SimpleFormat extends ActionViewContextCall, RawHelperCall {
242+
SimpleFormat() {
243+
this.getMethodName() = "simple_format" and
244+
this.getKeywordArgument("sanitize").getConstantValue().isBoolean(false)
245+
}
246+
247+
override Expr getRawArgument() { result = this.getArgument(0) }
248+
}
249+
250+
/**
251+
* `ActionView::Helpers::TextHelper#truncate`.
252+
*
253+
* `truncate(x, escape: false)`.
254+
*/
255+
private class Truncate extends ActionViewContextCall, RawHelperCall {
256+
Truncate() {
257+
this.getMethodName() = "truncate" and
258+
this.getKeywordArgument("escape").getConstantValue().isBoolean(false)
259+
}
260+
261+
override Expr getRawArgument() { result = this.getArgument(0) }
262+
}
263+
264+
/**
265+
* `ActionView::Helpers::TextHelper#highlight`.
266+
*
267+
* `truncate(x, y, sanitize: false)`.
268+
*/
269+
private class Highlight extends ActionViewContextCall, RawHelperCall {
270+
Highlight() {
271+
this.getMethodName() = "highlight" and
272+
this.getKeywordArgument("sanitize").getConstantValue().isBoolean(false)
273+
}
274+
275+
override Expr getRawArgument() { result = this.getArgument(0) }
276+
}
277+
278+
/**
279+
* `ActionView::Helpers::JavascriptHelper#javascript_tag`.
280+
*
281+
* `javascript_tag(x)`.
282+
*/
283+
private class JavascriptTag extends ActionViewContextCall, RawHelperCall {
284+
JavascriptTag() { this.getMethodName() = "javascript_tag" }
285+
286+
override Expr getRawArgument() { result = this.getArgument(0) }
287+
}
288+
289+
/**
290+
* `ActionView::Helpers::TagHelper#tag`.
291+
*
292+
* `tag(x, x, y, false)`.
293+
*/
294+
private class ContentTag extends ActionViewContextCall, RawHelperCall {
295+
ContentTag() {
296+
this.getMethodName() = "content_tag" and
297+
this.getArgument(3).getConstantValue().isBoolean(false)
298+
}
299+
300+
override Expr getRawArgument() { result = this.getArgument(1) }
301+
}
302+
303+
/**
304+
* `ActionView::Helpers::TagHelper#tag`.
305+
*
306+
* `tag(x, x, y, false)`.
307+
*/
308+
private class Tag extends ActionViewContextCall, RawHelperCall {
309+
Tag() {
310+
this.getMethodName() = "tag" and
311+
this.getArgument(3).getConstantValue().isBoolean(false)
312+
}
313+
314+
override Expr getRawArgument() { result = this.getArgument(0) }
315+
}
316+
317+
/**
318+
* `ActionView::Helpers::TagHelper#tag.<tag name>`.
319+
*
320+
* `tag.h1(x, escape: false)`.
321+
*/
322+
private class TagMethod extends MethodCall, RawHelperCall {
323+
TagMethod() {
324+
inActionViewContext(this) and
325+
this.getReceiver().(MethodCall).getMethodName() = "tag" and
326+
this.getKeywordArgument("escape").getConstantValue().isBoolean(false)
327+
}
328+
329+
override Expr getRawArgument() { result = this.getArgument(0) }
330+
}
331+
}

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,18 @@ private module Shared {
7575
RawCallArgumentAsSink() { this.getCall() instanceof RawCall }
7676
}
7777

78+
/**
79+
* An argument to an ActionView helper method which is not escaped,
80+
* considered as a flow sink.
81+
*/
82+
class RawHelperCallArgumentAsSink extends Sink, ErbOutputMethodCallArgumentNode {
83+
RawHelperCallArgumentAsSink() {
84+
exists(ErbOutputDirective d, ActionViewHelpers::RawHelperCall c |
85+
d.getTerminalStmt() = c and this.asExpr().getExpr() = c.getRawArgument()
86+
)
87+
}
88+
}
89+
7890
/**
7991
* A argument to a call to the `link_to` method, which does not expect
8092
* unsanitized user-input, considered as a flow sink.

‎ruby/ql/test/library-tests/frameworks/ActionView.expected‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,3 +30,12 @@ httpResponses
3030
| app/controllers/foo/bars_controller.rb:36:12:36:67 | call to render_to_string | app/controllers/foo/bars_controller.rb:36:29:36:33 | @user | application/json |
3131
| app/controllers/foo/bars_controller.rb:38:5:38:50 | call to render | app/controllers/foo/bars_controller.rb:38:12:38:22 | call to backtrace | text/plain |
3232
| app/controllers/foo/bars_controller.rb:44:5:44:17 | call to render | app/controllers/foo/bars_controller.rb:44:12:44:17 | "show" | text/html |
33+
rawHelperCalls
34+
| action_view/helpers.erb:4:1:4:36 | call to simple_format | action_view/helpers.erb:4:15:4:15 | call to x |
35+
| action_view/helpers.erb:7:1:7:26 | call to truncate | action_view/helpers.erb:7:10:7:10 | call to x |
36+
| action_view/helpers.erb:10:1:10:29 | call to highlight | action_view/helpers.erb:10:11:10:11 | call to x |
37+
| action_view/helpers.erb:12:1:12:17 | call to javascript_tag | action_view/helpers.erb:12:16:12:16 | call to x |
38+
| action_view/helpers.erb:15:1:15:27 | call to content_tag | action_view/helpers.erb:15:16:15:16 | call to y |
39+
| action_view/helpers.erb:18:1:18:19 | call to tag | action_view/helpers.erb:18:5:18:5 | call to x |
40+
| action_view/helpers.erb:21:1:21:24 | call to h1 | action_view/helpers.erb:21:8:21:8 | call to x |
41+
| action_view/helpers.erb:24:1:24:23 | call to p | action_view/helpers.erb:24:7:24:7 | call to x |

‎ruby/ql/test/library-tests/frameworks/ActionView.ql‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
private import ruby
12
private import codeql.ruby.frameworks.ActionController
23
private import codeql.ruby.frameworks.ActionView
34
private import codeql.ruby.Concepts
@@ -16,3 +17,7 @@ query predicate linkToCalls(LinkToCall c) { any() }
1617
query predicate httpResponses(Http::Server::HttpResponse r, DataFlow::Node body, string mimeType) {
1718
r.getBody() = body and r.getMimetype() = mimeType
1819
}
20+
21+
query predicate rawHelperCalls(ActionViewHelpers::RawHelperCall c, Expr arg) {
22+
arg = c.getRawArgument()
23+
}

‎ruby/ql/test/query-tests/security/cwe-079/ReflectedXSS.expected‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ edges
2222
| app/views/foo/bars/show.html.erb:44:76:44:87 | call to display_text : | app/views/foo/bars/show.html.erb:44:64:44:87 | ... + ... : |
2323
| app/views/foo/bars/show.html.erb:54:29:54:34 | call to params : | app/views/foo/bars/show.html.erb:54:29:54:44 | ...[...] |
2424
| app/views/foo/bars/show.html.erb:57:13:57:18 | call to params : | app/views/foo/bars/show.html.erb:57:13:57:28 | ...[...] |
25+
| app/views/foo/bars/show.html.erb:74:19:74:24 | call to params : | app/views/foo/bars/show.html.erb:74:19:74:34 | ...[...] |
2526
nodes
2627
| app/controllers/foo/bars_controller.rb:9:12:9:17 | call to params : | semmle.label | call to params : |
2728
| app/controllers/foo/bars_controller.rb:9:12:9:29 | ...[...] : | semmle.label | ...[...] : |
@@ -50,6 +51,8 @@ nodes
5051
| app/views/foo/bars/show.html.erb:54:29:54:44 | ...[...] | semmle.label | ...[...] |
5152
| app/views/foo/bars/show.html.erb:57:13:57:18 | call to params : | semmle.label | call to params : |
5253
| app/views/foo/bars/show.html.erb:57:13:57:28 | ...[...] | semmle.label | ...[...] |
54+
| app/views/foo/bars/show.html.erb:74:19:74:24 | call to params : | semmle.label | call to params : |
55+
| app/views/foo/bars/show.html.erb:74:19:74:34 | ...[...] | semmle.label | ...[...] |
5356
subpaths
5457
#select
5558
| app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params : | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
@@ -64,3 +67,4 @@ subpaths
6467
| app/views/foo/bars/show.html.erb:51:5:51:18 | call to user_name_memo | app/controllers/foo/bars_controller.rb:13:20:13:25 | call to params : | app/views/foo/bars/show.html.erb:51:5:51:18 | call to user_name_memo | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:13:20:13:25 | call to params | user-provided value |
6568
| app/views/foo/bars/show.html.erb:54:29:54:44 | ...[...] | app/views/foo/bars/show.html.erb:54:29:54:34 | call to params : | app/views/foo/bars/show.html.erb:54:29:54:44 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/views/foo/bars/show.html.erb:54:29:54:34 | call to params | user-provided value |
6669
| app/views/foo/bars/show.html.erb:57:13:57:28 | ...[...] | app/views/foo/bars/show.html.erb:57:13:57:18 | call to params : | app/views/foo/bars/show.html.erb:57:13:57:28 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/views/foo/bars/show.html.erb:57:13:57:18 | call to params | user-provided value |
70+
| app/views/foo/bars/show.html.erb:74:19:74:34 | ...[...] | app/views/foo/bars/show.html.erb:74:19:74:24 | call to params : | app/views/foo/bars/show.html.erb:74:19:74:34 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/views/foo/bars/show.html.erb:74:19:74:24 | call to params | user-provided value |

‎ruby/ql/test/query-tests/security/cwe-079/app/views/foo/bars/show.html.erb‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,3 +69,6 @@
6969
html_escaped_in_template = h params[:text]
7070
html_escaped_in_template.html_safe
7171
%>
72+
73+
<%# BAD: simple_format called with sanitize: false %>
74+
<%= simple_format(params[:comment], sanitize: false) %>

0 commit comments

Comments
 (0)