terl / lazysodium-java

A Java implementation of the Libsodium crypto library. For the lazy dev.
https://github.com/terl/lazysodium-java/wiki
Mozilla Public License 2.0
135 stars 47 forks source link

cryptoPwHashStrVerify sometimes has false negatives #39

Closed Quackster closed 5 years ago

Quackster commented 6 years ago

Hello,

When using cryptoPwHashStrVerify argon2 hashing for my project, sometimes the password verification fails (sometimes a lot) despite the password being entered correctly.

Sample code (verifies a password that is "lol" when not hashed):


while (true) {
    byte[] hashedPassword = "$argon2id$v=19$m=65536,t=2,p=1$ZrWMVZiMs4tvs0QwVc7T7A$LIl6XlgIZsuozRpC3bCe5ew8LEWgDQvQE8qwsZ9ISps".getBytes(StandardCharsets.UTF_8);
    byte[] pass = "lol".getBytes(StandardCharsets.UTF_8);

    boolean success = pwHash.cryptoPwHashStrVerify(hashedPassword, pass, pass.length);

    if (!success) {
        System.out.println("Failure");
        break;
    } else {
        System.out.println("Success");
    }
}

Demonstration:

Please fix this. 😢

gurpreet- commented 6 years ago

Hello @Quackster,

Thanks for reporting this.

I had considerable trouble getting password hashing to work with lazysodium, so you're not alone in this. I'll try and explain the problems I've come across in this comment to give you some ideas as to how to fix your problem.

Human error

I can rule out any human error as you've inputted everything correctly for cryptoPwHashStr and cryptoPwHashStrVerify. It can be hard to understand if you should hexadecimal or raw strings in the parameters of lazysodium.

Coding error

Seems to me like you've used the right functions. I can see from your gif image that you've used the Native interfaces which is correct as using the Lazy interfaces mean that your hash needs to be in hex form like A2BD32203... and not the raw string like $argon2id$v=19$m=65....

The memory problem

When I started testing the password hashing algorithms with Lazysodium, some of them would fail and some of them would not fail. I setup my tests just like how you're testing them now in your comment. Exactly like your tests I found out that for the first 10 or so times when I tested cryptoPwHashStrVerify it worked, but then they failed on the 11th time. I was confused. Why would cryptoPwHashStrVerify work the first 10 times and not the 11th?

Libsodium does not give us good error messages so I couldn't easily debug the issue. So I tried trial and error and I found out that the higher the memory requirements, the quicker the cryptoPwHashStrVerify function would fail 🤔

You see, Argon2 is very flexible but this flexibility allows the developer to make many mistakes. The wrong parameters for the operations limit and the memory limit are fatal for a program.

So from your Argon2 string, I can see that $m=65536 which means your total memory needed is around 67.1 megabytes1. So whenever you do perform cryptoPwHashStrVerify it takes approx 67.1mb to 'solve' your hash. Now imagine running cryptoPwHashStrVerify 10 times. That means you are effectively using up 671mb of memory. This is why on the 11th time, my tests failed and why you're having some problems with your mini program too. It's because your argon2 hash requires a lot of memory.

TL;DR

The solution may be to use less memory and perhaps look at the number of operations you use too. You can do that using the Native interface in Lazysodium OR you can use the raw functions using lazySodium.getSodium().crypto_pwhash.


1 The way I worked this out is by multiplying 65536 by 1024 which equals 67108864 bytes which is around 67.1 megabytes.

emansom commented 6 years ago

Shouldn't the memory be freed after each function call? Thus never reaching the 671 megabytes?

gurpreet- commented 6 years ago

@emansom, you are correct and this does happen in reality. But we cannot control the Java garbage collector timings for doing so, so it's really up to you to code various safeguards or have the knowledge beforehand of what your memory requirements are.

As @Quackster was performing cryptoPwHashStrVerify in a while loop, it will of course fill up the memory faster, so I can see how @Quackster's tests would fail. For other use cases you as the developer would need to know the constraints of the system that Lib/Lazysodium is running on.

Another tip: if you are deploying Lazysodium to a server for example and requests are coming in, then all of those requests will be costing some amounts of memory if you are running cryptoPwHashStrVerify and could easily overwhelm the server. It's in fact better to send the argon2 hash to the user's device for decrypting, so lowering the memory limits would be wise so that those devices can cope with decryption.

Hope this helps 😄

emansom commented 5 years ago

I have several gigabytes of free memory, yet the password verification still seems to fail randomly.

gurpreet- commented 5 years ago

It's still a tricky one to answer as there's so much involved. In my tests, I have the operations low and the memory low and it works every time.

On the whole I'm pretty sure it's related to memory, but it could be something else. It's worth testing Libsodium directly to see if there's anything wrong with the Argon2 implementation. Lazysodium is nothing but a wrapper around Libsodium.

I will try and test Libsodium myself in the coming week.

Quackster commented 5 years ago

The systems we're currently using are both in PHP and Java, and for Java we're obviously using LazySodium. The login never fails for the PHP (frontend). The Java program was originally coded in C and that never failed either. It only happened once we moved the project to Java.

For reference, this is what the PHP project uses: https://github.com/jedisct1/libsodium-php. It uses the sodium_crypto_pwhash_str_verify function.

So from what I can tell, an issue lies somewhere within the Java ecosystem of this project. I discussed with @emansom that it's quite possibly an issue with JNA either not feeding libsodium enough memory and/or not letting libsodium spawn multiple threads, and nothing to do with LazySodium itself. 🤔

gurpreet- commented 5 years ago

Thanks for this @Quackster that is very useful to know. I am currently testing it using a memory profiler. If you both have any more suggestions or PRs I would greatly appreciate them.

This issue has been recurring ever since I first started this project and I am keen to get it sorted. A couple of points that may be affecting it:

gurpreet- commented 5 years ago

Hey I think I remember why it didn't work for me earlier.

Try including a null byte at the end of your Argon2 hash. If you were using the Lazy interfaces this would have been handled for you, but as you are not using those functions, then you have to manage all that.

Let's take the first post as an example. So instead of:

byte[] hashedPassword = "$argon2id$v=19$m=65536,t=2,p=1$ZrWMVZiMs4tvs0QwVc7T7A$LIl6XlgIZsuozRpC3bCe5ew8LEWgDQvQE8qwsZ9ISps".getBytes(StandardCharsets.UTF_8);

It will be:

byte[] hashedPassword = "$argon2id$v=19$m=65536,t=2,p=1$ZrWMVZiMs4tvs0QwVc7T7A$LIl6XlgIZsuozRpC3bCe5ew8LEWgDQvQE8qwsZ9ISps\0".getBytes(StandardCharsets.UTF_8);

Notice the null byte at the end. You may have to program something in to detect if a null byte is actually at the end of your Argon2 hash. You can take inspiration from this library to do that. This null byte is a requirement of Libsodium.

Hope this works for you!

emansom commented 5 years ago

We've been running with the appended null-byte at the end, for a week in production; haven't experienced any failed logins. Issue resolved. :tada:

I'm not able to use the Lazy interfaces, as those do not compare the hashes in the same way PHP does via it's libsodium extension.

An new function or interface in LazySodium that uses UTF-8 byte buffers natively, with an appended null-byte at the end; would help developers that use a PHP frontend with an Java backend.

gurpreet- commented 5 years ago

I will look into it. Glad you got it resolved :)