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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim’s picture

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.

greggles’s picture

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.

pillarsdotnet’s picture

jbrown’s picture

Status: Active » Needs review
FileSize
689 bytes

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

Dries’s picture

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.

webchick’s picture

Issue tags: +Needs security review
Dave Reid’s picture

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

pillarsdotnet’s picture

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

catch’s picture

Issue tags: -Needs security review
jbrown’s picture

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

greggles’s picture

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

Dave Reid’s picture

Issue tags: +Needs backport to D6

Agreed.

Dries’s picture

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.

jbrown’s picture

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

catch’s picture

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.

greggles’s picture

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

catch’s picture

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.

webchick’s picture

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.

webchick’s picture

Issue tags: -Needs backport to D7

No longer needs backport to D7.

jbrown’s picture

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

greggles’s picture

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

jbrown’s picture

Okay - I understand webchick's comment now.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review

Anybody tested on Drupal 6?

vectoroc’s picture

mr.baileys’s picture

Priority: Critical » Normal
Status: Needs review » Reviewed & tested by the community
FileSize
645 bytes

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.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed.

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