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

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

I attach a patch

I think format date should do this to protect all calls

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.

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

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

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

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

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

Lets see what do tests say.

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

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.

Status:Needs review» Needs work

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

Assigned:Unassigned» larowlan

Working on tests.

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

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

Lee

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.

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

Status:Postponed» Needs review

Yay, this is green now

Status:Needs review» Reviewed & tested by the community

We are good to go!

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.

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

D7 patch.

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

A quick and simple D7 patch.

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

D7 patch with back-ported tests too.

Status:Patch (to be ported)» Needs review

StatusFileSize
new1.43 KB
FAILED: [[SimpleTest]]: [MySQL] 39,719 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
new881 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.

StatusFileSize
new1.41 KB
PASSED: [[SimpleTest]]: [MySQL] 39,688 pass(es).
[ View ]
new863 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,716 pass(es).
[ View ]

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

Status:Needs work» Needs review

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.

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().

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

Assigned:larowlan» Unassigned