adixon / ca.civicrm.logviewer

CiviCRM Log Viewer
Other
9 stars 11 forks source link

XSS - Sanitize log entries in some way? #11

Closed GreysonStalcup closed 1 year ago

GreysonStalcup commented 1 year ago

Hello there!

The Values of the actual log should probably be slightly sanitized. A bot was trying random XSS payloads on one of our client sites. They were unsuccessful on the front-end. However, looking through the logs, some of the payloads successfully activate.

image image
adixon commented 1 year ago

So, the concern is that by viewing the log via the web interface, the content in the log being displayed could be a risk (of XSS)?

What you're showing on the screen looks to be some kind of output from a debugging entry, but I suppose that could be dangerous, though I'm still not clear how (presumably the person viewing the log would need to take some kind of action?). Ah, or are we looking at the risk of js embedded in a url? Not clear how that would be activated.

I imagine this would be a similar risk in the wordpress or drupal log viewing facilities (i'm only familiar with the drupal "watchdog").

Have you seen any code in either of those that would mitigate this risk?

GreysonStalcup commented 1 year ago

So, the concern is that by viewing the log via the web interface, the content in the log being displayed could be a risk (of XSS)?

Yes, correct. The output is not sanitized resulting in a possibility of someone viewing the log to be exposed to XSS.

What you're showing on the screen looks to be some kind of output from a debugging entry, but I suppose that could be dangerous, though I'm still not clear how (presumably the person viewing the log would need to take some kind of action?). Ah, or are we looking at the risk of js embedded in a url? Not clear how that would be activated.

Correct, as with any XSS vulnerability, the actual danger is dependent on a lot of factors. However, any sort of unsanitized output from unverified sources can be of potential danger. Yeah, it probably is not extremely likely that this would result in any sort of actual payload delivery, it probably should be fixed. The fact that potentially malicious javascript code is being executed in the admin area based solely on a random scan can potentially be further developed to perform actual attack attempts.

I imagine this would be a similar risk in the wordpress or drupal log viewing facilities (i'm only familiar with the drupal "watchdog").

It could be. I have not encountered the above issue with any other log viewer, but yes. Generally speaking, this would require some looking into to determine exactly what the issue is. However, I can potentially make a PR to include output sanitization.

Have you seen any code in either of those that would mitigate this risk?

GreysonStalcup commented 1 year ago

@adixon I created a PR with a simple addition that fixes this issue -

13

adixon commented 1 year ago

Okay, I think I'm understanding it all, thanks for your work here.

FWIW, I found this in answer to my question about how Drupal handles XSS risks in the drupal watchdog:

https://www.drupal.org/project/drupal/issues/2240273

The short version is that the module or code that is writing to the watchdog is responsible for the sanitization (using the "watchdog" function).

For CiviCRM, the logging process isn't doing any sanitization, perhaps on the assumption that you're not at risk of viewing it in a browser (or maybe by oversight).

In any case, I'm convinced by your example and notes and am happy to pull your PR.

After all, since this output isn't going anywhere else, the only potential downside is making the log less readable.