jedisct1 / libsodium-doc

Gitbook documentation for libsodium
https://libsodium.org
ISC License
166 stars 159 forks source link

pwhash_str_verify: Is `passwd` really required to be null-terminated? #51

Closed fujimotos closed 6 years ago

fujimotos commented 6 years ago

For now, the documentation on crypto_pwhash_str_verify() states that the password string argument passwd must be zero-terminated:

int crypto_pwhash_str_verify(const char str[crypto_pwhash_STRBYTES],
                             const char * const passwd,
                             unsigned long long passwdlen);
...
str and passwd have to be zero-terminated.

But why do we need to zero-terminate passwd?

Since crypto_pwhash_str_verify knows the exact length of passwd (we pass it as the third argument), it makes little sense to require it. In fact, as far as I inspected the code, it just works fine without any null-termination.

My current opinion is that the manual is somewhat confused and misdescribing the parameter, thus we would better update it. But what do you think about it?

Note

The description in question is introduced by 47e5915db639cb269ce20a92e96277b6cea713c8.

jedisct1 commented 6 years ago

The password length is not known, since the proposed password is not assumed to be the same as the initial password. But this doesn't affected the password hash length no matter what.

pwhash_str() returns a zero-terminated string, so the corresponding verification function expects the same thing. If it "works" right now without this requirement, it's by accident and not by design, so don't rely on it.

fujimotos commented 6 years ago

I'm bit confused. Maybe you are talking about the str argument, not passwd?

int crypto_pwhash_str_verify(const char str[crypto_pwhash_STRBYTES],
                             const char * const passwd,
                             unsigned long long passwdlen);

I actually agree with you that the password hash str (which pwhash_str() produces) should be null-terminated. In fact, since it gets passed to strlen() in argon2.c^, we definitely need to zero-terminate it.

But, I (and my coworker) still cannot see the reason why the password argument passwd is required to be null-teminated. Can you shed some light on it?

[^] https://github.com/jedisct1/libsodium/blob/master/src/libsodium/crypto_pwhash/argon2/argon2.c#L218

jedisct1 commented 6 years ago

Yes, I was referring the str.

The password doesn't have to be nul terminated.

fujimotos commented 6 years ago

@jedisct1 Aha. It was my fault. I've put a wrong title to this ticket, and this was the source of confusion.

Yes, I was referring the str.

And I was talking about passwd. I think we're basically in an agreement that the current manual is stating wrong. Here I quote the documentation again, look at the bold part:

str and passwd have to be zero-terminated.

The pull request #52 is the patch to fix the sentence.