Adam Majer
Sat, 30 Jan 2010 22:15:14 -0800
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 ad...@zombino.com
commit c15a8c2e95c7098d2372e10be0a4381c36d4fd3b
Author: Gabe da Silveira <g...@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 <mich...@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 <packageth...@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 <jer...@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