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

Files: 
CommentFileSizeAuthor
#25 1145700-improve-dblog-santization.patch645 bytesmr.baileys
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#4 sanitize-watchdog-link.patch689 bytesjbrown
PASSED: [[SimpleTest]]: [MySQL] 30,021 pass(es).
[ View ]

Comments

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.

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.

Status:Active» Needs review
StatusFileSize
new689 bytes
PASSED: [[SimpleTest]]: [MySQL] 30,021 pass(es).
[ View ]

We filter on output, so we don't need to move filter_xss() for this.

Status:Needs review» Reviewed & tested by the community

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.

Issue tags:+needs security review

+1 from me. filter_xss() is the appropriate filtering in this case.

Issue tags:+needs backport to D7

After committing to 8.x, needs to be moved to 7.x for adding docs as suggested in #1/#2

Issue tags:-needs security review

Why can't the patch be applied to 6 and 7 also?

@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.

Issue tags:+needs backport to D6

Agreed.

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Needs work

Committed to 8.x. Now we need to backport a solution to 7.x. See #2.

I thought the consensus was that the same patch could be applied to 6, 7 and 8.

Status:Needs work» Needs review

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.

I I agree with catch in #15 - we should apply #4 as is to Drupal 7.

Status:Needs review» Reviewed & tested by the community

Since that's three of us, and greggles posted #2 which had the choices, I'm going to move this back to RTBC.

Version:7.x-dev» 6.x-dev

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.

Issue tags:-needs backport to D7

No longer needs backport to D7.

I don't understand the updated comment in #18. Was it meant for a different issue?

@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.

Okay - I understand webchick's comment now.

Status:Reviewed & tested by the community» Needs review

Anybody tested on Drupal 6?

Priority:Critical» Normal
Status:Needs review» Reviewed & tested by the community
StatusFileSize
new645 bytes
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

Anybody tested on Drupal 6?

I have now, and can confirm it works as expected. Straight reroll of the patch in #4 since it no longer applied cleanly.

Status:Reviewed & tested by the community» Fixed

Thanks, committed.

Automatically closed -- issue fixed for 2 weeks with no activity.