Skip to content

Commit 8f36b0d

Browse files
committed
Simplify guard in SQL injection tests
We don't (yet) properly sanitize taint in cases like this foo = "A" unless foo == "B" So for now, use a simpler guard in the SQL injection test. We can resurrect the old, more idiomatic guard when we can support it.
1 parent 5698356 commit 8f36b0d

File tree

2 files changed

+14
-7
lines changed

2 files changed

+14
-7
lines changed

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

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,8 +71,15 @@ def some_other_request_handler
7171
def safe_paths
7272
dir = params[:order]
7373
# GOOD: barrier guard prevents taint flow
74-
dir = "DESC" unless dir == "ASC"
75-
User.order("name #{dir}")
74+
if dir == "ASC"
75+
User.order("name #{dir}")
76+
else
77+
dir = "DESC"
78+
User.order("name #{dir}")
79+
end
80+
# TODO: a more idiomatic form of this guard is the following:
81+
# dir = "DESC" unless dir == "ASC"
82+
# but our taint tracking can't (yet) handle that properly
7683

7784
name = params[:user_name]
7885
# GOOD: barrier guard prevents taint flow

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,8 @@ edges
1212
| ActiveRecordInjection.rb:56:38:56:43 | call to params : | ActiveRecordInjection.rb:56:38:56:50 | ...[...] : |
1313
| ActiveRecordInjection.rb:56:38:56:50 | ...[...] : | ActiveRecordInjection.rb:8:31:8:34 | pass : |
1414
| ActiveRecordInjection.rb:62:10:62:15 | call to params : | ActiveRecordInjection.rb:68:21:68:33 | ... + ... |
15-
| ActiveRecordInjection.rb:94:22:94:27 | call to params : | ActiveRecordInjection.rb:94:22:94:45 | ...[...] : |
16-
| ActiveRecordInjection.rb:94:22:94:45 | ...[...] : | ActiveRecordInjection.rb:20:23:20:31 | condition : |
15+
| ActiveRecordInjection.rb:101:22:101:27 | call to params : | ActiveRecordInjection.rb:101:22:101:45 | ...[...] : |
16+
| ActiveRecordInjection.rb:101:22:101:45 | ...[...] : | ActiveRecordInjection.rb:20:23:20:31 | condition : |
1717
nodes
1818
| ActiveRecordInjection.rb:8:25:8:28 | name : | semmle.label | name : |
1919
| ActiveRecordInjection.rb:8:31:8:34 | pass : | semmle.label | pass : |
@@ -36,12 +36,12 @@ nodes
3636
| ActiveRecordInjection.rb:56:38:56:50 | ...[...] : | semmle.label | ...[...] : |
3737
| ActiveRecordInjection.rb:62:10:62:15 | call to params : | semmle.label | call to params : |
3838
| ActiveRecordInjection.rb:68:21:68:33 | ... + ... | semmle.label | ... + ... |
39-
| ActiveRecordInjection.rb:94:22:94:27 | call to params : | semmle.label | call to params : |
40-
| ActiveRecordInjection.rb:94:22:94:45 | ...[...] : | semmle.label | ...[...] : |
39+
| ActiveRecordInjection.rb:101:22:101:27 | call to params : | semmle.label | call to params : |
40+
| ActiveRecordInjection.rb:101:22:101:45 | ...[...] : | semmle.label | ...[...] : |
4141
#select
4242
| ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | ActiveRecordInjection.rb:56:23:56:28 | call to params : | ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:56:23:56:28 | call to params | a user-provided value |
4343
| ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | ActiveRecordInjection.rb:56:38:56:43 | call to params : | ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:56:38:56:43 | call to params | a user-provided value |
44-
| ActiveRecordInjection.rb:23:17:23:25 | condition | ActiveRecordInjection.rb:94:22:94:27 | call to params : | ActiveRecordInjection.rb:23:17:23:25 | condition | This SQL query depends on $@. | ActiveRecordInjection.rb:94:22:94:27 | call to params | a user-provided value |
44+
| ActiveRecordInjection.rb:23:17:23:25 | condition | ActiveRecordInjection.rb:101:22:101:27 | call to params : | ActiveRecordInjection.rb:23:17:23:25 | condition | This SQL query depends on $@. | ActiveRecordInjection.rb:101:22:101:27 | call to params | a user-provided value |
4545
| ActiveRecordInjection.rb:35:30:35:44 | ...[...] | ActiveRecordInjection.rb:35:30:35:35 | call to params : | ActiveRecordInjection.rb:35:30:35:44 | ...[...] | This SQL query depends on $@. | ActiveRecordInjection.rb:35:30:35:35 | call to params | a user-provided value |
4646
| ActiveRecordInjection.rb:39:21:39:43 | "id = '#{...}'" | ActiveRecordInjection.rb:39:30:39:35 | call to params : | ActiveRecordInjection.rb:39:21:39:43 | "id = '#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:39:30:39:35 | call to params | a user-provided value |
4747
| ActiveRecordInjection.rb:43:23:43:45 | "id = '#{...}'" | ActiveRecordInjection.rb:43:32:43:37 | call to params : | ActiveRecordInjection.rb:43:23:43:45 | "id = '#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:43:32:43:37 | call to params | a user-provided value |

0 commit comments

Comments
 (0)