diff --git a/lib/brakeman/checks/check_sql.rb b/lib/brakeman/checks/check_sql.rb index 4ceabaa827474ad4fb0d60749ae457610aa49d1d..5a2312e3f1da70552907f76fba44e25c564a3a0d 100644 --- a/lib/brakeman/checks/check_sql.rb +++ b/lib/brakeman/checks/check_sql.rb @@ -458,13 +458,39 @@ class Brakeman::CheckSQL < Brakeman::BaseCheck target = exp.target method = exp.method - if string? target or string? exp.first_arg - return exp if STRING_METHODS.include? method - elsif STRING_METHODS.include? method and call? target - return unsafe_sql? target + + if STRING_METHODS.include? method + if string? target + check_string_arg exp.first_arg + elsif string? exp.first_arg + check_string_arg target + elsif call? target + check_for_string_building target + end + else + nil end + end - nil + def check_string_arg exp + if safe_value? exp + nil + elsif string_building? exp + check_for_string_building exp + elsif node_type? exp, :string_interp, :dstr + check_string_interp exp + else + exp + end + end + + def string_building? exp + return false unless call? exp and STRING_METHODS.include? exp.method + + string? exp.target or + string? exp.first_arg or + string_building? exp.target or + string_building? exp.first_arg end IGNORE_METHODS_IN_SQL = Set[:id, :merge_conditions, :table_name, :to_i, :to_f, diff --git a/test/apps/rails2/app/models/user.rb b/test/apps/rails2/app/models/user.rb index bb4c3c7d2e2b3f50b6f8d35fd8fe685abd15a3d9..855fcdae96b1484e25fb7917d2791e22e83977c6 100644 --- a/test/apps/rails2/app/models/user.rb +++ b/test/apps/rails2/app/models/user.rb @@ -30,4 +30,12 @@ class User < ActiveRecord::Base WHERE t.something = '#{value}')) end + + def self.test_sanitized_sql input + self.connection.select_all("select * from cool_table where stuff = " + self.sanitize_sql(input)) + end + + def more_sanitized_sql + self.connection.execute("DELETE FROM cool_table WHERE cool_id=" + quote_value(self.cool_id) + " AND my_id=" + quote_value(self.id)) + end end diff --git a/test/tests/rails2.rb b/test/tests/rails2.rb index ee7888e1d0c8c9a9da468da905f2626971c18463..e7c0a194d6024e10f77852ef715176ffb4fb79aa 100644 --- a/test/tests/rails2.rb +++ b/test/tests/rails2.rb @@ -308,6 +308,30 @@ class Rails2Tests < Test::Unit::TestCase :relative_path => "app/controllers/home_controller.rb" end + def test_sql_injection_false_positive_quote_value + assert_no_warning :type => :warning, + :warning_code => 0, + :fingerprint => "6ea8fe3abe8eac86e5ecb790b53fb064b1152b2574b14d9354a40d07269a952e", + :warning_type => "SQL Injection", + :line => 30, + :message => /^Possible\ SQL\ injection/, + :confidence => 1, + :relative_path => "app/models/user.rb", + :user_input => s(:call, s(:call, s(:str, "DELETE FROM cool_table WHERE cool_id="), :+, s(:call, nil, :quote_value, s(:call, s(:self), :cool_id))), :+, s(:str, " AND my_id=")) + end + + def test_sql_injection_sanitize_sql + assert_no_warning :type => :warning, + :warning_code => 0, + :fingerprint => "7481ff666ae949b8442400cf516615ce8b04b87f7e11e33e29d4ad1303d24dd0", + :warning_type => "SQL Injection", + :line => 26, + :message => /^Possible\ SQL\ injection/, + :confidence => 1, + :relative_path => "app/models/user.rb", + :user_input => s(:call, s(:str, "select * from cool_table where stuff = "), :+, s(:call, s(:self), :sanitize_sql, s(:lvar, :input))) + end + def test_csrf_protection assert_warning :type => :controller, :warning_type => "Cross-Site Request Forgery",