Skip to content

Commit 42a97b2

Browse files
hmachmac
authored
Merge pull request #10316 from hmac/hmac/actionview
Ruby: Model ActionView
2 parents 98f4caf + e48665a commit 42a97b2

File tree

12 files changed

+377
-4
lines changed

12 files changed

+377
-4
lines changed

‎ruby/ql/lib/change-notes/2022-09-27-actionview.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Various XSS sinks in the ActionView library are now recognized.

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ private import codeql.ruby.controlflow.CfgNodes
88
private import codeql.ruby.DataFlow
99
private import codeql.ruby.dataflow.RemoteFlowSources
1010
private import codeql.ruby.ApiGraphs
11-
private import ActionView
11+
private import codeql.ruby.frameworks.ActionView
1212
private import codeql.ruby.frameworks.ActionDispatch
1313

1414
/**

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

Lines changed: 135 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ private class ActionViewHtmlSafeCall extends HtmlSafeCall {
3737
*/
3838
abstract class HtmlEscapeCall extends MethodCall {
3939
// "h" is aliased to "html_escape" in ActiveSupport
40-
HtmlEscapeCall() { this.getMethodName() = ["html_escape", "html_escape_once", "h"] }
40+
HtmlEscapeCall() { this.getMethodName() = ["html_escape", "html_escape_once", "h", "sanitize"] }
4141
}
4242

