lib/brakeman/checks/check_execute.rb in brakeman-min-2.1.2 vs lib/brakeman/checks/check_execute.rb in brakeman-min-2.2.0

@@ -11,10 +11,14 @@
 class Brakeman::CheckExecute < Brakeman::BaseCheck
   Brakeman::Checks.add self
 
   @description = "Finds instances of possible command injection"
 
+  SAFE_VALUES = [s(:const, :RAILS_ROOT),
+                  s(:call, s(:const, :Rails), :root),
+                  s(:call, s(:const, :Rails), :env)]
+
   #Check models, controllers, and views for command injection.
   def run_check
     Brakeman.debug "Finding system calls using ``"
     check_for_backticks tracker
 
@@ -36,13 +40,13 @@
     args = call.arglist
     first_arg = call.first_arg
 
     case call.method
     when :system, :exec
-      failure = include_user_input?(first_arg) || include_interp?(first_arg)
+      failure = include_user_input?(first_arg) || dangerous_interp?(first_arg)
     else
-      failure = include_user_input?(args) || include_interp?(args)
+      failure = include_user_input?(args) || dangerous_interp?(args)
     end
 
     if failure and not duplicate? result
       add_result result
 
@@ -80,19 +84,56 @@
     exp = result[:call]
 
     if input = include_user_input?(exp)
       confidence = CONFIDENCE[:high]
       user_input = input.match
-    else
+    elsif input = dangerous?(exp)
       confidence = CONFIDENCE[:med]
-      user_input = nil
+      user_input = input
+    else
+      return
     end
 
     warn :result => result,
       :warning_type => "Command Injection",
       :warning_code => :command_injection,
       :message => "Possible command injection",
       :code => exp,
       :user_input => user_input,
       :confidence => confidence
+  end
+
+  def dangerous? exp
+    exp.each_sexp do |e|
+      next if node_type? e, :lit, :str
+      next if SAFE_VALUES.include? e
+
+      if call? e and e.method == :to_s
+        e = e.target
+      end
+
+      if node_type? e, :or, :evstr, :string_eval, :string_interp
+        if res = dangerous?(e)
+          return res
+        end
+      else
+        return e
+      end
+    end
+
+    false
+  end
+
+  def dangerous_interp? exp
+    match = include_interp? exp
+    return unless match
+    interp = match.match
+
+    interp.each_sexp do |e|
+      if res = dangerous?(e)
+        return Match.new(:interp, res)
+      end
+    end
+
+    false
   end
 end