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

Make PasswordHasher use Argon2id #34

Merged
merged 4 commits into from Apr 9, 2018

Conversation

Projects
None yet
2 participants
@hynek
Owner

hynek commented Mar 22, 2018

Also introduce hash type agility to stay backward compatible.

C.f. https://tools.ietf.org/html/draft-irtf-cfrg-argon2-03#section-4

Fixes #33

@codecov

This comment has been minimized.

codecov bot commented Mar 22, 2018

Codecov Report

Merging #34 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #34   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           7      7           
  Lines         142    150    +8     
  Branches       15     15           
=====================================
+ Hits          142    150    +8
Impacted Files Coverage Δ
src/argon2/low_level.py 100% <ø> (ø) ⬆️
src/argon2/exceptions.py 100% <100%> (ø) ⬆️
src/argon2/_password_hasher.py 100% <100%> (ø) ⬆️
src/argon2/__main__.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2ea053...3146346. Read the comment docs.

@lvh

lvh approved these changes Mar 22, 2018

Looks good to me.

The fundamental question here is: is it safe to call verify() with arbitrary input? And the answer is: yes it is; you've always called argon2 with arbitrary input (the password, directly attacker controlled). Since you're probably storing the password hashes, it's much harder to mess with that record than it is to mess with the password.

So what happens when an attacker does get to mess with that record? Well, you can't find pathological inputs for argon2id anyway, and you're just comparing against that value, so there's no increased DoS risk or something like that by having the hash type be controllable. Obviously an attacker who gets to write to that record gets to set the password, that's unpreventable.

As non-binding recommendations, you could perhaps get the convenience class to not accept keys with a higher (memory, time) cost than you configure to fix the DoS hole, but I think that's a weird thing to exploit. Additionally, you may want to make the types parameterizable and expose the fact that the password is not complaint with the current standard (be it type or either kind of cost) via an API, documenting the way people could upgrade their existing password stores that way. All of these are nice to haves, and none of them should block this PR.

As an aside, I'm not happy that somehow we had a PHC to finally get this right and apparently the answer is "choose between these 3 subtly different things based on specialized knowledge you're not supposed to have". It's why I still recommend scrypt. But I digress :)

Thanks for working on this. Great job!

@@ -13,7 +13,7 @@ Unless you have any special needs, all you need to know is:
>>> ph = PasswordHasher()
>>> hash = ph.hash("s3kr3tp4ssw0rd")
>>> hash # doctest: +SKIP
'$argon2i$v=19$m=512,t=2,p=2$5VtWOO3cGWYQHEMaYGbsfQ$AcmqasQgW/wI6wAHAMk4aQ'
'$argon2id$v=19$m=512,t=2,p=2$Wd3CJItnL93pSLG/Mvel2g$DnsqcrS7+Enwu8EdJrUX2Q'

This comment has been minimized.

@lvh

lvh Mar 22, 2018

sanity check: I'm assuming the contract only ever was "pass me a string back that I once gave you to begin with"?

This comment has been minimized.

@hynek

hynek Mar 23, 2018

Owner

There's not contract here really because the salt is random. So I basically created one by hand and put it there.

The current recommended best practice is as follow:
#. Choose whether you want Argon2i, Argon2d, or Argon2id (``type``).
If you don't know what that means, choose Argon2id (:attr:`argon2.Type.ID`).

This comment has been minimized.

@lvh
@@ -26,10 +27,11 @@ class PasswordHasher(object):
r"""
High level class to hash passwords with sensible defaults.
Uses *always* Argon2\ **i** and a random salt_.
Uses *always* Argon2\ **id** and a random salt_ for hashing, but it can

This comment has been minimized.

@lvh

lvh Mar 22, 2018

Suggestion: always uses, not uses always

This comment has been minimized.

@hynek

hynek Mar 23, 2018

Owner

Gah I always get this one wrong.

hash = _ensure_bytes(hash, "ascii")
try:
hash_type = self._header_to_type[hash[:9]]
except (IndexError, KeyError):

This comment has been minimized.

@lvh

lvh Mar 22, 2018

Suggestion: LookupError

#. Select the salt length.
16 bytes is sufficient for all applications, but can be reduced to 8 bytes in the case of space constraints.
#. Choose a hash length (``hash_len``, called "tag length" in the documentation).
16 bytes is sufficient for most applications, including key derivation.

This comment has been minimized.

@lvh

lvh Mar 22, 2018

I usually tell people to derive 256 bit symmetric keys, but you don't have to edit that. 16 is fine for password verification, definitely. Since the output of the derivation function has a header in it you would probably still wring it through HKDF or something.

This comment has been minimized.

@hynek

hynek Mar 23, 2018

Owner

I’ll rephrase it to make it about password verification.

@hynek

This comment has been minimized.

Owner

hynek commented Mar 23, 2018

So what happens when an attacker does get to mess with that record?

I’ve added a blurb reinforcing that argon2_cffi does not expect malicious hashes.

Additionally, you may want to make the types parameterizable and expose the fact that the password is not complaint with the current standard (be it type or either kind of cost) via an API, documenting the way people could upgrade their existing password stores that way

I’ve opened #35 and #36 for that if I understand you correctly.

As an aside, I'm not happy that somehow we had a PHC to finally get this right and apparently the answer is "choose between these 3 subtly different things based on specialized knowledge you're not supposed to have".

Ironically, it seems like we’ve just arrived there with Argon2id. :)


I made some updates (including stressing some more, that (ye)script is fine. It would be great if you could give it one last glance!

@hynek

This comment has been minimized.

Owner

hynek commented Mar 23, 2018

The last remaining question for me is the recommended hash verification time. Some people say 0.5ms others 500ms which is…quite the spread. :| I don't run anything at massive scale, so I'm not sure what to recommend?

@lvh

This comment has been minimized.

lvh commented Mar 23, 2018

Re: #35, #36: yep, that's a start, but not the entirety of it. One part is "given a token I am willing to accept and a password that hashes to it, should I re-hash the password to these new parameters?". That's #36 and #35 is a necessary prereq. The other question is: "given this token, should I be willing to attempt to validate a password against it at all"? I.e. what happens when the token tells me to allocate eleventy gajillion jiggabytes? That doesn't seem captured in either ticket.

Re: 5ms vs 500ms,more is better, but UX is the limiting factor. Some guidance:

  • No setting will save you from an awful password. At 500ms, the top 10k passwords still only take me less than two hours to try if I have no concurrency at all.
  • 500ms is pretty high. .5ms seems low. I get what you're saying, don't have ranges. I'd suggest 50ms as a default, but document that people have suggested anywhere from .5ms (the blog post) to 500ms (the RFC), and the real answer is "more is better and UX is the limiting factor", and 50ms is a good trade-off for most people. (Similarly I think the memory default is pretty high, but I'm OK with secure defaults :))
@lvh

This comment has been minimized.

lvh commented Mar 23, 2018

Some more concrete advice: libsodium has this nice thing where they have presets:

crypto_pwhash_{TYPE}LIMIT_INTERACTIVE
crypto_pwhash_{TYPE}LIMIT_MODERATE
crypto_pwhash_{TYPE}LIMIT_SENSITIVE

Perhaps that should map to roughly .5, 50, 500ms with 50 the default?

@hynek hynek merged commit 406833e into master Apr 9, 2018

6 checks passed

codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to a2ea053
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@hynek hynek deleted the agility branch Apr 9, 2018

@hynek

This comment has been minimized.

Owner

hynek commented Apr 9, 2018

I’ve added a clarification – thanks for you help! <3

@lvh

This comment has been minimized.

lvh commented Apr 9, 2018

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