Rails XSS hole
Hello,
Not sure if this is best place to ask this.
Rails has a small XSS bug #558685 [1]. This bug is assigned
CVE-2009-4214. The other bug in the bug report, CVE-2008-7248, has
been already corrected in both Stable, Testing and Sid.
I have prepared a package with the changes. The patch is attached
(patch1 - 1 line fix). One of the unit tests added in the security
patch exposes another bug in rails in stable. This bug can be easily
fixed via the 2nd patch (patch2, attached - 6 line fix). Is it
possible to include both of these patches into a security change or
only the security patch is permitted? Without the 2nd patch, it *may*
be possible for 3rd parties to crash any rails application that
attempts to sanitize some input data.
I'll need someone from security team to contact me regarding this
update. (ie. when to upload, etc.)
Please CC me as I'm not on the list.
- Adam
[1] - http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=558685
--
Adam Majer
adamm@zombino.com
commit c15a8c2e95c7098d2372e10be0a4381c36d4fd3b
Author: Gabe da Silveira <gabe@websaviour.com>
Date: Mon Nov 16 21:17:35 2009 -0800
Make sure strip_tags removes tags which start with a non-printable character
Signed-off-by: Michael Koziarski <michael@koziarski.com>
diff --git a/actionpack/lib/action_controller/vendor/html-scanner/html/node.rb b/actionpack/lib/action_controller/vendor/html-scanner/html/node.rb
index 472c5b2..9dc36b2 100644
--- a/actionpack/lib/action_controller/vendor/html-scanner/html/node.rb
+++ b/actionpack/lib/action_controller/vendor/html-scanner/html/node.rb
@@ -155,7 +155,7 @@ module HTML #:nodoc:
end
closing = ( scanner.scan(/\//) ? :close : nil )
- return Text.new(parent, line, pos, content) unless name = scanner.scan(/[\w:-]+/)
+ return Text.new(parent, line, pos, content) unless name = scanner.scan(/[-:\w\x00-\x09\x0b-\x0c\x0e-\x1f]+/)
name.downcase!
unless closing
diff --git a/actionpack/test/controller/html-scanner/sanitizer_test.rb b/actionpack/test/controller/html-scanner/sanitizer_test.rb
index db142f0..f1efd50 100644
--- a/actionpack/test/controller/html-scanner/sanitizer_test.rb
+++ b/actionpack/test/controller/html-scanner/sanitizer_test.rb
@@ -17,6 +17,9 @@ class SanitizerTest < Test::Unit::TestCase
%{This is a test.\n\n\nIt no longer contains any HTML.\n}, sanitizer.sanitize(
%{<title>This is <b>a <a href="" target="_blank">test</a></b>.</title>\n\n<!-- it has a comment -->\n\n<p>It no <b>longer <strong>contains <em>any <strike>HTML</strike></em>.</strong></b></p>\n}))
assert_equal "This has a here.", sanitizer.sanitize("This has a <!-- comment --> here.")
+ assert_equal "This has a here.", sanitizer.sanitize("This has a <![CDATA[<section>]]> here.")
+ assert_equal "This has an unclosed ", sanitizer.sanitize("This has an unclosed <![CDATA[<section>]] here...")
+ assert_equal "non printable char is a tag", sanitizer.sanitize("<\x07a href='/hello'>non printable char is a tag</a>")
[nil, '', ' '].each { |blank| assert_equal blank, sanitizer.sanitize(blank) }
end
commit 44cb1490ca8328bd1db160e6de48137332ad4d67
Author: Jeffrey Hardy <packagethief@gmail.com>
Date: Wed Oct 22 16:03:21 2008 -0400
Fix that HTML::Node.parse would blow up on unclosed CDATA sections.
If an unclosed CDATA section is encountered and parsing is strict, an
exception will be raised. Otherwise, we consider the remainder of the line to
be the section contents. This is consistent with HTML::Tokenizer#scan_tag.
Signed-off-by: Jeremy Kemper <jeremy@bitsweat.net>
diff --git a/actionpack/lib/action_controller/vendor/html-scanner/html/node.rb b/actionpack/lib/action_controller/vendor/html-scanner/html/node.rb
index 9dc36b2..594d37a 100644
--- a/actionpack/lib/action_controller/vendor/html-scanner/html/node.rb
+++ b/actionpack/lib/action_controller/vendor/html-scanner/html/node.rb
@@ -150,7 +150,14 @@ module HTML #:nodoc:
end
if scanner.skip(/!\[CDATA\[/)
- scanner.scan_until(/\]\]>/)
+ unless scanner.skip_until(/\]\]>/)
+ if strict
+ raise "expected ]]> (got #{scanner.rest.inspect} for #{content})"
+ else
+ scanner.skip_until(/\Z/)
+ end
+ end
+
return CDATA.new(parent, line, pos, scanner.pre_match.gsub(/<!\[CDATA\[/, ''))
end
diff --git a/actionpack/test/controller/html-scanner/node_test.rb b/actionpack/test/controller/html-scanner/node_test.rb
index 240f01a..b0df368 100644
--- a/actionpack/test/controller/html-scanner/node_test.rb
+++ b/actionpack/test/controller/html-scanner/node_test.rb
@@ -65,4 +65,25 @@ class NodeTest < Test::Unit::TestCase
assert_nothing_raised { node = HTML::Node.parse(nil,0,0,s,false) }
assert node.attributes.has_key?("onmouseover")
end
+
+ def test_parse_with_valid_cdata_section
+ s = "<![CDATA[<span>contents</span>]]>"
+ node = nil
+ assert_nothing_raised { node = HTML::Node.parse(nil,0,0,s,false) }
+ assert_kind_of HTML::CDATA, node
+ assert_equal '<span>contents</span>', node.content
+ end
+
+ def test_parse_strict_with_unterminated_cdata_section
+ s = "<![CDATA[neverending..."
+ assert_raise(RuntimeError) { HTML::Node.parse(nil,0,0,s) }
+ end
+
+ def test_parse_relaxed_with_unterminated_cdata_section
+ s = "<![CDATA[neverending..."
+ node = nil
+ assert_nothing_raised { node = HTML::Node.parse(nil,0,0,s,false) }
+ assert_kind_of HTML::CDATA, node
+ assert_equal 'neverending...', node.content
+ end
end
diff --git a/actionpack/test/controller/html-scanner/sanitizer_test.rb b/actionpack/test/controller/html-scanner/sanitizer_test.rb
index f1efd50..ebf5b0d 100644
--- a/actionpack/test/controller/html-scanner/sanitizer_test.rb
+++ b/actionpack/test/controller/html-scanner/sanitizer_test.rb
@@ -246,6 +246,14 @@ class SanitizerTest < Test::Unit::TestCase
assert_sanitized %(<img src='vbscript:msgbox("XSS")' />), '<img />'
end
+ def test_should_sanitize_cdata_section
+ assert_sanitized "<![CDATA[<span>section</span>]]>", "<![CDATA[<span>section</span>]]>"
+ end
+
+ def test_should_sanitize_unterminated_cdata_section
+ assert_sanitized "<![CDATA[<span>neverending...", "<![CDATA[<span>neverending...]]>"
+ end
+
protected
def assert_sanitized(input, expected = nil)
@sanitizer ||= HTML::WhiteListSanitizer.new
Reply to: