On #2535192: Security: LoggerChannelInterface doxygen needs a little love, we patched the interface for logging in drupal 8 to add a security note:

+ * SECURITY NOTE: the caller might also set a 'link' in the $context array
+ * which will be printed as-is by the dblog module under an "operations"
+ * header. Usually this is a "view", "edit" or similar relevant link. Make sure
+ * to use proper, secure link generation facilities; some are listed below.
+ *
+ * @see \Drupal\Core\Logger\RfcLoggerTrait
+ * @see \Psr\Log\LoggerInterface
+ * @see \Drupal\Core\Logger\\LoggerChannelFactoryInterface
+ * @see \Drupal\Core\Utility\LinkGeneratorInterface
+ * @see \Drupal\Core\Routing\LinkGeneratorTrait::l()
+ * @see \Drupal\Core\Entity\EntityInterface::link()

We need to add a similar notice here, but just tell people to use the l() function, with its sanitization features, to make links (the list of "secure link generation facilities" in Drupal 7 is only the l() function).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikemadison’s picture

Here is a patch that adds the necessary text to boostrap.inc under function watchdog().

mikemadison’s picture

Assigned: Unassigned » mikemadison
Status: Active » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Great start, thanks! A few things to fix:

a) The text in your patch is not quite accurate:
- watchdog() doesn't actually print messages or links itself. The dblog module or other logging module does the printing.
- the link is not part of $message, it is its own parameter.

b) Because of (a), this documentation should be put in as part of the @param $link section of the documentation.

c) Wording: again because of (a), I wouldn't say "the caller" in this case, because we're putting the warning into the function they are calling. Since we want to document it as part of the $link parameter, you could start off this addition by saying "SECURITY NOTE: Make sure your link is properly sanitized, because it will be printed as-is by logging modules." or something like that.

d) Several lines in your patch have extra space at the end of the line. You can probably set up your code editor either to highlight or remove end-of-line spaces.

e) If you want to add @see l(), it needs to go at the end of the documentation block.

f) This text is a bit awkward:
"Make sure to use properly the l() function"
I would just say "Use the l() function to generate secure links".

mikemadison’s picture

Status: Needs work » Needs review
FileSize
751 bytes

Relocated the comment per your suggestion, and also reworded it. I've removed the spaces at the end of the lines, and also moved the @see to the end of the section with the others.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

MUCH better, thanks! Short and to the point.

I think that since this is an important SECURITY NOTE that we should relax the usual idea of wrapping the entire @param doc into all one paragraph. I think it stands out better this way for people reading the code docs in the code or in an IDE.

Thanks!

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

I'm not sure this is correct, is it?

Typically in Drupal we sanitize things on output so I think it might be the responsibility of the code that outputs this to HTML to sanitize it.

For example, the core dblog module saves this to the database as is, then sanitizes it using filter_xss() in dblog_overview() when it's displayed to an administrator. As mentioned in the other issue, it looks like dblog_event() does not sanitize it, but isn't that a bug? (It's certainly some kind of bug that the two are inconsistent.)

The link is expected to be HTML, and l() is the right way to generate a link, so the patch isn't totally wrong but I'm not sure we want to say "it will be printed as-is by logging modules" when in fact a logging module probably should not be printing it as-is.

David_Rothstein’s picture

Issue tags: +Security improvements
jhodgdon’s picture

Title: Add note to watchdog() about link security » Fix watchdog() link security
Component: documentation » dblog.module
Status: Needs review » Needs work

I guess we'd need to see if the equivalent is still being done in 8 first, and then figure out what to do in 7. But yes, probably a bug-ish, although since $link is coming from a module in the first place, not really a security problem per se...

swarad07’s picture

Status: Needs work » Needs review
FileSize
422 bytes

Based on the feedback on the last patch, I am changing the patch slightly. Adding filter_xss() for the "log link" in dblog_event() page callback just like it is there for dblog_overview().

swarad07’s picture

I forgot what the issue was trying to solve in the first place :-/.

This patch is more correct, it adds a security note as well as sanitizes the link in the dblog_event() page callback.

cilefen’s picture

+++ b/includes/bootstrap.inc
@@ -1702,6 +1702,8 @@ function watchdog_exception($type, Exception $exception, $message = NULL, $varia
+ *   SECURITY NOTE: Make sure your link is properly sanitized,
+ *   Use the l() function to generate secure links.

This is two sentences connected with a comma.

swarad07’s picture

Status: Needs review » Needs work
poker10’s picture

Status: Needs review » Reviewed & tested by the community

The patch still applies and I think this is a good change.

In the watchdog listing, we are already using the filter_xss(), see: https://git.drupalcode.org/project/drupal/-/blob/7.x/modules/dblog/dblog.admin.inc#L69.

In the watchdog entry detail, the filter_xss() is missing on the same field: https://git.drupalcode.org/project/drupal/-/blob/7.x/modules/dblog/dblog.admin.inc#L191.

RTBC for me, thanks!

poker10’s picture

Issue tags: +Pending Drupal 7 commit
poker10’s picture

Title: Fix watchdog() link security » Sanitize watchdog() link in dblog_event()

Updated the title to better reflect the change in the patch.

  • mcdruid committed 1acc41ae on 7.x
    Issue #2540830 by swarad07, mikemadison, jhodgdon, poker10,...
mcdruid’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit

Great, thanks everyone!

Status: Fixed » Closed (fixed)

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