Re: [CVE-2016-6316] Possible XSS Vulnerability in Action View

[Aaron Patterson <tenderlove () ruby-lang org>]

Hi,

There is a bug in the patch for the 3.2 series.  I've attached a
combined patch here.  The attached patch is a combination of these two
patches:

  https://github.com/rails/rails/commit/4bcccf5ecd81a6272479537911b7d9760c5be164
  https://github.com/rails/rails/commit/5aabcf25caefbe84f656256a9d3e7fc0c9e14ecc

Sorry for the problems.

On Thu, Aug 11, 2016 at 10:52:09AM -0700, Aaron Patterson wrote:
> # Possible XSS Vulnerability in Action View
>
> There is a possible XSS vulnerability in Action View.  Text declared as "HTML
> safe" will not have quotes escaped when used as attribute values in tag
> helpers.  This vulnerability has been assigned the CVE identifier
> CVE-2016-6316.
>
> Versions Affected:  >= 3.0.0.
> Not affected:       < 3.0.0
> Fixed Versions:     5.0.0.1, 4.2.7.1, 3.2.22.3
>
> Impact
> ------
> Text declared as "HTML safe" when passed as an attribute value to a tag helper
> will not have quotes escaped which can lead to an XSS attack.  Impacted code
> looks something like this:
>
> ```
> content_tag(:div, "hi", title: user_input.html_safe)
> ```
>
> Some helpers like the `sanitize` helper will automatically mark strings as
> "HTML safe", so impacted code could also look something like this:
>
> ```
> content_tag(:div, "hi", title: sanitize(user_input))
> ```
>
> All users running an affected release should either upgrade or use one of the
> workarounds immediately.
>
> Releases
> --------
> The FIXED releases are available at the normal locations.
>
> Workarounds
> -----------
> You can work around this issue by either *not* marking arbitrary user input as
> safe, or by manually escaping quotes like this:
>
> ```
> def escape_quotes(value)
>   value.gsub(/"/, '&quot;'.freeze)
> end
>
> content_tag(:div, "hi", title: escape_quotes(sanitize(user_input)))
> ```
>
> Patches
> -------
> To aid users who aren't able to upgrade immediately we have provided patches for
> the two supported release series. They are in git-am format and consist of a
> single changeset.
>
> * 3-2-attribute-xss.patch - Patch for 3.2 series
> * 4-2-attribute-xss.patch - Patch for 4.2 series
> * 5-0-attribute-xss.patch - Patch for 5.0 series
>
> Please note that only the 5.0.x and 4.2.x series are supported at present. Users
> of earlier unsupported releases are advised to upgrade as soon as possible as we
> cannot guarantee the continued availability of security fixes for unsupported
> releases.
>
> Credits
> -------
>
> Thanks to Andrew Carpenter of Critical Juncture for reporting this issue and
> sending a patch to fix it!
>
> -- 
> Aaron Patterson
> http://tenderlovemaking.com/

