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 [email protected]
commit c15a8c2e95c7098d2372e10be0a4381c36d4fd3b Author: Gabe da Silveira <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
