Skip to content

Commit 8db23dc

Browse files
alexrfordalexrford
committed
Ruby: refine ActiveRecord update_all as an SQL sink
1 parent 013e7aa commit 8db23dc

File tree

4 files changed

+40
-10
lines changed

4 files changed

+40
-10
lines changed

‎ruby/ql/lib/change-notes/2023-10-30-update-all.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+
* Improved modeling for `ActiveRecord`s `update_all` method

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

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ private predicate sqlFragmentArgumentInner(DataFlow::CallNode call, DataFlow::No
173173
"delete_all", "delete_by", "destroy_all", "destroy_by", "exists?", "find_by", "find_by!",
174174
"find_or_create_by", "find_or_create_by!", "find_or_initialize_by", "find_by_sql", "from",
175175
"group", "having", "joins", "lock", "not", "order", "reorder", "pluck", "where", "rewhere",
176-
"select", "reselect", "update_all"
176+
"select", "reselect"
177177
]) and
178178
sink = call.getArgument(0)
179179
or
@@ -198,6 +198,20 @@ private predicate sqlFragmentArgumentInner(DataFlow::CallNode call, DataFlow::No
198198
or
199199
call = activeRecordConnectionInstance().getAMethodCall("execute") and
200200
sink = call.getArgument(0)
201+
or
202+
call = activeRecordQueryBuilderCall("update_all") and
203+
(
204+
// `update_all([sink, var1, var2, var3])`
205+
sink = call.getArgument(0).getALocalSource().(DataFlow::ArrayLiteralNode).getElement(0)
206+
or
207+
// or arg0 is not of a known "safe" type
208+
sink = call.getArgument(0) and
209+
not (
210+
sink.getALocalSource() = any(DataFlow::ArrayLiteralNode arr) or
211+
sink.getALocalSource() = any(DataFlow::HashLiteralNode hash) or
212+
sink.getALocalSource() = any(DataFlow::PairNode pair)
213+
)
214+
)
201215
}
202216

203217
private predicate sqlFragmentArgument(DataFlow::CallNode call, DataFlow::Node sink) {

‎ruby/ql/test/query-tests/security/cwe-089/ActiveRecordInjection.rb

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -91,17 +91,17 @@ def some_request_handler
9191
# where `params[:fields]` is unsanitized
9292
User.update_all(params[:fields])
9393

94+
# GOOD -- `update_all` sanitizes its bind variable arguments
95+
User.find_by(name: params[:user_name])
96+
.update_all(['name = ?', params[:new_user_name]])
9497

98+
# BAD -- `update_all` does not sanitize its query (array arg)
99+
User.find_by(name: params[:user_name])
100+
.update_all(["name = '#{params[:new_user_name]}'"])
95101

96-
97-
98-
99-
100-
101-
102-
103-
104-
102+
# BAD -- `update_all` does not sanitize its query (string arg)
103+
User.find_by(name: params[:user_name])
104+
.update_all("name = '#{params[:new_user_name]}'")
105105

106106
User.reorder(params[:direction])
107107

‎ruby/ql/test/query-tests/security/cwe-089/SqlInjection.expected

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,10 @@ edges
2929
| ActiveRecordInjection.rb:84:19:84:24 | call to params | ActiveRecordInjection.rb:84:19:84:33 | ...[...] |
3030
| ActiveRecordInjection.rb:88:18:88:23 | call to params | ActiveRecordInjection.rb:88:18:88:35 | ...[...] |
3131
| ActiveRecordInjection.rb:92:21:92:26 | call to params | ActiveRecordInjection.rb:92:21:92:35 | ...[...] |
32+
| ActiveRecordInjection.rb:100:31:100:36 | call to params | ActiveRecordInjection.rb:100:31:100:52 | ...[...] |
33+
| ActiveRecordInjection.rb:100:31:100:52 | ...[...] | ActiveRecordInjection.rb:100:20:100:55 | "name = '#{...}'" |
34+
| ActiveRecordInjection.rb:104:30:104:35 | call to params | ActiveRecordInjection.rb:104:30:104:51 | ...[...] |
35+
| ActiveRecordInjection.rb:104:30:104:51 | ...[...] | ActiveRecordInjection.rb:104:19:104:54 | "name = '#{...}'" |
3236
| ActiveRecordInjection.rb:106:18:106:23 | call to params | ActiveRecordInjection.rb:106:18:106:35 | ...[...] |
3337
| ActiveRecordInjection.rb:108:23:108:28 | call to params | ActiveRecordInjection.rb:108:23:108:47 | ...[...] |
3438
| ActiveRecordInjection.rb:114:5:114:6 | ps | ActiveRecordInjection.rb:115:11:115:12 | ps |
@@ -121,6 +125,12 @@ nodes
121125
| ActiveRecordInjection.rb:88:18:88:35 | ...[...] | semmle.label | ...[...] |
122126
| ActiveRecordInjection.rb:92:21:92:26 | call to params | semmle.label | call to params |
123127
| ActiveRecordInjection.rb:92:21:92:35 | ...[...] | semmle.label | ...[...] |
128+
| ActiveRecordInjection.rb:100:20:100:55 | "name = '#{...}'" | semmle.label | "name = '#{...}'" |
129+
| ActiveRecordInjection.rb:100:31:100:36 | call to params | semmle.label | call to params |
130+
| ActiveRecordInjection.rb:100:31:100:52 | ...[...] | semmle.label | ...[...] |
131+
| ActiveRecordInjection.rb:104:19:104:54 | "name = '#{...}'" | semmle.label | "name = '#{...}'" |
132+
| ActiveRecordInjection.rb:104:30:104:35 | call to params | semmle.label | call to params |
133+
| ActiveRecordInjection.rb:104:30:104:51 | ...[...] | semmle.label | ...[...] |
124134
| ActiveRecordInjection.rb:106:18:106:23 | call to params | semmle.label | call to params |
125135
| ActiveRecordInjection.rb:106:18:106:35 | ...[...] | semmle.label | ...[...] |
126136
| ActiveRecordInjection.rb:108:23:108:28 | call to params | semmle.label | call to params |
@@ -191,6 +201,8 @@ subpaths
191201
| ActiveRecordInjection.rb:84:19:84:33 | ...[...] | ActiveRecordInjection.rb:84:19:84:24 | call to params | ActiveRecordInjection.rb:84:19:84:33 | ...[...] | This SQL query depends on a $@. | ActiveRecordInjection.rb:84:19:84:24 | call to params | user-provided value |
192202
| ActiveRecordInjection.rb:88:18:88:35 | ...[...] | ActiveRecordInjection.rb:88:18:88:23 | call to params | ActiveRecordInjection.rb:88:18:88:35 | ...[...] | This SQL query depends on a $@. | ActiveRecordInjection.rb:88:18:88:23 | call to params | user-provided value |
193203
| ActiveRecordInjection.rb:92:21:92:35 | ...[...] | ActiveRecordInjection.rb:92:21:92:26 | call to params | ActiveRecordInjection.rb:92:21:92:35 | ...[...] | This SQL query depends on a $@. | ActiveRecordInjection.rb:92:21:92:26 | call to params | user-provided value |
204+
| ActiveRecordInjection.rb:100:20:100:55 | "name = '#{...}'" | ActiveRecordInjection.rb:100:31:100:36 | call to params | ActiveRecordInjection.rb:100:20:100:55 | "name = '#{...}'" | This SQL query depends on a $@. | ActiveRecordInjection.rb:100:31:100:36 | call to params | user-provided value |
205+
| ActiveRecordInjection.rb:104:19:104:54 | "name = '#{...}'" | ActiveRecordInjection.rb:104:30:104:35 | call to params | ActiveRecordInjection.rb:104:19:104:54 | "name = '#{...}'" | This SQL query depends on a $@. | ActiveRecordInjection.rb:104:30:104:35 | call to params | user-provided value |
194206
| ActiveRecordInjection.rb:106:18:106:35 | ...[...] | ActiveRecordInjection.rb:106:18:106:23 | call to params | ActiveRecordInjection.rb:106:18:106:35 | ...[...] | This SQL query depends on a $@. | ActiveRecordInjection.rb:106:18:106:23 | call to params | user-provided value |
195207
| ActiveRecordInjection.rb:108:23:108:47 | ...[...] | ActiveRecordInjection.rb:108:23:108:28 | call to params | ActiveRecordInjection.rb:108:23:108:47 | ...[...] | This SQL query depends on a $@. | ActiveRecordInjection.rb:108:23:108:28 | call to params | user-provided value |
196208
| ActiveRecordInjection.rb:120:20:120:32 | ... + ... | ActiveRecordInjection.rb:114:10:114:15 | call to params | ActiveRecordInjection.rb:120:20:120:32 | ... + ... | This SQL query depends on a $@. | ActiveRecordInjection.rb:114:10:114:15 | call to params | user-provided value |

0 commit comments

Comments
 (0)