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

[Aaron Patterson]

# 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/

[Aaron Patterson]

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.

> From cbdb7d367c4f15ecb85c308a0d78f61d629a74c1 Mon Sep 17 00:00:00 2001
> From: Andrew Carpenter <and...@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 <and...@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 <and...@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