Problem/Motivation

@mpdonadio, the DateTime subsystem maintainer, discovered an exploitable XSS situation in the date formats configuration.

This vulnerability can be fixed publicly (and not at https://security.drupal.org/node/add/project-issue/drupal) as per http://drupal.org/security-advisory-policy because it requires the compromised account to have an advanced permission that already makes the site compromised.

Steps to create in 8.1.8:

  1. Visit admin/config/regional/date-time
  2. Add a format
  3. Name the format XSS, and for the format string use "c -- \<\s\c\r\i\p\t\>\a\l\e\r\t\(\'\f\o\o\'\)\<\/\s\c\r\i\p\t\>" without the double quotes
  4. Note that you should have gotten one or more alerts
  5. Save the format
  6. Edit the format, admin/config/regional/date-time/formats/manage/xss
  7. Note that you should have gotten the alert

This would allow an admin to add XSS to a date format. If another admin edits the format, they will get the XSS exploit.

He is still evaluating whether this is exploitable elsewhere date formats are used, or in 8.2.x. He is not seeing any other places he has tested (like with the DateTime widgets for formatters).

Proposed resolution

The output of date formats is not trusted in Drupal 8 and there are several tests about this - see \Drupal\system\Tests\Common\FormatDateTest::testFormatDate and \Drupal\system\Tests\System\DateTimeTest::testDateFormatConfiguration. The problem here is dateFormatHandler in system.date.js which does $preview.html(dateString); which trusts the dateString.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#28 2780285-2-28.patch3.26 KBalexpott
#28 2780285-2-28.test-only.patch2.35 KBalexpott
#28 27-28-interdiff.txt1.64 KBalexpott
#27 2780285-2-27.patch2.98 KBalexpott
#27 2780285-2-27.test-only.patch2.07 KBalexpott
#17 2780285-17.patch1.05 KBth_tushar
#11 2780285-11.patch11.3 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott credited mpdonadio.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented 
   * @return string
   *   A translated date string in the requested format. Since the format may
   *   contain user input, this value should be escaped when output.
   */
  public function format($timestamp, $type = 'medium', $format = '', $timezone = NULL, $langcode = NULL);

Note the docs say that the output is not to be trusted AND in Drupal 8 this string is not marked safe so it will be auto-escaped by default see \Drupal\Core\Datetime\DateFormatterInterface::format()

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Active » Needs review
FileSize
2780285.5.patch468 bytes
mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented

The problem also exists in D7. There, the problem happens both on the format listing page (admin/config/regional/date-time/formats) and the entry page. On the listing page, the input is displayed unsanitized, so

diff --git a/modules/system/system.admin.inc b/modules/system/system.admin.inc
index 0f525c6..221c9ec 100644
--- a/modules/system/system.admin.inc
+++ b/modules/system/system.admin.inc
@@ -2854,7 +2854,7 @@ function system_date_time_formats() {
   if (!empty($formats)) {
     foreach ($formats as $format) {
       $row = array();
-      $row[] = array('data' => format_date(REQUEST_TIME, 'custom', $format['format']));
+      $row[] = array('data' => filter_xss_admin(format_date(REQUEST_TIME, 'custom', $format['format'])));
       $row[] = array('data' => l(t('edit'), 'admin/config/regional/date-time/formats/' . $format['dfid'] . '/edit'));
       $row[] = array('data' => l(t('delete'), 'admin/config/regional/date-time/formats/' . $format['dfid'] . '/delete'));
       $rows[] = $row;

fixes that. Haven't gotten done tracing through the JS section.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented

Didn't mean to remove that file. I thought it was the patch I had uploaded...

+++ b/core/modules/system/js/system.date.js
@@ -41,7 +41,7 @@
-        $preview.html(dateString);
+        $preview.html(Drupal.checkPlain(dateString));

Using .text() accomplished instead of .html() accomplishes the same thing.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented
Issue tags: +Needs backport to D7

@alexpott and I had a quick conversation about this in IRC. He had the idea to move DateTimeTest::testDateFormatConfiguration to a JavascriptTestBase so we can adequately test the identified vector.

I went through the existing test coverage, usages, and did a lot of manual testing with this, and am pretty sure this is limited just to the JS preview on date format add/edit.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue tags: +Security improvements
YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue summary: View changes

clarifying in the issue that this can be a public issue

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2780285-11.patch11.3 KB

Discovered #2780475: assertEscaped() and assertUnescaped don't work as you would expect in JavascriptTestBase whilst working on the test. The test will fail because of it. Also the test does not yet test for the error reported in the issue summary it is just a conversion.

Status: Needs review » Needs work

The last submitted patch, 11: 2780285-11.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Category: Task » Bug report
Priority: Normal » Major

Bumping priority since this is still not a nice bug.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Category: Bug report » Task
Priority: Major » Normal
Status: Needs work » Needs review
Parent issue: » #1892640: XSS in date formats admin page

I think for Drupal 7 this might be handled by #1892640: XSS in date formats admin page (an older issue which was committed to Drupal 8 but still needs to be backported to Drupal 7).

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Category: Task » Bug report
Priority: Normal » Major
Status: Needs review » Needs work

Cross-post.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

th_tushar’s picture

th_tushar CreditAttribution: th_tushar commented
Status: Needs work » Needs review
FileSize
2780285-17.patch1.05 KB

Attached is the patch to fix this issue.

Status: Needs review » Needs work

The last submitted patch, 17: 2780285-17.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@th_tushar the fix in #11 is the one we want - how come you ignored the prior work on this issue? Atm we're blocked on #2780475: assertEscaped() and assertUnescaped don't work as you would expect in JavascriptTestBase

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented
Title: XSS in date format configuration » [PP-1] XSS in date format configuration
Status: Needs work » Postponed
Related issues: +#2780475: assertEscaped() and assertUnescaped don't work as you would expect in JavascriptTestBase

Blocked per #19 on #2780475: assertEscaped() and assertUnescaped don't work as you would expect in JavascriptTestBase.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Title: [PP-1] XSS in date format configuration » XSS in date format configuration

The blocker is in.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Version: 8.8.x-dev » 8.7.x-dev
Status: Postponed » Needs review
Issue tags: -Needs tests
FileSize
2780285-2-27.test-only.patch2.07 KB
2780285-2-27.patch2.98 KB

rerolled. Decided not to convert all of \Drupal\Tests\system\Functional\System\DateTimeTest::testDateFormatConfiguration() to javascript testing - way too much change.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
27-28-interdiff.txt1.64 KB
2780285-2-28.test-only.patch2.35 KB
2780285-2-28.patch3.26 KB

Whoops... the test doesn't fail... this one does.

The last submitted patch, 28: 2780285-2-28.test-only.patch, failed testing. View results

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented
Status: Needs review » Reviewed & tested by the community

Change and test looks good.

  • larowlan committed c4a7d0c on 8.8.x 
    Issue #2780285 by alexpott, th_tushar, mpdonadio: XSS in date format...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Committed c4a7d0c and pushed to 8.8.x. Thanks!

c/p to 8.7.x

  • larowlan committed 19824fd on 8.7.x 
    Issue #2780285 by alexpott, th_tushar, mpdonadio: XSS in date format...

Status: Fixed » Closed (fixed)

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