Skip to content

Commit e5fbc92

Browse files
alexrfordalexrford
committed
Ruby: generalize rails flow step for accessing render locals hash in view
1 parent 976b040 commit e5fbc92

File tree

4 files changed

+116
-45
lines changed

4 files changed

+116
-45
lines changed

‎ruby/ql/lib/codeql/ruby/dataflow/FlowSummary.qll

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ module SummaryComponent {
3030

3131
predicate withContent = SC::withContent/1;
3232

33+
class SyntheticGlobal = SC::SyntheticGlobal;
34+
3335
/** Gets a summary component that represents a receiver. */
3436
SummaryComponent receiver() { result = argument(any(ParameterPosition pos | pos.isSelf())) }
3537

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

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ private import codeql.ruby.frameworks.ActiveStorage
1010
private import codeql.ruby.frameworks.internal.Rails
1111
private import codeql.ruby.ApiGraphs
1212
private import codeql.ruby.security.OpenSSL
13+
private import codeql.ruby.dataflow.FlowSummary
1314

1415
/**
1516
* Provides classes for working with Rails.
@@ -287,5 +288,104 @@ private class CookiesSameSiteProtectionSetting extends Settings::NillableStringl
287288
result = "Unsetting 'SameSite' can disable same-site cookie restrictions in some browsers."
288289
}
289290
}
291+
290292
// TODO: initialization hooks, e.g. before_configuration, after_initialize...
291293
// TODO: initializers
294+
private string getErbFileIdentifier(ErbFile erbFile) { result = erbFile.getRelativePath() }
295+
296+
/** A synthetic global to represent the value passed to the `locals` argument of a render call for a specific ERB file. */
297+
private class LocalAssignsHashSyntheticGlobal extends SummaryComponent::SyntheticGlobal {
298+
private ErbFile erbFile;
299+
private string id;
300+
301+
LocalAssignsHashSyntheticGlobal() {
302+
this = "LocalAssignsHashSyntheticGlobal+" + id and
303+
id = getErbFileIdentifier(erbFile)
304+
}
305+
306+
/** Gets the `ErbFile` which this locals hash is accessible from. */
307+
ErbFile getErbFile() { result = erbFile }
308+
309+
/** Gets the identifier for this particular locals hash synthetic global. */
310+
string getId() { result = id }
311+
}
312+
313+
/** A summary for `render` calls linked to some specific ERB file. */
314+
private class RenderLocalsSummary extends SummarizedCallable {
315+
private string id;
316+
private LocalAssignsHashSyntheticGlobal glob;
317+
318+
RenderLocalsSummary() {
319+
this = "rails_render_locals()" + id and
320+
glob.getId() = id
321+
}
322+
323+
override Rails::RenderCall getACall() { result.getTemplateFile() = glob.getErbFile() }
324+
325+
override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
326+
input = "Argument[locals:]" and
327+
output = "SyntheticGlobal[" + glob + "]" and
328+
preservesValue = true
329+
}
330+
}
331+
332+
/** A summary for calls to `local_assigns` in a view to access a `render` call `locals` hash. */
333+
private class AccessLocalsSummary extends SummarizedCallable {
334+
private string id;
335+
private LocalAssignsHashSyntheticGlobal glob;
336+
337+
AccessLocalsSummary() {
338+
this = "rails_local_assigns()" + id and
339+
glob.getId() = id
340+
}
341+
342+
override MethodCall getACall() {
343+
id = getErbFileIdentifier(result.getLocation().getFile()) and
344+
result.getMethodName() = "local_assigns"
345+
}
346+
347+
override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
348+
input = "SyntheticGlobal[" + glob + "]" and
349+
output = "ReturnValue" and
350+
preservesValue = true
351+
}
352+
}
353+
354+
private string getAMethodNameFromErbFile(ErbFile f) {
355+
result = any(MethodCall c | c.getLocation().getFile() = f).getMethodName()
356+
}
357+
358+
private predicate renderHasLocalsKey(Rails::RenderCall c, string key) {
359+
exists(DataFlow::HashLiteralNode hashLitNode, DataFlow::CallNode renderCall |
360+
renderCall.asExpr().getExpr() = c and
361+
hashLitNode.flowsTo(renderCall.getKeywordArgument("locals"))
362+
|
363+
key = hashLitNode.getAKeyValuePair().getKey().getConstantValue().getStringlikeValue()
364+
)
365+
}
366+
367+
private class AccessLocalsKeySummary extends SummarizedCallable {
368+
private string id;
369+
private LocalAssignsHashSyntheticGlobal glob;
370+
private string methodName;
371+
372+
AccessLocalsKeySummary() {
373+
this = "rails_locals_key()" + id and
374+
id = glob.getId() + "#" + methodName and
375+
methodName = getAMethodNameFromErbFile(glob.getErbFile())
376+
// TODO: this would cut down massively on impossible flow steps, but fails due to non-monotonic recusrion problems
377+
// and
378+
// renderHasLocalsKey(any(Rails::RenderCall c | c.getTemplateFile() = erbFile), methodName))
379+
}
380+
381+
override MethodCall getACall() {
382+
result.getLocation().getFile() = glob.getErbFile() and
383+
result.getMethodName() = methodName
384+
}
385+
386+
override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
387+
input = "SyntheticGlobal[" + glob + "].Element[:" + methodName + "]" and
388+
output = "ReturnValue" and
389+
preservesValue = false
390+
}
391+
}

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

