Skip to content

Commit

Permalink
Interpolation in %W is not regular interpolation
Browse files Browse the repository at this point in the history
Fixes #1399
  • Loading branch information
presidentbeef committed Oct 15, 2019
1 parent 5633686 commit 853e03b
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 1 deletion.
24 changes: 23 additions & 1 deletion lib/brakeman/checks/base_check.rb
Expand Up @@ -39,6 +39,7 @@ def initialize(tracker)
@active_record_models = nil
@mass_assign_disabled = nil
@has_user_input = nil
@in_array = false
@safe_input_attributes = Set[:to_i, :to_f, :arel_table, :id]
@comparison_ops = Set[:==, :!=, :>, :<, :>=, :<=]
end
Expand Down Expand Up @@ -109,9 +110,16 @@ def process_cookies exp
exp
end

def process_array exp
@in_array = true
process_default exp
ensure
@in_array = false
end

#Does not actually process string interpolation, but notes that it occurred.
def process_dstr exp
unless @string_interp # don't overwrite existing value
unless array_interp? exp or @string_interp # don't overwrite existing value
@string_interp = Match.new(:interp, exp)
end

Expand All @@ -120,6 +128,20 @@ def process_dstr exp

private

# Checking for
#
# %W[#{a}]
#
# which will be parsed as
#
# s(:array, s(:dstr, "", s(:evstr, s(:call, nil, :a))))
def array_interp? exp
@in_array and
string_interp? exp and
exp[1] == "".freeze and
exp.length == 3 # only one interpolated value
end

def always_safe_method? meth
@safe_input_attributes.include? meth or
@comparison_ops.include? meth
Expand Down
5 changes: 5 additions & 0 deletions test/apps/rails5.2/lib/shell.rb
Expand Up @@ -89,4 +89,9 @@ def file_constant_use
# __FILE__ should not change based on absolute path
`cp #{__FILE__} #{somewhere_else}`
end

def interpolated_in_percent_W
# Should not warn
system(*%W(foo bar #{value}))
end
end
12 changes: 12 additions & 0 deletions test/tests/rails52.rb
Expand Up @@ -370,6 +370,18 @@ def test_command_injection_with__file__
:user_input => s(:call, nil, :somewhere_else)
end

def test_command_injection_percent_W
assert_no_warning :type => :warning,
:warning_code => 14,
:warning_type => "Command Injection",
:line => 95,
:message => /^Possible\ command\ injection/,
:confidence => 1,
:relative_path => "lib/shell.rb",
:code => s(:call, nil, :system, s(:splat, s(:array, s(:str, "foo"), s(:str, "bar"), s(:dstr, "", s(:evstr, s(:call, nil, :value)))))),
:user_input => s(:call, nil, :value)
end

def test_cross_site_scripting_haml_sass
assert_warning :type => :template,
:warning_code => 2,
Expand Down

0 comments on commit 853e03b

Please sign in to comment.