Posted by joachim on May 3, 2011 at 3:09pm
11 followers
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | dblog.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | needs backport to D6, Security improvements |
Issue Summary
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.)
Comments
#1
Suggestions 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.
#2
Since 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.
#3
We would need to move
filter_xss()fromcommon.inctobootstrap.incif we want to call it fromwatchdog().See #1174496: WSOD: drupal_error_handler() assumes filter_xss() is defined
#4
We filter on output, so we don't need to move filter_xss() for this.
#5
I 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.
#6
#7
+1 from me. filter_xss() is the appropriate filtering in this case.
#8
After committing to 8.x, needs to be moved to 7.x for adding docs as suggested in #1/#2
#9
#10
Why can't the patch be applied to 6 and 7 also?
#11
@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.
#12
Agreed.
#13
Committed to 8.x. Now we need to backport a solution to 7.x. See #2.
#14
I thought the consensus was that the same patch could be applied to 6, 7 and 8.
#15
Putting 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.
#16
I I agree with catch in #15 - we should apply #4 as is to Drupal 7.
#17
Since that's three of us, and greggles posted #2 which had the choices, I'm going to move this back to RTBC.
#18
I 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.
#19
No longer needs backport to D7.
#20
I don't understand the updated comment in #18. Was it meant for a different issue?
#21
@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.
#22
Okay - I understand webchick's comment now.
#23
Anybody tested on Drupal 6?
#24
#1235536: too long link breaks admin/report/dblog table related issue
#25
I have now, and can confirm it works as expected. Straight reroll of the patch in #4 since it no longer applied cleanly.
#26
Thanks, committed.
#27
Automatically closed -- issue fixed for 2 weeks with no activity.