-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Update crypto library, CryptoJS CVE & deprecation #9350
Conversation
…nd update in database when logging in a user
const md5Hashes = [] | ||
let digest = password | ||
for (let i = 0; i < 3; i++) { | ||
md5Hashes[i] = crypto.createHash('md5').update(digest).digest() |
Check failure
Code scanning / CodeQL
Use of password hash with insufficient computational effort High
an access to password
Password from
an access to password
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the legacy decrypt function, we know it's insecure, that's why we're making this PR!
const md5Hashes = [] | ||
let digest = password | ||
for (let i = 0; i < 3; i++) { | ||
md5Hashes[i] = crypto.createHash('md5').update(digest).digest() |
Check failure
Code scanning / CodeQL
Use of a broken or weak cryptographic algorithm High
A broken or weak cryptographic algorithm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the legacy decrypt function, we know it's insecure, that's why we're making this PR!
Mentioned to @cannikin about the idea we should store the scrypt hash parameters with the hash into the database. This would allow us to change the default hash parameters should it be required to maintain security. Otherwise it would not be possible to know what set of parameters were used when we need to compare input to an existing hash. This is something you can see with argon2 where the encoded hash typically looks like: |
# Conflicts: # packages/cli/package.json # yarn.lock
…rt to string first?
…to convert to string first?" This reverts commit 537c5da.
A change to one of the same files I'm editing had a merge conflict from |
@cannikin re the |
@cannikin we can remove crypto-js now right? looks like there's no imports from it but still listed in a few package.json files |
Oh yeah! I think I saw it in the lock and assumed it was a child dep of some other package, but maybe it was actually us! |
these tests (and others) were using an older more verbose playwright api. playwright now recommends using aria-backed selectors (`page.getByRole` instead of `page.locator`). i've also added some logic to the authChecks graphql test so that you can run it more than one time in succession locally. before you'd have to reset your database between runs. lastly, i don't think the `Promise.race()` logic was correct. fixed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @cannikin, let's get this in the patch RC
So CryptoJS just dropped a bomb: everything they do by default is not as strong as it could be. Oh and by the way, the entire library is now deprecated. :( GHSA-xwcq-pm8m-c4vf Unfortunately we can't just upgrade to the latest release 4.2.0 because the hashing algorithm has changed, and a user would no longer be able to login: the default hash generated by CryptoJS 4.2.0 won't match the hash generated by CryptoJS 4.1.0. Note that this is only an issue if someone got the contents of your database and wanted to figure out user passwords (but it still cost [$45,000 per password](https://eprint.iacr.org/2020/014.pdf) apparently?). In the wake of this CVE we're going to convert dbAuth to use the built-in `node:crypto` library instead, with more sensible default configuration. There are two areas where we use the crypto libs: 1. Hashing the user's password to store in the DB and compare on login 2. Encrypting/decrypting the session data in a cookie We're going to do this in a non-breaking way by supporting *both* the original CryptoJS-derived values, and the new `node:crypto` ones. The alternative would be to require every user to change their password, which seems like a non-starter. 1. On signup, store the hashedPassword using the new `node:crypto` algorithm 2. On login, compare the user's hashedPassword using the `node:crypto` algorithm: * If a match is found, user is logged in * If a match fails, fall back to the original CryptoJS algorithm (but using the `node:crypto` implementation) * If a match is found, update the `hashedPassword` in the database to the new algorithm, user is logged in * If a match is still not found, the user entered the wrong password. Likewise for cookies and login: 1. When encrypting the user's session, always use the new `node:crypto` algorithm 2. When decrypting the user's session, first try with `node:crypto` * If decrypting works, user is logged in * If decrypting fails, try the older CryptoJS algorithm ([I haven't figured how](brix/crypto-js#468) to use `node:crypto` to decrypt something that was encrypted with CryptoJS yet, so we'll need to keep the dependency on CryptoJS around for now) * If decrypting works, re-encrypt the cookie using the new `node:crypto` algorithm, user is logged in * If decrypting still fails, the session is invalid (someone tampered with the cookie) so log them out We could announce in the Release Notes that if a platform wants the absolute safest route, they should change their `SESSION_SECRET` *and* have users change their password if, for example, they suspect that their database may have been compromised before our release. The next most secure thing would be to just change `SESSION_SECRET` which would log everyone out, and on next login their password will get re-hashed with the new algorithm. But the default for most people will probably be to just go about business as usual, and as time goes by more and more users' passwords will be re-hashed on login. Related to #9337 #9338 #9339 #9340 --------- Co-authored-by: Dominic Saadi <[email protected]>
So CryptoJS just dropped a bomb: everything they do by default is not as strong as it could be. Oh and by the way, the entire library is now deprecated. :( GHSA-xwcq-pm8m-c4vf Unfortunately we can't just upgrade to the latest release 4.2.0 because the hashing algorithm has changed, and a user would no longer be able to login: the default hash generated by CryptoJS 4.2.0 won't match the hash generated by CryptoJS 4.1.0. Note that this is only an issue if someone got the contents of your database and wanted to figure out user passwords (but it still cost [$45,000 per password](https://eprint.iacr.org/2020/014.pdf) apparently?). In the wake of this CVE we're going to convert dbAuth to use the built-in `node:crypto` library instead, with more sensible default configuration. There are two areas where we use the crypto libs: 1. Hashing the user's password to store in the DB and compare on login 2. Encrypting/decrypting the session data in a cookie We're going to do this in a non-breaking way by supporting *both* the original CryptoJS-derived values, and the new `node:crypto` ones. The alternative would be to require every user to change their password, which seems like a non-starter. 1. On signup, store the hashedPassword using the new `node:crypto` algorithm 2. On login, compare the user's hashedPassword using the `node:crypto` algorithm: * If a match is found, user is logged in * If a match fails, fall back to the original CryptoJS algorithm (but using the `node:crypto` implementation) * If a match is found, update the `hashedPassword` in the database to the new algorithm, user is logged in * If a match is still not found, the user entered the wrong password. Likewise for cookies and login: 1. When encrypting the user's session, always use the new `node:crypto` algorithm 2. When decrypting the user's session, first try with `node:crypto` * If decrypting works, user is logged in * If decrypting fails, try the older CryptoJS algorithm ([I haven't figured how](brix/crypto-js#468) to use `node:crypto` to decrypt something that was encrypted with CryptoJS yet, so we'll need to keep the dependency on CryptoJS around for now) * If decrypting works, re-encrypt the cookie using the new `node:crypto` algorithm, user is logged in * If decrypting still fails, the session is invalid (someone tampered with the cookie) so log them out We could announce in the Release Notes that if a platform wants the absolute safest route, they should change their `SESSION_SECRET` *and* have users change their password if, for example, they suspect that their database may have been compromised before our release. The next most secure thing would be to just change `SESSION_SECRET` which would log everyone out, and on next login their password will get re-hashed with the new algorithm. But the default for most people will probably be to just go about business as usual, and as time goes by more and more users' passwords will be re-hashed on login. Related to #9337 #9338 #9339 #9340 --------- Co-authored-by: Dominic Saadi <[email protected]>
So CryptoJS just dropped a bomb: everything they do by default is not as strong as it could be. Oh and by the way, the entire library is now deprecated. :( GHSA-xwcq-pm8m-c4vf Unfortunately we can't just upgrade to the latest release 4.2.0 because the hashing algorithm has changed, and a user would no longer be able to login: the default hash generated by CryptoJS 4.2.0 won't match the hash generated by CryptoJS 4.1.0.
Note that this is only an issue if someone got the contents of your database and wanted to figure out user passwords (but it still cost $45,000 per password apparently?).
In the wake of this CVE we're going to convert dbAuth to use the built-in
node:crypto
library instead, with more sensible default configuration.There are two areas where we use the crypto libs:
We're going to do this in a non-breaking way by supporting both the original CryptoJS-derived values, and the new
node:crypto
ones. The alternative would be to require every user to change their password, which seems like a non-starter.node:crypto
algorithmnode:crypto
algorithm:node:crypto
implementation)hashedPassword
in the database to the new algorithm, user is logged inLikewise for cookies and login:
node:crypto
algorithmnode:crypto
node:crypto
to decrypt something that was encrypted with CryptoJS yet, so we'll need to keep the dependency on CryptoJS around for now)node:crypto
algorithm, user is logged inNotifying Users
We could announce in the Release Notes that if a platform wants the absolute safest route, they should change their
SESSION_SECRET
and have users change their password if, for example, they suspect that their database may have been compromised before our release.The next most secure thing would be to just change
SESSION_SECRET
which would log everyone out, and on next login their password will get re-hashed with the new algorithm.But the default for most people will probably be to just go about business as usual, and as time goes by more and more users' passwords will be re-hashed on login.
Related to #9337 #9338 #9339 #9340