Patch Arbitrary Code Execution

[aaronclong]

My company's internal deps auditing system is beginning to flag loguru because of this potential exploit. I don't know if it has come to your attention.

I don't know exactly how to prevent it, but am willing to help out if you need support. I will study the issue more.

418sec/huntr#1592

[aaronclong]

It looks like this method is the culprit: _from_pickled_value

I wonder if making it an instance method with mangling would solve the problem. A better solution would be to find another way to do this without pickling.

[aaronclong]

As far as I can tell, this pickingling is for multiprocessing that makes this so much more complex.

It was introduce with the 0.3.0 release in response to this issue: #102

[Delgan]

Hi @aaronclong, thanks for reporting this issue.

The Huntr team contacted me some time ago about the vulnerability you're mentioning: 418sec#1

I'm copy-pasting the comment I had left:

Hi.

Thanks for the security report. However, I'm not sure protecting __reduce__() in this case is really relevant. Sure, I understand that pickle.loads() is dangerous, but only when used with an untrusted source.

The RecordException is only meant to serialize Python errors. It will not be used again arbitrary data coming from network for example.

From what I see, the PoC is not different than calling os.system() in __str__:

import os

class MyClass:
    def __str__(self):
        os.system("xcalc")

logger.info(MyClass())

I am still quite confused by this report and I do not understand why Loguru is to blame.

The user receiving untrusted data should be responsible for sanitizing it before processing it. We can't expect this job to be done by the third-library, otherwise there might be infinite way to execute arbitrary code as demonstrated by the above example. This has little to do with the pickle module. Loguru isn't fetching and executing arbitrary code by itself, that's why it is difficult for me to recognize that there is a security problem in this library.

It is of course a pity that Loguru is reported as a dangerous library, and I wish it could be avoided. Perhaps you disagree with my analysis and can explain to me how my reasoning is wrong?

Of course, I'm in favor of improving Loguru safety and thanks for offering your help. However I want to understand the problem first and foremost to justify changes that may have multiple impacts (on features, performances and code complexity).

[aaronclong]

I wonder if dill would be viable to work around this, since this library controls its own serialization.

[aaronclong]

@Delgan Since you are using this for exception handeling, it is possible that an exception could trigger by a third-party library or whatever that would trigger it. Loguru would be just a catalyst in that workflow. Likewise, because it is in exceptions, it is extremely hard to sanitize that input.

My personal opinion that it is a logging library's responsibility to cover rogue execution. Using pickling is in general kind of dangerous.

[aaronclong]

@Delgan if you don't have any plans on fixing this or remedying it, I understand. I would like to know though because I will need to remove it from my applications unfortunately.

[Delgan]

Since you are using this for exception handeling, it is possible that an exception could trigger by a third-party library or whatever that would trigger it. Loguru would be just a catalyst in that workflow. Likewise, because it is in exceptions, it is extremely hard to sanitize that input.

Shouldn't the third-library with malicious __reduce__ method be flagged instead?

My personal opinion that it is a logging library's responsibility to cover rogue execution. Using pickling is in general kind of dangerous.

How is it different from a third-library with malicious __str__ method?

[aaronclong]

@Delgan I agree with you that the third party should be flagged as well, but I don't agree that absolves your library in this case. It is on all sides, especially as zero trust principles are concerned.

[aaronclong]

@Delgan likewise this code could in theory cause the same issue as the log4j exploit that happened this last month. It is quite similar to their arbitrary execution exploit.

Again, it's your library. No stress if this isn't the the path you want to go, I just need to know to make the appropriate changes on my end.

[aaronclong]

@Delgan likewise this code could in theory cause the same issue as the log4j exploit that happened this last month. It is quite similar to their arbitrary execution exploit.

Again, it's your library. No stress if this isn't the the path you want to go, I just need to know to make the appropriate changes on my end.

[Delgan]

As I said, I'm all in favor or improving Loguru's security, but I don't know how to do it in this case without compromising the features.

The exception needs to be somehow serialized. The exception comes from user's code. I can't restrain the allowed attributes during serialization because it it would unfairly limit usability for users with custom and trusted exceptions.

I'm genuinely trying to understand. Do you think Loguru should be responsible for analyzing __str__ method of every argument before displaying it?

Here is another example:

import os
from multiprocessing import Pool


class Malicious:
    def __reduce__(self):
        os.system("firefox")
        return (Malicious, ())

def f(x):
    return 0

if __name__ == "__main__":
    with Pool(processes=4) as pool:
        pool.map(f, [Malicious()])

Should multiprocessing be flagged because it can execute arbitrary code?

[aaronclong]

@Delgan I think __str__ is different that __reduce__. __reduce__ being automatically triggered by the interpreter causes problems. These are just long standing design flaws within python that unfortunately we have to patch over.

I don't know necessarily how to solve this problem, maybe dill or another format would make this less problematic. It could also be a custom internal format that is simple and easy to serialize/deserialize. We could even look into how the built in logger handles this scenario. I am certain we can find other resources for recommendation.

As for multiprocessing, it is a library with some flaws in it's design. However, I understand the need to support it. In general, there are people that choose to use alternatives to the built in multiprocessor like pathos. I am neither for nor against multiprocessing, but just want to acknowledge those issues.

[Delgan]

Sorry for maybe sounding too defensing , by the way. It's just that I truly don't understand how pickle would be different that any other method in this very specific case. I feel every library is flagged as unsafe just for using pickle. 😕

For example, __str__ can also be "automatically triggered by the interpreter":

import os

class Malicious:
    def __str__(self):
        os.system("firefox")
        return "M()"

m = Malicious()
string = f"M: {m}"

Anyway, initially you were only trying to solve a problem reported by a third party, so I don't want to bother you further. Thanks for answering my questions. :)

I had already looked for safe alternatives to pickle but I didn't find anything that suited me. The dill library offers more features than pickle but suffers from the same vulnerability issues. The standard logging library also uses pickle similarly to share the record to another process (which is also why Loguru needs to serialize the exception). Again, sorry to insist, but Loguru is just using pickle to serialize an user's class, so it's not different if the malicious code lived in the __reduce__ of another user's class.

I'll try to get in touch with Huntr team for discussing this issue further.

[Delgan]

Also I don't think it's fair to compare this issue with the Log4Shell exploit. To clarify, there is no way for Loguru to execute arbitrary code from user string input alone.

[aaronclong]

@Delgan it is being flagged for pickle and how it exposes pickle. To be clear, pickle isn't a safe method of serialization. It is a problematic legacy format.

Unfortunately, you can convert a pickle response to string and back again. Not saying it is the most likely scenario, but it is possible. Yes, it is quite similar to the log4j issue because it allowed for Java Objects to be loaded dynamically by the logger from another source. The string expansion was only one half of the equation. For more context, read here.