Skip to content

Commit 3445a6a

Browse files
alexrfordalexrford
committed
fix flow steps from controller instance var assignement to view read access
1 parent b993723 commit 3445a6a

4 files changed

Lines changed: 51 additions & 28 deletions

File tree

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -139,15 +139,18 @@ module ReflectedXSS {
139139
// instance variables in the controller
140140
exists(
141141
ActionControllerActionMethod action, VariableReadAccess viewVarRead,
142-
VariableWriteAccess controllerVarWrite
142+
AssignExpr ae, VariableWriteAccess controllerVarWrite
143143
|
144144
viewVarRead = node2.asExpr().(CfgNodes::ExprNodes::VariableReadAccessCfgNode).getExpr() and
145145
action.getDefaultTemplateFile() = viewVarRead.getLocation().getFile() and
146146
controllerVarWrite.getVariable() instanceof InstanceVariable and
147+
// match read to write on variable name
147148
viewVarRead.getVariable().getName() = controllerVarWrite.getVariable().getName() and
148149
// TODO: include only final assignment along a path
149-
node1.asExpr().getExpr() = controllerVarWrite and
150-
controllerVarWrite.getParent+() = action
150+
// propagate taint from assignment RHS expr to variable read access in view
151+
node1.asExpr().getExpr() = ae.getRightOperand() and
152+
ae.getLeftOperand() = controllerVarWrite and
153+
ae.getParent+() = action
151154
)
152155
or
153156
// flow from template into controller helper method

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

Lines changed: 32 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,38 +1,49 @@
11
edges
22
| app/controllers/foo/bars_controller.rb:10:12:10:17 | call to params : | app/controllers/foo/bars_controller.rb:10:12:10:29 | ...[...] : |
3-
| app/controllers/foo/bars_controller.rb:10:12:10:29 | ...[...] : | app/views/foo/bars/show.html.erb:46:5:46:13 | call to user_name |
4-
| app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params : | app/controllers/foo/bars_controller.rb:20:53:20:54 | dt : |
5-
| app/controllers/foo/bars_controller.rb:20:53:20:54 | dt : | app/views/foo/bars/show.html.erb:5:9:5:20 | call to display_text |
6-
| app/controllers/foo/bars_controller.rb:20:53:20:54 | dt : | app/views/foo/bars/show.html.erb:8:9:8:36 | ...[...] |
7-
| app/controllers/foo/bars_controller.rb:20:53:20:54 | dt : | app/views/foo/bars/show.html.erb:12:9:12:26 | ...[...] |
8-
| app/controllers/foo/bars_controller.rb:20:53:20:54 | dt : | app/views/foo/bars/show.html.erb:33:3:33:14 | call to display_text |
9-
| app/controllers/foo/bars_controller.rb:20:53:20:54 | dt : | app/views/foo/bars/show.html.erb:43:76:43:87 | call to display_text : |
10-
| app/views/foo/bars/show.html.erb:43:64:43:87 | ... + ... : | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text |
11-
| app/views/foo/bars/show.html.erb:43:64:43:87 | ... + ... : | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] |
12-
| app/views/foo/bars/show.html.erb:43:76:43:87 | call to display_text : | app/views/foo/bars/show.html.erb:43:64:43:87 | ... + ... : |
13-
| app/views/foo/bars/show.html.erb:53:29:53:34 | call to params : | app/views/foo/bars/show.html.erb:53:29:53:44 | ...[...] |
3+
| app/controllers/foo/bars_controller.rb:10:12:10:29 | ...[...] : | app/views/foo/bars/show.html.erb:54:5:54:13 | call to user_name |
4+
| app/controllers/foo/bars_controller.rb:18:21:18:26 | call to params : | app/controllers/foo/bars_controller.rb:18:21:18:36 | ...[...] : |
5+
| app/controllers/foo/bars_controller.rb:18:21:18:36 | ...[...] : | app/views/foo/bars/show.html.erb:2:18:2:30 | @user_website |
6+
| app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params : | app/controllers/foo/bars_controller.rb:20:22:20:23 | dt : |
7+
| app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params : | app/controllers/foo/bars_controller.rb:21:53:21:54 | dt : |
8+
| app/controllers/foo/bars_controller.rb:20:22:20:23 | dt : | app/views/foo/bars/show.html.erb:41:3:41:16 | @instance_text |
9+
| app/controllers/foo/bars_controller.rb:21:53:21:54 | dt : | app/views/foo/bars/show.html.erb:5:9:5:20 | call to display_text |
10+
| app/controllers/foo/bars_controller.rb:21:53:21:54 | dt : | app/views/foo/bars/show.html.erb:8:9:8:36 | ...[...] |
11+
| app/controllers/foo/bars_controller.rb:21:53:21:54 | dt : | app/views/foo/bars/show.html.erb:12:9:12:26 | ...[...] |
12+
| app/controllers/foo/bars_controller.rb:21:53:21:54 | dt : | app/views/foo/bars/show.html.erb:36:3:36:14 | call to display_text |
13+
| app/controllers/foo/bars_controller.rb:21:53:21:54 | dt : | app/views/foo/bars/show.html.erb:51:76:51:87 | call to display_text : |
14+
| app/views/foo/bars/show.html.erb:51:64:51:87 | ... + ... : | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text |
15+
| app/views/foo/bars/show.html.erb:51:64:51:87 | ... + ... : | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] |
16+
| app/views/foo/bars/show.html.erb:51:76:51:87 | call to display_text : | app/views/foo/bars/show.html.erb:51:64:51:87 | ... + ... : |
17+
| app/views/foo/bars/show.html.erb:61:29:61:34 | call to params : | app/views/foo/bars/show.html.erb:61:29:61:44 | ...[...] |
1418
nodes
1519
| app/controllers/foo/bars_controller.rb:10:12:10:17 | call to params : | semmle.label | call to params : |
1620
| app/controllers/foo/bars_controller.rb:10:12:10:29 | ...[...] : | semmle.label | ...[...] : |
21+
| app/controllers/foo/bars_controller.rb:18:21:18:26 | call to params : | semmle.label | call to params : |
22+
| app/controllers/foo/bars_controller.rb:18:21:18:36 | ...[...] : | semmle.label | ...[...] : |
1723
| app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params : | semmle.label | call to params : |
18-
| app/controllers/foo/bars_controller.rb:20:53:20:54 | dt : | semmle.label | dt : |
24+
| app/controllers/foo/bars_controller.rb:20:22:20:23 | dt : | semmle.label | dt : |
25+
| app/controllers/foo/bars_controller.rb:21:53:21:54 | dt : | semmle.label | dt : |
1926
| app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | semmle.label | call to display_text |
2027
| app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] | semmle.label | ...[...] |
28+
| app/views/foo/bars/show.html.erb:2:18:2:30 | @user_website | semmle.label | @user_website |
2129
| app/views/foo/bars/show.html.erb:5:9:5:20 | call to display_text | semmle.label | call to display_text |
2230
| app/views/foo/bars/show.html.erb:8:9:8:36 | ...[...] | semmle.label | ...[...] |
2331
| app/views/foo/bars/show.html.erb:12:9:12:26 | ...[...] | semmle.label | ...[...] |
24-
| app/views/foo/bars/show.html.erb:33:3:33:14 | call to display_text | semmle.label | call to display_text |
25-
| app/views/foo/bars/show.html.erb:43:64:43:87 | ... + ... : | semmle.label | ... + ... : |
26-
| app/views/foo/bars/show.html.erb:43:76:43:87 | call to display_text : | semmle.label | call to display_text : |
27-
| app/views/foo/bars/show.html.erb:46:5:46:13 | call to user_name | semmle.label | call to user_name |
28-
| app/views/foo/bars/show.html.erb:53:29:53:34 | call to params : | semmle.label | call to params : |
29-
| app/views/foo/bars/show.html.erb:53:29:53:44 | ...[...] | semmle.label | ...[...] |
32+
| app/views/foo/bars/show.html.erb:36:3:36:14 | call to display_text | semmle.label | call to display_text |
33+
| app/views/foo/bars/show.html.erb:41:3:41:16 | @instance_text | semmle.label | @instance_text |
34+
| app/views/foo/bars/show.html.erb:51:64:51:87 | ... + ... : | semmle.label | ... + ... : |
35+
| app/views/foo/bars/show.html.erb:51:76:51:87 | call to display_text : | semmle.label | call to display_text : |
36+
| app/views/foo/bars/show.html.erb:54:5:54:13 | call to user_name | semmle.label | call to user_name |
37+
| app/views/foo/bars/show.html.erb:61:29:61:34 | call to params : | semmle.label | call to params : |
38+
| app/views/foo/bars/show.html.erb:61:29:61:44 | ...[...] | semmle.label | ...[...] |
3039
#select
3140
| app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | app/controllers/foo/bars_controller.rb:19:10:19: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 $@. | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params | a user-provided value |
3241
| app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params : | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] | Cross-site scripting vulnerability due to $@. | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params | a user-provided value |
42+
| app/views/foo/bars/show.html.erb:2:18:2:30 | @user_website | app/controllers/foo/bars_controller.rb:18:21:18:26 | call to params : | app/views/foo/bars/show.html.erb:2:18:2:30 | @user_website | Cross-site scripting vulnerability due to $@. | app/controllers/foo/bars_controller.rb:18:21:18:26 | call to params | a user-provided value |
3343
| app/views/foo/bars/show.html.erb:5:9:5:20 | call to display_text | app/controllers/foo/bars_controller.rb:19:10:19: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 $@. | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params | a user-provided value |
3444
| app/views/foo/bars/show.html.erb:8:9:8:36 | ...[...] | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params : | app/views/foo/bars/show.html.erb:8:9:8:36 | ...[...] | Cross-site scripting vulnerability due to $@. | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params | a user-provided value |
3545
| app/views/foo/bars/show.html.erb:12:9:12:26 | ...[...] | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params : | app/views/foo/bars/show.html.erb:12:9:12:26 | ...[...] | Cross-site scripting vulnerability due to $@. | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params | a user-provided value |
36-
| app/views/foo/bars/show.html.erb:33:3:33:14 | call to display_text | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params : | app/views/foo/bars/show.html.erb:33:3:33:14 | call to display_text | Cross-site scripting vulnerability due to $@. | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params | a user-provided value |
37-
| app/views/foo/bars/show.html.erb:46:5:46:13 | call to user_name | app/controllers/foo/bars_controller.rb:10:12:10:17 | call to params : | app/views/foo/bars/show.html.erb:46:5:46:13 | call to user_name | Cross-site scripting vulnerability due to $@. | app/controllers/foo/bars_controller.rb:10:12:10:17 | call to params | a user-provided value |
38-
| app/views/foo/bars/show.html.erb:53:29:53:44 | ...[...] | app/views/foo/bars/show.html.erb:53:29:53:34 | call to params : | app/views/foo/bars/show.html.erb:53:29:53:44 | ...[...] | Cross-site scripting vulnerability due to $@. | app/views/foo/bars/show.html.erb:53:29:53:34 | call to params | a user-provided value |
46+
| app/views/foo/bars/show.html.erb:36:3:36:14 | call to display_text | app/controllers/foo/bars_controller.rb:19:10:19: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 $@. | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params | a user-provided value |
47+
| app/views/foo/bars/show.html.erb:41:3:41:16 | @instance_text | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params : | app/views/foo/bars/show.html.erb:41:3:41:16 | @instance_text | Cross-site scripting vulnerability due to $@. | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params | a user-provided value |
48+
| app/views/foo/bars/show.html.erb:54:5:54:13 | call to user_name | app/controllers/foo/bars_controller.rb:10:12:10:17 | call to params : | app/views/foo/bars/show.html.erb:54:5:54:13 | call to user_name | Cross-site scripting vulnerability due to $@. | app/controllers/foo/bars_controller.rb:10:12:10:17 | call to params | a user-provided value |
49+
| app/views/foo/bars/show.html.erb:61:29:61:44 | ...[...] | app/views/foo/bars/show.html.erb:61:29:61:34 | call to params : | app/views/foo/bars/show.html.erb:61:29:61:44 | ...[...] | Cross-site scripting vulnerability due to $@. | app/views/foo/bars/show.html.erb:61:29:61:34 | call to params | a user-provided value |

