From 9e8cd798144131cf572fb52be7c0b2359f1acbb6 Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Sat, 31 Aug 2024 23:12:24 -0600 Subject: [PATCH] Revamp command injection in `pipeline*` calls Fixes #1862 --- lib/brakeman/checks/check_execute.rb | 28 +++++++++ .../rails3/app/controllers/home_controller.rb | 9 ++- test/tests/rails3.rb | 58 ++++++++++++++++++- 3 files changed, 93 insertions(+), 2 deletions(-) diff --git a/lib/brakeman/checks/check_execute.rb b/lib/brakeman/checks/check_execute.rb index bd93861ee4..a2484b7e00 100644 --- a/lib/brakeman/checks/check_execute.rb +++ b/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- diff --git a/test/apps/rails3/app/controllers/home_controller.rb b/test/apps/rails3/app/controllers/home_controller.rb index 4cf6edbb07..b574cd180d 100644 --- a/test/apps/rails3/app/controllers/home_controller.rb +++ b/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 diff --git a/test/tests/rails3.rb b/test/tests/rails3.rb index b84b2415a7..d92c8021dc 100644 --- a/test/tests/rails3.rb +++ b/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,