Merge pull request #1868 from presidentbeef/revamp_pipeline_check

Revamp command injection in `pipeline*` calls

lib/brakeman/checks/check_execute.rb

@@ -53,6 +53,7 @@ def process_result result
     call = result[:call]
     args = call.arglist
     first_arg = call.first_arg
+    failure = nil
 
     case call.method
     when :popen
@@ -71,6 +72,33 @@ def process_result result
                   dangerous_interp?(first_arg[3]) ||
                   dangerous_string_building?(first_arg[3])
       end
+    when :pipeline, :pipline_r, :pipeline_rw, :pipeline_w, :pipeline_start
+      # Since these pipeline commands pipe together several commands,
+      # need to check each argument. If it's an array, check first argument
+      # (the command) and also check for `bash -c`. Otherwise check the argument
+      # as a unit.
+
+      args.each do |arg|
+        next unless sexp? arg
+
+        if array?(arg)
+          # Check first element of array
+          failure = include_user_input?(arg[1]) ||
+            dangerous_interp?(arg[1]) ||
+            dangerous_string_building?(arg[1])
+
+          # Check for ['bash', '-c', user_input]
+          if dash_c_shell_command?(arg[1], arg[2])
+            failure = include_user_input?(arg[3]) ||
+              dangerous_interp?(arg[3]) ||
+              dangerous_string_building?(arg[3])
+          end
+        else
+          failure = include_user_input?(arg)
+        end
+
+        break if failure
+      end
     when :system, :exec
       # Normally, if we're in a `system` or `exec` call, we only are worried
       # about shell injection when there's a single argument, because comma-

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

@@ -146,7 +146,7 @@ def test_more_ways_to_execute
     Open3.capture2 "ls #{params[:dir]}"
     Open3.capture2e "ls #{params[:dir]}"
     Open3.capture3 "ls #{params[:dir]}"
-    Open3.pipeline "sort", "uniq", :in => params[:file] 
+    Open3.pipeline "sort", "uniq", :in => params[:file]
     Open3.pipeline_r "sort #{params[:file]}", "uniq"
     Open3.pipeline_rw params[:cmd], "sort -g"
     Open3.pipeline_start *params[:cmds]
@@ -158,6 +158,13 @@ def test_only_path_also_correct
     redirect_to(params.merge(:only_path => true, :display => nil))
   end
 
+  def test_more_uses_of_pipelines
+    Open3.pipeline ['sort', params[:file]] # safe-ish
+    Open3.pipeline_r ['ls', '*'], "sort #{params[:order]}"
+    Open3.pipeline_rw ['ls'], [params[:cmd]]
+    Open3.pipeline_start ['bash', '-c', params[:cmd]]
+  end
+
   private
 
   def filter_it

test/tests/rails3.rb

@@ -14,7 +14,7 @@ def expected
       :controller => 1,
       :model => 9,
       :template => 41,
-      :generic => 76
+      :generic => 79
     }
 
     if RUBY_PLATFORM == 'java'
@@ -167,6 +167,62 @@ def test_command_injection_pipeline_start
       :relative_path => "app/controllers/home_controller.rb"
   end
 
+  def test_command_injection_pipeline_safe_ish
+    assert_no_warning check_name: "Execute",
+      type: :warning,
+      warning_code: 14,
+      fingerprint: "69631817be6f91b3e9115935a0b5e23b6cd642bdb44ac5edb83ce9bf5c207528",
+      warning_type: "Command Injection",
+      line: 162,
+      message: /^Possible\ command\ injection/,
+      confidence: 0,
+      relative_path: "app/controllers/home_controller.rb",
+      code: s(:call, s(:const, :Open3), :pipeline, s(:array, s(:str, "sort"), s(:call, s(:params), :[], s(:lit, :file)))),
+      user_input: s(:call, s(:params), :[], s(:lit, :file))
+  end
+
+  def test_command_injection_pipeline_array_cmd
+    assert_warning check_name: "Execute",
+      type: :warning,
+      warning_code: 14,
+      fingerprint: "209d96d55ba1cdbce58da49efaea6c3da266411c9a4e3ba914b80969b0ebc4c8",
+      warning_type: "Command Injection",
+      line: 163,
+      message: /^Possible\ command\ injection/,
+      confidence: 0,
+      relative_path: "app/controllers/home_controller.rb",
+      code: s(:call, s(:const, :Open3), :pipeline_r, s(:array, s(:str, "ls"), s(:str, "*")), s(:dstr, "sort ", s(:evstr, s(:call, s(:params), :[], s(:lit, :order))))),
+      user_input: s(:call, s(:params), :[], s(:lit, :order))
+  end
+
+  def test_command_injection_pipeline_two_array_commands
+    assert_warning check_name: "Execute",
+      type: :warning,
+      warning_code: 14,
+      fingerprint: "a64f8ffded9992faa6291a1448ea49b9121b29d00cc09d01f3608c57131f778a",
+      warning_type: "Command Injection",
+      line: 164,
+      message: /^Possible\ command\ injection/,
+      confidence: 0,
+      relative_path: "app/controllers/home_controller.rb",
+      code: s(:call, s(:const, :Open3), :pipeline_rw, s(:array, s(:str, "ls")), s(:array, s(:call, s(:params), :[], s(:lit, :cmd)))),
+      user_input: s(:call, s(:params), :[], s(:lit, :cmd))
+  end
+
+  def test_command_injection_pipeline_bash_c
+    assert_warning check_name: "Execute",
+      type: :warning,
+      warning_code: 14,
+      fingerprint: "386d96b25b2ca16ff668be680ddf4669fe4f37e12f60506f67971d99ba2f4250",
+      warning_type: "Command Injection",
+      line: 165,
+      message: /^Possible\ command\ injection/,
+      confidence: 0,
+      relative_path: "app/controllers/home_controller.rb",
+      code: s(:call, s(:const, :Open3), :pipeline_start, s(:array, s(:str, "bash"), s(:str, "-c"), s(:call, s(:params), :[], s(:lit, :cmd)))),
+      user_input: s(:call, s(:params), :[], s(:lit, :cmd))
+  end
+
   def test_command_injection_spawn
     assert_warning :type => :warning,
       :warning_code => 14,