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

Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20160811193005.GB57149@TC.local>
Date: Thu, 11 Aug 2016 12:30:05 -0700
From: Aaron Patterson <tenderlove@...y-lang.org>
To: Aaron Patterson <tenderlove@...y-lang.org>
Cc: security@...e.de, rubyonrails-security@...glegroups.com,
	oss-security@...ts.openwall.com, ruby-security-ann@...glegroups.com
Subject: Re: [CVE-2016-6316] Possible XSS Vulnerability in Action View

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@...ticaljuncture.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@...ticaljuncture.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@...ticaljuncture.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/

View attachment "3-2-attribute-xss.patch" of type "text/plain" (3537 bytes)

Content of type "application/pgp-signature" skipped
Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.