Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do dataflow on all of 'library' code #1554

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/brakeman/processor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,8 @@ def process_initializer file_name, src

#Process source for a library file
def process_lib src, file_name
LibraryProcessor.new(@tracker).process_library src, file_name
result = LibraryProcessor.new(@tracker).process_library src, file_name
AliasProcessor.new(@tracker, file_name).process result if result
end
end
end
11 changes: 11 additions & 0 deletions lib/brakeman/processors/alias_processor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ def initialize tracker = nil, current_file = nil
@helper_method_info = Hash.new({})
@or_depth_limit = (tracker && tracker.options[:branch_limit]) || 5 #arbitrary default
@meth_env = nil
@initializer_envs = {}
@current_file = current_file
set_env_defaults
end
Expand Down Expand Up @@ -432,7 +433,17 @@ def process_block exp
#Process a method definition.
def process_defn exp
meth_env do
unless exp.method_name == :initialize
env.current.merge! @initializer_envs.fetch(@current_class, {}).to_h
end

exp.body = process_all! exp.body

if @current_class and exp.method_name == :initialize
# Call Env#current to get just a hash of
# instance variables and their values
@initializer_envs[@current_class] = only_ivars.current
end
end
exp
end
Expand Down
9 changes: 0 additions & 9 deletions lib/brakeman/processors/library_processor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,6 @@ def process_module exp
end

def process_defn exp
if exp.method_name == :initialize
@alias_processor.process_safely exp.body_list
@initializer_env = @alias_processor.only_ivars
elsif node_type? exp, :defn
exp = @alias_processor.process_safely exp, @initializer_env
else
exp = @alias_processor.process exp
end

if @current_class
exp.body = process_all! exp.body
@current_class.add_method :public, exp.method_name, exp, @current_file
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
SOME_CONSTANT = '1'
8 changes: 8 additions & 0 deletions test/apps/rails6/lib/run_stuff.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,12 @@ def run
`cat #{temp_file.path}`
end
end

RUN_THINGS = {
SOME_CONSTANT => "ASafeString"
}

def use_group_things
RUN_THINGS[params[:key]].constantize.new
end
end
13 changes: 13 additions & 0 deletions test/tests/rails6.rb
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,19 @@ def test_remote_code_execution_not_query_parameters
:user_input => s(:call, s(:call, s(:const, :User), :first), :some_method_thing)
end

def test_constants_in_libraries
assert_no_warning :type => :warning,
:warning_code => 24,
:fingerprint => "bf29c627bcf090125caf10c73e33cd3f7b66f5f1a2d5ba03b418ed51e3e15b72",
:warning_type => "Remote Code Execution",
:line => 13,
:message => /^Unsafe\ reflection\ method\ `constantize`\ c/,
:confidence => 1,
:relative_path => "lib/run_stuff.rb",
:code => s(:call, s(:call, s(:const, :RUN_THINGS), :[], s(:call, s(:params), :[], s(:lit, :key))), :constantize),
:user_input => s(:call, s(:params), :[], s(:lit, :key))
end

def test_safe_yaml_load_option
assert_no_warning :type => :warning,
:warning_code => 25,
Expand Down