Problem/Motivation

Now that we have had an SA for Views 7.x-3.x, we opted to address the D8 vulnerability publicly.

http://drupalcode.org/project/views.git/commit/ddf8181bd13f69ffbeeee14ae... ( Google cache )
http://drupal.org/node/1948358

Proposed resolution

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Add automated tests Instructions
Add steps to reproduce the issue, or confirm if it can't be reproduced Novice Instructions

User interface changes

N/A

API changes

N/A

Files: 
CommentFileSizeAuthor
#35 patch-xss-views-1948418-34.patch9.26 KBwebflo
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,712 pass(es). View
#35 patch-xss-views-1948418-34.interdiff.txt2.45 KBwebflo
#33 patch-xss-views-1948418-33.patch10.34 KBwebflo
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,495 pass(es), 4 fail(s), and 0 exception(s). View
#31 patch-xss-views-1948418-31.patch8.37 KBwebflo
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,714 pass(es), 11 fail(s), and 0 exception(s). View
#30 views.view_.sa_contrib_2013_035.yml6.66 KBwebflo
#27 address-1948418-27.patch4.32 KBgaurav.goyal
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,546 pass(es). View
#23 address-1948418-23.patch4.32 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,309 pass(es). View
#23 interdiff-21-23.txt682 bytescilefen
#21 drupal-1948418-21.patch4.32 KBamitgoyal
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,327 pass(es), 3 fail(s), and 0 exception(s). View
#18 interdiff-16-18.txt1.15 KBmartin107
#18 drupal-1948418-18.patch4.33 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-1948418-18.patch. Unable to apply patch. See the log in the details link for more information. View
#16 drupal-1948418-16.patch4.56 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/views/src/Plugin/views/field/FieldPluginBase.php. View
#10 drupal-1948418-10.patch5.12 KBgalooph
PASSED: [[SimpleTest]]: [MySQL] 58,849 pass(es). View
#4 drupal-1948418-4.patch4.16 KBgalooph
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#3 drupal-1948418-3.patch4.78 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 53,281 pass(es). View
#1 xss_admin_config_pages_d8-82088.patch4.34 KBdstol
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch xss_admin_config_pages_d8-82088.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

dstol’s picture

Status: Active » Needs review
FileSize
4.34 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch xss_admin_config_pages_d8-82088.patch. Unable to apply patch. See the log in the details link for more information. View

Here's a potential patch from s.d.o. I am not the author of it. All the credit is due to grisendo

Status: Needs review » Needs work

The last submitted patch, xss_admin_config_pages_d8-82088.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.78 KB
PASSED: [[SimpleTest]]: [MySQL] 53,281 pass(es). View

Rerolled that it can be committed.

galooph’s picture

FileSize
4.16 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Rerolled so that it will apply - file locations have changed since the last patch.

Not too sure how this should be done now:

 function theme_views_ui_view_info($variables) {
   $output = '';
-  $output .= '<h3 class="views-ui-view-title">' . $variables['title'] . "</h3>\n";
+  $output .= '<h3 class="views-ui-view-title">' . check_plain($variables['title']) . "</h3>\n";
   $output .= '<div class="views-ui-view-displays">' . $variables['displays'] . "</div>\n";
   return $output;
 }

Do we use the striptags function in twig. I've done <h3 class="views-ui-view-title views-table-filter-text-source">{{ title | striptags }}</h3> for now, but not sure if that's correct!

Status: Needs review » Needs work

The last submitted patch, drupal-1948418-4.patch, failed testing.

galooph’s picture

Status: Needs work » Needs review

#4: drupal-1948418-4.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-1948418-4.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

Instead of check_plain() we should better use String::checkPlain().

Status: Needs review » Needs work

The last submitted patch, drupal-1948418-4.patch, failed testing.

galooph’s picture

FileSize
5.12 KB
PASSED: [[SimpleTest]]: [MySQL] 58,849 pass(es). View

Thanks for the review, dawehner.

I've changed the patch to use String::checkPlain() and I've used it in template_preprocess_views_ui_view_info() to sanitise the title variable there, instead of in the twig template.

galooph’s picture

Status: Needs work » Needs review
xjm’s picture

Priority: Normal » Critical
Issue summary: View changes

Forward-porting an SA sounds critical.

xjm’s picture

Issue tags: +Needs tests
dawehner’s picture

I wonder whether we actually still needs due the fact that auto-espacing landed in core.

cilefen’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
martin107’s picture

Status: Needs work » Needs review
FileSize
4.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/views/src/Plugin/views/field/FieldPluginBase.php. View

