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

StatusFileSize
new638 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

StatusFileSize
new449 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
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

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

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

D7 patch.

kim.pepper’s picture

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

A quick and simple D7 patch.

kim.pepper’s picture

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.

kim.pepper’s picture

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

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.

kim.pepper’s picture

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.

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