Ruby: ActiveRecord - refine conditions argument as an SQLi sink

[alexrford]

See https://guides.rubyonrails.org/v2.3/active_record_querying.html#conditions

We previously assumed a string argument - but array arguments are only vulnerable if the first element is tainted.

[joefarebrother]

Would this find cases like this?

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

[alexrford]

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.

[joefarebrother]

Looks good 👍