4343
/**
@@ -215,4 +215,138 @@ class FileSystemResolverAccess extends DataFlow::CallNode, FileSystemAccess::Ran
215215

216216
override DataFlow::Node getAPathArgument() { result = this.getArgument(0) }
217217
}
218+
218219
// TODO: model flow in/out of template files properly,
220+
// TODO: Move the classes and predicates above inside this module.
221+
/** Modeling for `ActionView`. */
222+
module ActionView {
223+
/**
224+
* Action view helper methods which are XSS sinks.
225+
*/
226+
module Helpers {
227+
/**
228+
* A call to an ActionView helper which renders its argument without escaping.
229+
* The argument should be treated as an XSS sink. In the documentation for
230+
* classes in this module, the vulnerable argument is named `x`.
231+
*/
232+
abstract class RawHelperCall extends MethodCall {
233+
/**
234+
* Get an argument which is rendered without escaping.
235+
*/
236+
abstract Expr getRawArgument();
237+
}
238+
239+
/**
240+
* `ActionView::Helpers::TextHelper#simple_format`.
241+
*
242+
* `simple_format(x, y, sanitize: false)`.
243+
*/
244+
private class SimpleFormat extends ActionViewContextCall, RawHelperCall {
245+
SimpleFormat() {
246+
this.getMethodName() = "simple_format" and
247+
this.getKeywordArgument("sanitize").getConstantValue().isBoolean(false)
248+
}
249+
250+
override Expr getRawArgument() { result = this.getArgument(0) }
251+
}
252+
253+
/**
254+
* `ActionView::Helpers::TextHelper#truncate`.
255+
*
256+
* `truncate(x, escape: false)`.
257+
*/
258+
private class Truncate extends ActionViewContextCall, RawHelperCall {
259+
Truncate() {
260+
this.getMethodName() = "truncate" and
261+
this.getKeywordArgument("escape").getConstantValue().isBoolean(false)
262+
}
263+
264+
override Expr getRawArgument() { result = this.getArgument(0) }
265+
}
266+
267+
/**
268+
* `ActionView::Helpers::TextHelper#highlight`.
269+
*
270+
* `highlight(x, y, sanitize: false)`.
271+
*/
272+
private class Highlight extends ActionViewContextCall, RawHelperCall {
273+
Highlight() {
274+
this.getMethodName() = "highlight" and
275+
this.getKeywordArgument("sanitize").getConstantValue().isBoolean(false)
276+
}
277+
278+
override Expr getRawArgument() { result = this.getArgument(0) }
279+
}
280+
281+
/**
282+
* `ActionView::Helpers::JavascriptHelper#javascript_tag`.
283+
*
284+
* `javascript_tag(x)`.
285+
*/
286+
private class JavascriptTag extends ActionViewContextCall, RawHelperCall {
287+
JavascriptTag() { this.getMethodName() = "javascript_tag" }
288+
289+
override Expr getRawArgument() { result = this.getArgument(0) }
290+
}
291+
292+
/**
293+
* `ActionView::Helpers::TagHelper#content_tag`.
294+
*
295+
* `content_tag(x, x, y, false)`.
296+
*/
297+
private class ContentTag extends ActionViewContextCall, RawHelperCall {
298+
ContentTag() {
299+
this.getMethodName() = "content_tag" and
300+
this.getArgument(3).getConstantValue().isBoolean(false)
301+
}
302+
303+
override Expr getRawArgument() { result = this.getArgument(1) }
304+
}
305+
306+
/**
307+
* `ActionView::Helpers::TagHelper#tag`.
308+
*
309+
* `tag(x, x, y, false)`.
310+
*/
311+
private class Tag extends ActionViewContextCall, RawHelperCall {
312+
Tag() {
313+
this.getMethodName() = "tag" and
314+
this.getArgument(3).getConstantValue().isBoolean(false)
315+
}
316+
317+
override Expr getRawArgument() { result = this.getArgument(0) }
318+
}
319+
320+
/**
321+
* `ActionView::Helpers::TagHelper#tag.<tag name>`.
322+
*
323+
* `tag.h1(x, escape: false)`.
324+
*/
325+
private class TagMethod extends MethodCall, RawHelperCall {
326+
TagMethod() {
327+
inActionViewContext(this) and
328+
this.getReceiver().(MethodCall).getMethodName() = "tag" and
329+
this.getKeywordArgument("escape").getConstantValue().isBoolean(false)
330+
}
331+
332+
override Expr getRawArgument() { result = this.getArgument(0) }
333+
}
334+
}
335+
336+
/**
337+
* An argument to a method call which constructs a script tag, interpreting the
338+
* argument as a URL. Remote input flowing to this argument may allow loading of
339+
* arbitrary javascript.
340+
*/
341+
class ArgumentInterpretedAsUrl extends DataFlow::Node {
342+
ArgumentInterpretedAsUrl() {
343+
exists(DataFlow::CallNode call |
344+
call.getMethodName() = ["javascript_include_tag", "javascript_path", "path_to_javascript"] and
345+
this = call.getArgument(0)
346+
or
347+
call.getMethodName() = "javascript_url" and
348+
this = call.getKeywordArgument("host")
349+
)
350+
}
351+
}
352+
}

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ private import codeql.ruby.DataFlow
99
private import codeql.ruby.dataflow.FlowSummary
1010
private import codeql.ruby.ApiGraphs
1111
private import codeql.ruby.frameworks.stdlib.Logger::Logger as StdlibLogger
12+
private import codeql.ruby.frameworks.data.ModelsAsData
1213