> From cbdb7d367c4f15ecb85c308a0d78f61d629a74c1 Mon Sep 17 00:00:00 2001
> From: Andrew Carpenter <andrew () criticaljuncture org>
> Date: Thu, 28 Jul 2016 16:12:21 -0700
> Subject: [PATCH] ensure tag/content_tag escapes " in attribute vals
>
> Many helpers mark content as HTML-safe without escaping double quotes -- including `sanitize`. Regardless of whether 
> or not the attribute values are HTML-escaped, we want to be sure they don't include double quotes, as that can cause 
> XSS issues. For example: `content_tag(:div, "foo", title: sanitize('" onmouseover="alert(1);//'))`
>
> CVE-2016-6316
> ---
>  actionpack/lib/action_view/helpers/tag_helper.rb | 15 +++++++++++----
>  actionpack/test/template/tag_helper_test.rb      | 10 ++++++++++
>  2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/actionpack/lib/action_view/helpers/tag_helper.rb b/actionpack/lib/action_view/helpers/tag_helper.rb
> index 7f58a27..34741b8 100644
> --- a/actionpack/lib/action_view/helpers/tag_helper.rb
> +++ b/actionpack/lib/action_view/helpers/tag_helper.rb
> @@ -141,20 +141,27 @@ module ActionView
>                    unless v.is_a?(String) || v.is_a?(Symbol) || v.is_a?(BigDecimal)
>                      v = v.to_json
>                    end
> -                  v = ERB::Util.html_escape(v) if escape
> -                  attrs << %(data-#{k.to_s.dasherize}="#{v}")
> +                  attrs << tag_option("data-#{k.to_s.dasherize}", v, escape)
>                  end
>                elsif BOOLEAN_ATTRIBUTES.include?(key)
>                  attrs << %(#{key}="#{key}") if value
>                elsif !value.nil?
>                  final_value = value.is_a?(Array) ? value.join(" ") : value
> -                final_value = ERB::Util.html_escape(final_value) if escape
> -                attrs << %(#{key}="#{final_value}")
> +                attrs << tag_option(key, value, escape)
>                end
>              end
>              " #{attrs.sort * ' '}".html_safe unless attrs.empty?
>            end
>          end
> +
> +        def tag_option(key, value, escape)
> +          if value.is_a?(Array)
> +            value = escape ? safe_join(value, " ") : value.join(" ")
> +          else
> +            value = escape ? ERB::Util.html_escape(value) : value
> +          end
> +          %(#{key}="#{value.gsub(/"/, '&quot;'.freeze)}")
> +        end
>      end
>    end
>  end
> diff --git a/actionpack/test/template/tag_helper_test.rb b/actionpack/test/template/tag_helper_test.rb
> index e362955..9c3d636 100644
> --- a/actionpack/test/template/tag_helper_test.rb
> +++ b/actionpack/test/template/tag_helper_test.rb
> @@ -101,6 +101,16 @@ class TagHelperTest < ActionView::TestCase
>      end
>    end
>  
> +  def test_tag_does_not_honor_html_safe_double_quotes_as_attributes
> +    assert_dom_equal '<p title="&quot;">content</p>',
> +      content_tag('p', "content", title: '"'.html_safe)
> +  end
> +
> +  def test_data_tag_does_not_honor_html_safe_double_quotes_as_attributes
> +    assert_dom_equal '<p data-title="&quot;">content</p>',
> +      content_tag('p', "content", data: { title: '"'.html_safe })
> +  end
> +
>    def test_skip_invalid_escaped_attributes
>      ['&1;', '&#1dfa3;', '& #123;'].each do |escaped|
>        assert_equal %(<a href="#{escaped.gsub(/&/, '&amp;')}" />), tag('a', :href => escaped)
> -- 
> 2.8.1

> From e4abbc8636e1300d14b1fd7e3f05e4e25bc8289e Mon Sep 17 00:00:00 2001
> From: Andrew Carpenter <andrew () criticaljuncture org>
> Date: Thu, 28 Jul 2016 16:12:21 -0700
> Subject: [PATCH 1/2] ensure tag/content_tag escapes " in attribute vals
>
> Many helpers mark content as HTML-safe without escaping double quotes -- including `sanitize`. Regardless of whether 
> or not the attribute values are HTML-escaped, we want to be sure they don't include double quotes, as that can cause 
> XSS issues. For example: `content_tag(:div, "foo", title: sanitize('" onmouseover="alert(1);//'))`
>
> CVE-2016-6316
> ---
>  actionview/lib/action_view/helpers/tag_helper.rb |  2 +-
>  actionview/test/template/tag_helper_test.rb      | 10 ++++++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/actionview/lib/action_view/helpers/tag_helper.rb b/actionview/lib/action_view/helpers/tag_helper.rb
> index b203857..f09595d 100644
> --- a/actionview/lib/action_view/helpers/tag_helper.rb
> +++ b/actionview/lib/action_view/helpers/tag_helper.rb
> @@ -181,7 +181,7 @@ module ActionView
>            else
>              value = escape ? ERB::Util.unwrapped_html_escape(value) : value
>            end
> -          %(#{key}="#{value}")
> +          %(#{key}="#{value.gsub(/"/, '&quot;'.freeze)}")
>          end
>      end
>    end
> diff --git a/actionview/test/template/tag_helper_test.rb b/actionview/test/template/tag_helper_test.rb
> index ce89d57..8332dd0 100644
> --- a/actionview/test/template/tag_helper_test.rb
> +++ b/actionview/test/template/tag_helper_test.rb
> @@ -140,6 +140,16 @@ class TagHelperTest < ActionView::TestCase
>      assert_equal '<p class="song> play&gt;" />', str
>    end
>  
> +  def test_tag_does_not_honor_html_safe_double_quotes_as_attributes
> +    assert_dom_equal '<p title="&quot;">content</p>',
> +      content_tag('p', "content", title: '"'.html_safe)
> +  end
> +
> +  def test_data_tag_does_not_honor_html_safe_double_quotes_as_attributes
> +    assert_dom_equal '<p data-title="&quot;">content</p>',
> +      content_tag('p', "content", data: { title: '"'.html_safe })
> +  end
> +
>    def test_skip_invalid_escaped_attributes
>      ['&1;', '&#1dfa3;', '& #123;'].each do |escaped|
>        assert_equal %(<a href="#{escaped.gsub(/&/, '&amp;')}" />), tag('a', :href => escaped)
> -- 
> 2.8.1

> From 0a3487c7a06a60569817266ffdd39ef0409839d4 Mon Sep 17 00:00:00 2001
> From: Andrew Carpenter <andrew () criticaljuncture org>
> Date: Thu, 28 Jul 2016 16:12:21 -0700
> Subject: [PATCH] ensure tag/content_tag escapes " in attribute vals
>
> Many helpers mark content as HTML-safe without escaping double quotes -- including `sanitize`. Regardless of whether 
> or not the attribute values are HTML-escaped, we want to be sure they don't include double quotes, as that can cause 
> XSS issues. For example: `content_tag(:div, "foo", title: sanitize('" onmouseover="alert(1);//'))`
>
> CVE-2016-6316
> ---
>  actionview/lib/action_view/helpers/tag_helper.rb |  2 +-
>  actionview/test/template/tag_helper_test.rb      | 12 +++++++++++-
>  2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/actionview/lib/action_view/helpers/tag_helper.rb b/actionview/lib/action_view/helpers/tag_helper.rb
> index 42e7358..ac26c29 100644
> --- a/actionview/lib/action_view/helpers/tag_helper.rb
> +++ b/actionview/lib/action_view/helpers/tag_helper.rb
> @@ -189,7 +189,7 @@ def tag_option(key, value, escape)
>            else
>              value = escape ? ERB::Util.unwrapped_html_escape(value) : value
>            end
> -          %(#{key}="#{value}")
> +          %(#{key}="#{value.gsub(/"/, '&quot;'.freeze)}")
>          end
>      end
>    end
> diff --git a/actionview/test/template/tag_helper_test.rb b/actionview/test/template/tag_helper_test.rb
> index f3956a3..fe5ec03 100644
> --- a/actionview/test/template/tag_helper_test.rb
> +++ b/actionview/test/template/tag_helper_test.rb
> @@ -150,6 +150,16 @@ def test_tag_honors_html_safe_with_escaped_array_class
>      assert_equal '<p class="song> play&gt;" />', str
>    end
>  
> +  def test_tag_does_not_honor_html_safe_double_quotes_as_attributes
> +    assert_dom_equal '<p title="&quot;">content</p>',
> +      content_tag('p', "content", title: '"'.html_safe)
> +  end
> +
> +  def test_data_tag_does_not_honor_html_safe_double_quotes_as_attributes
> +    assert_dom_equal '<p data-title="&quot;">content</p>',
> +      content_tag('p', "content", data: { title: '"'.html_safe })
> +  end
> +
>    def test_skip_invalid_escaped_attributes
>      ['&1;', '&#1dfa3;', '& #123;'].each do |escaped|
>        assert_equal %(<a href="#{escaped.gsub(/&/, '&amp;')}" />), tag('a', :href => escaped)
> @@ -177,6 +187,6 @@ def test_aria_attributes
>    def test_link_to_data_nil_equal
>      div_type1 = content_tag(:div, 'test', { 'data-tooltip' => nil })
>      div_type2 = content_tag(:div, 'test', { data: {tooltip: nil} })
> -    assert_dom_equal div_type1, div_type2 
> +    assert_dom_equal div_type1, div_type2
>    end
>  end
> -- 
> 2.8.1




-- 
Aaron Patterson
http://tenderlovemaking.com/

Attachment: 3-2-attribute-xss.patch
Description:

Attachment: _bin
Description: