There is an XSS in date formats admin page (admin/config/regional/date-time) when entering a patter like
\<\s\c\r\i\p\t\>\a\l\e\r\t\(\'\X\S\S\'\)\;\<\/\s\c\r\i\p\t\>

It also generates XSS every time is printed (format_date()). (i.e, Views admin, views result...)
Should it be format_date sanitized? Or every time any other module writes it should sanitize output?

Files: 
CommentFileSizeAuthor
#39 1892640-39.patch1.17 KBmpdonadio
#26 xss-date-formats-1892640-26-test-only-fail.patch863 byteskim.pepper
PASSED: [[SimpleTest]]: [MySQL] 39,716 pass(es). View
#26 xss-date-formats-1892640-26-pass.patch1.41 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 39,688 pass(es). View
#24 xss-date-formats-1892640-24-test-only-fail.patch881 byteskim.pepper
FAILED: [[SimpleTest]]: [MySQL] 39,664 pass(es), 0 fail(s), and 1 exception(s). View
#24 xss-date-formats-1892640-24-pass.patch1.43 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 39,719 pass(es), 0 fail(s), and 1 exception(s). View
#22 xss-date-formats-1892640-22.patch1.43 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 39,786 pass(es), 0 fail(s), and 1 exception(s). View
#21 xss-date-formats-1892640-20.patch579 byteskim.pepper
PASSED: [[SimpleTest]]: [MySQL] 39,778 pass(es). View
#20 xss-date-formats-1892640-20.patch579 byteskim.pepper
PASSED: [[SimpleTest]]: [MySQL] 39,790 pass(es). View
#12 xss-date-formats-1892640.12.interdiff.txt1.06 KBlarowlan
#12 xss-date-formats-1892640.12.fail_.patch1.06 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 50,652 pass(es), 1 fail(s), and 0 exception(s). View
#12 xss-date-formats-1892640.12.pass_.patch1.49 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 50,770 pass(es). View
#7 drupal-xss_date_formats_admin-1892640-6960152.patch449 bytesgrisendo
PASSED: [[SimpleTest]]: [MySQL] 50,683 pass(es). View
#1 drupal-xss_date_formats_admin-1892640.patch638 bytesgrisendo
PASSED: [[SimpleTest]]: [MySQL] 50,590 pass(es). View

Comments

grisendo’s picture

FileSize
638 bytes
PASSED: [[SimpleTest]]: [MySQL] 50,590 pass(es). View

I attach a patch

larowlan’s picture

I think format date should do this to protect all calls

larowlan’s picture

Issue tags: +Needs tests

Also I think we should use filter_xss_admin because some configurations might use markup in formats
Also a need a tests here.

larowlan’s picture

Note this effects 7 too but security team happy for it to be in public because protected by a restricted permission

scor’s picture

yes, see our policy: http://drupal.org/node/475848

grisendo’s picture

Yes, "Administer site configuration" is the permission required.

So, should it be good to apply a filter_xss_admin to format_date?

grisendo’s picture

FileSize
449 bytes
PASSED: [[SimpleTest]]: [MySQL] 50,683 pass(es). View

Lets see what do tests say.

grisendo’s picture

Do I need to create separated issues for 7.x and 6.x or just mark (somehow) that needs backports?

larowlan’s picture

We just tag it (like so), after the 8.x fix is committed, the committer will update the version back to 7.x, at which point we'll attach the 7.x fix and so on.

larowlan’s picture

Status: Needs review » Needs work

Patch looks good, but we need a test still, to prevent a regression.

larowlan’s picture

Assigned: Unassigned » larowlan

Working on tests.

larowlan’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.49 KB
PASSED: [[SimpleTest]]: [MySQL] 50,770 pass(es). View
1.06 KB
FAILED: [[SimpleTest]]: [MySQL] 50,652 pass(es), 1 fail(s), and 0 exception(s). View
1.06 KB

Ok, this patch adds tests.
Please find attached interdiff, fail patch (tests only) and pass patch (tests + patch).

Lee

nick_schuch’s picture

Have reviewed. Given tests come back green I will rtbc.

Status: Needs review » Needs work

The last submitted patch, xss-date-formats-1892640.12.pass_.patch, failed testing.

larowlan’s picture

Status: Needs work » Postponed

Looks like random test failures, postponing for now until #1893800: Something is very, very wrong with update.php / upgrade tests... demons suspected is resolved

larowlan’s picture

Status: Postponed » Needs review
larowlan’s picture

Yay, this is green now

nick_schuch’s picture

Status: Needs review » Reviewed & tested by the community

We are good to go!

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Awesome, thanks folks!

Committed and pushed to 8.x.

Back down to 7.x.

kim.pepper’s picture

FileSize
579 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,790 pass(es). View

D7 patch.

kim.pepper’s picture

FileSize
579 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,778 pass(es). View

A quick and simple D7 patch.

kim.pepper’s picture

FileSize
1.43 KB
FAILED: [[SimpleTest]]: [MySQL] 39,786 pass(es), 0 fail(s), and 1 exception(s). View

D7 patch with back-ported tests too.

kim.pepper’s picture

Status: Patch (to be ported) » Needs review
kim.pepper’s picture

FileSize
1.43 KB
FAILED: [[SimpleTest]]: [MySQL] 39,719 pass(es), 0 fail(s), and 1 exception(s). View
881 bytes
FAILED: [[SimpleTest]]: [MySQL] 39,664 pass(es), 0 fail(s), and 1 exception(s). View

Here is a a fail patch and a pass patch as requested by @larowlan. Sorry for all the posts!

Status: Needs review » Needs work

The last submitted patch, xss-date-formats-1892640-24-pass.patch, failed testing.

kim.pepper’s picture

FileSize
1.41 KB
PASSED: [[SimpleTest]]: [MySQL] 39,688 pass(es). View
863 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,716 pass(es). View

Didn't realise the function params changed between D7 => D8.

kim.pepper’s picture

Status: Needs work » Needs review
larowlan’s picture

Status: Needs review » Needs work

Damn, date formats work different in Drupal 7, the test needs to be more involved. We have to assign it to a new type and then output it somewhere else, like on a node view, as the table does not include an example like in Drupal 8.

c960657’s picture

Version: 7.x-dev » 8.x-dev

I don't think this was fixed the right way.

First of all, the return value of format_date() was changed from to plain-text to HTML, so we need to update the Doxygen block to reflect this. It is important that we are very explicit about whether we return plain-text or HTML. Otherwise we end up with a lot of confusion and undefined behaviour (see e.g. #974782: Resulting string format of token_replace(..., array('sanitize' => FALSE)) is undefined), and that easily results in more XSS bugs. Forgetting to escape input using e.g. check_plain() is bad, but passing it through check_plain() once too many is also bad.

Also, we need to update any callers that assumes the return value is plain-text (e.g. when passing the return value into t(), we should do it using !date, not @date or %date).

Finally, custom date formats have never been supposed to be treated as HTML. I don't think there is a use-case for allowing HTML tags in custom date formats. Hence, filter_xss_admin() is not appropriate here. Instead we should use check_plain().

larowlan’s picture

FWIW You could always use html in your date formats using the \ escaping.

larowlan’s picture

Assigned: larowlan » Unassigned
catch’s picture

Priority: Normal » Major
Issue summary: View changes

Bumping priority.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • webchick committed da31660 on 8.3.x
    Issue #1892640 by grisendo, larowlan: Fixed XSS in date formats admin...

  • webchick committed da31660 on 8.3.x
    Issue #1892640 by grisendo, larowlan: Fixed XSS in date formats admin...
David_Rothstein’s picture

Version: 8.1.x-dev » 7.x-dev

I think everything in #29 was handled later on by #2568613: Remove HTML support from date formats and replace remaining !placeholder where format_date() and 'date.formatter' => format() are used so this can be moved back to Drupal 7.

The filter_xss_admin()-within-format_date() implementation mostly makes sense to me for Drupal 7 (as @larowlan said, date formats already allow HTML). The one problem I can see is that a lot of code out there probably does output them using check_plain() (such as with a @placeholder in translated text), and a filter_xss_admin() followed by a check_plain() can cause double-escaping bugs (e.g. in the rare case where the date format has a & in it). Technically that existing code is doing it wrong, but it's hard to expect that developers would have thought about it.

Having core call filter_xss_admin() at the point of output (like suggested here) would be a bit safer, but also less comprehensive.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
1.15 KB

The problem needs to be fixed on the listing page, and in the JS. This is one option. The other option is a Drupal.checkPlain in Drupal.behaviors.dateTime instead of the change to system_date_time_lookup().

I don't see the attached as a BC break. AFAICT, it's only ever used internally, and would be a pain to call manually b/c it uses `$_GET` directly.

Status: Needs review » Needs work

The last submitted patch, 37: 1892640-d7-37.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
1.17 KB

Wow. Hadn't pulled my D7 test site in a while.

David_Rothstein’s picture

So basically we're deciding between the approach in #26 (which was originally committed to Drupal 8) and the one in #39.

I think I lean towards #39 also... one other argument in its favor (which is related to what I wrote in #36 although not exactly the same thing) is that doing this in format_date() (like #26) assumes the output of that function will always be used in an HTML context, but in reality there's no reason it has to be - that assumption only makes sense if the person constructing the date format deliberately put HTML in there.

I think we should include tests such as the earlier patch had though.