1314
/**
1415
* Modeling for `ActiveSupport`.
@@ -138,4 +139,28 @@ module ActiveSupport {
138139
}
139140
}
140141
}
142+
143+
/**
144+
* `ActiveSupport::SafeBuffer` wraps a string, providing HTML-safe methods
145+
* for concatenation.
146+
* It is possible to insert tainted data into `SafeBuffer` that won't get
147+
* sanitized, and this taint is then propagated via most of the methods.
148+
*/
149+
private class SafeBufferSummary extends ModelInput::SummaryModelCsv {
150+
// TODO: SafeBuffer also reponds to all String methods.
151+
// Can we model this without repeating all the existing summaries we have
152+
// for String?
153+
override predicate row(string row) {
154+
row =
155+
[
156+
// SafeBuffer.new(x) does not sanitize x
157+
"activesupport;;Member[ActionView].Member[SafeBuffer].Method[new];Argument[0];ReturnValue;taint",
158+
// SafeBuffer#safe_concat(x) does not sanitize x
159+
"activesupport;;Member[ActionView].Member[SafeBuffer].Instance.Method[safe_concat];Argument[0];ReturnValue;taint",
160+
"activesupport;;Member[ActionView].Member[SafeBuffer].Instance.Method[safe_concat];Argument[0];Argument[self];taint",
161+
// These methods preserve taint in self
162+
"activesupport;;Member[ActionView].Member[SafeBuffer].Instance.Method[concat,insert,prepend,to_s,to_param];Argument[self];ReturnValue;taint",
163+
]
164+
}
165+
}
141166
}

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,25 @@ 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 {
83+
RawHelperCallArgumentAsSink() {
84+
exists(ErbOutputDirective d, ActionView::Helpers::RawHelperCall c |
85+
d.getTerminalStmt() = c and this.asExpr().getExpr() = c.getRawArgument()
86+
)
87+
}
88+
}
89+
90+
/**
91+
* An argument that is used to construct the `src` attribute of a `<script>`
92+
* tag.
93+
*/
94+
class ArgumentInterpretedAsUrlAsSink extends Sink, ErbOutputMethodCallArgumentNode,
95+
ActionView::ArgumentInterpretedAsUrl { }
96+
7897
/**
7998
* A argument to a call to the `link_to` method, which does not expect
8099
* 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: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
1+
private import ruby
2+
private import codeql.ruby.AST
13
private import codeql.ruby.frameworks.ActionController
24
private import codeql.ruby.frameworks.ActionView
35
private import codeql.ruby.Concepts
4-
private import codeql.ruby.DataFlow
56

67
query predicate htmlSafeCalls(HtmlSafeCall c) { any() }
78

@@ -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(ActionView::Helpers::RawHelperCall c, Expr arg) {
22+
arg = c.getRawArgument()
23+
}

‎ruby/ql/test/library-tests/frameworks/action_view/helpers.erb‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
<%=
2+
3+
simple_format(x, y, sanitize: true)
4+
simple_format(x, y, sanitize: false)
5+
6+
truncate(x, escape: true)
7+
truncate(x, escape: false)
8+
9+
highlight(x, sanitize: true)
10+
highlight(x, sanitize: false)
11+
12+
javascript_tag(x)
13+
14+
content_tag(x, y, z, true)
15+
content_tag(x, y, z, false)
16+
17+
tag(x, y, z, true)
18+
tag(x, y, z, false)
19+
20+
tag.h1(x, escape: true)
21+
tag.h1(x, escape: false)
22+
23+
tag.p(x, escape: true)
24+
tag.p(x, escape: false)
25+
26+
%>

‎ruby/ql/test/library-tests/frameworks/active_support/ActiveSupportDataFlow.expected‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,40 @@ edges
102102
| active_support.rb:134:10:134:10 | b [element] : | active_support.rb:134:10:134:13 | ...[...] |
103103
| active_support.rb:135:10:135:10 | b [element] : | active_support.rb:135:10:135:13 | ...[...] |
104104
| active_support.rb:135:10:135:10 | b [element] : | active_support.rb:135:10:135:13 | ...[...] |
105+
| active_support.rb:139:7:139:16 | call to source : | active_support.rb:140:34:140:34 | x : |
106+
| active_support.rb:140:7:140:35 | call to new : | active_support.rb:141:8:141:8 | y |
107+
| active_support.rb:140:34:140:34 | x : | active_support.rb:140:7:140:35 | call to new : |
108+
| active_support.rb:146:7:146:16 | call to source : | active_support.rb:147:21:147:21 | b : |
109+
| active_support.rb:147:7:147:22 | call to safe_concat : | active_support.rb:148:8:148:8 | y |
110+
| active_support.rb:147:21:147:21 | b : | active_support.rb:147:7:147:22 | call to safe_concat : |
111+
| active_support.rb:153:7:153:16 | call to source : | active_support.rb:154:17:154:17 | b : |
112+
| active_support.rb:154:3:154:3 | [post] x : | active_support.rb:155:8:155:8 | x |
113+
| active_support.rb:154:17:154:17 | b : | active_support.rb:154:3:154:3 | [post] x : |
114+
| active_support.rb:159:7:159:16 | call to source : | active_support.rb:161:34:161:34 | a : |
115+
| active_support.rb:161:7:161:35 | call to new : | active_support.rb:162:7:162:7 | x : |
116+
| active_support.rb:161:34:161:34 | a : | active_support.rb:161:7:161:35 | call to new : |
117+
| active_support.rb:162:7:162:7 | x : | active_support.rb:162:7:162:17 | call to concat : |
118+
| active_support.rb:162:7:162:17 | call to concat : | active_support.rb:163:8:163:8 | y |
119+
| active_support.rb:167:7:167:16 | call to source : | active_support.rb:169:34:169:34 | a : |
120+
| active_support.rb:169:7:169:35 | call to new : | active_support.rb:170:7:170:7 | x : |
121+
| active_support.rb:169:34:169:34 | a : | active_support.rb:169:7:169:35 | call to new : |
122+
| active_support.rb:170:7:170:7 | x : | active_support.rb:170:7:170:20 | call to insert : |
123+
| active_support.rb:170:7:170:20 | call to insert : | active_support.rb:171:8:171:8 | y |
124+
| active_support.rb:175:7:175:16 | call to source : | active_support.rb:177:34:177:34 | a : |
125+
| active_support.rb:177:7:177:35 | call to new : | active_support.rb:178:7:178:7 | x : |
126+
| active_support.rb:177:34:177:34 | a : | active_support.rb:177:7:177:35 | call to new : |
127+
| active_support.rb:178:7:178:7 | x : | active_support.rb:178:7:178:18 | call to prepend : |
128+
| active_support.rb:178:7:178:18 | call to prepend : | active_support.rb:179:8:179:8 | y |
129+
| active_support.rb:183:7:183:16 | call to source : | active_support.rb:184:34:184:34 | a : |
130+
| active_support.rb:184:7:184:35 | call to new : | active_support.rb:185:7:185:7 | x : |
131+
| active_support.rb:184:34:184:34 | a : | active_support.rb:184:7:184:35 | call to new : |
132+
| active_support.rb:185:7:185:7 | x : | active_support.rb:185:7:185:12 | call to to_s : |
133+
| active_support.rb:185:7:185:12 | call to to_s : | active_support.rb:186:8:186:8 | y |
134+
| active_support.rb:190:7:190:16 | call to source : | active_support.rb:191:34:191:34 | a : |
135+
| active_support.rb:191:7:191:35 | call to new : | active_support.rb:192:7:192:7 | x : |
136+
| active_support.rb:191:34:191:34 | a : | active_support.rb:191:7:191:35 | call to new : |
137+
| active_support.rb:192:7:192:7 | x : | active_support.rb:192:7:192:16 | call to to_param : |
138+
| active_support.rb:192:7:192:16 | call to to_param : | active_support.rb:193:8:193:8 | y |
105139
nodes
106140
| active_support.rb:9:9:9:18 | call to source : | semmle.label | call to source : |
107141
| active_support.rb:10:10:10:10 | x : | semmle.label | x : |
@@ -234,6 +268,48 @@ nodes
234268
| active_support.rb:135:10:135:10 | b [element] : | semmle.label | b [element] : |
235269
| active_support.rb:135:10:135:13 | ...[...] | semmle.label | ...[...] |
236270
| active_support.rb:135:10:135:13 | ...[...] | semmle.label | ...[...] |
271+
| active_support.rb:139:7:139:16 | call to source : | semmle.label | call to source : |
272+
| active_support.rb:140:7:140:35 | call to new : | semmle.label | call to new : |
273+
| active_support.rb:140:34:140:34 | x : | semmle.label | x : |
274+
| active_support.rb:141:8:141:8 | y | semmle.label | y |
275+
| active_support.rb:146:7:146:16 | call to source : | semmle.label | call to source : |
276+
| active_support.rb:147:7:147:22 | call to safe_concat : | semmle.label | call to safe_concat : |
277+
| active_support.rb:147:21:147:21 | b : | semmle.label | b : |
278+
| active_support.rb:148:8:148:8 | y | semmle.label | y |
279+
| active_support.rb:153:7:153:16 | call to source : | semmle.label | call to source : |
280+
| active_support.rb:154:3:154:3 | [post] x : | semmle.label | [post] x : |
281+
| active_support.rb:154:17:154:17 | b : | semmle.label | b : |
282+
| active_support.rb:155:8:155:8 | x | semmle.label | x |
283+
| active_support.rb:159:7:159:16 | call to source : | semmle.label | call to source : |
284+
| active_support.rb:161:7:161:35 | call to new : | semmle.label | call to new : |
285+
| active_support.rb:161:34:161:34 | a : | semmle.label | a : |
286+
| active_support.rb:162:7:162:7 | x : | semmle.label | x : |
287+
| active_support.rb:162:7:162:17 | call to concat : | semmle.label | call to concat : |
288+
| active_support.rb:163:8:163:8 | y | semmle.label | y |
289+
| active_support.rb:167:7:167:16 | call to source : | semmle.label | call to source : |
290+
| active_support.rb:169:7:169:35 | call to new : | semmle.label | call to new : |
291+
| active_support.rb:169:34:169:34 | a : | semmle.label | a : |
292+
| active_support.rb:170:7:170:7 | x : | semmle.label | x : |
293+
| active_support.rb:170:7:170:20 | call to insert : | semmle.label | call to insert : |
294+
| active_support.rb:171:8:171:8 | y | semmle.label | y |
295+
| active_support.rb:175:7:175:16 | call to source : | semmle.label | call to source : |
296+
| active_support.rb:177:7:177:35 | call to new : | semmle.label | call to new : |
297+
| active_support.rb:177:34:177:34 | a : | semmle.label | a : |
298+
| active_support.rb:178:7:178:7 | x : | semmle.label | x : |
299+
| active_support.rb:178:7:178:18 | call to prepend : | semmle.label | call to prepend : |
300+
| active_support.rb:179:8:179:8 | y | semmle.label | y |
301+
| active_support.rb:183:7:183:16 | call to source : | semmle.label | call to source : |
302+
| active_support.rb:184:7:184:35 | call to new : | semmle.label | call to new : |
303+
| active_support.rb:184:34:184:34 | a : | semmle.label | a : |
304+
| active_support.rb:185:7:185:7 | x : | semmle.label | x : |
305+
| active_support.rb:185:7:185:12 | call to to_s : | semmle.label | call to to_s : |
306+
| active_support.rb:186:8:186:8 | y | semmle.label | y |
307+
| active_support.rb:190:7:190:16 | call to source : | semmle.label | call to source : |
308+
| active_support.rb:191:7:191:35 | call to new : | semmle.label | call to new : |
309+
| active_support.rb:191:34:191:34 | a : | semmle.label | a : |
310+
| active_support.rb:192:7:192:7 | x : | semmle.label | x : |
311+
| active_support.rb:192:7:192:16 | call to to_param : | semmle.label | call to to_param : |
312+
| active_support.rb:193:8:193:8 | y | semmle.label | y |
237313
subpaths
238314
#select
239315
| active_support.rb:106:10:106:13 | ...[...] | active_support.rb:104:10:104:17 | call to source : | active_support.rb:106:10:106:13 | ...[...] | $@ | active_support.rb:104:10:104:17 | call to source : | call to source : |

0 commit comments

Comments
 (0)