Merge branch 'master' into add_content_tag_check

Conflicts:
	test/apps/rails2/app/controllers/home_controller.rb
	test/tests/test_rails2.rb

lib/brakeman.rb

@@ -39,6 +39,7 @@ module Brakeman
   #  * :safe_methods - array of methods to consider safe
   #  * :skip_libs - do not process lib/ directory (default: false)
   #  * :skip_checks - checks not to run (run all if not specified)
+  #  * :relative_path - show relative path of each file(default: false)
   #  * :summary_only - only output summary section of report 
   #                    (does not apply to tabs format)
   #
@@ -119,6 +120,7 @@ module Brakeman
       :ignore_model_output => false,
       :message_limit => 100,
       :parallel_checks => true,
+      :relative_path => false,
       :quiet => true,
       :report_progress => true,
       :html_style => "#{File.expand_path(File.dirname(__FILE__))}/brakeman/format/style.css" 

lib/brakeman/checks/check_cross_site_scripting.rb

@@ -34,7 +34,7 @@ class Brakeman::CheckCrossSiteScripting < Brakeman::BaseCheck
   FORM_BUILDER = Sexp.new(:call, Sexp.new(:const, :FormBuilder), :new, Sexp.new(:arglist)) 
 
   #Run check
-  def run_check 
+  def run_check
     @ignore_methods = Set[:button_to, :check_box, :content_tag, :escapeHTML, :escape_once,
                            :field_field, :fields_for, :h, :hidden_field,
                            :hidden_field, :hidden_field_tag, :image_tag, :label,
@@ -58,6 +58,16 @@ class Brakeman::CheckCrossSiteScripting < Brakeman::BaseCheck
       @known_dangerous << :strip_tags
     end
 
+    matches = tracker.check_initializers :ActiveSupport, :escape_html_entities_in_json=
+    json_escape_on = matches.detect {|result| true? result[-1].first_arg}
+
+    if !json_escape_on or version_between? "0.0.0", "2.0.99"
+      @known_dangerous << :to_json
+      Brakeman.debug("Automatic to_json escaping not enabled, consider to_json dangerous")
+    else
+      Brakeman.debug("Automatic to_json escaping is enabled.")
+    end
+
     tracker.each_template do |name, template|
       @current_template = template
       template[:outputs].each do |out|
@@ -115,12 +125,20 @@ class Brakeman::CheckCrossSiteScripting < Brakeman::BaseCheck
           confidence = CONFIDENCE[:med]
         end
 
+        message = "Unescaped model attribute"
+        link_path = "cross_site_scripting"
+        if node_type?(out, :call, :attrasgn) && out.method == :to_json
+          message += " in JSON hash" 
+          link_path += "_to_json"
+        end
+
         code = find_chain out, match
         warn :template => @current_template,
           :warning_type => "Cross Site Scripting", 
-          :message => "Unescaped model attribute",
+          :message => message,
           :code => code,
-          :confidence => confidence
+          :confidence => confidence,
+          :link_path => link_path
       end
 
     else
@@ -173,8 +191,13 @@ class Brakeman::CheckCrossSiteScripting < Brakeman::BaseCheck
         if message and not duplicate? exp
           add_result exp
 
-          if exp.target.nil? and @known_dangerous.include? exp.method
+          link_path = "cross_site_scripting"
+          if @known_dangerous.include? exp.method
             confidence = CONFIDENCE[:high]
+            if exp.method == :to_json
+              message += " in JSON hash"
+              link_path += "_to_json"
+            end
           else
             confidence = CONFIDENCE[:low]
           end
@@ -184,7 +207,8 @@ class Brakeman::CheckCrossSiteScripting < Brakeman::BaseCheck
             :message => message,
             :code => exp,
             :user_input => @matched.match,
-            :confidence => confidence
+            :confidence => confidence,
+            :link_path => link_path
         end
       end
 

lib/brakeman/options.rb

@@ -169,6 +169,10 @@ module Brakeman::Options
           options[:summary_only] = true
         end
 
+        opts.on "--relative-paths", "Output relative file paths in reports" do
+          options[:relative_paths] = true
+        end
+
         opts.on "-w", 
           "--confidence-level LEVEL", 
           ["1", "2", "3"], 

lib/brakeman/report.rb

 require 'cgi'
 require 'set'
+require 'pathname'
 require 'brakeman/processors/output_processor'
 require 'brakeman/util'
 require 'terminal-table'
@@ -551,7 +552,7 @@ class Brakeman::Report
       message
     end <<
     "<table id='#{code_id}' class='context' style='display:none'>" <<
-    "<caption>#{(warning.file || '').gsub(tracker.options[:app_path], "")}</caption>"
+    "<caption>#{warning_file(warning, :relative) || ''}</caption>"
 
     unless context.empty?
       if warning.line - 1 == 1 or warning.line + 1 == 1
@@ -614,7 +615,7 @@ class Brakeman::Report
       checks.send(meth).map do |w|
         line = w.line || 0
         w.warning_type.gsub!(/[^\w\s]/, ' ')
-        "#{file_for w}\t#{line}\t#{w.warning_type}\t#{category}\t#{w.format_message}\t#{TEXT_CONFIDENCE[w.confidence]}"
+        "#{warning_file w}\t#{line}\t#{w.warning_type}\t#{category}\t#{w.format_message}\t#{TEXT_CONFIDENCE[w.confidence]}"
       end.join "\n"
 
     end.join "\n"
@@ -637,7 +638,6 @@ class Brakeman::Report
           w.code = ""
         end
         w.context = context_for(w).join("\n")
-        w.file = file_for w
       end
     end
 
@@ -650,7 +650,14 @@ class Brakeman::Report
     require 'json'
 
     errors = tracker.errors.map{|e| { :error => e[:error], :location => e[:backtrace][0] }}
-    warnings = all_warnings.map { |w| w.to_hash }.sort_by{|w| w[:file]}
+    app_path = tracker.options[:app_path]
+
+    warnings = all_warnings.map do |w|
+      hash = w.to_hash
+      hash[:file] = warning_file w
+      hash
+    end.sort_by { |w| w[:file] }
+
     scan_info = {
       :app_path => File.expand_path(tracker.options[:app_path]),
       :rails_version => rails_version,
@@ -680,6 +687,16 @@ class Brakeman::Report
     Set.new(tracker.templates.map {|k,v| v[:name].to_s[/[^.]+/]}).length
   end
 
+  def warning_file warning, relative = false
+    return nil if warning.file.nil?
+
+    if @tracker.options[:relative_paths] or relative
+      Pathname.new(warning.file).relative_path_from(Pathname.new(tracker.options[:app_path])).to_s
+    else
+      warning.file
+    end
+  end
+
   private
 
   def load_and_render_erb file, bind

test/apps/rails2/app/controllers/home_controller.rb

@@ -141,6 +141,12 @@ class HomeController < ApplicationController
     @user = User.find(current_user)
   end
 
+  def test_to_json
+    @model_json = User.find(current_user).to_json
+    @not_json = {:thing => params[:thing]}
+    @json = {:json_thing => params[:json_thing]}.to_json
+  end
+
   def test_content_tag
     @user = User.find(current_user)
   end

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

+Detection of to_json
+
+<%= @model_json %>
+
+In the view
+
+<%= @not_json.to_json %>
+
+In the controller
+
+<%= @json %>
+
+You would break the json formatting by doing this, but it's technically safe...
+<%= h(@json) %>

test/apps/rails_with_xss_plugin/app/controllers/users_controller.rb

@@ -125,4 +125,8 @@ class UsersController < ApplicationController
   def results
     @users = User.all(:conditions => "display_name like '%#{params[:query]}%'")
   end
+
+  def to_json
+
+  end
 end

test/apps/rails_with_xss_plugin/app/views/users/to_json.html.erb

+<%= raw({:asdf => params[:asdf]}.to_json) %>
\ No newline at end of file

test/tests/test_rails2.rb

@@ -11,13 +11,13 @@ class Rails2Tests < Test::Unit::TestCase
       @expected ||= {
         :controller => 1,
         :model => 2,
-        :template => 38,
+        :template => 41,
         :warning => 31}
     else
       @expected ||= {
         :controller => 1,
         :model => 2,
-        :template => 38,
+        :template => 41,
         :warning => 32 }
     end
   end
@@ -754,5 +754,32 @@ class Rails2Tests < Test::Unit::TestCase
       :confidence => 0,
       :file => /test_strip_tags\.html\.erb/
   end
+
+  def test_to_json
+    assert_warning :type => :template,
+      :warning_type => "Cross Site Scripting",
+      :line => 3,
+      :message => /^Unescaped model attribute in JSON hash/,
+      :confidence => 0,
+      :file => /test_to_json\.html\.erb/
+    assert_warning :type => :template,
+      :warning_type => "Cross Site Scripting",
+      :line => 7,
+      :message => /^Unescaped parameter value in JSON hash/,
+      :confidence => 0,
+      :file => /test_to_json\.html\.erb/
+    assert_warning :type => :template,
+      :warning_type => "Cross Site Scripting",
+      :line => 11,
+      :message => /^Unescaped parameter value in JSON hash/,
+      :confidence => 0,
+      :file => /test_to_json\.html\.erb/
+    assert_no_warning :type => :template,
+      :warning_type => "Cross Site Scripting",
+      :line => 14,
+      :message => /^Unescaped parameter value in JSON hash/,
+      :confidence => 0,
+      :file => /test_to_json\.html\.erb/
+  end
 end
 

test/tests/test_rails_with_xss_plugin.rb

@@ -10,7 +10,7 @@ class RailsWithXssPluginTests < Test::Unit::TestCase
     @expected ||= {
       :controller => 1,
       :model => 3,
-      :template => 1,
+      :template => 2,
       :warning => 14 }
   end
 
@@ -257,4 +257,13 @@ class RailsWithXssPluginTests < Test::Unit::TestCase
       :confidence => 0,
       :file => /Gemfile/
   end
+
+  def test_to_json
+    assert_warning :type => :template,
+      :warning_type => "Cross Site Scripting",
+      :line => 1,
+      :message => /^Unescaped parameter value in JSON hash/,
+      :confidence => 0,
+      :file => /users\/to_json\.html\.erb/
+  end  
 end