Catch raw hrefs as bad, warn unless wrapped in a "safe" method

Add concept of a safe-ening method to mark hrefs as safe

Feature:
Warn when using unsafe hrefs.  This is a very specific case that as of now produces a ton of noise.  This came out of an xss vuln where the value was escaped but still vulnerable.

    link_to 'asdf', h(@scary)

where

    @scary = 'javascript:alert(1)'

or

    @scary = 'data:  # http://palpapers.plynt.com/issues/2010Oct/bypass-xss-filters/

This branch accomplishes slightly intelligent warnings by adding a new command line option to declare methods that make a string URL safe (unless there is already a standard one out there).  e.g.:

    $ brakeman . --url-safe-methods ensure_valid_protocol!

    link_to 'asdf', ensure_valid_protocol!(@scary, :javascript)

lib/brakeman/checks/check_link_to.rb

@@ -6,19 +6,20 @@ require 'brakeman/checks/check_cross_site_scripting'
 #See https://rails.lighthouseapp.com/projects/8994/tickets/3518-link_to-doesnt-escape-its-input
 class Brakeman::CheckLinkTo < Brakeman::CheckCrossSiteScripting
   Brakeman::Checks.add self
+  IGNORE_METHODS = Set.new([: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] )
 
   @description = "Checks for XSS in link_to in versions before 3.0"
 
   def run_check
     return unless version_between?("2.0.0", "2.9.9") and not tracker.config[:escape_html]
 
