Merge pull request #1162 from presidentbeef/avoid_warning_about_shell…

…escape

Avoid command injection warning when using Shellwords.escape

lib/brakeman/checks/check_execute.rb

@@ -17,6 +17,10 @@ class Brakeman::CheckExecute < Brakeman::BaseCheck
                   s(:call, s(:const, :Rails), :root),
                   s(:call, s(:const, :Rails), :env)]
 
+  SHELL_ESCAPES = [:escape, :shellescape, :join]
+
+  SHELLWORDS = s(:const, :Shellwords)
+
   #Check models, controllers, and views for command injection.
   def run_check
     Brakeman.debug "Finding system calls using ``"
@@ -127,15 +131,17 @@ def process_backticks result
       :confidence => confidence
   end
 
+  # This method expects a :dstr or :evstr node
   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
 
+      next if node_type? e, :lit, :str
+      next if SAFE_VALUES.include? e
+      next if shell_escape? e
+
       if node_type? e, :or, :evstr, :dstr
         if res = dangerous?(e)
           return res
@@ -161,4 +167,16 @@ def dangerous_interp? exp
 
     false
   end
+
+  def shell_escape? exp
+    return false unless call? exp
+
+    if exp.target == SHELLWORDS and SHELL_ESCAPES.include? exp.method
+      true
+    elsif exp.method == :shelljoin
+      true
+    else
+      false
+    end
+  end
 end

test/apps/rails5.2/lib/shell.rb

@@ -0,0 +1,11 @@
+class ShellStuff
+  def initialize(one, two)
+    @one = Shellwords.shellescape(one)
+    @two = Shellwords.escape(two)
+  end
+
+  def run(ip)
+    ip = Shellwords.shellescape(ip)
+    `dig +short -x #{ip} @#{@one} -p #{@two}`
+  end
+end

test/tests/rails52.rb

@@ -52,4 +52,17 @@ def test_command_injection_1
       :code => s(:dxstr, "", s(:evstr, s(:ivar, :@blah))),
       :user_input => s(:ivar, :@blah)
   end
+
+  def test_command_injection_shellwords
+    assert_no_warning :type => :warning,
+      :warning_code => 14,
+      :fingerprint => "89b886281c6329f5c5f319932d98ea96527d50f1d188fde9fd85ff93130b7c50",
+      :warning_type => "Command Injection",
+      :line => 9,
+      :message => /^Possible\ command\ injection/,
+      :confidence => 1,
+      :relative_path => "lib/shell.rb",
+      :code => s(:dxstr, "dig +short -x ", s(:evstr, s(:call, s(:const, :Shellwords), :shellescape, s(:lvar, :ip))), s(:str, " @"), s(:evstr, s(:call, s(:const, :Shellwords), :shellescape, s(:lvar, :one))), s(:str, " -p "), s(:evstr, s(:call, s(:const, :Shellwords), :escape, s(:lvar, :two)))),
+      :user_input => s(:call, s(:const, :Shellwords), :shellescape, s(:lvar, :ip))
+  end
 end