Lines changed: 0 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -162,46 +162,6 @@ private module Shared {
162162
erb = call.getLocation().getFile()
163163
}
164164

165-
/**
166-
* Holds if some render call passes `value` for `hashKey` in the `locals`
167-
* argument, in ERB file `erb`.
168-
*/
169-
pragma[noinline]
170-
private predicate renderCallLocals(string hashKey, Expr value, ErbFile erb) {
171-
exists(Rails::RenderCall call, Pair kvPair |
172-
call.getLocals().getAKeyValuePair() = kvPair and
173-
kvPair.getValue() = value and
174-
kvPair.getKey().getConstantValue().isStringlikeValue(hashKey) and
175-
call.getTemplateFile() = erb
176-
)
177-
}
178-
179-
pragma[noinline]
180-
private predicate isFlowFromLocals0(
181-
CfgNodes::ExprNodes::ElementReferenceCfgNode refNode, string hashKey, ErbFile erb
182-
) {
183-
exists(DataFlow::Node argNode |
184-
argNode.asExpr() = refNode.getArgument(0) and
185-
refNode.getReceiver().getExpr().(MethodCall).getMethodName() = "local_assigns" and
186-
argNode.getALocalSource().getConstantValue().isStringlikeValue(hashKey) and
187-
erb = refNode.getFile()
188-
)
189-
}
190-
191-
private predicate isFlowFromLocals(DataFlow::Node node1, DataFlow::Node node2) {
192-
exists(string hashKey, ErbFile erb |
193-
// node1 is a `locals` argument to a render call...
194-
renderCallLocals(hashKey, node1.asExpr().getExpr(), erb)
195-
|
196-
// node2 is an element reference against `local_assigns`
197-
isFlowFromLocals0(node2.asExpr(), hashKey, erb)
198-
or
199-
// ...node2 is a "method call" to a "method" with `hashKey` as its name
200-
// TODO: This may be a variable read in reality that we interpret as a method call
201-
isMethodCall(node2.asExpr().getExpr(), hashKey, erb)
202-
)
203-
}
204-
205165
/**
206166
* Holds if `action` contains an assignment of `value` to an instance
207167
* variable named `name`, in ERB file `erb`.
@@ -275,8 +235,6 @@ private module Shared {
275235
* An additional step that is preserves dataflow in the context of XSS.
276236
*/
277237
predicate isAdditionalXssFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
278-
isFlowFromLocals(node1, node2)
279-
or
280238
isFlowFromControllerInstanceVariable(node1, node2)
281239
or
282240
isFlowIntoHelperMethod(node1, node2)

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

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,19 @@ edges
1515
| app/controllers/foo/bars_controller.rb:24:39:24:44 | call to params : | app/controllers/foo/bars_controller.rb:24:39:24:59 | ...[...] : |
1616
| app/controllers/foo/bars_controller.rb:24:39:24:59 | ...[...] : | app/controllers/foo/bars_controller.rb:24:39:24:59 | ... = ... |
1717
| app/controllers/foo/bars_controller.rb:26:53:26:54 | dt : | app/views/foo/bars/show.html.erb:5:9:5:20 | call to display_text |
18-
| app/controllers/foo/bars_controller.rb:26:53:26:54 | dt : | app/views/foo/bars/show.html.erb:8:9:8:36 | ...[...] |
19-
| app/controllers/foo/bars_controller.rb:26:53:26:54 | dt : | app/views/foo/bars/show.html.erb:12:9:12:26 | ...[...] |
18+
| app/controllers/foo/bars_controller.rb:26:53:26:54 | dt : | app/views/foo/bars/show.html.erb:8:9:8:21 | call to local_assigns [element :display_text] : |
19+
| app/controllers/foo/bars_controller.rb:26:53:26:54 | dt : | app/views/foo/bars/show.html.erb:12:9:12:21 | call to local_assigns [element :display_text] : |
20+
| app/controllers/foo/bars_controller.rb:26:53:26:54 | dt : | app/views/foo/bars/show.html.erb:18:15:18:27 | call to local_assigns [element :display_text] : |
2021
| app/controllers/foo/bars_controller.rb:26:53:26:54 | dt : | app/views/foo/bars/show.html.erb:36:3:36:14 | call to display_text |
2122
| app/controllers/foo/bars_controller.rb:26:53:26:54 | dt : | app/views/foo/bars/show.html.erb:44:76:44:87 | call to display_text : |
2223
| app/controllers/foo/bars_controller.rb:30:11:30:16 | call to params : | app/controllers/foo/bars_controller.rb:30:11:30:28 | ...[...] : |
2324
| app/controllers/foo/bars_controller.rb:30:11:30:28 | ...[...] : | app/controllers/foo/bars_controller.rb:31:5:31:7 | str |
25+
| app/views/foo/bars/_widget.html.erb:8:9:8:21 | call to local_assigns [element :display_text] : | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] |
26+
| app/views/foo/bars/show.html.erb:8:9:8:21 | call to local_assigns [element :display_text] : | app/views/foo/bars/show.html.erb:8:9:8:36 | ...[...] |
27+
| app/views/foo/bars/show.html.erb:12:9:12:21 | call to local_assigns [element :display_text] : | app/views/foo/bars/show.html.erb:12:9:12:26 | ...[...] |
28+
| app/views/foo/bars/show.html.erb:18:15:18:27 | call to local_assigns [element :display_text] : | app/views/foo/bars/show.html.erb:18:15:18:32 | ...[...] |
2429
| app/views/foo/bars/show.html.erb:44:64:44:87 | ... + ... : | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text |
25-
| app/views/foo/bars/show.html.erb:44:64:44:87 | ... + ... : | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] |
30+
| app/views/foo/bars/show.html.erb:44:64:44:87 | ... + ... : | app/views/foo/bars/_widget.html.erb:8:9:8:21 | call to local_assigns [element :display_text] : |
2631
| 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 | ... + ... : |
2732
| 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 | ...[...] |
2833
| 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 | ...[...] |
@@ -47,11 +52,16 @@ nodes
4752
| app/controllers/foo/bars_controller.rb:30:11:30:28 | ...[...] : | semmle.label | ...[...] : |
4853
| app/controllers/foo/bars_controller.rb:31:5:31:7 | str | semmle.label | str |
4954
| app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | semmle.label | call to display_text |
55+
| app/views/foo/bars/_widget.html.erb:8:9:8:21 | call to local_assigns [element :display_text] : | semmle.label | call to local_assigns [element :display_text] : |
5056
| app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] | semmle.label | ...[...] |
5157
| app/views/foo/bars/show.html.erb:2:18:2:30 | @user_website | semmle.label | @user_website |
5258
| app/views/foo/bars/show.html.erb:5:9:5:20 | call to display_text | semmle.label | call to display_text |
59+
| app/views/foo/bars/show.html.erb:8:9:8:21 | call to local_assigns [element :display_text] : | semmle.label | call to local_assigns [element :display_text] : |
5360
| app/views/foo/bars/show.html.erb:8:9:8:36 | ...[...] | semmle.label | ...[...] |
61+
| app/views/foo/bars/show.html.erb:12:9:12:21 | call to local_assigns [element :display_text] : | semmle.label | call to local_assigns [element :display_text] : |
5462
| app/views/foo/bars/show.html.erb:12:9:12:26 | ...[...] | semmle.label | ...[...] |
63+
| app/views/foo/bars/show.html.erb:18:15:18:27 | call to local_assigns [element :display_text] : | semmle.label | call to local_assigns [element :display_text] : |
64+
| app/views/foo/bars/show.html.erb:18:15:18:32 | ...[...] | semmle.label | ...[...] |
5565
| app/views/foo/bars/show.html.erb:36:3:36:14 | call to display_text | semmle.label | call to display_text |
5666
| app/views/foo/bars/show.html.erb:41:3:41:16 | @instance_text | semmle.label | @instance_text |
5767
| app/views/foo/bars/show.html.erb:44:64:44:87 | ... + ... : | semmle.label | ... + ... : |
@@ -76,6 +86,7 @@ subpaths
7686
| app/views/foo/bars/show.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/show.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 |
7787
| app/views/foo/bars/show.html.erb:8:9:8:36 | ...[...] | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params : | app/views/foo/bars/show.html.erb:8:9:8:36 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
7888
| app/views/foo/bars/show.html.erb:12:9:12:26 | ...[...] | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params : | app/views/foo/bars/show.html.erb:12:9:12:26 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
89+
| app/views/foo/bars/show.html.erb:18:15:18:32 | ...[...] | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params : | app/views/foo/bars/show.html.erb:18:15:18:32 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
7990
| app/views/foo/bars/show.html.erb:36:3:36:14 | call to display_text | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params : | app/views/foo/bars/show.html.erb:36:3:36:14 | 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 |
8091
| app/views/foo/bars/show.html.erb:41:3:41:16 | @instance_text | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params : | app/views/foo/bars/show.html.erb:41:3:41:16 | @instance_text | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
8192
| app/views/foo/bars/show.html.erb:47:5:47:13 | call to user_name | app/controllers/foo/bars_controller.rb:9:12:9:17 | call to params : | app/views/foo/bars/show.html.erb:47:5:47:13 | call to user_name | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:9:12:9:17 | call to params | user-provided value |

0 commit comments

Comments
 (0)