Sonatype team does not approve, CVSS 37.5

[bneijt]

Dear loguru developers,

I love the package, but security (Sonatype) does not approve, so I either have to get Loguru fixed or drop the implementation to be able to go live.

The issue has a simple description here: https://www.huntr.dev/bounties/73ebb08a-0415-41be-b9b0-0cea067f6771/
Summary: if you have a multi-line log message on a plain text terminal (pff, you uses that in prod, do structured logging already ;) ) you can not see the difference between injected text and extra log lines.

I think a simple solution could be: if we encounter multi-line log message on a plain text formatter, then add a space (or other continuation signifying character) before each other line to make sure it's different from a normal starting log line. Using a space should have minimal or no impact on the current production use cases. Using > for example, would have more visual impact, but still could be fine.

I would love to see the above implemented with either a space or another character and then I could probably start using loguru 🎉

Quote from Sonartype IQ:

The loguru package is vulnerable to Improper Neutralization of CRLF Sequences. The _log() function in the _logger.py file does not properly sanitize the user-supplied message parameter before writing log entries in the log file. An attacker can exploit this behavior by supplying a specially-crafted request containing special control characters such as Carriage Return (\r) and Line Feed (\n). This would allow the attacker to mislead a log audit, cover attack traces, or perform other malicious actions. Tainted data in the log files could lead to accountability, non-repudiation, and incident forensics failures.

Advisory Deviation Notice: The Sonatype security research team discovered that this vulnerability has not been fixed as stated in the advisory. Instead, in version 0.6.0, the maintainers added a warning in the documentation that the package may be vulnerable to log injection attacks if external data is used.

[Delgan]

Hi @bneijt, thank you for bringing up security concerns. This relates to #705.

It is correct that improper usage of Loguru can lead to a log injection attack. As you suggested, this could be somewhat mitigated internally by automatically escaping newline characters.

However, I'm reluctant to do so since I believe there are valid cases where a user may want to print plain new lines. We could find a solution, such as introducing a new opt() flag, for example, but I have the feeling that all this would just be window dressing.

You see, attempting to log user data is inherently unsafe. As highlighted in the documentation, there exist many creative ways to inadvertently introduce attack vectors. The CLRF injection is just the tip of the iceberg, Loguru can't really protect against all threats due to the non-sanitized logging of user input.

I also don't think it's fair to ban Loguru on the grounds that its misuse could lead to a vulnerability in the application code. Is Sonatype banning standard logging and libraries using it for the same reason? It's true that the default parameters are not optimal for security. However, Loguru does offer the possibility to mitigate the risks. As you said, it's just a simple serialize=True flag to turn on. Just because Loguru does not make the choice to make this flag the default does not mean Loguru is unsafe.

I'm sorry, this probably won't let you use Loguru in your application... I wish security analyzers would focus on alerting when an application tries to log unsanitized user input or when serialize=True is missing, instead of flagging the whole library.

[bneijt]

Sonatype is blocking the use of loguru at my organisation. For some reason, they don't care about the exact same issue in the default python logging implementation, and there is no easy way to enable serialization on that. So IMHO the Sonatype team is lazy, don't care, and using the default python logging framework instead is worse.

But enough of all that. What if we add line continuations on plain text formatters? So for example logger.info("a\nb") would result in:

2024-03-04 12:23:35.895 | INFO     | __main__:<module>:1 - a
| b

where the | is injected by the formatter doing the INFO level etc based on the message containing a newline. I know there are way way more interesting issues with logging (like terminal escapes etc), but this would at least make loguru better than default logging from Python and maybe, just maybe, the Sonatype security will lift the ban.

[bneijt]

I created a small patch, without test etc, that uses a special conversion to add support for line continuations here bneijt@692b85b

Concept is: change default format to use a new conversion c pattern that will inject a line continuation behind every newline.

Let me know what you think.

[Delgan]

Thanks for trying to fix that @bneijt, but there are other default behaviors that Sonatype will never approve. For example, the "Fully descriptive exceptions" feature of Loguru causes variable values to be automatically displayed in the stack trace.

