Removing search controller vulnabilities

Signed-off-by: Lucas Kanashiro <kanashiro.duarte@gmail.com> Signed-off-by: Macartur Sousa <macartur.sc@gmail.com>

Removing search controller vulnabilities
Signed-off-by: Lucas Kanashiro <kanashiro.duarte@gmail.com> Signed-off-by: Macartur Sousa <macartur.sc@gmail.com>
Francisco Júnior · Rodrigo Souto
1 parent f9687c15
Showing 5 changed files with 68 additions and 48 deletions Show diff stats
app/controllers/public/search_controller.rb
config/application.rb
config/initializers/sanitizer.rb
lib/sanitize_params.rb
test/functional/search_controller_test.rb
@@ -3,7 +3,10 @@ class SearchController &lt; PublicController
   helper TagsHelper
   include SearchHelper
   include ActionView::Helpers::NumberHelper
+  include SanitizeParams
  
+
+  before_filter :sanitize_params
   before_filter :redirect_asset_param, :except => [:assets, :suggestions]
   before_filter :load_category, :except => :suggestions
   before_filter :load_search_assets, :except => :suggestions
@@ -134,7 +137,8 @@ class SearchController &lt; PublicController
  
   def tag
     @tag = params[:tag]
-    @tag_cache_key = "tag_#{CGI.escape(@tag.to_s)}_env_#{environment.id.to_s}_page_#{params[:npage]}"
+    tag_str = @tag.kind_of?(Array) ? @tag.join(" ") : @tag.to_str
+    @tag_cache_key = "tag_#{CGI.escape(tag_str)}_env_#{environment.id.to_s}_page_#{params[:npage]}"
     if is_cache_expired?(@tag_cache_key)
       @searches[@asset] = {:results => environment.articles.tagged_with(@tag).paginate(paginate_options)}
     end
@@ -15,6 +15,12 @@ module Noosfero
  
     require 'noosfero/plugin'
  
+    config.action_view.sanitized_allowed_tags |= %w(object embed param table
+      tr th td applet comment iframe audio video source)
+    config.action_view.sanitized_allowed_attributes |= %w(align border alt
+      vspace hspace width heigth value type data style target codebase archive
+      classid code flashvars scrolling frameborder controls autoplay colspan)
+
     require 'noosfero/multi_tenancy'
     config.middleware.use Noosfero::MultiTenancy::Middleware
  
@@ -1,14 +0,0 @@
-require 'loofah/helpers'
-
-ActionView::Base.full_sanitizer = Loofah::Helpers::ActionView::FullSanitizer.new
-ActionView::Base.white_list_sanitizer = Loofah::Helpers::ActionView::WhiteListSanitizer.new
-
-Loofah::HTML5::WhiteList::ALLOWED_ELEMENTS_WITH_LIBXML2.merge %w[
-  img object embed param table tr th td applet comment iframe audio video source
-]
-
-Loofah::HTML5::WhiteList::ALLOWED_ATTRIBUTES.merge %w[
-  align border alt vspace hspace width heigth value type data
-  style target codebase archive classid code flashvars scrolling frameborder controls autoplay colspan
-]
-
@@ -2,40 +2,40 @@ module SanitizeParams
  
   protected
  
-    # Check each request parameter for 
-    # improper HTML or Script tags
-    def sanitize_params
-      sanitize_params_hash(request.params)    
-    end
+  # Check each request parameter for
+  # improper HTML or Script tags
+  def sanitize_params
+    sanitize_params_hash(params)
+  end
  
-    # Given a params list sanitize all
-    def sanitize_params_hash(params)
-      params.each { |k, v|
-        if v.is_a?(String)        
-          params[k] = sanitize_param v
-        elsif v.is_a?(Array)
-          params[k] = sanitize_array v
-        elsif v.kind_of?(Hash)
-          params[k] = sanitize_params_hash(v)
-        end
-      }    
-    end
+  # Given a params list sanitize all
+  def sanitize_params_hash(params)
+    params.each { |k, v|
+      if v.is_a?(String)
+        params[k] = sanitize_param v
+      elsif v.is_a?(Array)
+        params[k] = sanitize_array v
+      elsif v.kind_of?(Hash)
+        params[k] = sanitize_params_hash(v)
+      end
+    }
+  end
  
-    # If the parameter was an array, 
-    # try to sanitize each element in the array
-    def sanitize_array(array)
-      array.map! { |e| 
-        if e.is_a?(String)
-          sanitize_param e
-        end
-      }
-      return array
-    end
+  # If the parameter was an array,
+  # try to sanitize each element in the array
+  def sanitize_array(array)
+    array.map! { |e|
+      if e.is_a?(String)
+        sanitize_param e
+      end
+    }
+    return array
+  end
  
