Merge branch 'master-sec'

* master-sec:
  fix protocol checking in sanitization [CVE-2013-1857]
  JDOM XXE Protection [CVE-2013-1856]
  fix incorrect ^$ usage leading to XSS in sanitize_css [CVE-2013-1855]
  stop calling to_sym when building arel nodes [CVE-2013-1854]

9 files changed, 81 insertions, 18 deletions

diff --git a/actionpack/lib/action_view/vendor/html-scanner/html/sanitizer.rb b/actionpack/lib/action_view/vendor/html-scanner/html/sanitizer.rb
index 6b4ececda2..30b6b8b141 100644
--- a/actionpack/lib/action_view/vendor/html-scanner/html/sanitizer.rb
+++ b/actionpack/lib/action_view/vendor/html-scanner/html/sanitizer.rb
@@ -77,7 +77,7 @@ module HTML
 
     # A regular expression of the valid characters used to separate protocols like
     # the ':' in 'http://foo.com'
-    self.protocol_separator     = /:|(&#0*58)|(&#x70)|(%|%)3A/
+    self.protocol_separator     = /:|(&#0*58)|(&#x70)|(&#x0*3a)|(%|%)3A/i
 
     # Specifies a Set of HTML attributes that can have URIs.
     self.uri_attributes         = Set.new(%w(href src cite action longdesc xlink:href lowsrc))
@@ -121,8 +121,8 @@ module HTML
       style = style.to_s.gsub(/url\s*\(\s*[^\s)]+?\s*\)\s*/, ' ')
 
       # gauntlet
-      if style !~ /^([:,;#%.\sa-zA-Z0-9!]|\w-\w|\'[\s\w]+\'|\"[\s\w]+\"|\([\d,\s]+\))*$/ ||
-          style !~ /^(\s*[-\w]+\s*:\s*[^:;]*(;|$)\s*)*$/
+      if style !~ /\A([:,;#%.\sa-zA-Z0-9!]|\w-\w|\'[\s\w]+\'|\"[\s\w]+\"|\([\d,\s]+\))*\z/ ||
+          style !~ /\A(\s*[-\w]+\s*:\s*[^:;]*(;|$)\s*)*\z/
         return ''
       end
 
@@ -133,7 +133,7 @@ module HTML
         elsif shorthand_css_properties.include?(prop.split('-')[0].downcase)
           unless val.split().any? do |keyword|
             !allowed_css_keywords.include?(keyword) &&
-              keyword !~ /^(#[0-9a-f]+|rgb\(\d+%?,\d*%?,?\d*%?\)?|\d{0,2}\.?\d{0,2}(cm|em|ex|in|mm|pc|pt|px|%|,|\))?)$/
+              keyword !~ /\A(#[0-9a-f]+|rgb\(\d+%?,\d*%?,?\d*%?\)?|\d{0,2}\.?\d{0,2}(cm|em|ex|in|mm|pc|pt|px|%|,|\))?)\z/
           end
             clean << prop + ': ' + val + ';'
           end
@@ -182,7 +182,7 @@ module HTML
 
     def contains_bad_protocols?(attr_name, value)
       uri_attributes.include?(attr_name) &&
-      (value =~ /(^[^\/:]*):|(&#0*58)|(&#x70)|(%|&#37;)3A/ && !allowed_protocols.include?(value.split(protocol_separator).first.downcase.strip))
+      (value =~ /(^[^\/:]*):|(&#0*58)|(&#x70)|(&#x0*3a)|(%|&#37;)3A/i && !allowed_protocols.include?(value.split(protocol_separator).first.downcase.strip))
     end
   end
 end
diff --git a/actionpack/test/template/html-scanner/sanitizer_test.rb b/actionpack/test/template/html-scanner/sanitizer_test.rb
index d9b57776c9..b1c1b83807 100644
--- a/actionpack/test/template/html-scanner/sanitizer_test.rb
+++ b/actionpack/test/template/html-scanner/sanitizer_test.rb
@@ -200,6 +200,7 @@ class SanitizerTest < ActionController::TestCase
    %(<IMG SRC="jav&#x0A;ascript:alert('XSS');">),
    %(<IMG SRC="jav&#x0D;ascript:alert('XSS');">),
    %(<IMG SRC=" &#14;  javascript:alert('XSS');">),
+   %(<IMG SRC="javascript&#x3a;alert('XSS');">),
    %(<IMG SRC=`javascript:alert("RSnake says, 'XSS'")`>)].each_with_index do |img_hack, i|
     define_method "test_should_not_fall_for_xss_image_hack_#{i+1}" do
       assert_sanitized img_hack, "<img>"
@@ -279,6 +280,11 @@ class SanitizerTest < ActionController::TestCase
     assert_equal '', sanitize_css(raw)
   end
 
+  def test_should_sanitize_across_newlines
+    raw = %(\nwidth:\nexpression(alert('XSS'));\n)
+    assert_equal '', sanitize_css(raw)
+  end
+
   def test_should_sanitize_img_vbscript
     assert_sanitized %(<img src='vbscript:msgbox("XSS")' />), '<img />'
   end
@@ -299,6 +305,15 @@ class SanitizerTest < ActionController::TestCase
     assert_sanitized "<span class=\"\\", "<span class=\"\\\">"
   end
 
+  def test_x03a
+    assert_sanitized %(<a href="javascript&#x3a;alert('XSS');">), "<a>"
+    assert_sanitized %(<a href="javascript&#x003a;alert('XSS');">), "<a>"
+    assert_sanitized %(<a href="http&#x3a;//legit">), %(<a href="http://legit">)
+    assert_sanitized %(<a href="javascript&#x3A;alert('XSS');">), "<a>"
+    assert_sanitized %(<a href="javascript&#x003A;alert('XSS');">), "<a>"
+    assert_sanitized %(<a href="http&#x3A;//legit">), %(<a href="http://legit">)
+  end
+
 protected
   def assert_sanitized(input, expected = nil)
     @sanitizer ||= HTML::WhiteListSanitizer.new
diff --git a/activerecord/lib/active_record/relation/predicate_builder.rb b/activerecord/lib/active_record/relation/predicate_builder.rb
index bd783a94cf..f44d46d15b 100644
--- a/activerecord/lib/active_record/relation/predicate_builder.rb
+++ b/activerecord/lib/active_record/relation/predicate_builder.rb
@@ -48,7 +48,7 @@ module ActiveRecord
         column = reflection.foreign_key
       end
 
-      queries << build(table[column.to_sym], value)
+      queries << build(table[column], value)
       queries
     end
 
diff --git a/activerecord/test/cases/relation/where_chain_test.rb b/activerecord/test/cases/relation/where_chain_test.rb
index 8ce44636b4..92d1e013e8 100644
--- a/activerecord/test/cases/relation/where_chain_test.rb
+++ b/activerecord/test/cases/relation/where_chain_test.rb
@@ -6,26 +6,31 @@ module ActiveRecord
   class WhereChainTest < ActiveRecord::TestCase
     fixtures :posts
 
+    def setup
+      super
+      @name = 'title'
+    end
+
     def test_not_eq
-      expected = Arel::Nodes::NotEqual.new(Post.arel_table[:title], 'hello')
+      expected = Arel::Nodes::NotEqual.new(Post.arel_table[@name], 'hello')
       relation = Post.where.not(title: 'hello')
       assert_equal([expected], relation.where_values)
     end
 
     def test_not_null
-      expected = Arel::Nodes::NotEqual.new(Post.arel_table[:title], nil)
+      expected = Arel::Nodes::NotEqual.new(Post.arel_table[@name], nil)
       relation = Post.where.not(title: nil)
       assert_equal([expected], relation.where_values)
     end
 
     def test_not_in
-      expected = Arel::Nodes::NotIn.new(Post.arel_table[:title], %w[hello goodbye])
+      expected = Arel::Nodes::NotIn.new(Post.arel_table[@name], %w[hello goodbye])
       relation = Post.where.not(title: %w[hello goodbye])
       assert_equal([expected], relation.where_values)
     end
 
     def test_association_not_eq
-      expected = Arel::Nodes::NotEqual.new(Comment.arel_table[:title], 'hello')
+      expected = Arel::Nodes::NotEqual.new(Comment.arel_table[@name], 'hello')
       relation = Post.joins(:comments).where.not(comments: {title: 'hello'})
       assert_equal(expected.to_sql, relation.where_values.first.to_sql)
     end
@@ -33,20 +38,20 @@ module ActiveRecord
     def test_not_eq_with_preceding_where
       relation = Post.where(title: 'hello').where.not(title: 'world')
 
-      expected = Arel::Nodes::Equality.new(Post.arel_table[:title], 'hello')
+      expected = Arel::Nodes::Equality.new(Post.arel_table[@name], 'hello')
       assert_equal(expected, relation.where_values.first)
 
-      expected = Arel::Nodes::NotEqual.new(Post.arel_table[:title], 'world')
+      expected = Arel::Nodes::NotEqual.new(Post.arel_table[@name], 'world')
       assert_equal(expected, relation.where_values.last)
     end
 
     def test_not_eq_with_succeeding_where
       relation = Post.where.not(title: 'hello').where(title: 'world')
 
-      expected = Arel::Nodes::NotEqual.new(Post.arel_table[:title], 'hello')
+      expected = Arel::Nodes::NotEqual.new(Post.arel_table[@name], 'hello')
       assert_equal(expected, relation.where_values.first)
 
-      expected = Arel::Nodes::Equality.new(Post.arel_table[:title], 'world')
+      expected = Arel::Nodes::Equality.new(Post.arel_table[@name], 'world')
       assert_equal(expected, relation.where_values.last)
     end
 
@@ -65,10 +70,10 @@ module ActiveRecord
     def test_chaining_multiple
       relation = Post.where.not(author_id: [1, 2]).where.not(title: 'ruby on rails')
 
-      expected = Arel::Nodes::NotIn.new(Post.arel_table[:author_id], [1, 2])
+      expected = Arel::Nodes::NotIn.new(Post.arel_table['author_id'], [1, 2])
       assert_equal(expected, relation.where_values[0])
 
-      expected = Arel::Nodes::NotEqual.new(Post.arel_table[:title], 'ruby on rails')
+      expected = Arel::Nodes::NotEqual.new(Post.arel_table[@name], 'ruby on rails')
       assert_equal(expected, relation.where_values[1])
     end
   end
diff --git a/activesupport/lib/active_support/xml_mini/jdom.rb b/activesupport/lib/active_support/xml_mini/jdom.rb
index 4551dd2f2d..27c64c4dca 100644
--- a/activesupport/lib/active_support/xml_mini/jdom.rb
+++ b/activesupport/lib/active_support/xml_mini/jdom.rb
@@ -37,6 +37,12 @@ module ActiveSupport
         {}
       else
         @dbf = DocumentBuilderFactory.new_instance
+        # secure processing of java xml
+        # http://www.ibm.com/developerworks/xml/library/x-tipcfsx/index.html
+        @dbf.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false)
+        @dbf.setFeature("http://xml.org/sax/features/external-general-entities", false)
+        @dbf.setFeature("http://xml.org/sax/features/external-parameter-entities", false)
+        @dbf.setFeature(javax.xml.XMLConstants::FEATURE_SECURE_PROCESSING, true)
         xml_string_reader = StringReader.new(data)
         xml_input_source = InputSource.new(xml_string_reader)
         doc = @dbf.new_document_builder.parse(xml_input_source)
diff --git a/activesupport/test/fixtures/xml/jdom_doctype.dtd b/activesupport/test/fixtures/xml/jdom_doctype.dtd
new file mode 100644
index 0000000000..89480496ef
--- /dev/null
+++ b/activesupport/test/fixtures/xml/jdom_doctype.dtd
@@ -0,0 +1 @@
+<!ENTITY a "external entity">
diff --git a/activesupport/test/fixtures/xml/jdom_entities.txt b/activesupport/test/fixtures/xml/jdom_entities.txt
new file mode 100644
index 0000000000..0337fdaa08
--- /dev/null
+++ b/activesupport/test/fixtures/xml/jdom_entities.txt
@@ -0,0 +1 @@
+<!ENTITY a "hello">
diff --git a/activesupport/test/fixtures/xml/jdom_include.txt b/activesupport/test/fixtures/xml/jdom_include.txt
new file mode 100644
index 0000000000..239ca3afaf
--- /dev/null
+++ b/activesupport/test/fixtures/xml/jdom_include.txt
@@ -0,0 +1 @@
+include me
diff --git a/activesupport/test/xml_mini/jdom_engine_test.rb b/activesupport/test/xml_mini/jdom_engine_test.rb
index f77d78d42c..4d44b72df6 100644
--- a/activesupport/test/xml_mini/jdom_engine_test.rb
+++ b/activesupport/test/xml_mini/jdom_engine_test.rb
@@ -3,9 +3,12 @@ if RUBY_PLATFORM =~ /java/
   require 'active_support/xml_mini'
   require 'active_support/core_ext/hash/conversions'
 
+
   class JDOMEngineTest < ActiveSupport::TestCase
     include ActiveSupport
 
+    FILES_DIR = File.dirname(__FILE__) + '/../fixtures/xml'
+
     def setup
       @default_backend = XmlMini.backend
       XmlMini.backend = 'JDOM'
@@ -30,10 +33,41 @@ if RUBY_PLATFORM =~ /java/
        assert_equal 'image/png', file.content_type
     end
 
+    def test_not_allowed_to_expand_entities_to_files
+      attack_xml = <<-EOT
+      <!DOCTYPE member [
+        <!ENTITY a SYSTEM "file://#{FILES_DIR}/jdom_include.txt">
+      ]>
+      <member>x&a;</member>
+      EOT
+      assert_equal 'x', Hash.from_xml(attack_xml)["member"]
+    end
+
+  def test_not_allowed_to_expand_parameter_entities_to_files
+      attack_xml = <<-EOT
+      <!DOCTYPE member [
+        <!ENTITY % b SYSTEM "file://#{FILES_DIR}/jdom_entities.txt">
+        %b;
+      ]>
+      <member>x&a;</member>
+      EOT
+      assert_raise Java::OrgXmlSax::SAXParseException do
+        assert_equal 'x', Hash.from_xml(attack_xml)["member"]
+      end
+    end
+
+
+    def test_not_allowed_to_load_external_doctypes
+      attack_xml = <<-EOT
+      <!DOCTYPE member SYSTEM "file://#{FILES_DIR}/jdom_doctype.dtd">
+      <member>x&a;</member>
+      EOT
+      assert_equal 'x', Hash.from_xml(attack_xml)["member"]
+    end
+
     def test_exception_thrown_on_expansion_attack
-      assert_raise NativeException do
+      assert_raise Java::OrgXmlSax::SAXParseException do
         attack_xml = <<-EOT
-      <?xml version="1.0" encoding="UTF-8"?>
       <!DOCTYPE member [
         <!ENTITY a "&b;&b;&b;&b;&b;&b;&b;&b;&b;&b;">
         <!ENTITY b "&c;&c;&c;&c;&c;&c;&c;&c;&c;&c;">