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).
Comments
Comment #1
mikemadison CreditAttribution: mikemadison at Pacific Northwest National Laboratory commentedHere is a patch that adds the necessary text to boostrap.inc under function watchdog().
Comment #2
mikemadison CreditAttribution: mikemadison at Pacific Northwest National Laboratory commentedComment #3
jhodgdonGreat 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".
Comment #4
mikemadison CreditAttribution: mikemadison at Pacific Northwest National Laboratory commentedRelocated 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.
Comment #5
jhodgdonMUCH 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!
Comment #6
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI'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.
Comment #7
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #8
jhodgdonI 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...
Comment #9
swarad07Based 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().
Comment #10
swarad07I 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.
Comment #11
cilefen CreditAttribution: cilefen commentedThis is two sentences connected with a comma.
Comment #12
swarad07Comment #13
swarad07Thanks @cilefen. Fixing.
Comment #14
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedThe 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!
Comment #15
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedComment #16
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedUpdated the title to better reflect the change in the patch.
Comment #18
mcdruidGreat, thanks everyone!