‎ql/test/query-tests/security/cwe-079/app/controllers/foo/bars_controller.rb‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ def user_name_memo
1717
def show
1818
@user_website = params[:website]
1919
dt = params[:text]
20+
@instance_text = dt
2021
render "foo/bars/show", locals: { display_text: dt, safe_text: "hello" }
2122
end
2223
end

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

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
<%# BAD: An instance variable rendered without escaping %>
2-
<a href="<%= raw user_website %>">website</a>
2+
<a href="<%= raw @user_website %>">website</a>
33

44
<%# BAD: A local rendered raw as a local variable %>
55
<%= raw display_text %>
@@ -28,16 +28,24 @@
2828
full_text
2929
%>
3030

31+
<%# GOOD: default escaping of rendered text (from instance var) %>
32+
<%= @instance_text %>
33+
3134
<%# BAD: html_safe marks string as not requiring HTML escaping %>
3235
<%=
3336
display_text.html_safe
3437
%>
3538

3639
<%# BAD: html_safe marks string as not requiring HTML escaping %>
37-
<%# TODO: we miss that `@display_text` is marked here %>
3840
<%=
39-
@display_text.html_safe
40-
@display_text
41+
@instance_text.html_safe
42+
%>
43+
44+
<%# BAD: html_safe marks string as not requiring HTML escaping %>
45+
<%# TODO: we miss that `@instance_text` is marked here %>
46+
<%=
47+
@instance_text.html_safe
48+
@instance_text
4149
%>
4250

4351
<%= render partial: 'foo/bars/widget', locals: { display_text: "widget_" + display_text } %>

0 commit comments

Comments
 (0)