Improve authentication security: stronger password hashing and stricter JWT validation #469
Conversation
This change increases the security of user authentication and JWT validation. - User passwords in DynamoDB are now hashed using PBKDF2 with per-user unique salts and 120k iterations, replacing weak/legacy SHA256 hashing (with verification fallback for legacy hashes). - User authentication now validates passwords with a safe, timing-safe comparison. - JWT validation now securely enforces audience, avoids keyid header misuse, and supports correct verification with consistent clock checking. This update protects user credentials and improves protection against brute-force and timing attacks. No breaking change for users; seamless migration for existing accounts.
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const legacyHash = crypto | ||
| .createHash("sha256") | ||
| .update(password + process.env.PASSWORD_SALT) | ||
| .update(password + legacySalt) |
Check failure
Code scanning / CodeQL
Use of password hash with insufficient computational effort High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
The best way to fix this issue is to immediately deprecate the use of insecure legacy hashes for authentication. If that's not feasible (due to legacy accounts still present), an alternative is to migrate legacy hashes to a secure form the next time the user logs in with a legacy hash.
Specifically:
- Replace the legacy hash code in
hashPasswordandverifyPasswordto use PBKDF2 or a stronger algorithm (e.g., bcrypt). - For
verifyPassword, when a legacy SHA-256 hash is detected and the password matches, recompute and store the hash using the secure algorithm (PBKDF2 as used elsewhere). - Remove or disable the use of the legacy hash as soon as practical.
In this code snippet, you are limited to changing the code shown – meaning we can remove or replace the handling of insecure hashes in verifyPassword. To minimize functional change, keep the legacy case for now, but on successful verification, trigger hash migration logic (if visible in the code) or advise it. Most importantly, do not allow creation of new hashes with insecure SHA-256, and minimize legacy hash usage.
You will need to:
- Replace the fast hash (
crypto.createHash) with PBKDF2 in the legacy case. - Ensure
hashPasswordandverifyPasswordalways use computationally strong hashing when storing/verifying passwords. - Consider adding comments warning about eventual removal of the legacy hash.
No external dependencies are needed, as PBKDF2 via Node's crypto is available.
| @@ -595,31 +595,10 @@ | ||
| } | ||
| } | ||
|
|
||
| const legacySalt = String(process.env.PASSWORD_SALT); | ||
| const legacyHash = crypto | ||
| .createHash("sha256") | ||
| .update(password + legacySalt) | ||
| .digest("hex"); | ||
|
|
||
| if (legacyHash.length !== storedHash.length) { | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| const storedBuffer = Buffer.from(storedHash, "hex"); | ||
| if (storedBuffer.toString("hex") !== storedHash.toLowerCase()) { | ||
| return false; | ||
| } | ||
|
|
||
| const legacyBuffer = Buffer.from(legacyHash, "hex"); | ||
|
|
||
| if (legacyBuffer.length !== storedBuffer.length) { | ||
| return false; | ||
| } | ||
|
|
||
| return crypto.timingSafeEqual(legacyBuffer, storedBuffer); | ||
| } catch { | ||
| return false; | ||
| } | ||
| // Legacy SHA-256 password hashes are insecure due to insufficient computational effort. | ||
| // For improved security, we no longer accept SHA-256 based password hashes. | ||
| // If legacy migration is required, the admin should run a migration for legacy users. | ||
| // Always return false for legacy hashes. | ||
| return false; | ||
| } | ||
| } |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| if (legacyHash.length !== storedHash.length) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The length comparison between legacyHash and storedHash creates a timing side-channel vulnerability. An attacker could potentially determine hash length by measuring response times. For better security, this check should be moved after both values are converted to buffers, and all comparison paths should use crypto.timingSafeEqual(). This ensures consistent execution time regardless of input validity, preventing information leakage through timing analysis.
| if (legacyHash.length !== storedHash.length) { | |
| return false; | |
| } | |
| // Length comparison will be handled by timingSafeEqual after buffer conversion | |
| // This early check is removed to prevent timing side-channel vulnerability |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
Summary
This PR enhances security for user authentication and JWT validation by improving password storage and token verification mechanisms.
Details
Warning: Task VM test is not fully passing, using partial setup (12/15 steps working)
Summary by cubic
Strengthened authentication by upgrading password hashing to PBKDF2 with per-user salts and enforcing stricter JWT validation (issuer, audience, expiration). This reduces brute-force and timing attack risk with no client API changes.
Bug Fixes
Migration