-    # Santitize a single value
-    def sanitize_param(value)
-      allowed_tags = %w(a acronym b strong i em li ul ol h1 h2 h3 h4 h5 h6 blockquote br cite sub sup ins p)
-      ActionController::Base.helpers.sanitize(value, tags: allowed_tags, attributes: %w(href title))
-    end
+  # Santitize a single value
+  def sanitize_param(value)
+    allowed_tags = %w(a acronym b strong i em li ul ol h1 h2 h3 h4 h5 h6 blockquote br cite sub sup ins p)
+    ActionController::Base.helpers.sanitize(value, tags: allowed_tags, attributes: %w(href title))
+  end
  
-end    
+end
@@ -779,6 +779,30 @@ class SearchControllerTest &lt; ActionController::TestCase
     assert_equivalent [t1,t2,c1,c2,c3,c4] , assigns(:searches)[:communities][:results]
   end
  
+  should 'not raise an exception if tag query contains accented latin characters' do
+    tag_query = 'àáâãäå'
+    assert_nothing_raised(NoMethodError) { get :tag, :tag => tag_query }
+  end
+
+  should 'not allow query injection' do
+    injection = '<iMg SrC=x OnErRoR=document.documentElement.innerHTML=1>SearchParam'
+    get :tag, :tag => injection
+    tag = assigns(:tag)
+    assert !tag.upcase.include?('IMG')
+    assert tag.include?('SearchParam')
+  end
+
+  should 'not allow query injection in array' do
+    injection = ['<iMg SrC=x OnErRoR=document.documentElement.innerHTML=1>',
+                 '<script>document.innerHTML = \'x\'</script>']
+    get :tag, :tag => injection
+    tag = assigns(:tag)
+    tag.each { |t|
+      assert !t.upcase.include?('IMG')
+      assert !t.upcase.include?('SCRIPT')
+    }
+  end
+
   protected
  
   def create_event(profile, options)
...	...	@@ -3,7 +3,10 @@ class SearchController < PublicController
3	3	helper TagsHelper
4	4	include SearchHelper
5	5	include ActionView::Helpers::NumberHelper
	6	+ include SanitizeParams
6	7
	8	+
	9	+ before_filter :sanitize_params
7	10	before_filter :redirect_asset_param, :except => [:assets, :suggestions]
8	11	before_filter :load_category, :except => :suggestions
9	12	before_filter :load_search_assets, :except => :suggestions
...	...	@@ -134,7 +137,8 @@ class SearchController < PublicController
134	137
135	138	def tag
136	139	@tag = params[:tag]
137		- @tag_cache_key = "tag_#{CGI.escape(@tag.to_s)}_env_#{environment.id.to_s}_page_#{params[:npage]}"
	140	+ tag_str = @tag.kind_of?(Array) ? @tag.join(" ") : @tag.to_str
	141	+ @tag_cache_key = "tag_#{CGI.escape(tag_str)}_env_#{environment.id.to_s}_page_#{params[:npage]}"
138	142	if is_cache_expired?(@tag_cache_key)
139	143	@searches[@asset] = {:results => environment.articles.tagged_with(@tag).paginate(paginate_options)}
140	144	end
...	...
...	...	@@ -15,6 +15,12 @@ module Noosfero
15	15
16	16	require 'noosfero/plugin'
17	17
	18	+ config.action_view.sanitized_allowed_tags \|= %w(object embed param table
	19	+ tr th td applet comment iframe audio video source)
	20	+ config.action_view.sanitized_allowed_attributes \|= %w(align border alt
	21	+ vspace hspace width heigth value type data style target codebase archive
	22	+ classid code flashvars scrolling frameborder controls autoplay colspan)
	23	+
