Merge branch 'add_content_tag_check'

Conflicts:
	test/apps/rails3/app/controllers/home_controller.rb
	test/tests/test_rails3.rb

lib/brakeman/checks/check_content_tag.rb

+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

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

test/apps/rails2/app/views/home/test_content_tag.html.erb

+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]) %>

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
 

test/apps/rails3/app/views/home/test_content_tag.html.erb

+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" %>

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",

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",