Fix mass assignment and SQLi dupes in views #82

Merged
merged 5 commits into from Apr 19, 2012
+34 −15
Split
View
5 lib/brakeman/checks/check_mass_assignment.rb
@@ -21,7 +21,6 @@ def run_check
return if models.empty?
- @results = Set.new
Brakeman.debug "Finding possible mass assignment calls on #{models.length} models"
calls = tracker.find_call :chained => true, :targets => models, :methods => [:new,
@@ -45,8 +44,8 @@ def process_result res
check = check_call call
- if check and not @results.include? call
- @results << call
+ if check and not call.original_line and not duplicate? res
+ add_result res
model = tracker.models[res[:chain].first]
View
4 lib/brakeman/checks/check_sql.rb
@@ -17,9 +17,9 @@ def run_check
@rails_version = tracker.config[:rails_version]
if tracker.options[:rails3]
- @sql_targets = /^(find.*|last|first|all|count|sum|average|minumum|maximum|count_by_sql|where|order|group|having)$/
+ @sql_targets = /^(find|find_by_sql|last|first|all|count|sum|average|minumum|maximum|count_by_sql|where|order|group|having)$/
else
- @sql_targets = /^(find.*|last|first|all|count|sum|average|minumum|maximum|count_by_sql)$/
+ @sql_targets = /^(find|find_by_sql|last|first|all|count|sum|average|minumum|maximum|count_by_sql)$/
end
Brakeman.debug "Finding possible SQL calls on models"
View
6 lib/brakeman/checks/check_without_protection.rb
@@ -23,8 +23,6 @@ def run_check
return if models.empty?
- @results = Set.new
-
Brakeman.debug "Finding all mass assignments"
calls = tracker.find_call :targets => models, :methods => [:new,
:attributes=,
@@ -45,11 +43,11 @@ def process_result res
call = res[:call]
last_arg = call[3][-1]
- if hash? last_arg and not @results.include? call
+ if hash? last_arg and not call.original_line and not duplicate? res
if value = hash_access(last_arg, :without_protection)
if true? value
- @results << call
+ add_result res
if include_user_input? call[3]
confidence = CONFIDENCE[:high]
View
2 lib/brakeman/processors/erb_template_processor.rb
@@ -46,8 +46,10 @@ def process_call exp
exp[3] = process(exp[3])
make_render_in_view exp
else
+ #TODO: Is it really necessary to create a new Sexp here?
args = exp[3] = process(exp[3])
call = Sexp.new :call, target, method, args
+ call.original_line(exp.original_line)
call.line(exp.line)
call
end
View
2 lib/brakeman/processors/erubis_template_processor.rb
@@ -43,8 +43,10 @@ def process_call exp
exp[3] = process exp[3]
make_render_in_view exp
else
+ #TODO: Is it really necessary to create a new Sexp here?
args = exp[3] = process(exp[3])
call = Sexp.new :call, target, method, args
+ call.original_line(exp.original_line)
call.line(exp.line)
call
end
View
2 lib/brakeman/processors/haml_template_processor.rb
@@ -94,8 +94,10 @@ def process_call exp
exp[3] = process exp[3]
make_render_in_view exp
else
+ #TODO: Do we really need a new Sexp here?
args = process exp[3]
call = Sexp.new :call, target, method, args
+ call.original_line(exp.original_line)
call.line(exp.line)
call
end
View
4 lib/brakeman/processors/lib/render_helper.rb
@@ -115,7 +115,9 @@ def process_template name, args, called_from = nil
#that values came from another file
template_env.all.each do |var, value|
unless value.original_line
- value.original_line = value.line
+ #TODO: This has been broken for a while now and no one noticed
+ #so maybe we can skip it
+ value.original_line(value.line)
end
end
View
4 test/apps/rails2/app/controllers/home_controller.rb
@@ -112,6 +112,10 @@ def test_sanitized_param
params["something"] = h(params["something"])
end
+ def test_safe_find_by
+ User.find_or_create_by_name(params[:name], :code => (params[:x] + "code"))
+ end
+
private
def filter_it
View
9 test/tests/test_rails2.rb
@@ -219,6 +219,15 @@ def test_sql_injection_in_self_call
:file => /user\.rb/
end
+ def test_sql_user_input_in_find_by
+ assert_no_warning :type => :warning,
+ :warning_type => "SQL Injection",
+ :line => 116,
+ :message => /^Possible SQL injection near line 116: User.find_or_create_by_name/,
+ :confidence => 0,
+ :file => /home_controller\.rb/
+ end
+
def test_csrf_protection
assert_warning :type => :controller,
:warning_type => "Cross-Site Request Forgery",
View
5 test/tests/test_rails3.rb
@@ -14,7 +14,7 @@ def expected
@expected ||= {
:controller => 1,
:model => 5,
- :template => 22,
+ :template => 21,
:warning => 24
}
end
@@ -426,7 +426,8 @@ def test_indirect_xss
end
def test_sql_injection_in_template
- assert_warning :type => :template,
+ #SQL injection in controllers should not warn again in views
+ assert_no_warning :type => :template,
:warning_type => "SQL Injection",
:line => 3, #This should be line 4 :(
:message => /^Possible SQL injection/,
View
6 test/tests/test_rails_with_xss_plugin.rb
@@ -10,7 +10,7 @@ def expected
@expected ||= {
:controller => 1,
:model => 3,
- :template => 3,
+ :template => 1,
:warning => 13 }
end
@@ -169,7 +169,7 @@ def test_dynamic_render_path_15
def test_sql_injection_16
- assert_warning :type => :template,
+ assert_no_warning :type => :template,
:warning_type => "SQL Injection",
:line => 4,
:message => /^Possible\ SQL\ injection/,
@@ -179,7 +179,7 @@ def test_sql_injection_16
def test_sql_injection_17
- assert_warning :type => :template,
+ assert_no_warning :type => :template,
:warning_type => "SQL Injection",
:line => 7,
:message => /^Possible\ SQL\ injection/,