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
#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.