fix(security): harden backup code login path (hashing, rate limit, CSRF) - #126
Open
palmoni5 wants to merge 1 commit into
Open
fix(security): harden backup code login path (hashing, rate limit, CSRF)#126palmoni5 wants to merge 1 commit into
palmoni5 wants to merge 1 commit into
Conversation
- Store backup codes as SHA-256 hashes instead of plain text. Codes that were generated before this change are still accepted (matched as plain text) so existing users are not locked out. - Generate codes with crypto.randomBytes (12 hex chars). The previous implementation only stripped the first dash from the UUID, which left a 10-character code. - Rate-limit POST /login/2fa/backup the same way as the TOTP path: one attempt at a time per uid, 2s delay on failure, 10s penalty when spammed. - Apply CSRF protection to POST /login/2fa/totp and POST /login/2fa/backup. The templates were already rendering the token but under a field name (`csrf`) that core never reads; rename it to `csrf_token`. - Normalise submitted backup codes (trim, lowercase, strip spaces/dashes).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hardens the backup-code and TOTP challenge routes. All three issues are present on
master(8.0.9).1. Backup codes were stored in plain text and only 40 bits long
utils.generateUUID().replace('-', '')removes only the first dash, so the resulting code was 10 hex characters (~40 bits). Codes were then stored verbatim in2factor:uid:<uid>:backupCodes, so anyone with read access to the database had a working second factor for every user.crypto.randomBytes(6)(12 hex chars).2. No rate limiting on
POST /login/2fa/backupprocessTotpLoginhas a per-uid lock (locks:totp:<uid>), a 2s delay on failure, and a 10s penalty when spammed.processBackuphad none of these, so backup codes could be brute-forced online with no throttling. This PR mirrors the TOTP behaviour usinglocks:backup:<uid>.3. No CSRF protection on the two challenge POSTs
POST /login/2fa/totpandPOST /login/2fa/backupran withoutapplyCSRF, while thePUT /login/2fa/backuproute on the next line already had it. The templates rendered a hidden field namedcsrf, but core's CSRF middleware only readscsrf_token/_csrf, so the token was never checked. Both routes now usehostMiddleware.applyCSRFand the templates sendcsrf_token.Notes for review
csrf_tokenfield.locks:backup:<uid>is cleared on success, on failure (after the 2s delay) and on exception, matching the TOTP path.