Skip to content
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

Fixed up encoding issues #173

Merged
merged 1 commit into from
Oct 26, 2016
Merged

Fixed up encoding issues #173

merged 1 commit into from
Oct 26, 2016

Conversation

josephlr
Copy link
Contributor

I significantly constrained the space of admissible encodings to deal with #143 .

The keyid and data parameters in the encoding are gone, as they are either not used in validation, or they cause validation to fail. Similarly, the salt is now required. Before if no salt was provided, the encoding wouldn't include the salt or the hash value. Because there was no hash value, any later validation check would fail.

I also made changes to validate_context. Empty vs. NULL passwords and salts now produce the same results and errors. The salt was the real outlier here, a NULL salt was allowed while an empty salt produced an error (ARGON2_SALT_TOO_SHORT). This is inconsistent with the rest of Argon2, where switching a parameter from empty to NULL produces the exact same hash value (or produces the exact same error).

Note that this doesn't break virtually any consumers of the public API in argon2.h because all encodings emitted from functions like argon2_hash() are still valid. There are only two cases where an existing consumer would see breakage:

  1. They had manually added a data parameter to the string encoding. Note that this parameter has always been ignored.
  2. They were hashing with a NULL salt (because hashing with an empty salt doesn't work). This is quite insecure, and they already would have been unable to use argon2_verify() as it would just fail when decoding the encoded string.

I also added tests for a bad salt in an encoding string.

by significantly contraining the space of admissable encodings. This
doesn't break the public API, but makes some previously valid
encodings invalid.

Also made changes to validate_context. Empty vs. NULL passwords and
salts now produce the same results and errors.

Added tests for bad salt in encoding string.
@codecov-io
Copy link

Current coverage is 67.47% (diff: 61.53%)

Merging #173 into master will increase coverage by 0.71%

@@             master       #173   diff @@
==========================================
  Files             9          9          
  Lines          1041       1033     -8   
  Methods         132        132          
  Messages          0          0          
  Branches        171        166     -5   
==========================================
+ Hits            695        697     +2   
+ Misses          266        261     -5   
+ Partials         80         75     -5   

Powered by Codecov. Last update 07524a3...5d4f755

@veorq veorq merged commit ce6f6d1 into P-H-C:master Oct 26, 2016
@veorq
Copy link
Member

veorq commented Oct 26, 2016

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants