Authmereloaded: Timing Side-Channel in RoyalAuth.comparePassword(String, String)

[yannicnoller]

We have found a timing side-channel in class _fr.xephi.authme.security.crypts.RoyalAuth_, in the inherited method "public boolean comparePassword(String password, HashedPassword hashedPassword, String name)". This is due to the use of String.equals for comparison which returns as soon as a character does not match. This represents a timing side channel, which could be used by a potential attacker to obtain knowledge about the hidden secret password.
Do you agree with our findings?

A similar issue is present in several "comparePassword" methods, e.g., in the classes fr.xephi.authme.security.crypts.Sha256 and fr.xephi.authme.security.crypts.Pbkdf2.
We found these classes in the latest version of your GitHub repo.

This vulnerability can be easily avoided by using a safe String comparison like in method fr.xephi.authme.security.crypts.BCryptService.checkpw(String, String).
Do you agree with our patch proposal?

Please feel free to contact us for further clarification! You can reach us by the following email address:
yannic.[email protected]

Best regards,
Yannic Noller

[sgdc3]

Hello, we'll try to address that asap, thank you for the report.

@ljacqu hello, how are you? If you have time could we talk about that on discord?

[ljacqu]

Hi @nolleryc, thanks for creating this issue. We can gladly extract that way of doing an equals for the mentioned hash algorithms. What I'm less certain about is how much of a security concern this really is? To that effect I have two questions:

1) Seeing as we're talking about salted hashes here what insight can be gained by knowing how "far" the string comparison was successful? (Edit: Ah, RoyalAuth isn't salted so there it's more important. Still curious whether it's dangerous for our salted SHA256 for example? We can never learn enough about these things :smile:)

2) How serious do you think this flaw is for us? Seeing as we're a Minecraft plugin running in a Java environment are the small changes in time reliable at all (network latency, processes like GC happening at the same time, etc.)?

[sgdc3]

@nolleryc Fixed in the latest commits, thank you again for the report, have a nice day ;)

[yannicnoller]

Great! Thanks for fixing this.

@ljacqu regarding your questions:

1. The standard string comparison itself is vulnerable and should be not be used in such a context. While this vulnerability might not be exploitable by itself, it can be combined with other techniques (e.g. attacks against hash functions) to reveal secrets; it is the safe way to fix the string comparisons.

2. I agree that in your case this issue might not be critical. But as you never know what a potential attacker might be capable of, i.e. what kind of insights he/she has of your environment, it is the safe way to fix such issues. Since your code is open-source, the attacker will be aware of this vulnerability and will try to use it.

Thanks
Yannic