Check for unsafe SQL when two arguments are passed to AR methods

[patbl]

I think this conditional is meant to

1. Always check the first and last arguments.
2. Avoid checking the argument twice when there's only one argument.

The structure of call appears to be:

• 0: the symbol :call
• 1: the receiver of the method call
• 2: the method being called
• 3: the first argument
• 4: the second argument
• 5: the third argument (and so on)

So call.length > 5 will be true only if there are at least 3 arguments.

Before this change, the behavior was

User.count("#{params[:input]}") # triggers warning
User.count("#{params[:input]}", :foo) # doesn't trigger warning
User.count("#{params[:input]}", :foo, :bar) # triggers warning

After this change, the two-argument version will trigger the warning.

Another option would be to use if call.arglist.length > 1. This is more explicit, but it involves an additional method call, and I don't know what the performance implications of that are.

[dryrunsecurity]

This pull request involves a minor modification to Brakeman's SQL injection detection logic, adjusting the method argument length condition to potentially capture a broader range of method call scenarios for improved vulnerability scanning.

💭 Unconfirmed Findings (1)

Vulnerability	Potential Marginal SQL Injection Detection Impact
Description	A patch in Brakeman's SQL injection check modifies method argument length condition from call.length > 5 to call.length >= 5, potentially expanding the scope of SQL injection detection by capturing different method call scenarios. Located in lib/brakeman/checks/check_sql.rb, this change represents a minor logic adjustment that could slightly alter injection detection capabilities.

---

All finding details can be found in the DryRun Security Dashboard.

[presidentbeef]

I'm going to have to figure out what the original intent was here... it's been a while since those methods even accepted more than one argument.

(But I think you are right)

[presidentbeef]

@patbl Can you update to use the more explicit call.arglist.length > 1?

[patbl]

@presidentbeef I made that change.

[dryrunsecurity]

No security concerns detected in this pull request.

---

All finding details can be found in the DryRun Security Dashboard.

[soulcutter]

This change seems to have triggered a LOT of warnings across many of our applications that use a statsd-like interface for calling $stats.count("metric.name.#{custom_thing}", 1) - something we use WAY more than ActiveRecord count-with-2-args.

https://www.rubydoc.info/gems/dogstatsd-ruby/3.0.0/Datadog/Statsd#count-instance_method

Is there a way this check might be better-targeted to avoid this deluge of false positives?

[presidentbeef]

@soulcutter can you provide an example of the exact warning from Brakeman? Does it say $stats or does it know what $status is?

[soulcutter]

Confidence: Weak
Category: SQL Injection
Check: SQL
Message: Possible SQL injection
Code: $stats.count("prefix.#{variable}", :tags => ({ :result => "error_raised" }))
File: app/models/<model>.rb
Line: 61

The $stats global is initialized in dependency, not defined anywhere in the application code (it's not defined in app/models/<model>.rb for example). I do not know how to answer the question of if it knows what $stats is

[presidentbeef]

Maybe Brakeman can just ignore calls that look like ActiveRecord methods but are on globals 🤔

[patbl]

Maybe Brakeman can just ignore calls that look like ActiveRecord methods but are on globals 🤔

A global variable such as $statsd is one way of referencing this sort of object, but applications could instead use a module, class, method, constant, or local variable. (StatsD, STATSD, statsd...).

It would be nice if there were a way to tell Brakeman "this [$statsd, STATSD, etc.] isn't an Active Record class." But maybe that's too much scope creep (I'm assuming something like this doesn't already exist).

[presidentbeef]

Writing down for myself... for common methods like count this check can go back to only warning if the target is known to be an ActiveRecord model.