From #14 I suspect @dawehner might be correct, but don't want the "Needs roll" tags to slow up the review process, on an already slow moving critical.

Status: Needs review » Needs work

The last submitted patch, 16: drupal-1948418-16.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
4.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-1948418-18.patch. Unable to apply patch. See the log in the details link for more information. View
1.15 KB

Fixed up 2 things.

martin107 queued 18: drupal-1948418-18.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 18: drupal-1948418-18.patch, failed testing.

amitgoyal’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,327 pass(es), 3 fail(s), and 0 exception(s). View

Reroll of #18.

Status: Needs review » Needs work

The last submitted patch, 21: drupal-1948418-21.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
682 bytes
4.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,309 pass(es). View
--- b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -989,7 +989,7 @@
       if (!empty($handler->options['relationship']) && !empty($relationships[$handler->options['relationship']])) {
         $options[$id] = '(' . $relationships[$handler->options['relationship']] . ') ' . $options[$id];
       }
-      $options[$id] = String::checkPlain($label);
+      $options[$id] = String::checkPlain($options[$id]);
     }
     return $options;
   }

I am not sure how this was supposed to work.

catch’s picture

Issue tags: +D8 upgrade path, +beta target

Tagging with D8 upgrade path, see issue summary of #2341575: [meta] Provide a beta to beta/rc upgrade path for why.

xjm’s picture

gaurav.goyal’s picture

Assigned: Unassigned » gaurav.goyal
Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch needs a reroll, changing the status to needs work and adding the tag Needs reroll

gaurav.goyal’s picture

Assigned: gaurav.goyal » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,546 pass(es). View

Patch rerolled,

Thanks a ton @cilefen for mentoring me on this reroll.

Removing needs reroll tag.

xjm’s picture

Issue summary: View changes

So we need to:

  1. Determine whether this is still reproducible in D8 HEAD.
  2. If so, write an automated test that fails against HEAD and add it to the patch: https://drupal.org/contributor-tasks/write-tests
  3. If not, still write a passing automated test that demonstrates that it's not a problem.

In #3, we've discussed elsewhere that we should actually be removing early checkPlain() calls, so my recommendation would be to add only the test in that case.

chx’s picture

Issue summary: View changes
webflo’s picture

Assigned: Unassigned » webflo
FileSize
6.66 KB

Build a view in standard profile that exposes the vulnerabilities. But D8 is atm not vulnerable because some of this is nuked by twig autoescape and also HandlerBase::adminLabel contains a call to String::checkPlain.

webflo’s picture

FileSize
8.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,714 pass(es), 11 fail(s), and 0 exception(s). View

The test proves that Views UI is not vulnerable to SA-CONTRIB-2013-035. But it shows another bug, we double escape the admin labels in views ui.

Status: Needs review » Needs work

The last submitted patch, 31: patch-xss-views-1948418-31.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
10.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,495 pass(es), 4 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 33: patch-xss-views-1948418-33.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
2.45 KB
9.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,712 pass(es). View

The exported view was not valid because the Views Wizard added fields without plugin_id to the view.

jibran’s picture

Issue tags: +VDC
dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/views_ui/tests/modules/views_ui_test/config/install/views.view.sa_contrib_2013_035.yml
@@ -0,0 +1,215 @@
+      title: '<marquee>VIEWS TITLE</marquee>'
+  page_1:

More marquee!

The follow up is filled, so let's get this one in.

+++ b/core/modules/views_ui/src/Tests/XssTest.php
@@ -0,0 +1,34 @@
+    $this->assertRaw('&lt;script&gt;alert(&quot;foo&quot;);&lt;/script&gt;, &lt;marquee&gt;test&lt;/marquee&gt;', 'The view tag is properly escaped.');
...
+    $this->assertRaw('&amp;lt;marquee&amp;gt;test&amp;lt;/marquee&amp;gt;', 'Field admin label is properly escaped.');
...
+    $this->assertRaw('[title] == &amp;lt;marquee&amp;gt;test&amp;lt;/marquee&amp;gt;', 'Token label is properly escaped.');
+    $this->assertRaw('[title_1] == &amp;lt;script&amp;gt;alert(&amp;quot;XSS&amp;quot;)&amp;lt;/script&amp;gt;', 'Token label is properly escaped.');

Have you considered to check for String::checkPlain('actual-readable-string') instead?

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs tests

Committed 6efeaf7 and pushed to 8.0.x. Thanks!

  • alexpott committed 6efeaf7 on 8.0.x
    Issue #1948418 by webflo, martin107, galooph, cilefen, gaurav.goyal,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jcnventura’s picture