Deeper checking of SQL interpolation

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).
   #

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

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",