The $link parameter in watchdog() is what produces the 'Operations' column in the 'Recent log entries' page and on the page for a single log entry.
The intention is that you provide a link to the thing affected by the log item -- such as a node or a user.
Consequently, the output of this value is not run through check_plain()... which could present a security risk if a module is counting on user input in some way.
(BTW: the Drupal security team has cleared this issue to be fixed publicly.)
Comment | File | Size | Author |
---|---|---|---|
#25 | 1145700-improve-dblog-santization.patch | 645 bytes | mr.baileys |
#4 | sanitize-watchdog-link.patch | 689 bytes | jbrown |
Comments
Comment #1
joachim CreditAttribution: joachim commentedSuggestions for ways to improve this:
a) Explicitly state in the docs that it's the caller's responsibility to sanitize this parameter. This keeps us in somewhat inconsistent state, as the $message parameter features built-in sanitization.
b) change the $link parameter to accept an array of link items (in the style of the old hook_links). Downside here is that watchdog() needs to be independent of logging storage or output.
Comment #2
gregglesSince the text is meant to be a link it should be filtered with something that will preserve the allowed html and get rid of xss, something like filter_xss (see this cheat sheet about which function to use).
I like option A (document it) for Drupal 6/7 and either your option B or option C - use filter_xss - for Drupal 8.
Comment #3
pillarsdotnet CreditAttribution: pillarsdotnet commentedWe would need to move
filter_xss()
fromcommon.inc
tobootstrap.inc
if we want to call it fromwatchdog()
.See #1174496: WSOD: drupal_error_handler() assumes filter_xss() is defined
Comment #4
jbrown CreditAttribution: jbrown commentedWe filter on output, so we don't need to move filter_xss() for this.
Comment #5
Dries CreditAttribution: Dries commentedI believe the patch in #4 is correct but it would be nice to get another +1 from someone in the security team. Marking it RTBC.
Comment #6
webchickComment #7
Dave Reid+1 from me. filter_xss() is the appropriate filtering in this case.
Comment #8
pillarsdotnet CreditAttribution: pillarsdotnet commentedAfter committing to 8.x, needs to be moved to 7.x for adding docs as suggested in #1/#2
Comment #9
catchComment #10
jbrown CreditAttribution: jbrown commentedWhy can't the patch be applied to 6 and 7 also?
Comment #11
greggles@jbrown - it probably could. I think there's a debate about whether that's a change of functionality or not. This should be seemless for Drupal 6/7 (I doubt anyone is putting javascript in there) so it would make some sense to backport.
Comment #12
Dave ReidAgreed.
Comment #13
Dries CreditAttribution: Dries commentedCommitted to 8.x. Now we need to backport a solution to 7.x. See #2.
Comment #14
jbrown CreditAttribution: jbrown commentedI thought the consensus was that the same patch could be applied to 6, 7 and 8.
Comment #15
catchPutting this to CNR for D7 since the patch applies cleanly and we need a consensus between a new patch and committing the existing one.
There are two options:
1. Apply the patch from #4 as is to Drupal 7, there is a chance that someone is doing something really fancy there that would break, but it's likely a theoretical problem.
2. Write a new patch that documents the issue for module maintainers, but doesn't add the filter_xss() at all.
I would personally go for #1 - would rather have the security hardening - it is just a link in the watchdog table.
Also this is dblog module which doesn't really expose an API - if you use syslog module it handles the links differently anyway, so modules can't rely 100% on the link being rendered as HTML in a table.
Comment #16
gregglesI I agree with catch in #15 - we should apply #4 as is to Drupal 7.
Comment #17
catchSince that's three of us, and greggles posted #2 which had the choices, I'm going to move this back to RTBC.
Comment #18
webchickI think this makes sense, too. Worst-case, a table buried in an administrative page whose table rows get pruned every 1000 entries or so looks weird for a few minutes.
Committed to 7.x. Marking back for 6.x.
Comment #19
webchickNo longer needs backport to D7.
Comment #20
jbrown CreditAttribution: jbrown commentedI don't understand the updated comment in #18. Was it meant for a different issue?
Comment #21
greggles@jbrown - http://drupalcode.org/project/drupal.git/commit/2d884d2 The code was committed. I think the comment makes sense: she's saying this is a bit of an API change but that it's unlikely to affect people and, even if it does affect them, it just affects one part of a page that is not frequently used.
Comment #22
jbrown CreditAttribution: jbrown commentedOkay - I understand webchick's comment now.
Comment #23
Gábor HojtsyAnybody tested on Drupal 6?
Comment #24
vectoroc CreditAttribution: vectoroc commented#1235536: too long link breaks admin/report/dblog table related issue
Comment #25
mr.baileysI have now, and can confirm it works as expected. Straight reroll of the patch in #4 since it no longer applied cleanly.
Comment #26
Gábor HojtsyThanks, committed.