Solving log injection vulnerabilities

Development
1

Hi everyone,

SonarCloud keeps flagging log injection issues in XWiki. While I wouldn’t consider them very serious, they’re still not nice and can indeed be an issue when you need XWiki’s log for security purposes. Basically, the main issue is that it is possible to inject content that looks like it was another log message in any user-controlled string that we log, which could be used to conceal an attack. For admins, there are easy workarounds like storing the log in a structured format like JSON or in a database instead of a plain text file. However, still, the default logging configuration of XWiki should be secure and not vulnerable to this attack (secure by default). From what I understand, the main mitigation seems to be to remove or escape newline characters in user-supplied parameters (see, e.g., this discussion and this cheat sheet by OWASP). I suggest implementing a similar mitigation. I see basically two options:

  1. Ensure that all user-controlled strings are logged in parameters and configure a central formatting of log parameters that escapes parameter values before inserting them. This has the advantage that many existing log calls will be covered by this, and it is hard to forget the escaping, but all log messages where user-controlled strings are logged as parameters will need to be flagged as false-positives in SonarCloud. Further, we need to be careful, as the fix won’t apply to log messages where the user-controlled string is inserted into the log message itself so we can’t simply disable the whole rule in SonarCloud. We could also possibly just apply this escaping to the whole log message, but I wonder if this won’t make exception stack traces unreadable.
  2. Escape all user-controlled strings before logging them. We should provide a helper method for this (like this NewlineLogScrubber by OWASP). This requires changes across the codebase but SonarCloud can assist us in finding them and there is no risk of wrongly ignoring one of them.

We can of course also combine both approaches. Any opinions?

2

  1. At least we won’t forget, and I’m afraid we will constantly let stuff like this go through without it (but if Sonar catch them all, maybe it’s OK).

  2. It could be a bit costly to check every single log parameter for the few containing user-controlled strings. I also find it a bit annoying to make impossible to log multi lines values as parameter (but I don’t really have a use case in mind, and at worst it’s still possible to do it through concatenation, even if not very clean). On the other hand, it might be a huge work to fix all the currently missing escaping.

  3. A kind of mix of 1. and 2. could be interesting: parameter value are not escaped by default, but you can pass some a EscapeParameterMarker with the log to enable it (but this probably won’t help with the “Sonar false positive” aspect).

Not sure what’s best. Maybe a slight preference for 3. so far (mainly because it’s less impacting), but it’s not a -1 for 1 depending on what others think.

3

Hi,

thanks for bringing this up.
I have the feeling on my side that solution 2 would be the proper way to go on the long run, so I’d be +0 for going there. We’d have possibly a lot of occurrences in the codebase to review but it would be cleaner I think. And also we don’t need to apply the rule for all existing code right away: for old code we can do it when we refactor it only.

4

+1 to apply escape user controlled string is logs + limit their size (which is also a good storage use improvement)