Skip to content

Ruby: ActiveRecord - refine conditions argument as an SQLi sink #16185

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 22, 2024
Merged

Ruby: ActiveRecord - refine conditions argument as an SQLi sink #16185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f98479d
Ruby: prepare test case whitespace
alexrford Apr 11, 2024
2950890
Ruby: add more ActiveRecord conditions arg test cases
alexrford Apr 11, 2024
91bca4a
Ruby: limit ActiveRecord conditions sink to first array element
alexrford Apr 11, 2024
6b0e796
Ruby: prepare test case whitespace
alexrford Apr 24, 2024
98a6d0f
Ruby: add another SQLi AR conditions test case
alexrford Apr 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,14 @@ private predicate sqlFragmentArgumentInner(DataFlow::CallNode call, DataFlow::No
or
// This format was supported until Rails 2.3.8
call = activeRecordQueryBuilderCall(["all", "find", "first", "last"]) and
sink = call.getKeywordArgument("conditions")
exists(DataFlow::LocalSourceNode sn |
sn = call.getKeywordArgument("conditions").getALocalSource()
|
sink = sn.(DataFlow::ArrayLiteralNode).getElement(0)
or
sn.(DataFlow::LiteralNode).asLiteralAstNode() instanceof StringlikeLiteral and
sink = sn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this find cases like this?

conds = "name=#{name}"
find(:first, conditions: conds)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does (added a test case), although it marks the string as the sink rather than the conds argument in the find call as the sink which is not ideal.

In this particular case we could say call.getKeywordArgument("conditions") = sink instead, but there's a weird edge case for something like:

if some_condition
  conds = ["name = ?", name] # safe path
else
  conds = "name = #{name}" # dangerous path
end
find(:first, conditions: conds)

where the safe path would be flagged up because the SQLi query itself doesn't have the extra context of why the conditions argument is vulnerable.

I think it's possible for this logic to move into the query itself, but I worry that it could get quite complex. For example, there could be some sinks where a tainted array in general is vulnerable vs. cases like conditions where only arrays where the first element is tainted is vulnerable. I suspect we would have to introduce flow states to track this context, and then add information to each sink to note what kinds of values are potentially dangerous.

)
or
call = activeRecordQueryBuilderCall("reload") and
sink = call.getKeywordArgument("lock")
Expand Down
9 changes: 8 additions & 1 deletion ruby/ql/test/query-tests/security/cwe-089/ActiveRecordInjection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,13 @@ class User < ApplicationRecord
def self.authenticate(name, pass)
# BAD: possible untrusted input interpolated into SQL fragment
find(:first, :conditions => "name='#{name}' and pass='#{pass}'")
# BAD: interpolation in array argument
find(:first, conditions: ["name='#{name}' and pass='#{pass}'"])
# GOOD: using SQL parameters
find(:first, conditions: ["name = ? and pass = ?", name, pass])
# BAD: interpolation with flow
conds = "name=#{name}"
find(:first, conditions: conds)
end

def self.from(user_group_id)
Expand Down Expand Up @@ -117,7 +124,7 @@ def some_request_handler

# BAD: executes `SELECT users.* FROM #{params[:tab]}`
# where `params[:tab]` is unsanitized
User.all.from(params[:tab])
User.all.from(params[:tab])
# BAD: executes `SELECT "users".* FROM (SELECT "users".* FROM "users") #{params[:sq]}
User.all.from(User.all, params[:sq])
end
Expand Down
Loading