Error paths in argon2_verify do not set errno

[emilbayes]

Hi!

Some of the error code-paths in argon2_verify do not set errno leading to errors that you cannot report good error messages for, specifically an invalid formatted hash string. I guess technically an erroneously formatted string is technically a mismatching hash, so I'm willing to accept this issue as "wontfix", however it would be nice to be able to report for diagnostics purposes.

I don't know if this is something that should be fixed in argon2_verify or if the various return values from there should be interpreted in calling functions like crypto_pwhash_argon2id_str_verify/crypto_pwhash_argon2i_str_verify.

[jedisct1]

Even though I don't have any immediate attack scenario in mind, providing details about why verification failed (besides OOM) is a dangerous thing to do. The less an adversary learns about why an attempt failed, the better.

And things "for diagnostic purposes" always end up being mistakenly displayed in web pages and logged to files.

That being said, timing can easily make the difference between a format error and a hash verification mismatch.

What would you set errno to, besides the generic EINVAL here?

[emilbayes]

I agree that this leaves the possibility for indadvertedly disclosing more information to an adversary. I'm also willing to accept that this is not distinguishable from other input errors, the problem is just now that eg. crypto_pwhash_str_verify will return -1, but errno may be set by a previous error and not the one immediately caused by a malformed hash str.

I don't know if a errno such as EILSEQ would be appropriate here?

[emilbayes]

Hi again,

So in the light of my other issue with accessing errno on Windows (#711), would you be open to change the return code for the verify functions slightly, eg. -1 on error, 0 on success and 1 on mismatch? The verify functions are the only ones I personally have had issues with in regards to errno, so this would alleviate the issue I have at hand (errno would still be clobbered in the cases functions don't update errno on -1).

Doing a Github search for crypto_pwhash_str_verify it looks like most are checking for 0 currently.

[jedisct1]

Ouch no, we can't change the API at this point. That would have catastrophic implications on code testing against -1 for password failure. There is more code than the one on GitHub.

This is something we can change in libhydrogen (even though I still don't like having different error codes for such a function), but not in sodium.