snapframework / snap

Top-level package for the official Snap Framework libraries, includes the snaplets API as well as infrastructure for sessions, auth, and templates.
http://snapframework.com/
BSD 3-Clause "New" or "Revised" License
455 stars 68 forks source link

Moving away about pwstore-fast? #204

Open galenhuntington opened 6 years ago

galenhuntington commented 6 years ago

Snap uses pwstore-fast for its password hashing needs in the authentication snaplet. That package however is troubling in many ways:

  1. It appears abandoned. The last commit was almost four years ago, with issues and PRs going unanswered for about as long. Worryingly, there is an open issue (https://github.com/PeterScott/pwstore/issues/12) about one function possibly giving wrong results.

  2. It by default uses the obsolete and less secure PBKDF1, which is superseded by PBKDF2. The latter is (maybe) supported in pwstore-fast, but the snaplet uses the default. PBKDF1 should not be used for new applications (RFC 2898).

  3. It uses a custom format for storing the password hash, as opposed to established and portable formats such as bcrypt or MCF generally or PHC. While this is not necessarily a problem, a lack of vetting means stuff can be missed, and indeed the format fails to encode the derivation function. So, while an app can increase strength or change hashing functions for new users, as these are encoded, it cannot readily switch to PBKDF2 without locking out old users.

  4. It uses System.Random to generate salts, which is not cryptographically secure. This is a minor weakness, but several Haskell packages offer secure RNGs that could be used instead.

  5. It depends on the deprecated and unmaintained cryptohash package, which is thus pulled in and installed, and also whose module names conflict with its replacement cryptonite.

The ideal would be to just switch to a new algorithm; there's PBKDF2 and bcrypt and scrypt and Argon2 and so on. But that would break production uses of Snap auth.

There may be a backwards-compatible way to revise hashing while avoiding the above problems. Alternatively, auth could be modularized off into its own packages, so (e.g.) snaplet-old-auth and snaplet-auth-?? can coexist.

There should also be a way to customize the cost parameter(s), as the appropriate value changes (increases) over time.

I have never used the auth snaplet, and in fact removed those modules from my local install, for these reasons, so I don't have an informed opinion on what to do with it. But pinning to dated tech seems a bad option.

(Some of these issues were brought up in #85, which seems to have gone stale.)

tom-bop commented 6 years ago

Good summary! Any thoughts @mightybyte ?

@galenhuntington I'm I'm curious about your point (from bullet point 3):

So, while an app can increase strength or change hashing functions for new users, as these are encoded, it cannot readily switch to PBKDF2 without locking out old users.

Could there not be a scheme where (after switching new users to PBKDF2) for existing users:

galenhuntington commented 6 years ago

Could there not be a scheme where (after switching new users to PBKDF2) for existing users:

The problem is that there's no indication in the string of whether it is PBKDF1 or PBKDF2. However, the scheme could first try with PBKDF2, and if that doesn't match, try with PBKDF1. The latter can as you said then be upgraded; indeed, code support for upgrading hashes is desirable anyway, for instance to increase the cost parameter to keep pace with hardware.

Disadvantages: It doubles the wait time for mistyped passwords and triples the login time for old password hashes (though only once). Also, with no indication of which it is, if, say, after a security audit it is decided to force all users to PBKDF2, there isn't an easy way to see who has upgraded, and code support for PBKDF1 will have to be kept around indefinitely.

(The app could also store separately which PBKDF it is, but that adds complexity, or it could rely on the last login timestamp, but that is fragile.)

That said, these aren't huge problems, and this is a feasible upgrade path. In fact, even if a snaplet-old-auth is spun off, it may be preferable to at least get its users on PBKDF2.

There are still my other concerns. And I think in the long run it is best to use a current, standardized password hashing algorithm with a standard format. My personal opinion is that PBKDF2 is fourth-best after Argon2, scrypt, and bcrypt (in that order). PBKDF2 has one advantage, though, that it has NIST's imprimatur.

mightybyte commented 6 years ago

I agree that we should use a better hashing scheme. And if we're going to do that, we should use whatever is best in class and build in the ability to upgrade the hash function in the future. I also care a lot about preserving backwards compatibility. Reducing our dependency footprint would also be really nice. If we're going to make any of these changes we might as well try to hit as many of them as possible in one stroke.

I'm thinking that splitting the existing auth out into a separate snap-old-auth package is probably the way to go. That way upgrading existing apps is as simple as bumping bounds and adding snap-old-auth as a dependency.

I have other things I've been wanting to do to improve auth. Most notably, building it in a way that is still multi-backend but doesn't require the use of Snap's AuthUser data type and allows users to bring their own type.