diff --git a/lib/brakeman/processors/alias_processor.rb b/lib/brakeman/processors/alias_processor.rb index dd19186348..8d06c07f60 100644 --- a/lib/brakeman/processors/alias_processor.rb +++ b/lib/brakeman/processors/alias_processor.rb @@ -290,11 +290,78 @@ def process_call exp if string? target exp = process exp.target end + when :join + if array? target and target.length > 2 and (string? first_arg or first_arg.nil?) + begin + exp = process_array_join(target, first_arg) + rescue => e + p e + end + end end exp end + # Painful conversion of Array#join into string interpolation + def process_array_join array, join_str + result = s() + + join_value = if string? join_str + join_str.value + else + nil + end + + array[1..-2].each do |e| + result << join_item(e, join_value) + end + + result << join_item(array.last, nil) + + # Combine the strings at the beginning because that's what RubyParser does + combined_first = "" + result.each do |e| + if string? e + combined_first << e.value + elsif e.is_a? String + combined_first << e + else + break + end + end + + # Remove the strings at the beginning + result.reject! do |e| + if e.is_a? String or string? e + true + else + break + end + end + + result.unshift combined_first + + # Have to fix up strings that follow interpolation + result.reduce(s(:dstr)) do |memo, e| + if string? e and node_type? memo.last, :evstr + e.value = "#{join_value}#{e.value}" + end + + memo << e + end + end + + def join_item item, join_value + if item.is_a? String + "#{item}#{join_value}" + elsif string? item or symbol? item or number? item + s(:str, "#{item.value}#{join_value}") + else + s(:evstr, item) + end + end + def process_iter exp @exp_context.push exp exp[1] = process exp.block_call diff --git a/test/apps/rails5.2/lib/shell.rb b/test/apps/rails5.2/lib/shell.rb index eb542bbd53..918ba64440 100644 --- a/test/apps/rails5.2/lib/shell.rb +++ b/test/apps/rails5.2/lib/shell.rb @@ -22,4 +22,9 @@ def nested_system_interp filename = Shellwords.escape("#{file_prefix}.txt") system "echo #{filename}" end + + def system_array_join + command = ["ruby", method_that_returns_user_input, "--some-flag"].join(" ") + system(command) + end end diff --git a/test/tests/alias_processor.rb b/test/tests/alias_processor.rb index e8b00ccbd5..1b189cd36a 100644 --- a/test/tests/alias_processor.rb +++ b/test/tests/alias_processor.rb @@ -1051,4 +1051,18 @@ def test_array_destructuring_asgn y INPUT end + + def test_array_join_to_interpolation + assert_alias '"blah 1 #{thing} else"', <<-'INPUT' + x = ["blah", 1, thing, "else"].join(' ') + x + INPUT + end + + def test_array_join_no_separater + assert_alias '"blah1#{thing}else"', <<-'INPUT' + x = ["blah", 1, thing, "else"].join + x + INPUT + end end diff --git a/test/tests/rails52.rb b/test/tests/rails52.rb index 54a602202e..0f763df2aa 100644 --- a/test/tests/rails52.rb +++ b/test/tests/rails52.rb @@ -13,7 +13,7 @@ def expected :controller => 0, :model => 0, :template => 0, - :generic => 5 + :generic => 6 } end @@ -118,6 +118,19 @@ def test_command_injection_as_target :user_input => s(:lvar, :path) end + def test_command_injection_array_join + assert_warning :type => :warning, + :warning_code => 14, + :fingerprint => "478a39b6379df61bf0b016f435d054f279353e4fcd048304105152f6203fbdaa", + :warning_type => "Command Injection", + :line => 28, + :message => /^Possible\ command\ injection/, + :confidence => 1, + :relative_path => "lib/shell.rb", + :code => s(:call, nil, :system, s(:dstr, "ruby ", s(:evstr, s(:call, nil, :method_that_returns_user_input)), s(:str, " --some-flag"))), + :user_input => s(:call, nil, :method_that_returns_user_input) + end + def test_cross_site_scripting_loofah_CVE_2018_8048 assert_warning :type => :warning, :warning_code => 106,