diff --git a/lib/brakeman/checks/base_check.rb b/lib/brakeman/checks/base_check.rb index e916afbb2d2dd60b735574b98a4378ebf0086410..48cb86f00bda70691b22e2f4234d1583a6a3eac4 100644 --- a/lib/brakeman/checks/base_check.rb +++ b/lib/brakeman/checks/base_check.rb @@ -364,7 +364,11 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor if @safe_input_attributes.include? method false elsif call? target and not method.to_s[-1,1] == "?" - has_immediate_model? target, out + if res = has_immediate_model?(target, out) + exp + else + false + end elsif model_name? target exp else @@ -429,28 +433,6 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor end end - #Finds entire method call chain where +target+ is a target in the chain - def find_chain exp, target - return unless sexp? exp - - case exp.node_type - when :output, :format - find_chain exp.value, target - when :call - if exp == target or include_target? exp, target - return exp - end - else - exp.each do |e| - if sexp? e - res = find_chain e, target - return res if res - end - end - nil - end - end - #Returns true if +target+ is in +exp+ def include_target? exp, target return false unless call? exp diff --git a/lib/brakeman/checks/check_content_tag.rb b/lib/brakeman/checks/check_content_tag.rb index 5974fc2c1974b5fd17c9e00c82426c09b0d8850b..1d77182c123489bef172e6cba7e971162bdfd518 100644 --- a/lib/brakeman/checks/check_content_tag.rb +++ b/lib/brakeman/checks/check_content_tag.rb @@ -107,12 +107,10 @@ class Brakeman::CheckContentTag < Brakeman::CheckCrossSiteScripting :link_path => "content_tag" elsif not tracker.options[:ignore_model_output] and match = has_immediate_model?(arg) - method = match[2] - - unless IGNORE_MODEL_METHODS.include? method + unless IGNORE_MODEL_METHODS.include? match.method add_result result - if MODEL_METHODS.include? method or method.to_s =~ /^find_by/ + if likely_model_attribute? match confidence = CONFIDENCE[:high] else confidence = CONFIDENCE[:med] diff --git a/lib/brakeman/checks/check_cross_site_scripting.rb b/lib/brakeman/checks/check_cross_site_scripting.rb index c43a557042fd0ed9a11e2c6ce387b9efece112d8..fa64dec296a0439803ce0d26ff38ad70fd055062 100644 --- a/lib/brakeman/checks/check_cross_site_scripting.rb +++ b/lib/brakeman/checks/check_cross_site_scripting.rb @@ -122,7 +122,7 @@ class Brakeman::CheckCrossSiteScripting < Brakeman::BaseCheck unless IGNORE_MODEL_METHODS.include? method add_result exp - if MODEL_METHODS.include? method or method.to_s =~ /^find_by/ + if likely_model_attribute? match confidence = CONFIDENCE[:high] else confidence = CONFIDENCE[:med] @@ -138,12 +138,17 @@ class Brakeman::CheckCrossSiteScripting < Brakeman::BaseCheck warning_code = :xss_to_json end - code = find_chain out, match + code = if match == out + nil + else + match + end + warn :template => @current_template, :warning_type => "Cross Site Scripting", :warning_code => warning_code, :message => message, - :code => code, + :code => match, :confidence => confidence, :link_path => link_path end @@ -153,6 +158,19 @@ class Brakeman::CheckCrossSiteScripting < Brakeman::BaseCheck end end + #Call already involves a model, but might not be acting on a record + def likely_model_attribute? exp + return false unless call? exp + + method = exp.method + + if MODEL_METHODS.include? method or method.to_s.start_with? "find_by_" + true + else + likely_model_attribute? exp.target + end + end + #Process an output Sexp def process_output exp process exp.value.dup diff --git a/lib/brakeman/checks/check_link_to.rb b/lib/brakeman/checks/check_link_to.rb index 0b49bbb1ca3998bfbc7b2085b765aea64bdf2b28..9d3ecb5cdd6f476ecb8fd77dd6fe3a7e760320cf 100644 --- a/lib/brakeman/checks/check_link_to.rb +++ b/lib/brakeman/checks/check_link_to.rb @@ -82,7 +82,7 @@ class Brakeman::CheckLinkTo < Brakeman::CheckCrossSiteScripting return false if IGNORE_MODEL_METHODS.include? method confidence = CONFIDENCE[:med] - confidence = CONFIDENCE[:high] if MODEL_METHODS.include? method or method.to_s =~ /^find_by/ + confidence = CONFIDENCE[:high] if likely_model_attribute? match warn_xss(result, "Unescaped model attribute in link_to", match, confidence) end diff --git a/test/apps/rails3.1/app/controllers/other_controller.rb b/test/apps/rails3.1/app/controllers/other_controller.rb index 9c95e86eb99def9e739b760d088a7e4728463752..98a0cabc22100f1abbb01b5499ad165373e81963 100644 --- a/test/apps/rails3.1/app/controllers/other_controller.rb +++ b/test/apps/rails3.1/app/controllers/other_controller.rb @@ -72,4 +72,8 @@ class OtherController < ApplicationController Marshal.restore User.find(1).cool_stored_thing end + + def test_model_in_haml + @user = User.new + end end diff --git a/test/apps/rails3.1/app/views/other/test_model_in_haml.html.haml b/test/apps/rails3.1/app/views/other/test_model_in_haml.html.haml new file mode 100644 index 0000000000000000000000000000000000000000..1fc1f4054c8bd53fd0e91e6a065a9d20798a6d31 --- /dev/null +++ b/test/apps/rails3.1/app/views/other/test_model_in_haml.html.haml @@ -0,0 +1,7 @@ +%user + %footer + = @user.updated_at + + %h1= @user.name + + != @user.bio diff --git a/test/tests/rails31.rb b/test/tests/rails31.rb index 34fa045196ebbfe22b054814d8b836c64c6b682a..e153783c98ab4b2dce38bc4ffb5d89da68c7257f 100644 --- a/test/tests/rails31.rb +++ b/test/tests/rails31.rb @@ -13,7 +13,7 @@ class Rails31Tests < Test::Unit::TestCase def expected @expected ||= { :model => 3, - :template => 22, + :template => 23, :controller => 4, :generic => 72 } end @@ -1033,4 +1033,26 @@ class Rails31Tests < Test::Unit::TestCase :confidence => 1, :relative_path => "app/controllers/other_controller.rb" end + + def test_wrong_model_attributes_in_haml + assert_no_warning :type => :template, + :warning_code => 2, + :fingerprint => "8851713f0af477e60090607b814ba68055e4ac1cf19df0628fddd961ff87e763", + :warning_type => "Cross Site Scripting", + :line => 3, + :message => /^Unescaped\ model\ attribute/, + :confidence => 0, + :relative_path => "app/views/other/test_model_in_haml.html.haml" + end + + def test_right_model_attribute_in_haml + assert_warning :type => :template, + :warning_code => 2, + :fingerprint => "3310ef4a4bde8b120fd5d421565ee416af815404e7c116a8069052e8732589d0", + :warning_type => "Cross Site Scripting", + :line => 7, + :message => /^Unescaped\ model\ attribute/, + :confidence => 0, + :relative_path => "app/views/other/test_model_in_haml.html.haml" + end end