18	24	require 'noosfero/multi_tenancy'
19	25	config.middleware.use Noosfero::MultiTenancy::Middleware
20	26
...	...
...	...	@@ -1,14 +0,0 @@
1		-require 'loofah/helpers'
2		-
3		-ActionView::Base.full_sanitizer = Loofah::Helpers::ActionView::FullSanitizer.new
4		-ActionView::Base.white_list_sanitizer = Loofah::Helpers::ActionView::WhiteListSanitizer.new
5		-
6		-Loofah::HTML5::WhiteList::ALLOWED_ELEMENTS_WITH_LIBXML2.merge %w[
7		- img object embed param table tr th td applet comment iframe audio video source
8		-]
9		-
10		-Loofah::HTML5::WhiteList::ALLOWED_ATTRIBUTES.merge %w[
11		- align border alt vspace hspace width heigth value type data
12		- style target codebase archive classid code flashvars scrolling frameborder controls autoplay colspan
13		-]
14		-
...	...	@@ -2,40 +2,40 @@ module SanitizeParams
2	2
3	3	protected
4	4
5		- # Check each request parameter for
6		- # improper HTML or Script tags
7		- def sanitize_params
8		- sanitize_params_hash(request.params)
9		- end
	5	+ # Check each request parameter for
	6	+ # improper HTML or Script tags
	7	+ def sanitize_params
	8	+ sanitize_params_hash(params)
	9	+ end
10	10
11		- # Given a params list sanitize all
12		- def sanitize_params_hash(params)
13		- params.each { \|k, v\|
14		- if v.is_a?(String)
15		- params[k] = sanitize_param v
16		- elsif v.is_a?(Array)
17		- params[k] = sanitize_array v
18		- elsif v.kind_of?(Hash)
19		- params[k] = sanitize_params_hash(v)
20		- end
21		- }
22		- end
	11	+ # Given a params list sanitize all
	12	+ def sanitize_params_hash(params)
	13	+ params.each { \|k, v\|
	14	+ if v.is_a?(String)
	15	+ params[k] = sanitize_param v
	16	+ elsif v.is_a?(Array)
	17	+ params[k] = sanitize_array v
	18	+ elsif v.kind_of?(Hash)
	19	+ params[k] = sanitize_params_hash(v)
	20	+ end
	21	+ }
	22	+ end
23	23
24		- # If the parameter was an array,
25		- # try to sanitize each element in the array
26		- def sanitize_array(array)
27		- array.map! { \|e\|
28		- if e.is_a?(String)
29		- sanitize_param e
30		- end
31		- }
32		- return array
33		- end
	24	+ # If the parameter was an array,
	25	+ # try to sanitize each element in the array
	26	+ def sanitize_array(array)
	27	+ array.map! { \|e\|
	28	+ if e.is_a?(String)
	29	+ sanitize_param e
	30	+ end
	31	+ }
	32	+ return array
	33	+ end
34	34
35		- # Santitize a single value
36		- def sanitize_param(value)
37		- allowed_tags = %w(a acronym b strong i em li ul ol h1 h2 h3 h4 h5 h6 blockquote br cite sub sup ins p)
38		- ActionController::Base.helpers.sanitize(value, tags: allowed_tags, attributes: %w(href title))
39		- end
	35	+ # Santitize a single value
	36	+ def sanitize_param(value)
	37	+ allowed_tags = %w(a acronym b strong i em li ul ol h1 h2 h3 h4 h5 h6 blockquote br cite sub sup ins p)
	38	+ ActionController::Base.helpers.sanitize(value, tags: allowed_tags, attributes: %w(href title))
	39	+ end
40	40
41		-end
	41	+end
...	...
...	...	@@ -779,6 +779,30 @@ class SearchControllerTest < ActionController::TestCase
779	779	assert_equivalent [t1,t2,c1,c2,c3,c4] , assigns(:searches)[:communities][:results]
780	780	end
781	781
	782	+ should 'not raise an exception if tag query contains accented latin characters' do
	783	+ tag_query = 'àáâãäå'
	784	+ assert_nothing_raised(NoMethodError) { get :tag, :tag => tag_query }
	785	+ end
	786	+
	787	+ should 'not allow query injection' do
	788	+ injection = '<iMg SrC=x OnErRoR=document.documentElement.innerHTML=1>SearchParam'
	789	+ get :tag, :tag => injection
	790	+ tag = assigns(:tag)
	791	+ assert !tag.upcase.include?('IMG')
	792	+ assert tag.include?('SearchParam')
	793	+ end
	794	+
	795	+ should 'not allow query injection in array' do
	796	+ injection = ['<iMg SrC=x OnErRoR=document.documentElement.innerHTML=1>',
	797	+ '<script>document.innerHTML = \'x\'</script>']
	798	+ get :tag, :tag => injection
	799	+ tag = assigns(:tag)
	800	+ tag.each { \|t\|
	801	+ assert !t.upcase.include?('IMG')
	802	+ assert !t.upcase.include?('SCRIPT')
	803	+ }
	804	+ end
	805	+
782	806	protected
783	807
784	808	def create_event(profile, options)
...	...