Merge pull request #136 from oreoshake/warn_on_to_json

High confidence warnings on `to_json`

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
 

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
+
   private
 
   def filter_it

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 => 33,
+        :template => 36,
         :warning => 31}
     else
       @expected ||= {
         :controller => 1,
         :model => 2,
-        :template => 33,
+        :template => 36,
         :warning => 32 }
     end
   end
@@ -691,5 +691,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