New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose crypto_pwhash with libsodium 1.0.9 and newer #73

Merged
merged 3 commits into from Jan 11, 2016

Conversation

Projects
None yet
2 participants
@paragonie-scott
Contributor

paragonie-scott commented Jan 6, 2016

See #72

@jedisct1

This comment has been minimized.

Show comment
Hide comment
@jedisct1

jedisct1 Jan 6, 2016

Owner

Just one thing: crypto_pwhash_str() slightly differs from the scrypt version.

The scrypt string format has a fixed length no matter what the parameters are.

The argon2 string format has a variable length, because parameters are directly stored as decimal values. That's a bit annoying, but it's probably too late for them to change the specifications.

crypto_pwhash_strbytes() returns the maximum possible string length (including the final \0), and crypto_pwhash_str() pads with zeros.

crypto_pwhash_verify() doesn't care, as strings a zero-terminated anyway.

I'm not sure whether the PHP bindings should return a fixed-length string, with the padding, or just the required string length. Your code currently does the former. This is fine, but these extra zeros have to be removed or escaped in order to appear in SQL queries, which is not an unreasonable way to use the output of these functions.

Owner

jedisct1 commented Jan 6, 2016

Just one thing: crypto_pwhash_str() slightly differs from the scrypt version.

The scrypt string format has a fixed length no matter what the parameters are.

The argon2 string format has a variable length, because parameters are directly stored as decimal values. That's a bit annoying, but it's probably too late for them to change the specifications.

crypto_pwhash_strbytes() returns the maximum possible string length (including the final \0), and crypto_pwhash_str() pads with zeros.

crypto_pwhash_verify() doesn't care, as strings a zero-terminated anyway.

I'm not sure whether the PHP bindings should return a fixed-length string, with the padding, or just the required string length. Your code currently does the former. This is fine, but these extra zeros have to be removed or escaped in order to appear in SQL queries, which is not an unreasonable way to use the output of these functions.

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Jan 6, 2016

Contributor

The argon2 string format has a variable length, because parameters are directly stored as decimal values. That's a bit annoying, but it's probably too late for them to change the specifications.

Ah, yeah, thanks for pointing that out.

crypto_pwhash_strbytes() returns the maximum possible string length (including the final \0), and crypto_pwhash_str() pads with zeros.

Then the right thing to do, in my opinion, is strip the excess \0 bytes.

This is fine, but these extra zeros have to be removed or escaped in order to appear in SQL queries, which is not an unreasonable way to use the output of these functions.

Well, it kind of is. :)

xkcd-327b

Contributor

paragonie-scott commented Jan 6, 2016

The argon2 string format has a variable length, because parameters are directly stored as decimal values. That's a bit annoying, but it's probably too late for them to change the specifications.

Ah, yeah, thanks for pointing that out.

crypto_pwhash_strbytes() returns the maximum possible string length (including the final \0), and crypto_pwhash_str() pads with zeros.

Then the right thing to do, in my opinion, is strip the excess \0 bytes.

This is fine, but these extra zeros have to be removed or escaped in order to appear in SQL queries, which is not an unreasonable way to use the output of these functions.

Well, it kind of is. :)

xkcd-327b

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Jan 6, 2016

Contributor

Quick question: Is there a function in libsodium that returns the actual length of the returned argon2i hash? Or do I have to implement a "strip right padding, then make sure only 1 NUL remains" workaround?

Contributor

paragonie-scott commented Jan 6, 2016

Quick question: Is there a function in libsodium that returns the actual length of the returned argon2i hash? Or do I have to implement a "strip right padding, then make sure only 1 NUL remains" workaround?

@jedisct1

This comment has been minimized.

Show comment
Hide comment
@jedisct1

jedisct1 Jan 6, 2016

Owner

There is: strlen() :)

Owner

jedisct1 commented Jan 6, 2016

There is: strlen() :)

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Jan 6, 2016

Contributor

Oh, right. Okay, no more all-nighters for Scott. :)

Contributor

paragonie-scott commented Jan 6, 2016

Oh, right. Okay, no more all-nighters for Scott. :)

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Jan 7, 2016

Contributor

Not sure if my variable name choice is okay or not.

I also might want to add a memzero() on the hash that gets returned after copying it to the appropriately sized zend_string.

Contributor

paragonie-scott commented Jan 7, 2016

Not sure if my variable name choice is okay or not.

I also might want to add a memzero() on the hash that gets returned after copying it to the appropriately sized zend_string.

@jedisct1

This comment has been minimized.

Show comment
Hide comment
@jedisct1

jedisct1 Jan 7, 2016

Owner

I don't think the copy is necessary. We can probably allocate the maximum size and then use some PHP macro to truncate the string.

Owner

jedisct1 commented Jan 7, 2016

I don't think the copy is necessary. We can probably allocate the maximum size and then use some PHP macro to truncate the string.

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Jan 7, 2016

Contributor

@remicollet Is there a PHP macro to resize a string?

Contributor

paragonie-scott commented Jan 7, 2016

@remicollet Is there a PHP macro to resize a string?

@jedisct1

This comment has been minimized.

Show comment
Hide comment
@jedisct1

jedisct1 Jan 7, 2016

Owner

Yup: ZSTR_TRUNCATE(string, len)

Owner

jedisct1 commented Jan 7, 2016

Yup: ZSTR_TRUNCATE(string, len)

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Jan 7, 2016

Contributor

Okay, how does this look?

Contributor

paragonie-scott commented Jan 7, 2016

Okay, how does this look?

@jedisct1

This comment has been minimized.

Show comment
Hide comment
@jedisct1

jedisct1 Jan 11, 2016

Owner

Hi Scott,

And sorry for the delayed response. Was traveling without my laptop.

Looks good! Thanks a lot for your help!

Merging now.

Owner

jedisct1 commented Jan 11, 2016

Hi Scott,

And sorry for the delayed response. Was traveling without my laptop.

Looks good! Thanks a lot for your help!

Merging now.

jedisct1 added a commit that referenced this pull request Jan 11, 2016

Merge pull request #73 from paragonie/pwhash
Expose crypto_pwhash with libsodium 1.0.9 and newer

@jedisct1 jedisct1 merged commit b7e5d53 into jedisct1:master Jan 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@paragonie-security paragonie-security deleted the paragonie:pwhash branch Jan 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment