Make PasswordHasher use Argon2id #34

hynek · 2018-03-22T12:16:08Z

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

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

codecov · 2018-03-22T12:38:59Z

Codecov Report

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

@@          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

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!

lvh · 2018-03-22T22:25:00Z

docs/api.rst

@@ -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'


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

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

lvh · 2018-03-22T22:26:04Z

docs/parameters.rst

+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`).


lvh · 2018-03-22T22:29:05Z

src/argon2/_password_hasher.py

@@ -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


Suggestion: always uses, not uses always

Gah I always get this one wrong.

lvh · 2018-03-22T22:31:06Z

src/argon2/_password_hasher.py

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


Suggestion: LookupError

lvh · 2018-03-22T22:34:43Z

docs/parameters.rst

+#. 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.


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.

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

hynek · 2018-03-23T08:07:44Z

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 · 2018-03-23T08:09:19Z

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 · 2018-03-23T13:54:38Z

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 · 2018-03-23T14:19:08Z

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 · 2018-04-09T10:40:39Z

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

lvh · 2018-04-09T11:24:22Z

Happy to help :)

…

Sent from my iPhone

On Apr 9, 2018, at 05:40, Hynek Schlawack ***@***.***> wrote: I’ve added a clarification – thanks for you help! <3 — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

hynek added 2 commits March 22, 2018 13:13

Make PasswordHasher use Argon2id

07e4f60

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

Add PR link

60fe139

hynek mentioned this pull request Mar 22, 2018

Argon2id should be the default #33

Closed

lvh approved these changes Mar 22, 2018

View reviewed changes

This was referenced Mar 23, 2018

Add helper function to parse encoded parameters #35

Closed

Add a way to PasswordHasher to check whether a hash has same parameters #36

Closed

Address review comments

cb121d2

Add clarification to verification times

3146346

hynek merged commit 406833e into master Apr 9, 2018

hynek deleted the agility branch April 9, 2018 10:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make PasswordHasher use Argon2id #34

Make PasswordHasher use Argon2id #34

hynek commented Mar 22, 2018

codecov bot commented Mar 22, 2018 •

edited

lvh left a comment

lvh Mar 22, 2018

hynek Mar 23, 2018

lvh Mar 22, 2018

lvh Mar 22, 2018

hynek Mar 23, 2018

lvh Mar 22, 2018

lvh Mar 22, 2018

hynek Mar 23, 2018

hynek commented Mar 23, 2018

hynek commented Mar 23, 2018

lvh commented Mar 23, 2018

lvh commented Mar 23, 2018 •

edited

hynek commented Apr 9, 2018

lvh commented Apr 9, 2018 via email

Make PasswordHasher use Argon2id #34

Make PasswordHasher use Argon2id #34

Conversation

hynek commented Mar 22, 2018

codecov bot commented Mar 22, 2018 • edited

Codecov Report

lvh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hynek commented Mar 23, 2018

hynek commented Mar 23, 2018

lvh commented Mar 23, 2018

lvh commented Mar 23, 2018 • edited

hynek commented Apr 9, 2018

lvh commented Apr 9, 2018 via email

codecov bot commented Mar 22, 2018 •

edited

lvh commented Mar 23, 2018 •

edited