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

Moving away about pwstore-fast? #204

Open
galenhuntington opened this issue Aug 18, 2018 · 3 comments
Open

Moving away about pwstore-fast? #204

galenhuntington opened this issue Aug 18, 2018 · 3 comments

Comments

@galenhuntington
Copy link

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 (pbkdf2 seems to generate a wrong hash PeterScott/pwstore#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
Copy link
Contributor

tom-bop commented Aug 23, 2018

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:

  • Existing user submits password when attempting to log in
  • Check password against PBKDF1 hash
  • If the password matches, replace it with the PBKDF2 hash
  • Log the user in

@galenhuntington
Copy link
Author

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
Copy link
Member

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.

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

No branches or pull requests

3 participants