diff --git a/lib/brakeman/checks/check_content_tag.rb b/lib/brakeman/checks/check_content_tag.rb new file mode 100644 index 0000000000000000000000000000000000000000..05374bf86d83dfa9ed7cf97c4c9a4d3bda82ab2f --- /dev/null +++ b/lib/brakeman/checks/check_content_tag.rb @@ -0,0 +1,179 @@ +require 'brakeman/checks/check_cross_site_scripting' + +#Checks for unescaped values in `content_tag` +# +# content_tag :tag, body +# ^-- Unescaped in Rails 2.x +# +# content_tag, :tag, body, attribute => value +# ^-- Unescaped in all versions +# +# content_tag, :tag, body, attribute => value +# ^ +# | +# Escaped by default, can be explicitly escaped +# or not by passing in (true|false) as fourth argument +class Brakeman::CheckContentTag < Brakeman::CheckCrossSiteScripting + Brakeman::Checks.add self + + @description = "Checks for XSS in calls to content_tag" + + def run_check + @ignore_methods = Set[:button_to, :check_box, :escapeHTML, :escape_once, + :field_field, :fields_for, :h, :hidden_field, + :hidden_field, :hidden_field_tag, :image_tag, :label, + :mail_to, :radio_button, :select, + :submit_tag, :text_area, :text_field, + :text_field_tag, :url_encode, :url_for, + :will_paginate].merge tracker.options[:safe_methods] + + @known_dangerous = [] + methods = tracker.find_call :target => false, :method => :content_tag + + @models = tracker.models.keys + @inspect_arguments = tracker.options[:check_arguments] + + Brakeman.debug "Checking for XSS in content_tag" + methods.each do |call| + process_result call + end + end + + def process_result result + return if duplicate? result + + call = result[:call] = result[:call].dup + + args = call.arglist + + tag_name = args[1] + content = args[2] + attributes = args[3] + escape_attr = args[4] + + @matched = false + + #Silly, but still dangerous if someone uses user input in the tag type + check_argument result, tag_name + + #Versions before 3.x do not escape body of tag, nor does the rails_xss gem + unless @matched or (tracker.options[:rails3] and not raw? content) + check_argument result, content + end + + #Attribute keys are never escaped, so check them for user input + if not @matched and hash? attributes and not request_value? attributes + hash_iterate(attributes) do |k, v| + check_argument result, k + return if @matched + end + end + + #By default, content_tag escapes attribute values passed in as a hash. + #But this behavior can be disabled. So only check attributes hash + #if they are explicitly not escaped. + if not @matched and attributes and false? escape_attr + if request_value? attributes or not hash? attributes + check_argument result, attributes + else #check hash values + hash_iterate(attributes) do |k, v| + check_argument result, v + return if @matched + end + end + end + end + + def check_argument result, exp + #Check contents of raw() calls directly + if call? exp and exp.method == :raw + arg = process exp.first_arg + else + arg = process exp + end + + if input = has_immediate_user_input?(arg) + case input.type + when :params + message = "Unescaped parameter value in content_tag" + when :cookies + message = "Unescaped cookie value in content_tag" + else + message = "Unescaped user input value in content_tag" + end + + add_result result + + warn :result => result, + :warning_type => "Cross Site Scripting", + :message => message, + :user_input => input.match, + :confidence => CONFIDENCE[:high], + :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 + add_result result + + if MODEL_METHODS.include? method or method.to_s =~ /^find_by/ + confidence = CONFIDENCE[:high] + else + confidence = CONFIDENCE[:med] + end + + warn :result => result, + :warning_type => "Cross Site Scripting", + :message => "Unescaped model attribute in content_tag", + :user_input => match, + :confidence => confidence, + :link_path => "content_tag" + end + + elsif @matched + message = "Unescaped " + + case @matched.type + when :model + return if tracker.options[:ignore_model_output] + message << "model attribute" + when :params + message << "parameter" + when :cookies + message << "cookie" + when :session + message << "session" + else + message << "user input" + end + + message << " value in content_tag" + + add_result result + + warn :result => result, + :warning_type => "Cross Site Scripting", + :message => message, + :user_input => @matched.match, + :confidence => CONFIDENCE[:med], + :link_path => "content_tag" + end + end + + def process_call exp + if @mark + actually_process_call exp + else + @mark = true + actually_process_call exp + @mark = false + end + + exp + end + + def raw? exp + call? exp and exp.method == :raw + end +end diff --git a/test/apps/rails2/app/controllers/home_controller.rb b/test/apps/rails2/app/controllers/home_controller.rb index 871250ae2f8443e351beccc29e3af0fc34edf00f..2b75358efe5c6d55861ca94ad5a3a8f14a24438b 100644 --- a/test/apps/rails2/app/controllers/home_controller.rb +++ b/test/apps/rails2/app/controllers/home_controller.rb @@ -147,6 +147,10 @@ class HomeController < ApplicationController @json = {:json_thing => params[:json_thing]}.to_json end + def test_content_tag + @user = User.find(current_user) + end + private def filter_it diff --git a/test/apps/rails2/app/views/home/test_content_tag.html.erb b/test/apps/rails2/app/views/home/test_content_tag.html.erb new file mode 100644 index 0000000000000000000000000000000000000000..202f07ea23da861c8a88780f2af984824eddcae6 --- /dev/null +++ b/test/apps/rails2/app/views/home/test_content_tag.html.erb @@ -0,0 +1,26 @@ +Should not warn +<%= content_tag :p, h(params[:something]) %> + +Should warn +<%= content_tag :span, @user.name %> + +Should not warn +<%= content_tag :div, "Blah!", { :class => params[:class] }, true %> + +Should warn +<%= content_tag :div, "Blah!", { cookies[:weird] => "bad idea" } %> + +Should not warn +<%= content_tag :h1, params[:x] == 1 ? "totally" : "safe" %> + +Should still warn +<%= content_tag :div, "Blah!", { @user.something => "bad idea"}, true %> + +Should not warn +<%= content_tag :div, "Blah!", { :class => params[:class] } %> + +Should warn +<%= content_tag :div, "Blah!", { :id => @user.name }, false %> + +Should warn, medium confidence +<%= content_tag :div, x(params[:maybe_bad]) %> diff --git a/test/apps/rails3/app/controllers/home_controller.rb b/test/apps/rails3/app/controllers/home_controller.rb index 74d171d00e2b3f8486e9de6399d9053a3d958e9d..2bb5d462b81033b568463da4c456a470c80b0bbf 100644 --- a/test/apps/rails3/app/controllers/home_controller.rb +++ b/test/apps/rails3/app/controllers/home_controller.rb @@ -97,6 +97,9 @@ class HomeController < ApplicationController redirect_to params end + def test_content_tag + @user = User.find(current_user) + end private diff --git a/test/apps/rails3/app/views/home/test_content_tag.html.erb b/test/apps/rails3/app/views/home/test_content_tag.html.erb new file mode 100644 index 0000000000000000000000000000000000000000..282dbe420617f1f5807972ca3d6cdb34bf62fe5e --- /dev/null +++ b/test/apps/rails3/app/views/home/test_content_tag.html.erb @@ -0,0 +1,35 @@ +Should not warn +<%= content_tag :p, h(params[:something]) %> + +Should not warn +<%= content_tag :span, @user.name %> + +Should warn +<%= content_tag :span, raw(params[:blah]) %> + +Should not warn +<%= content_tag :div, "Blah!", { :class => params[:class] }, true %> + +Should warn +<%= content_tag :div, "Blah!", { cookies[:weird] => "bad idea" } %> + +Should not warn +<%= content_tag :h1, params[:x] == 1 ? "totally" : "safe" %> + +Should still warn +<%= content_tag :div, "Blah!", { @user.something => "bad idea"}, true %> + +Should not warn +<%= content_tag :div, "Blah!", { :class => params[:class] } %> + +Should warn +<%= content_tag :div, "Blah!", { :id => @user.name }, false %> + +Should not warn +<%= content_tag :div, x(params[:maybe_bad]) %> + +Should warn +<%= content_tag params[:whyyy], "Don't do this" %> + +Should warn +<%= content_tag @user.preferred_markup, "Seriously" %> diff --git a/test/tests/test_rails2.rb b/test/tests/test_rails2.rb index 20fe8ecdaf80789d0ef08e0a1be0b7569cfba597..952b8500752e77e641065d795cbbd13b88bb62ac 100644 --- a/test/tests/test_rails2.rb +++ b/test/tests/test_rails2.rb @@ -11,13 +11,13 @@ class Rails2Tests < Test::Unit::TestCase @expected ||= { :controller => 1, :model => 2, - :template => 36, + :template => 41, :warning => 31} else @expected ||= { :controller => 1, :model => 2, - :template => 36, + :template => 41, :warning => 32 } end end @@ -651,6 +651,69 @@ class Rails2Tests < Test::Unit::TestCase :file => /test_strip_tags\.html\.erb/ end + def test_xss_content_tag_body + assert_warning :type => :template, + :warning_type => "Cross Site Scripting", + :line => 5, + :message => /^Unescaped\ model\ attribute\ in\ content_tag/, + :confidence => 0, + :file => /test_content_tag\.html\.erb/ + end + + def test_xss_content_tag_escaped + assert_no_warning :type => :template, + :warning_type => "Cross Site Scripting", + :line => 8, + :message => /^Unescaped\ cookie\ value\ in\ content_tag/, + :confidence => 0, + :file => /test_content_tag\.html\.erb/ + end + + def test_xss_content_tag_attribute_name + assert_warning :type => :template, + :warning_type => "Cross Site Scripting", + :line => 11, + :message => /^Unescaped\ cookie\ value\ in\ content_tag/, + :confidence => 0, + :file => /test_content_tag\.html\.erb/ + end + + def test_xss_content_tag_attribute_name_even_with_escape_set + assert_warning :type => :template, + :warning_type => "Cross Site Scripting", + :line => 17, + :message => /^Unescaped\ model\ attribute\ in\ content_tag/, + :confidence => 0, + :file => /test_content_tag\.html\.erb/ + end + + def test_cross_site_scripting_escaped_by_default + assert_no_warning :type => :template, + :warning_type => "Cross Site Scripting", + :line => 20, + :message => /^Unescaped\ parameter\ value\ in\ content_tag/, + :confidence => 0, + :file => /test_content_tag\.html\.erb/ + end + + def test_xss_content_tag_unescaped_on_purpose + assert_warning :type => :template, + :warning_type => "Cross Site Scripting", + :line => 23, + :message => /^Unescaped\ model\ attribute\ in\ content_tag/, + :confidence => 0, + :file => /test_content_tag\.html\.erb/ + end + + def test_xss_content_tag_indirect_body + assert_warning :type => :template, + :warning_type => "Cross Site Scripting", + :line => 26, + :message => /^Unescaped\ parameter\ value\ in\ content_tag/, + :confidence => 1, + :file => /test_content_tag\.html\.erb/ + end + def test_cross_site_scripting_single_quotes_CVE_2012_3464 assert_warning :type => :warning, :warning_type => "Cross Site Scripting", diff --git a/test/tests/test_rails3.rb b/test/tests/test_rails3.rb index 671e23962b61cdf5f4b66ee90e3cab3f9dc1c34f..6cacdcb0802efd228aa14fa98c0271a8aecc4caf 100644 --- a/test/tests/test_rails3.rb +++ b/test/tests/test_rails3.rb @@ -14,7 +14,7 @@ class Rails3Tests < Test::Unit::TestCase @expected ||= { :controller => 1, :model => 5, - :template => 23, + :template => 29, :warning => 30 } end @@ -586,6 +586,60 @@ class Rails3Tests < Test::Unit::TestCase :file => /_form\.html\.erb/ end + def test_xss_content_tag_raw_content + assert_warning :type => :template, + :warning_type => "Cross Site Scripting", + :line => 8, + :message => /^Unescaped\ parameter\ value\ in\ content_tag/, + :confidence => 0, + :file => /test_content_tag\.html\.erb/ + end + + def test_xss_content_tag_attribute_name + assert_warning :type => :template, + :warning_type => "Cross Site Scripting", + :line => 14, + :message => /^Unescaped\ cookie\ value\ in\ content_tag/, + :confidence => 0, + :file => /test_content_tag\.html\.erb/ + end + + def test_xss_content_tag_attribute_name_even_with_escape + assert_warning :type => :template, + :warning_type => "Cross Site Scripting", + :line => 20, + :message => /^Unescaped\ model\ attribute\ in\ content_tag/, + :confidence => 0, + :file => /test_content_tag\.html\.erb/ + end + + def test_xss_content_tag_unescaped_attribute + assert_warning :type => :template, + :warning_type => "Cross Site Scripting", + :line => 26, + :message => /^Unescaped\ model\ attribute\ in\ content_tag/, + :confidence => 0, + :file => /test_content_tag\.html\.erb/ + end + + def test_xss_content_tag_in_tag_name + assert_warning :type => :template, + :warning_type => "Cross Site Scripting", + :line => 32, + :message => /^Unescaped\ parameter\ value\ in\ content_tag/, + :confidence => 0, + :file => /test_content_tag\.html\.erb/ + end + + def test_cross_site_scripting_model_in_tag_name + assert_warning :type => :template, + :warning_type => "Cross Site Scripting", + :line => 35, + :message => /^Unescaped\ model\ attribute\ in\ content_tag/, + :confidence => 0, + :file => /test_content_tag\.html\.erb/ + end + def test_cross_site_scripting_request_parameters assert_warning :type => :template, :warning_type => "Cross Site Scripting",