Problem/Motivation

theme_system_modules_details($variables) calls SafeMarkup::set() which is meant to be for internal use only.

Proposed resolution

  • Remove the call by refactoring the code.
  • If refactoring is not possible, thoroughly document where the string is coming from and why it is safe, and why SafeMarkup::set() is required.

Remaining tasks

  1. Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123

  2. Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it.

  3. If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.

Manual testing steps (for double escaping)

Do these steps both with HEAD and with the patch applied:

  1. Clean install of Drupal 8.
  2. Visit the modules list aka "Extend" (admin/modules)
  3. Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping.

User interface changes

N/A

API changes

N/A

Files: 
CommentFileSizeAuthor
#38 Extend___drupal_8_0_x.png158 KBjoelpittet
#37 interdiff.txt525 bytesgolddragon007
#37 remove_or_document-2501737-37.patch2.06 KBgolddragon007
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,329 pass(es). View
#34 remove_or_document-2501737-34.patch2.04 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,018 pass(es). View
#24 interdiff.txt1.12 KBlauriii
#24 remove_or_document-2501737-24.patch4.25 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,025 pass(es). View
#22 remove_or_document-2501737-22.patch3.51 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,878 pass(es), 1 fail(s), and 0 exception(s). View
#22 interdiff-250-19-22.txt1.14 KBcilefen
#20 interdiff.txt2.02 KBjoelpittet
#20 remove_or_document-2501737-19.patch2.03 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,267 pass(es). View
#10 remove_or_document-2501737-10.patch1.51 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,745 pass(es). View
#8 remove_or_document-2501737-8.patch4.42 KBsclapp
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,285 pass(es), 0 fail(s), and 39,462 exception(s). View
#3 remove_or_document-2501737-2.patch1.08 KBsclapp
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,711 pass(es). View

Comments

sclapp’s picture

will approach by refactoring, slightly extraneous comment

sclapp’s picture

Status: Active » Needs review
FileSize
0 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
sclapp’s picture

FileSize
1.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,711 pass(es). View

The last submitted patch, 2: remove_or_document-2501737-2.patch, failed testing.

Cottser’s picture

Title: Remove or document SafeMarkup::set in theme_system_modules_details($variables) » Remove or document SafeMarkup::set in theme_system_modules_details()

Minor title tweak :)

joelpittet’s picture

Status: Needs review » Needs work

