diff --git a/lib/brakeman/checks/check_sql.rb b/lib/brakeman/checks/check_sql.rb index 5a2312e3f1da70552907f76fba44e25c564a3a0d..1a798a2949ec380879f1770435110d359f8efa9a 100644 --- a/lib/brakeman/checks/check_sql.rb +++ b/lib/brakeman/checks/check_sql.rb @@ -356,12 +356,38 @@ class Brakeman::CheckSQL < Brakeman::BaseCheck #unless safe_value? explicitly returns true. def check_string_interp arg arg.each do |exp| - return exp.value if node_type?(exp, :string_eval, :evstr) and not safe_value?(exp.value) + if dangerous = unsafe_string_interp?(exp) + return dangerous + end end nil end + #Returns value if interpolated value is not something safe + def unsafe_string_interp? exp + if node_type? exp, :string_eval, :evstr + value = exp.value + else + value = exp + end + + return unless sexp? value + + case value.node_type + when :or + return unsafe_string_interp?(value.lhs) || unsafe_string_interp?(value.rhs) + when :string_interp, :dstr + if dangerous = check_string_interp(value) + return dangerous + end + else + if not safe_value? value + return value + end + end + end + #Checks the given expression for unsafe SQL values. If an unsafe value is #found, returns that value (may be the given _exp_ or a subexpression). # diff --git a/test/apps/rails3.1/app/models/user.rb b/test/apps/rails3.1/app/models/user.rb index 301a250d5e0888eec9156a70dadf3bcc94f17dce..bc3445d1fd0d370815d26b60889e3bda7c77bc9b 100644 --- a/test/apps/rails3.1/app/models/user.rb +++ b/test/apps/rails3.1/app/models/user.rb @@ -27,4 +27,9 @@ class User < ActiveRecord::Base belongs_to :account attr_accessible :admin, :as => :admin + + def self.sql_stuff + condition = parent_id.blank? ? " IS NULL" : " = #{parent_id}" + self.connection.select_values("SELECT max(id) FROM content_pages WHERE parent_content_page_id #{condition}")[0].to_i + end end diff --git a/test/tests/rails31.rb b/test/tests/rails31.rb index 14feed69a3e0127f36ebe397c46c9e9243306f3b..8af7ad8ecc9f70bc2bbf92049d88f84432e3e92b 100644 --- a/test/tests/rails31.rb +++ b/test/tests/rails31.rb @@ -15,7 +15,7 @@ class Rails31Tests < Test::Unit::TestCase :model => 3, :template => 23, :controller => 4, - :generic => 77 } + :generic => 78 } end def test_without_protection @@ -954,6 +954,19 @@ class Rails31Tests < Test::Unit::TestCase :file => /users_controller\.rb/ end + + def test_sql_injection_with_interpolated_value + assert_warning :type => :warning, + :warning_code => 0, + :fingerprint => "37dc35cac607340b6244a25f7001fb6a67bf830b8c7395e4034f94c60f3d310e", + :warning_type => "SQL Injection", + :line => 33, + :message => /^Possible\ SQL\ injection/, + :confidence => 1, + :relative_path => "app/models/user.rb", + :user_input => s(:call, nil, :parent_id) + end + def test_validates_format assert_warning :type => :model, :warning_type => "Format Validation",