From d34a11b22e1ed36e69368eeed034eee08d28b1b0 Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Sat, 5 May 2018 12:35:42 -0700 Subject: [PATCH 1/3] When searching for string interpolation, stop at first --- lib/brakeman/checks/base_check.rb | 5 ++++- test/apps/rails5.2/lib/shell.rb | 5 +++++ test/tests/rails52.rb | 13 +++++++++++++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/lib/brakeman/checks/base_check.rb b/lib/brakeman/checks/base_check.rb index 71e37b0dd3..ddb1100d07 100644 --- a/lib/brakeman/checks/base_check.rb +++ b/lib/brakeman/checks/base_check.rb @@ -119,7 +119,10 @@ def process_cookies exp #Does not actually process string interpolation, but notes that it occurred. def process_dstr exp - @string_interp = Match.new(:interp, exp) + unless @string_interp # don't overwrite existing value + @string_interp = Match.new(:interp, exp) + end + process_default exp end diff --git a/test/apps/rails5.2/lib/shell.rb b/test/apps/rails5.2/lib/shell.rb index 36a2facbdb..eb542bbd53 100644 --- a/test/apps/rails5.2/lib/shell.rb +++ b/test/apps/rails5.2/lib/shell.rb @@ -17,4 +17,9 @@ def process_pid # should not warn `something #{Process.pid}` end + + def nested_system_interp + filename = Shellwords.escape("#{file_prefix}.txt") + system "echo #{filename}" + end end diff --git a/test/tests/rails52.rb b/test/tests/rails52.rb index fe2cbbc43b..54a602202e 100644 --- a/test/tests/rails52.rb +++ b/test/tests/rails52.rb @@ -92,6 +92,19 @@ def test_command_injection_shellwords :user_input => s(:call, s(:const, :Shellwords), :shellescape, s(:lvar, :ip)) end + def test_command_injection_nested_shellwords + assert_no_warning :type => :warning, + :warning_code => 14, + :fingerprint => "acb19548a2a44c3070d35c62754216f4b3365f372d6004470417cca587af0f47", + :warning_type => "Command Injection", + :line => 23, + :message => /^Possible\ command\ injection/, + :confidence => 1, + :relative_path => "lib/shell.rb", + :code => s(:call, nil, :system, s(:dstr, "echo ", s(:evstr, s(:call, s(:const, :Shellwords), :escape, s(:dstr, "", s(:evstr, s(:call, nil, :file_prefix)), s(:str, ".txt")))))), + :user_input => s(:call, nil, :file_prefix) + end + def test_command_injection_as_target assert_warning :type => :warning, :warning_code => 14, From 0d6feab0ea157943d7d5f168116ec3915b5a37bf Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Sun, 6 May 2018 12:18:40 -0700 Subject: [PATCH 2/3] Convert Array#join to string interpolation --- lib/brakeman/processors/alias_processor.rb | 67 ++++++++++++++++++++++ test/tests/alias_processor.rb | 14 +++++ 2 files changed, 81 insertions(+) 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/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 From e800c61d9751e24e27fb6bccac85132b0c1c85ef Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Sun, 6 May 2018 12:23:01 -0700 Subject: [PATCH 3/3] Add test for system call with Array#join --- test/apps/rails5.2/lib/shell.rb | 5 +++++ test/tests/rails52.rb | 15 ++++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) 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/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,