Skip to content
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.

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

Ignore calls with number targets in SQL #1586

Merged
merged 1 commit into from May 3, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 14 additions & 1 deletion lib/brakeman/checks/check_sql.rb
Expand Up @@ -592,7 +592,8 @@ def safe_value? exp
IGNORE_METHODS_IN_SQL.include? exp.method or
quote_call? exp or
arel? exp or
exp.method.to_s.end_with? "_id"
exp.method.to_s.end_with? "_id" or
number_target? exp
end
when :if
safe_value? exp.then_clause and safe_value? exp.else_clause
Expand Down Expand Up @@ -695,4 +696,16 @@ def connect_call? result
active_record_models.include? klass
end
end

def number_target? exp
return unless call? exp

if number? exp.target
true
elsif call? exp.target
number_target? exp.target
else
false
end
end
end
5 changes: 5 additions & 0 deletions test/apps/rails6/app/models/group.rb
Expand Up @@ -2,4 +2,9 @@ class Group < ApplicationRecord
def uuid_in_sql
ActiveRecord::Base.connection.exec_query("select * where x = #{User.uuid}")
end

def date_in_sql
date = 30.days.ago
Arel.sql("created_at > '#{date}'")
end
end
13 changes: 13 additions & 0 deletions test/tests/rails6.rb
Expand Up @@ -121,6 +121,19 @@ def test_sql_injection_uuid_false_positive
:user_input => s(:call, s(:const, :User), :uuid)
end

def test_sql_injection_date_integer_target_false_positive
assert_no_warning :type => :warning,
:warning_code => 0,
:fingerprint => "5ec829ba8790c01a39faf6788f0754d39879a6e68a9de8804c6f25ac9c2f1ee6",
:warning_type => "SQL Injection",
:line => 8,
:message => /^Possible\ SQL\ injection/,
:confidence => 1,
:relative_path => "app/models/group.rb",
:code => s(:call, s(:const, :Arel), :sql, s(:dstr, "created_at > '", s(:evstr, s(:call, s(:call, s(:lit, 30), :days), :ago)), s(:str, "'"))),
:user_input => s(:call, s(:call, s(:lit, 30), :days), :ago)
end

def test_cross_site_scripting_sanity
assert_warning :type => :template,
:warning_code => 2,
Expand Down