-    @ignore_methods = Set.new([: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]
+    @ignore_methods = IGNORE_METHODS.merge tracker.options[:safe_methods]
 
     @known_dangerous = []
     #Ideally, I think this should also check to see if people are setting

lib/brakeman/checks/check_link_to_href.rb

+require 'brakeman/checks/check_cross_site_scripting'
+
+#Checks for calls to link_to which pass in potentially hazardous data
+#to the second argument.  While this argument must be html_safe to not break 
+#the html, it must also be url safe as determined by calling a 
+#:url_safe_method.  This prevents attacks such as javascript:evil() or 
+#data:<encoded XSS> which is html_safe, but not safe as an href
+#Props to Nick Green for the idea.
+class Brakeman::CheckLinkToHref < Brakeman::CheckLinkTo
+  Brakeman::Checks.add self
+                        
+  @description = "Checks to see if values used for hrefs are sanitized using a :url_safe_method to protect against javascript:/data: XSS"
+
+  def run_check
+    @ignore_methods = IGNORE_METHODS.merge(tracker.options[:url_safe_methods] || []).reject{|item| [:h, :escapeHTML, :escape_once].include? item}
+
+    @models = tracker.models.keys
+    @inspect_arguments = tracker.options[:check_arguments]
+
+    methods = tracker.find_call :target => false, :method => :link_to 
+    methods.each do |call|
+      process_result call
+    end
+  end
+
+  def process_result result
+    #Have to make a copy of this, otherwise it will be changed to
+    #an ignored method call by the code above.
+    call = result[:call] = result[:call].dup
+    @matched = false
+    url_arg = process call[3][2]
+    type, match = has_immediate_user_input? url_arg
+
+    if type
+      case type
+      when :params
+        message = "Unsafe parameter value in link_to href"
+      when :cookies
+        message = "Unsafe cookie value in link_to href"
+      else
+        message = "Unsafe user input value in link_to href"
+      end
+
+      unless duplicate? result
+        add_result result
+        warn :result => result,
+          :warning_type => "Cross Site Scripting", 
+          :message => message,
+          :confidence => CONFIDENCE[:high]
+      end
+    elsif has_immediate_model? url_arg
+
+      # Decided NOT warn on models.  polymorphic_path is called it a model is 
+      # passed to link_to (which passes it to url_for)
+
+    elsif hash? url_arg
+
+      # url_for uses the key/values pretty carefully and I don't see a risk.
+      # IF you have default routes AND you accept user input for :controller
+      # and :only_path, then MAYBE you could trigger a javascript:/data: 
+      # attack. 
+
+    elsif @matched
+      if @matched == :model and not tracker.options[:ignore_model_output]
+        message = "Unsafe model attribute in link_to href"
+      elsif @matched == :params
+        message = "Unsafe parameter value in link_to href"
+      end
+
+      if message and not duplicate? result
+        add_result result
+        warn :result => result, 
+          :warning_type => "Cross Site Scripting", 
+          :message => message,
+          :confidence => CONFIDENCE[:med]
+      end
+    end
+  end
+end

lib/brakeman/options.rb

@@ -84,6 +84,11 @@ module Brakeman::Options
           options[:safe_methods].merge methods.map {|e| e.to_sym }
         end
 
+        opts.on "--url-safe-methods method1,method2,etc", Array, "Do not warn of XSS if the link_to href parameter is wrapped in a safe method" do |methods|
+          options[:url_safe_methods] ||= Set.new
+          options[:url_safe_methods].merge methods.map {|e| e.to_sym }
+        end        
+
         opts.on "--skip-files file1,file2,etc", Array, "Skip processing of these files" do |files|
           options[:skip_files] ||= Set.new
           options[:skip_files].merge files.map {|f| f.to_sym }

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

@@ -8,3 +8,11 @@ More: <%= @indirect %>
 And: <%= params[:x][:y] %>
 
 Should not warn: <%= link_to h(params[:blah]), "some_url" %>
+
+Not-so-dangerous href: <%= link_to "some text", ensure_valid_proto!(params[:not_so_bad], :js) %>
+
+Dangerous href: <%= link_to "more text", params[:dangerous] %>
+
+Still dangerous hrefs: <%= link_to "donkey", not_safe(params[:bad_robot]) %>
+
+Not completely safe: <%= link_to "Helvetica hoodie bushwick", h(params[:js_xss]) %>

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

@@ -8,3 +8,11 @@ More: <%= raw @indirect %>
 Indirectly: <%= some_method params[:bad_stuff] %>
 
 And: <%= raw params[:x][:y] %>
+
+Not-so-dangerous href: <%= link_to "some text", ensure_valid_proto!(params[:not_so_bad], :js) %>
+
+Dangerous href: <%= link_to "more text", params[:dangerous] %>
+
+Still dangerous hrefs: <%= link_to "donkey", not_safe(params[:bad_robot]) %>
+
+Not completely safe: <%= link_to "Helvetica hoodie bushwick", h(params[:js_xss]) %>
\ No newline at end of file

test/test.rb

@@ -12,7 +12,8 @@ module BrakemanTester
     #Run scan on app at the given path
     def run_scan path, name = nil, opts = {}
       opts.merge! :app_path => "#{TEST_PATH}/apps/#{path}",
-        :quiet => false
+        :quiet => false,
+        :url_safe_methods => [:ensure_valid_proto!]
 
       announce "Processing #{name} application..."
 

test/tests/test_rails2.rb

@@ -11,13 +11,13 @@ class Rails2Tests < Test::Unit::TestCase
       @expected ||= {
         :controller => 1,
         :model => 2,
-        :template => 19,
+        :template => 22,
         :warning => 22 }
     else
       @expected ||= {
         :controller => 1,
         :model => 2,
-        :template => 19,
+        :template => 22,
         :warning => 23 }
     end
   end
@@ -307,6 +307,37 @@ class Rails2Tests < Test::Unit::TestCase
       :file => /test_params\.html\.erb/
   end
 
+  def test_encoded_href_parameter_in_link_to
+    assert_no_warning :type => :template,
+      :warning_type => "Cross Site Scripting",
+      :line => 12,
+      :message => /^Unsafe parameter value in link_to href/,
+      :confidence => 0,
+      :file => /test_params\.html\.erb/
+  end  
+
+  def test_href_parameter_in_link_to
+    assert_warning :type => :template,
+      :warning_type => "Cross Site Scripting",
+      :line => 14,
+      :message => /^Unsafe parameter value in link_to href/,
+      :confidence => 0,
+      :file => /test_params\.html\.erb/
+
+    assert_warning :type => :template,
+      :warning_type => "Cross Site Scripting",
+      :line => 16,
+      :message => /^Unsafe parameter value in link_to href/,
+      :confidence => 1,
+      :file => /test_params\.html\.erb/      
+
+    assert_warning :type => :template,
+      :warning_type => "Cross Site Scripting",
+      :line => 18,
+      :message => /^Unsafe parameter value in link_to href/,
+      :confidence => 1,
+      :file => /test_params\.html\.erb/            
+  end
 
   def test_filter
     assert_warning :type => :template,

test/tests/test_rails3.rb

@@ -14,7 +14,7 @@ class Rails3Tests < Test::Unit::TestCase
     @expected ||= {
       :controller => 1,
       :model => 5,
-      :template => 18,
+      :template => 21,
       :warning => 22
     }
   end
@@ -294,6 +294,38 @@ class Rails3Tests < Test::Unit::TestCase
       :file => /test_model\.html\.erb/
   end
 
+  def test_encoded_href_parameter_in_link_to
+    assert_no_warning :type => :template,
+      :warning_type => "Cross Site Scripting",
+      :line => 12,
+      :message => /^Unsafe parameter value in link_to href/,
+      :confidence => 0,
+      :file => /test_params\.html\.erb/
+  end  
+ 
+  def test_href_parameter_in_link_to
+    assert_warning :type => :template,
+      :warning_type => "Cross Site Scripting",
+      :line => 14,
+      :message => /^Unsafe parameter value in link_to href/,
+      :confidence => 0,
+      :file => /test_params\.html\.erb/
+ 
+    assert_warning :type => :template,
+      :warning_type => "Cross Site Scripting",
+      :line => 16,
+      :message => /^Unsafe parameter value in link_to href/,
+      :confidence => 1,
+      :file => /test_params\.html\.erb/      
+ 
+    assert_warning :type => :template,
+      :warning_type => "Cross Site Scripting",
+      :line => 18,
+      :message => /^Unsafe parameter value in link_to href/,
+      :confidence => 1,
+      :file => /test_params\.html\.erb/            
+  end  
+
   def test_file_access_in_template
     assert_warning :type => :template,
       :warning_type => "File Access",