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?
Comments
Comment #1
grisendo CreditAttribution: grisendo commentedI attach a patch
Comment #2
larowlanI think format date should do this to protect all calls
Comment #3
larowlanAlso I think we should use filter_xss_admin because some configurations might use markup in formats
Also a need a tests here.
Comment #4
larowlanNote this effects 7 too but security team happy for it to be in public because protected by a restricted permission
Comment #5
scor CreditAttribution: scor commentedyes, see our policy: http://drupal.org/node/475848
Comment #6
grisendo CreditAttribution: grisendo commentedYes, "Administer site configuration" is the permission required.
So, should it be good to apply a filter_xss_admin to format_date?
Comment #7
grisendo CreditAttribution: grisendo commentedLets see what do tests say.
Comment #8
grisendo CreditAttribution: grisendo commentedDo I need to create separated issues for 7.x and 6.x or just mark (somehow) that needs backports?
Comment #9
larowlanWe 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.
Comment #10
larowlanPatch looks good, but we need a test still, to prevent a regression.
Comment #11
larowlanWorking on tests.
Comment #12
larowlanOk, this patch adds tests.
Please find attached interdiff, fail patch (tests only) and pass patch (tests + patch).
Lee
Comment #13
nick_schuch CreditAttribution: nick_schuch commentedHave reviewed. Given tests come back green I will rtbc.
Comment #15
larowlanLooks like random test failures, postponing for now until #1893800: Something is very, very wrong with update.php / upgrade tests... demons suspected is resolved
Comment #16
larowlan#12: xss-date-formats-1892640.12.pass_.patch queued for re-testing.
Comment #17
larowlanYay, this is green now
Comment #18
nick_schuch CreditAttribution: nick_schuch commentedWe are good to go!
Comment #19
webchickAwesome, thanks folks!
Committed and pushed to 8.x.
Back down to 7.x.
Comment #20
kim.pepperD7 patch.
Comment #21
kim.pepperA quick and simple D7 patch.
Comment #22
kim.pepperD7 patch with back-ported tests too.
Comment #23
kim.pepperComment #24
kim.pepperHere is a a fail patch and a pass patch as requested by @larowlan. Sorry for all the posts!
Comment #26
kim.pepperDidn't realise the function params changed between D7 => D8.
Comment #27
kim.pepperComment #28
larowlanDamn, 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.
Comment #29
c960657 CreditAttribution: c960657 commentedI 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().
Comment #30
larowlanFWIW You could always use html in your date formats using the \ escaping.
Comment #31
larowlanComment #32
catchBumping priority.
Comment #36
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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.
Comment #37
mpdonadioThe 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.
Comment #39
mpdonadioWow. Hadn't pulled my D7 test site in a while.
Comment #40
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedSo 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.