Repository navigation
Password Hashing API #21766
Description
In a few words: The new scrypt API in v10.5 is dangerously designed and will cause people to make security mistakes. A new API is needed for the 98% problem (i.e. password hashing). Additionally, the documentation for the old API should make it abundantly clear that it's not meant for non-expert users, and to instead use whatever the new API is called.
Copying my comment (with slight corrections) from #20816
This is how PHP does password hashing:
// All in one go:
$hash = password_hash($userProvidedPassword, PASSWORD_DEFAULT);
// Later:
if (password_verify($userProvidedPassword, $hash)) {
// Everything works out. Optionally, do this too:
if (password_needs_rehash($userProvidedPassword, PASSWORD_DEFAULT)) {
// Someone upgraded PHP and there's a new default algorithm. We should rehash the password.
}
}This is almost an optimal UX for PHP developers for password storage: Salts are explicitly generated by the kernel's CSPRNG, encoded, and included in the password hash.
A step further would be making PASSWORD_DEFAULT--, well, the default.
You should only need one input for generating a new password hash: The password.
You should only need two inputs for verification: The challenge and the attempt (or the hash and password, respectively).
All other parameters should be optional.
Note: Key derivation is a separate concern, where you don't want the salt stored alongside the password hash, but that's a smaller corner of the cryptography usability market than password hashing.
Let's look at how another library handles both corner cases:
Key Derivation
#define PASSWORD "Correct Horse Battery Staple"
#define KEY_LEN crypto_box_SEEDBYTES
unsigned char salt[crypto_pwhash_SALTBYTES];
unsigned char key[KEY_LEN];
randombytes_buf(salt, sizeof salt);
if (crypto_pwhash
(key, sizeof key, PASSWORD, strlen(PASSWORD), salt,
crypto_pwhash_OPSLIMIT_INTERACTIVE, crypto_pwhash_MEMLIMIT_INTERACTIVE,
crypto_pwhash_ALG_DEFAULT) != 0) {
/* out of memory */
}Password Hashing
#define PASSWORD "Correct Horse Battery Staple"
char hashed_password[crypto_pwhash_STRBYTES];
if (crypto_pwhash_str
(hashed_password, PASSWORD, strlen(PASSWORD),
crypto_pwhash_OPSLIMIT_SENSITIVE, crypto_pwhash_MEMLIMIT_SENSITIVE) != 0) {
/* out of memory */
}
if (crypto_pwhash_str_verify
(hashed_password, PASSWORD, strlen(PASSWORD)) != 0) {
/* wrong password */
}Usable cryptography API design is a nontrivial undertaking, and getting it wrong will mean years (or even decades) of clean-up.
I would propose, at minimum, an alternative API that has a similar user experience to PHP's password_hash() that transparently wraps scrypt as part of the Node.js core, and then recommend that API for users who want to store password hashes (which is roughly 98% of the use case when someone reaches for bcrypt, scrypt, or Argon2).
Or you could always just incorporate the design of @joepie91's scrypt-for-humans into the Node.js core. Designing cryptography APIs for humans is a good idea.
Activity
joepie91 commentedon Jul 11, 2018
(Continuing on from the previous thread.)
@ChALkeR While I understand that strictly speaking it cannot be removed due to stability concerns, this was merged less than a month ago, and the security impact of this issue is significant. It is very unlikely that the feature has seen any real-world adoption yet, especially considering that there have been userland options available for a long time, and the addition of this feature to core has not been widely publicized.
Taking into account these data points, I think removing the API is justifiable even under the stability constraints of Node.js core.
Documentation warnings, while better than nothing, are a substandard option; these warnings are often ignored, and they rarely get included in third-party documentation about a feature (eg. tutorials, screencasts, and so on). They are not likely to be an effective deterrent.
All things considered, removal-for-the-time-being of the feature seems the most reasonable course of action here.
vsemozhetbyt commentedon Jul 11, 2018
cc @nodejs/crypto
ChALkeR commentedon Jul 11, 2018
@joepie91 Do you think those concerns could be resolved with non-breaking changes?
E.g.: warnings in docs, making
saltoptional and suggesting to avoid supplying it, introducing a safe verification API, even more doc fixes?joepie91 commentedon Jul 11, 2018
I don't see a reasonable way in which this could be fixed in a non-breaking manner, personally; documentation warnings 'rot away' in the copy-pasting mill of tutorials and such, and an optional
saltparameter would likely still cause confusion as to whether it's "more secure" if provided; I've found the idea that "more explicit options means safer" to be rather common amongst developers.But most critically: the current return format is totally incompatible with a safe verification API. For a safe verification API to work as suggested, all the options and the salt need to be encoded into the output of the hashing function (such that the verification function can reproduce the hash), but as far as I can tell it only returns a 'raw' derivedKey right now. Changing the return format would be a breaking change either way.
As an aside regarding the option of removing the unsafe API: 10.x is not yet in LTS, so the constraints on what can be changed API-wise are a little less strict, if I understand correctly.
Also, perhaps it's a good idea to ping the Security WG on this as well?
ChALkeR commentedon Jul 11, 2018
/cc @nodejs/security-wg
tniessen commentedon Jul 11, 2018
I completely understand the issue, and the usability of the new API has already been criticized in the PR. However, this API was not designed to provide a comfortable way to hash passwords but to provide a full-featured scrypt implementation. If a crypto API claims to implement scrypt, I expect to be able to specify the salt and additional parameters and to get the raw, unfiltered hash out of it.
We can probably add a new API that is designed for easy-to-use password hashing similar to what PHP does, but it has downsides. For example, PHP includes metadata in the output, something you probably would not expect, and which can easily break interoperability with other libraries.
Many of these arguments apply to other crypto APIs: Users have to set certain parameters, derive IVs for cipher objects, pick cipher algorithms etc., and I still wouldn't want to remove all those features and just use defaults everywhere. If you look at the crypto commit log, you will see that we are doing a lot to prevent people from improperly using these APIs, but there are no guarantees. Again, we could add "simpler" APIs for each and every little crypto task (
encrypt,encryptWithAuthentication, ...), but even having those does not justify removing the feature-complete ("complex") APIs.Summary: I am not opposed to a new API that is designed for easy-to-use password-hashing, but having a full-featured scrypt API is justified.
paragonie-scott commentedon Jul 11, 2018
Specifically, they use a
crypt(3)-compatible output.I've started on an implementation of a password-hashing wrapper to submit in a pull request (which will use scrypt by default). I hope to have a first draft ready to be reviewed this evening.
tniessen commentedon Jul 11, 2018
@ChALkeR I am removing the
securitylabel. Based on my understanding, the only security risk is improper use of this API, which applies to most if not all existing crypto APIs. Feel free to add the label again if the security risk extends beyond that.paragonie-scott commentedon Jul 11, 2018
Correct, but with a caveat worth noting: cryptography API design is very important to application security in a lot of ways that aren't always immediately obvious.
Node.js cryptography API security is, therefore, emphatically a security issue that affects Node.js.
Whether or not that fact is sufficient to qualify for the
securitylabel as is used internally by Node.js is something I will not speak to, because I do not know how your team manages labels/delegation.As long as we're all clear on that note, let's carry on.
deian commentedon Jul 11, 2018
I couldn't agree more with @joepie91 and @paragonie-scott. "Experts" can always write their native add-on and use the underlying crypto spaceship that is OpenSSL (to quote Matthew Green), they can also get at low-level implementations indirectly (e.g., we can define something like crypto-subtle) but the front-and-center APIs should address the 99% use case and make it impossible to misuse.
Adding red tape warnings to the docs is a good start and maybe best be can do without breaking compat (unless there are usage metrics that say it's not really used), but we should really try to set up a process for crypto and security APIs to avoid repeating OpenSSL/WebCrypto pitfalls.
drifkin commentedon Jul 11, 2018
I opened an issue in the security-wg nodejs/security-wg#339 to discuss some of the broader issues that are being brought up here.
I personally think that we should be very careful adding features to
cryptothat are likely to be misused. I know that there's a lot of precedent here with the other exposed OpenSSL APIs, but if we want to continue in that direction, IMO we should strongly discourage average developers from using these low level APIs. Developers are commonly told to "just use scrypt", so by providing this API, I suspect this will lead to a lot of people unknowingly creating security vulnerabilities in their applications.19 remaining items
drifkin commentedon Jul 19, 2018
👍@ChALkeR: I think given that reverting is considered semver major, this is a very good solution. As @joepie91 says above this will make it really unlikely that someone unknowingly uses the "unsafe" version.
Just a reminder that for the high-level API, we'll need more than just
crypto.scrypt(), since we'll need a corresponding verification function in order to prevent timing attacks from users manually checking hash equality (originally brought up in #20816 (comment)). But we can work out the full API details once we decide whether this is the way to go. And from nodejs/security-wg#339 (comment), it sounds like @tniessen and @joepie91 have already been discussing this together, though in a slightly different context.lirantal commentedon Jul 19, 2018
The
unsafekind of naming convention has also been employed by the React team with thedangerouslySetInnerHTML(ref: https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml) but I believe this should be scoped to some methods and not the entire module (as in, not to rename the module itself tocrypto-unsafe)bricss commentedon Jul 19, 2018
And I still believe that it's better to update the documentation, than rename or spawn methods in API.
If by default we assume that developers do not read the documentation and ignore all warnings in it, then probably we don't need documentation at all.
Documentation should lead to clarity. Developers should read and understand what is going on behind the wheel, in any way, and not blindly use some magic wand API.
Just take a look at Haskell example: http://hackage.haskell.org/package/scrypt/docs/Crypto-Scrypt.html
Plus we already have high-level lib for it: https://www.npmjs.com/package/scrypt-for-humans
paragonie-scott commentedon Jul 19, 2018
Developers aren't homogenous. Some will read the official documentation directly (and keep themselves up to date), others will understand it filtered through podcasts, vlogs, blogs, and technical discussions on websites like Reddit or HN.
What mechanism do you have to ensure all the secondary and tertiary sources of developer understanding beyond the official documentation get updated in a timely fashion? How do you inform every Node.js developer on the planet that they need to refresh their current understanding of the language and recent changes?
Because arguing for "just update the documentation" just kicks the can down the road, so that the other developers can be "blamed" for writing insecure code, and doesn't actually improve security.
The author of the high-level library is @joepie91 who I am quite certain disagrees with your argument in this discussion. (He can speak for himself on this point, of course.)
joepie91 commentedon Jul 19, 2018
This argument doesn't make a lot of sense. The point of documentation is to explain, and I never called into question the effectiveness of using documentation for that purpose. What I'm calling into question is the effectiveness of using it to warn, which is pretty well-known to just.. not work.
Therefore, people ignoring warnings in documentation (which they frequently do, especially when writing their own material, since it is in their eyes not necessary for understanding the documented thing), does not in any way translate to "we don't need documentation at all"; it still fulfills the purpose of explanation just fine. We just shouldn't rely on it for the purpose it's bad at, ie. "warning people".
I fully agree. Unfortunately, that's not what developers actually do, with copious security issues as a result (that people other than the developer suffer from!), and there is no reason to believe that this will ever change given human nature. Therefore everybody in the chain, including the core team, has a responsibility to strive for harm reduction even for the cases where developers just plain aren't paying attention.
Essentially, "developers should just pay attention" is an ideological argument, not a pragmatic one.
As @paragonie-scott already pointed out, I'm the author of that library. It currently wraps
node-scrypt, and is no substitute for a safe API in core, given that many developers assign special importance to implementations provided by core and/or standards (even when that is not justified).In fact, the way I found out about this addition to core in the first place, was because somebody said "it's in core now!" when I suggested to somebody that they should use
scrypt-for-humans. Just to illustrate the point made above.Trott commentedon Aug 13, 2018
@ChALkeR Should this still be left on the TSC agenda? If so, will you be able to attend this week's meeting or is this likely to need to be discussed at a future meeting instead?
fhinkel commentedon Aug 29, 2018
Ping @ChALkeR
mhdawson commentedon Sep 11, 2018
@ChALkeR Should this still be left on the TSC agenda? If so, will you be able to attend this week's meeting or is this likely to need to be discussed at a future meeting instead?
targos commentedon Sep 17, 2018
Ping @ChALkeR
ChALkeR commentedon Sep 19, 2018
@Trott @fhinkel @mhdawson @targos
I don't think this needs to be discussed at the meeting now, so I de-labeled it.
I think that I will add the label again when this will need further discussion.
Sorry for the lack of response here :-/.
bnoordhuis commentedon Feb 10, 2019
I'm closing out this issue because the discussion seems to have stalled without reaching consensus. One way forward is to propose a generic, higher-level API - i.e., covering more than just scrypt.
crypto.scrypt()isn't going anywhere at this point: it's been out for some time now and it's in use by applications and libraries.