Problem/Motivation

SA-CONTRIB-2013-082 was fixed for bean.module, but D8 has exactly the same problem with block_content (and maybe other custom block implementations as well)

To reproduce, just create a block content with <script>alert('booo')</script>. It's fine on the block content overview, it's fine on the block selection sidebar, but when you click on it, you get a boo dialog, because the block description is not escaped.

Proposed resolution

The question is whether admin_label is supposed to be a safe string, so that implementations, like block_content need to check it, like bean.module did in 7.x, or if we should enforce it in block.module and run them through checkPlain()/filterXss().

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#10 block_content_titles-2446995-10.patch4.38 KBtim.plunkett
#7 block_content_titles-2446995-7-PASS.patch3.59 KBtim.plunkett
#7 block_content_titles-2446995-7-FAIL.patch2.75 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Related issues: +#2419941: Check every Drupal 7 contrib SA that may affect Drupal 8 core modules
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented 
    $form['admin_label'] = array(
      '#type' => 'item',
      '#title' => $this->t('Block description'),
      '#markup' => $definition['admin_label'],
    );

Which means that #2273925: Ensure #markup is XSS escaped in Renderer::doRender() would have prevented this I guess.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Note: You can do the same with menus and I suspect that the same is true for views.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

#2025649: Add title-related methods to BlockPluginInterface to help clarify and resolve many issues was trying to clarify that the admin_label is a raw and must be filtered.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Related issues: +#2025649: Add title-related methods to BlockPluginInterface to help clarify and resolve many issues
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: Unassigned » tim.plunkett

Working on a test.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review
FileSize
block_content_titles-2446995-7-FAIL.patch2.75 KB
block_content_titles-2446995-7-PASS.patch3.59 KB

The last submitted patch, 7: block_content_titles-2446995-7-FAIL.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 7: block_content_titles-2446995-7-PASS.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
block_content_titles-2446995-10.patch4.38 KB
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

Oops, forgot interdiff. Was just this:

diff --git a/core/modules/block/src/Tests/BlockInterfaceTest.php b/core/modules/block/src/Tests/BlockInterfaceTest.php
index 3c81acb..08980c5 100644
--- a/core/modules/block/src/Tests/BlockInterfaceTest.php
+++ b/core/modules/block/src/Tests/BlockInterfaceTest.php
@@ -7,6 +7,7 @@
 
 namespace Drupal\block\Tests;
 
+use Drupal\Component\Utility\String;
 use Drupal\Core\Form\FormState;
 use Drupal\simpletest\KernelTestBase;
 use Drupal\block\BlockInterface;
@@ -75,7 +76,7 @@ public function testBlockInterface() {
       'admin_label' => array(
         '#type' => 'item',
         '#title' => t('Block description'),
-        '#markup' => $definition['admin_label'],
+        '#markup' => String::checkPlain($definition['admin_label']),
       ),
       'label' => array(
         '#type' => 'textfield',
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review

Off my game

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
Status: Needs review » Reviewed & tested by the community

Reviewed with @jibran and looks good.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Issue tags: +The Great Contrib SA Hunt 2015
Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented
Issue summary: View changes
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Makes sense. Thanks for the quick identification AND turnaround of this one!

Committed and pushed to 8.0.x. Thanks!

  • webchick committed ee74fc6 on 8.0.x 
    Issue #2446995 by tim.plunkett, Berdir: Block content titles are not...

Status: Fixed » Closed (fixed)

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