Couple of nitpicks but I think this is looking good thanks @sclapp

  1. +++ b/core/modules/system/system.admin.inc
    @@ -229,8 +229,13 @@ function theme_system_modules_details($variables) {
    +        ['@id' => $id,
    +        '@enableid' => $module['enable']['#id'],
    +        '@modulename' => $module_name]);
    

    The format of the array is not in the format of our coding standards, will discuss IRL.

  2. +++ b/core/modules/system/system.admin.inc
    @@ -229,8 +229,13 @@ function theme_system_modules_details($variables) {
    +    $row[] = array('class' => array('module'), 'data' => $col2);
    

    Array syntax should be the short syntax.

sclapp’s picture

Status: Needs work » Needs review
FileSize
4.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,285 pass(es), 0 fail(s), and 39,462 exception(s). View

Changed the syntax to short array format, in line with coding standards in the patch

Status: Needs review » Needs work

The last submitted patch, 8: remove_or_document-2501737-8.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,745 pass(es). View

I think this should fix some of the fails... one sec I'll explain...

joelpittet’s picture

+++ b/core/modules/system/system.admin.inc
@@ -229,8 +229,8 @@ function theme_system_modules_details($variables) {
     // Add the module label and expand/collapse functionality.
     $id = Html::getUniqueId('module-' . $key);

@@ -259,7 +259,7 @@ function theme_system_modules_details($variables) {
-      '#title' => SafeMarkup::set('<span class="text"> ' . drupal_render($module['description']) . '</span>'),
+      '#title' => SafeMarkup::format('<span class="text"> @description</span>', ['@description' => drupal_render($module['description'])]),

There was another SafeMarkup::set in that file.

  1. +++ b/core/modules/system/system.admin.inc
    @@ -225,17 +225,16 @@ function theme_system_modules_details($variables) {
    -    $row[] = array('class' => array('checkbox'), 'data' => drupal_render($module['enable']));
    +    $row[] = ['class' => ['checkbox'], 'data' => drupal_render($module['enable'])];
    

    As much as I love short syntax this line wasn't changed for the issue so the short syntax doesn't need to change.

  2. +++ b/core/modules/system/system.admin.inc
    @@ -225,17 +225,16 @@ function theme_system_modules_details($variables) {
    -    // Add the module label and expand/collapse functionality.
    -    $id = Html::getUniqueId('module-' . $key);
    

    These lines were accidentally deleted. Likely the reason for the failures.

  3. +++ b/core/modules/system/system.admin.inc
    @@ -225,17 +225,16 @@ function theme_system_modules_details($variables) {
    +    $col2 = SafeMarkup::format('<label id="@id" for="@enableid" class="module-name table-filter-text-source">@modulename</label>',['@id' => $id, '@enableid' => $module['enable']['#id'], '@modulename' => $module_name]);
    

    There was a missing space between the , and the [. And all the words in keywords I put an underscore to separate the words.

  4. +++ b/core/modules/system/system.admin.inc
    @@ -225,17 +225,16 @@ function theme_system_modules_details($variables) {
    -    $description .= '<div class="admin-requirements">' . t('Machine name: !machine-name', array('!machine-name' => '<span dir="ltr" class="table-filter-text-source">' . $key . '</span>')) . '</div>';
    +    $description .= '<div class="admin-requirements">' . t('Machine name: !machine-name', ['!machine-name' => '<span dir="ltr" class="table-filter-text-source">' . $key . '</span>']) . '</div>';
    

    This was also out of scope.

  5. +++ b/core/modules/update/update.module
    @@ -517,10 +517,10 @@ function _update_message_text($msg_type, $msg_reason, $report_link = FALSE, $lan
    -        $text = t('There was a problem checking <a href="@update-report">available updates</a> for Drupal.', array('@update-report' => \Drupal::url('update.status')), array('langcode' => $langcode));
    +        $text = t('There was a problem checking <a href="@update-report">available updates</a> for Drupal.', ['@update-report' => \Drupal::url('update.status')], ['langcode' => $langcode]);
    ...
    -        $text = t('There was a problem checking <a href="@update-report">available updates</a> for your modules or themes.', array('@update-report' => \Drupal::url('update.status')), array('langcode' => $langcode));
    +        $text = t('There was a problem checking <a href="@update-report">available updates</a> for your modules or themes.', ['@update-report' => \Drupal::url('update.status')], ['langcode' => $langcode]);
    
    @@ -532,10 +532,10 @@ function _update_message_text($msg_type, $msg_reason, $report_link = FALSE, $lan
    -      $text .= ' ' . t('See the <a href="@available_updates">available updates</a> page for more information and to install your missing updates.', array('@available_updates' => \Drupal::url('update.report_update', [], ['language' => $language])), array('langcode' => $langcode));
    +      $text .= ' ' . t('See the <a href="@available_updates">available updates</a> page for more information and to install your missing updates.', ['@available_updates' => \Drupal::url('update.report_update', [], ['language' => $language])], ['langcode' => $langcode]);
    ...
    -      $text .= ' ' . t('See the <a href="@available_updates">available updates</a> page for more information.', array('@available_updates' => \Drupal::url('update.status', [], ['language' => $language])), array('langcode' => $langcode));
    +      $text .= ' ' . t('See the <a href="@available_updates">available updates</a> page for more information.', ['@available_updates' => \Drupal::url('update.status', [], ['language' => $language])], ['langcode' => $langcode]);
    

    more of my favourite syntax that is out of scope.

akalata’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

manually tested #10; the HTML output is identical. (joelpiettet's comments in #11 apply to #8)

akalata’s picture

Assigned: sclapp » Unassigned

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: remove_or_document-2501737-10.patch, failed testing.

Status: Needs work » Needs review
akalata’s picture

Status: Needs review » Needs work

Manually tested this patch; the HTML output is identical.

Patch replaces SafeMarkup::set with SafeMarkup::format in 2 instances

  1. +++ b/core/modules/system/system.admin.inc
    @@ -229,8 +229,8 @@ function theme_system_modules_details($variables) {
    +    $row[] = ['class' => ['module'], 'data' => $col2];
    

    - @id/@enable_id/@module_name are generated using module-specific values generated by system_rebuild_module_data(). I could not find specifically if/where ExtensionDiscovery::scan, InfoParser, etc sanitizes data pulled from the module's .info.yml file
    - updating second line to short array syntax

  2. +++ b/core/modules/system/system.admin.inc
    @@ -259,7 +259,7 @@ function theme_system_modules_details($variables) {
    +      '#title' => SafeMarkup::format('<span class="text"> @description</span>', ['@description' => drupal_render($module['description'])]),
    

    - @description uses module-specific values from same source as above

It does not appear that content provided in the .info.yml file is appropriately sanitized. In some quick testing, adding

into a module description will run the script where the description is displayed (module list), and HTML tags are rendered for both description and name/title -- so I'm thinking this needs to be fixed before we can declare the content safe? Or are we just trusting module devs at this point?
akalata’s picture

Status: Needs work » Needs review
lauriii’s picture

Status: Needs review » Needs work

So the variables are being sanitized because they are using @placeholder format. We still need to investigate if its necessary to have the drupal_render in the theme_system_modules_details.

chx’s picture

> Or are we just trusting module devs at this point?

Yes, we do, you are about to run their PHP code on your server and you are worried about them XSS'ing you?? I don't think this is a real problem. Also, it makes bad_judgement auto enabling itself possible :P

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing
FileSize
2.03 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,267 pass(es). View
2.02 KB

Since safemarkup::format approach is poo-poo'd. Inline template is probably better and @lauriii is right no need for the drupal_render, especially with this approach.

I gave it a go, but could use a second pair of eyes.

lauriii’s picture

Status: Needs review » Needs work
Issue tags: -Needs manual testing +Needs tests

Tested this manually and seems to work. The code is also good and we will have one less drupal_render call! How ever we still need test coverage for this.

cilefen’s picture

Status: Needs work » Needs review
FileSize
1.14 KB
3.51 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,878 pass(es), 1 fail(s), and 0 exception(s). View

Is the test needed for an XSS in the module description? That is what I did here. Is is not escaped.

Status: Needs review » Needs work

The last submitted patch, 22: remove_or_document-2501737-22.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
4.25 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,025 pass(es). View
1.12 KB

This should fix the tests

dawehner’s picture

+++ b/core/modules/system/src/Form/ModulesListForm.php
@@ -264,7 +264,7 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
-    $row['description']['#markup'] = $this->t($module->info['description']);
+    $row['description']['#markup'] = $this->t(SafeMarkup::escape($module->info['description']));

Isn't this case similar to the local task/menu links issue? So should the description here actually return a Translation Wrapper?

This doesn't protected against XSS, but to be honest a module author can produce an XSS in a different way SO easily

lauriii’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs tests

I agree that there is too many ways module author can produce a XSS or even worse things to happen so maybe it doesn't make any sense to test this even though it would be nice. So lets remove the test and the SafeMarkup::escape().

YesCT’s picture

I'm not getting why "This doesn't protected against XSS,"
the patch in #24 looks to be escaping html from the description and has a test to prove it is.

#24 looks good to me...

xjm’s picture

xjm’s picture

So this is interesting.

I've temporarily unpublished this issue because the issue also exists in D7. @alexpott pointed out that one difference between this and other exploits a module author could introduce is that it can be executed without even installing the module; it just needs to be on the file system. That probably isn't a reason to consider this a sec issue and it is probably security hardening at most. But we should confirm this first with the sec team. @alexpott said he'd do so.

There is a use case for putting HTML in a module description I think, so XSS filtering over escaping would be preferable if we do decide it needs to be sanitized. Can someone manually test safe HTML in a module description, and if that is currently supported, add test coverage for it as well?

Also, in either case, we need to document what is allowed for module descriptions and whether or not it's expected to be safe.

Also, @alexpott pointed out that doing the escaping inside the translation breaks the translation, so the filtering or escaping should be done on the result of the t() call instead.

xjm’s picture

theme_system_modules_details() could also potentially be converted to a template straight up, but that is likely a lot of work. It would be good to review it.

mlhess’s picture

The security issue is known and can be public.

pwolanin’s picture

also: #2527544: Module list page is not XSS safe

I would worry more (especially for D7) about a module that just buried a PHP shell among its files.

alexpott’s picture

So I think this issue should drop trying to make the description safe and leave that to #2527544: Module list page is not XSS safe. It should just focus on Remove or document SafeMarkup::set in theme_system_modules_details()

lauriii’s picture

Status: Needs work » Needs review
FileSize
2.04 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,018 pass(es). View

This is a rerolled version of #20 with extra space removed from the inline template.

Cottser’s picture

Here is the related conversion issue.

joelpittet’s picture

Title: Remove or document SafeMarkup::set in theme_system_modules_details() » Remove SafeMarkup::set in theme_system_modules_details()
Status: Needs review » Needs work

This looks really close, thanks for the re-roll.

+++ b/core/modules/system/system.admin.inc
@@ -255,14 +263,18 @@ function theme_system_modules_details($variables) {
+      '#template' => '<span class="text">{{ module_description }}</span>',
...
-      '#title' => SafeMarkup::set('<span class="text module-description">' . drupal_render($module['description']) . '</span>'),

missing the CSS class "module-description".

golddragon007’s picture

Status: Needs work » Needs review
FileSize
2.06 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,329 pass(es). View
525 bytes

I've added that missing class name.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
158 KB

Thank you @golddragon007. I did a markup comparsion to make sure things aren't missing. Looks good thank you!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Late rendering++

Committed 8402939 and pushed to 8.0.x. Thanks!

  • alexpott committed 8402939 on 8.0.x
    Issue #2501737 by sclapp, joelpittet, lauriii, cilefen, golddragon007,...

Status: Fixed » Closed (fixed)

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