Merge pull request #216 from presidentbeef/fix_check_execute

Fix command injection test to warn on any interpolation

lib/brakeman/checks/base_check.rb

@@ -104,6 +104,12 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor
     exp
   end
 
+  #Does not actually process string interpolation, but notes that it occurred.
+  def process_string_interp exp
+    @string_interp = Match.new(:interp, exp)
+    process_default exp
+  end
+
   private
 
   #Report a warning
@@ -220,17 +226,6 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor
     false
   end
 
-  #Ignores ignores
-  def process_ignore exp
-    exp
-  end
-
-  #Does not actually process string interpolation, but notes that it occurred.
-  def process_string_interp exp
-    @string_interp = Match.new(:interp, exp)
-    exp
-  end
-
   #Checks if an expression contains string interpolation.
   #
   #Returns Match with :interp type if found.

test/apps/rails3/app/controllers/other_controller.rb

@@ -43,4 +43,9 @@ class OtherController < ApplicationController
   def test_mail_to
     @user = User.find(current_user)
   end
+
+  def test_command_injection_locals
+    `#{some_command}`
+    system("ls #{some_files}")
+  end
 end

test/tests/test_rails3.rb

@@ -15,7 +15,7 @@ class Rails3Tests < Test::Unit::TestCase
       :controller => 1,
       :model => 5,
       :template => 30,
-      :warning => 31
+      :warning => 33
     }
   end
 
@@ -63,6 +63,24 @@ class Rails3Tests < Test::Unit::TestCase
       :file => /home_controller\.rb/
   end
 
+  def test_command_injection_non_user_input_backticks
+    assert_warning :type => :warning,
+      :warning_type => "Command Injection",
+      :line => 48,
+      :message => /^Possible\ command\ injection/,
+      :confidence => 1,
+      :file => /other_controller\.rb/
+  end
+
+  def test_command_injection_non_user_input_system
+    assert_warning :type => :warning,
+      :warning_type => "Command Injection",
+      :line => 49,
+      :message => /^Possible\ command\ injection/,
+      :confidence => 1,
+      :file => /other_controller\.rb/
+  end
+
   def test_file_access_concatenation
     assert_warning :type => :warning,
       :warning_type => "File Access",