I'm not keen of cluttering the default behavior of Loguru to please what I think are flawed security analyzers (again, they should flag unsafe usage of Loguru, not Loguru itself). Loguru aim to produce user-friendly defaults, which seems incompatible with security tool expectations.

We could somehow fix the issue you reported, but that's would be smoke and mirrors, and Sonatype could invalidate Loguru again for another reason.

However, one idea that would please me more would be to publish a package specifically dedicated to enforcing all possible security measures. This could have been an environment variable or an option in Loguru, but as we've seen, it's not possible since the entire package is flagged...
We could notably implement the new-line escaping, default opening permissions and disabled variable values of stacktraces.
For example, loguru-hardened could be a nice package. However, I don't know if that would be enough for it to be authorized by Sonatype? What if loguru-hardened depends on loguru internally? Do we need an entire fork of Loguru instead?

[bneijt]

Sonatype is flagging because of a public bounty related issue posted, so indeed they don’t consider how the package is used and just flag it completely.

If we would create a new package with hardened defaults, it would have to not depend on loguru directly, but be a complete fork to harden it. Otherwise loguru would still be flagged. I’m going to give it some thought. If there is an easy way to maintain the fork I think it might make sense.

Btw, I think introducing continuation characters would not harm user experience and close the one cve against loguru, so I still think it might be worth considering.

For now I’ll close the issue and see if I can find the time to make and maintain a hardened fork 🤔

[Delgan]

Btw, I think introducing continuation characters would not harm user experience and close the one cve against loguru, so I still think it might be worth considering.

My point is that when Sonatype will eventually consider another of the mentioned issues to be a security threat, they will flag the whole Loguru package again, and we'll back to square one.

For now I’ll close the issue and see if I can find the time to make and maintain a hardened fork 🤔

I would like to help on this, so please don't hesitate to share your ideas with me.

I'm thinking the best way would be to have some kind of "security_options.py" file in which we could list the desired default behavior. The fork would just need to update these values, without touching the rest of the code.
Upstream I could even unit test all these options.
Maybe we don't even need a fork, and loguru-hardened could be a second package published within this repository, additionally to the standard loguru package.

I'll keep thinking about it myself as well, because I believe it's an important topic.

[bneijt]

Ok, so we need a new package, and I think I could do with just a few simple changes.

My current take would be: fork, change some of the defaults (diagnose false, serialize true) and publish that as a new package called loguru-hardened.

We could also have the patches applied during the release, but maybe development would be more difficult.
What would you say: a) publish a hardened set of defaults from the same repository or b) fork the repository and apply rename patches there?

[Delgan]

I think it might be more maintainable in the long term if loguru-hardened were implemented within this repository. We could easily guarantee that the code is always up to date and fully tested.

However, I had done a quick search and hadn't found exactly how it could be done. Generally, it seems one setup.py can only be associated with one published package. So I don't have a technical answer to that yet.

[bneijt]

I looked into a few options:

• Use a separate repo with patches rebased on each new loguru release: hard to maintain, lots of work.
• Use a separate folder with setup.py and automate in there: brittle and does not play well with the python -m build isolated environment approach.
• Use a script to patch loguru in a dirty git, test and build: ugly, but it works and might be the easiest to maintain atm.

Not explored:

• Using another build tool and see if the multi-package approach fits in there better (like pants maybe?)

I created a small PR to show the implementation of the last item on the list here: #1136

But for some reason a few of the tests fail, so it still needs work (maybe because the tests require serialize to be false?)

[Delgan]

Thanks for the PR @bneijt, it looks like a good approach.

Eventually I could even handle the whole process of publishing two packages through Github Actions.

[bneijt]

If you would like, I have a package publishing from github already and could put a release workflow in the same PR if you want, let me know.

[Delgan]

If you would like, I have a package publishing from github already and could put a release workflow in the same PR if you want, let me know.

That would be very welcome. It's something I've wanted to do for a long time, but I haven't taken the time to do it.