From 64cfeaa00c8c75d2731e880aa3ad40cd94b8a2be Mon Sep 17 00:00:00 2001
From: Justin Collins <justin@presidentbeef.com>
Date: Mon, 3 Feb 2014 00:33:54 -0800
Subject: [PATCH] Ignore sanitized SQL in string building

---
 lib/brakeman/checks/check_sql.rb    | 36 +++++++++++++++++++++++++----
 test/apps/rails2/app/models/user.rb |  8 +++++++
 test/tests/rails2.rb                | 24 +++++++++++++++++++
 3 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/lib/brakeman/checks/check_sql.rb b/lib/brakeman/checks/check_sql.rb
index 4ceabaa8..5a2312e3 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 bb4c3c7d..855fcdae 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 ee7888e1..e7c0a194 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",
-- 
GitLab