Skip to content

Commit 8f20d76

Browse files
authored
sec(auth): fail-closed MFA, CSRF HMAC derivation, bounded last-used, errors.Is (#1018 #1027 #1028 #1029) (#1044)
* sec(auth): verifyTOTP and consumeRecoveryCode fail closed on empty input (#1018) verifyTOTP("","") previously returned true because generateTOTP("") returns "" on base32-decode failure, and ConstantTimeCompare("","") == 1. This created an MFA-bypass primitive callable by any session holder via MFARegenerateRecoveryCodes with code="". - verifyTOTP: early-return false when code=="" or secret=="" - consumeRecoveryCode: early-return false when normalized input is empty - Fix misleading comment at verifyPasswordAndMFA (service.go:212) that described the old panic-safe behaviour; the function now fails closed Closes #1018 * fix(auth): bound API key last-used updates via singleflight (#1027) ValidateUserAPIKey fired one unbounded goroutine per request to update the last-used timestamp. Under a burst of authenticated requests (or a revoked key being presented repeatedly), this spawned an unbounded number of goroutines writing to the DB with context.Background() and logged a Warn for every "key not found" event. - Replace raw go func() with singleflight.Group.Do: at most one DB write per key is in-flight at any moment; concurrent requests for the same key are coalesced rather than each spawning a writer - Demote "failed to update last-used" log from Warn to Debug Closes #1027 * fix(credentials): use errors.Is for pgx.ErrNoRows sentinel in LoadRaw (#1028) store.go:LoadRaw used direct pointer equality (err == pgx.ErrNoRows). Pool and transaction wrappers wrap the sentinel, so the check returned false and LoadRaw propagated a hard error instead of the documented (nil, nil) "not found" contract, breaking callers like resolveAccessKeyProvider. - Change to errors.Is(err, pgx.ErrNoRows) to unwrap the error chain - Add errors import Closes #1028 * sec(auth): derive CSRF token as HMAC-SHA256 of session token, not random (#1029) The CSRF token was a random hex string stored plaintext in the sessions row alongside the hashed session token. A DB read (SQLi, backup, replica) yielded a directly usable CSRF token, undermining the double-submit protection. New approach (no DB migration needed): - createSession derives CSRF as HMAC-SHA256(csrfKey, rawSessionToken) - The MAC is written to csrf_token for diagnostics/observability but is never used for validation - ValidateCSRFToken recomputes the HMAC from the raw session token and compares with the presented token; the stored column is not read - csrfKey added to Service struct and ServiceConfig; if not supplied, NewService generates a random ephemeral key and logs a warning The token is now cryptographically bound to the session token AND the server-side key. A DB read alone cannot produce a forgeable CSRF token. Closes #1029 * test(auth,credentials): add regression tests for #1018, #1027, #1028, #1029 - TestVerifyTOTP_FailClosed: verifyTOTP("","") must return false (was true) - TestConsumeRecoveryCode_EmptyInput: empty code must be rejected before bcrypt - TestMFARegenerateRecoveryCodes_EmptyCode: empty code must return error without calling UpdateUser - TestCSRFToken_DerivedFromSessionToken: CSRF equals HMAC(csrfKey, rawToken); ValidateCSRFToken accepts derived token, rejects cleartext legacy tokens - TestCSRFToken_CrossSessionRejected: CSRF token from session A is rejected for B - TestValidateUserAPIKey_LastUsedSingleflight: background last-used update runs; done channel replaces time.Sleep per feedback_no_sleep_in_tests - TestLoadRaw_ErrorsIs_WrappedNoRows: errors.Is unwraps a wrapped pgx.ErrNoRows where == would return false (the pre-fix bug) * fix(credentials): explicit 32-byte key guard + hard error on multi-key config 03-M1: add `len(key) != 32` guard at the top of Encrypt and Decrypt so a 16- or 24-byte key returns a clear error rather than silently selecting AES-128/192. AES spec accepts shorter keys without error; the guard is needed because callers can supply a key directly, bypassing LoadKey. 03-M2: treat multiple key-source env vars simultaneously set as a hard startup error (ErrMultipleKeys) instead of a WARN + first-wins. Ambiguous key selection on a data-encryption key is a misconfiguration that must not proceed silently, especially given the rekey workflow. Regression tests updated/extended for both changes. Closes #1058 (partial) * fix(secrets): GCP ListSecrets uses client-side HasPrefix, not API filter expression 03-M3: The Resolver interface documents GCP as using strings.HasPrefix filtering, but GCPResolver.ListSecrets was forwarding the caller's filter directly as a GCP expression-language Filter field, making the three implementations (Azure/env use HasPrefix; GCP used expression-language; AWS does API substring) inconsistent. Fix: remove the Filter from the ListSecretsRequest; apply strings.HasPrefix on the short name (path.Base) client-side so all implementations honour the documented contract. Updated regression tests confirm the API request carries no Filter expression. Closes #1058 (partial) * sec(oidc): validate Azure exponent range and precise half-configured error 03-M6: guard `new(big.Int).SetBytes(E).Int64()` in AzureKeyVaultSigner.resolveOnce with an IsInt64 + range check before the int() conversion. An oversized or non-positive exponent now returns an explicit error instead of silently corrupting the rsa.PublicKey and producing an unusable JWKS. 03-M7: NewSignerFromEnv Azure branch now emits a precise error when exactly one of CUDLY_SIGNING_KEY_VAULT_URL and CUDLY_SIGNING_KEY_NAME is set, naming the missing variable, instead of deferring to the constructor's generic error. Regression tests added for both cases. Closes #1058 (partial) * fix(auth): rate-limit password reset requests per email 03-M5: RequestPasswordReset now silently no-ops when a reset token was issued within PasswordResetRateLimit (1 minute). An adversary who knows a victim's email could previously call the endpoint repeatedly to perpetually invalidate the victim's legitimate reset link. Detection is via the existing PasswordResetExpiry field (no schema change): if the token's age is less than PasswordResetRateLimit, the function returns nil without overwriting the token or sending a new email. The existing token remains valid. The response is indistinguishable from a successful request to preserve email-enumeration protection. Regression test: TestService_RequestPasswordReset_RateLimit. Closes #1058 (partial) * fix(auth,oidc): L1-L6 low-severity hardening in auth and OIDC subsystems 03-L1: redactEmail now masks both local prefix and domain hostname (was "us***@example.com", now "us***@ex***.com") to reduce PII exposure for low-entropy addresses in debug logs. 03-L2: apiKey[:8] slice in CreateAPIKey guarded with len check so a future change in key-generation length cannot panic. 03-L3: containsSequentialChars renamed to containsRepeatedChars and sequentialCharThreshold to repeatedCharThreshold; the function counts identical consecutive chars, not sequential runs -- the "sequential" naming was misleading. 03-L4: SetIssuerURL now validates that the URL is an absolute https:// URL and returns an error for non-https, relative, or unparseable URLs. Callers updated (lambda_issuer, handler_oidc, server/app). 03-L5: IsUnrestrictedAccess doc extended with a prominent fail-open WARNING; empty allowed-list = unrestricted is documented as intentional backward-compat but flagged so new callers cannot accidentally rely on it for "deny all" semantics. 03-L6: matchStringListConstraints doc extended to explain the empty-list semantics (empty either side = satisfied) and the authz rationale. Closes #1058 (partial) * fix(auth,credentials): N4 drop pointless SHA-256 in token gen; N5 validate unmarshal fields 03-N4: generateToken now encodes 32 CSPRNG bytes directly as base64url instead of hashing them through SHA-256. Hashing CSPRNG output adds no entropy -- the raw bytes already carry 256 bits. The SHA-256 step could mislead readers into thinking the input was sensitive pre-image material. 03-N5: resolveAccessKeyProvider and ResolveAzureCredentials now validate that required payload fields (access_key_id, secret_access_key, client_secret) are non-empty after JSON unmarshal. Previously an empty field would flow through to the AWS SDK or Azure SDK and fail with a less descriptive error deep in the credential chain. Regression tests added for both N5 paths. Closes #1058 (partial)
1 parent ee8212e commit 8f20d76

28 files changed

Lines changed: 977 additions & 134 deletions

‎internal/api/handler_oidc.go‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"strings"
88

99
"github.com/LeanerCloud/CUDly/internal/oidc"
10+
"github.com/LeanerCloud/CUDly/pkg/logging"
1011
"github.com/aws/aws-lambda-go/events"
1112
)
1213

@@ -124,7 +125,13 @@ func (h *Handler) resolveIssuerURL(req *events.LambdaFunctionURLRequest) string
124125
return ""
125126
}
126127
issuer := base + OIDCBasePath
127-
oidc.SetIssuerURL(issuer)
128+
if err := oidc.SetIssuerURL(issuer); err != nil {
129+
// The computed issuer URL is derived from the Lambda request's DomainName
130+
// (or the configured DASHBOARD_URL). A rejection here means the env is
131+
// misconfigured. Log and return empty so callers treat OIDC as unavailable.
132+
logging.Warnf("oidc: resolved issuer URL rejected: %v", err)
133+
return ""
134+
}
128135
return issuer
129136
}
130137

‎internal/auth/service.go‎

Lines changed: 47 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,15 @@ package auth
22

33
import (
44
"context"
5+
"crypto/rand"
56
"crypto/subtle"
67
"fmt"
78
"net/mail"
89
"strings"
910
"time"
1011

1112
"github.com/LeanerCloud/CUDly/pkg/logging"
13+
"golang.org/x/sync/singleflight"
1214
)
1315

1416
// Configuration constants
@@ -41,6 +43,15 @@ type Service struct {
4143
dashboardURL string
4244
bcryptCostOverride int // if > 0, overrides bcryptCost const (used by tests for speed)
4345
onPasswordChange func(ctx context.Context, userID, newPassword string)
46+
// csrfKey is the server-side key used to derive CSRF tokens as
47+
// HMAC-SHA256(csrfKey, rawSessionToken). Tokens are never stored
48+
// in cleartext; validation recomputes the HMAC and compares.
49+
// A random key is generated at NewService time when not supplied.
50+
csrfKey []byte
51+
// lastUsedSFG deduplicates concurrent UpdateLastUsed calls for the
52+
// same API key so a burst of authenticated requests does not spawn
53+
// an unbounded number of goroutines.
54+
lastUsedSFG singleflight.Group
4455
}
4556

4657
// ServiceConfig holds configuration for the auth service
@@ -50,6 +61,11 @@ type ServiceConfig struct {
5061
SessionDuration time.Duration
5162
DashboardURL string
5263
OnPasswordChange func(ctx context.Context, userID, newPassword string)
64+
// CSRFKey is the server-side secret used to derive CSRF tokens as
65+
// HMAC-SHA256(CSRFKey, rawSessionToken). Must be 32 bytes for
66+
// 256-bit security. When empty, NewService generates a random key and
67+
// logs a warning; all existing sessions will require re-login on restart.
68+
CSRFKey []byte
5369
}
5470

5571
// NewService creates a new auth service
@@ -83,12 +99,26 @@ func NewService(cfg ServiceConfig) *Service {
8399
}
84100
}
85101

102+
csrfKey := cfg.CSRFKey
103+
if len(csrfKey) == 0 {
104+
// No key supplied: generate a random ephemeral key. CSRF tokens issued
105+
// before a restart will become invalid (re-login required). Operators
106+
// should supply a stable key via CSRFKey to avoid this on restarts.
107+
logging.Warnf("auth: CSRFKey not set — generating ephemeral key; all CSRF tokens will be invalidated on service restart. Set CSRFKey to a stable 32-byte value.")
108+
csrfKey = make([]byte, 32)
109+
if _, err := rand.Read(csrfKey); err != nil {
110+
// rand.Read only fails on catastrophic OS errors; panic is appropriate.
111+
panic(fmt.Sprintf("auth: failed to generate ephemeral CSRF key: %v", err))
112+
}
113+
}
114+
86115
return &Service{
87116
store: cfg.Store,
88117
emailSender: cfg.EmailSender,
89118
sessionDuration: cfg.SessionDuration,
90119
dashboardURL: cfg.DashboardURL,
91120
onPasswordChange: cfg.OnPasswordChange,
121+
csrfKey: csrfKey,
92122
}
93123
}
94124

@@ -209,8 +239,8 @@ func (s *Service) verifyPasswordAndMFA(ctx context.Context, user *User, req Logi
209239
logging.Errorf("MFA enabled but secret missing for user %s -- possible data integrity issue", user.ID)
210240
return fmt.Errorf("Check your email address and password and try again")
211241
}
212-
// verifyTOTP is panic-safe: a malformed secret causes generateTOTP to return ""
213-
// (base32 decode error), resulting in a comparison miss rather than a panic.
242+
// verifyTOTP fails closed on empty or malformed inputs: empty code, empty
243+
// secret, and base32-decode errors all return false rather than a match.
214244
if verifyTOTP(user.MFASecret, req.MFACode) {
215245
return nil
216246
}
@@ -312,26 +342,30 @@ func (s *Service) ValidateSession(ctx context.Context, token string) (*Session,
312342
return &result, nil
313343
}
314344

315-
// ValidateCSRFToken validates the CSRF token for a session
345+
// ValidateCSRFToken validates the CSRF token for a session.
346+
//
347+
// The expected CSRF token is derived as HMAC-SHA256(csrfKey, rawSessionToken),
348+
// matching the token produced by createSession. Validation never reads the
349+
// stored csrf_token column; it recomputes the MAC from the raw session token
350+
// so a database read (SQLi, backup, replica) cannot yield a usable CSRF token.
316351
func (s *Service) ValidateCSRFToken(ctx context.Context, sessionToken, csrfToken string) error {
317352
if csrfToken == "" {
318353
return fmt.Errorf("CSRF token is required")
319354
}
320355

321-
session, err := s.ValidateSession(ctx, sessionToken)
322-
if err != nil {
356+
// ValidateSession is called for its side-effects (expiry check, session
357+
// existence) and to obtain the raw session token for HMAC derivation.
358+
// The session object itself is not used for the CSRF comparison.
359+
if _, err := s.ValidateSession(ctx, sessionToken); err != nil {
323360
return fmt.Errorf("invalid session: %w", err)
324361
}
325362

326-
if session.CSRFToken == "" {
327-
// Security: Reject sessions without CSRF tokens instead of allowing bypass
328-
// Legacy sessions must re-authenticate to get a proper CSRF token
329-
logging.Warnf("Rejecting session without CSRF token")
330-
return fmt.Errorf("session requires re-authentication for CSRF protection")
331-
}
363+
// Derive the expected CSRF token from the raw session token and the
364+
// server-side key. This is identical to what createSession produced.
365+
expected := deriveCSRFToken(s.csrfKey, sessionToken)
332366

333-
// Use constant-time comparison to prevent timing attacks
334-
if subtle.ConstantTimeCompare([]byte(session.CSRFToken), []byte(csrfToken)) != 1 {
367+
// Use constant-time comparison to prevent timing attacks.
368+
if subtle.ConstantTimeCompare([]byte(expected), []byte(csrfToken)) != 1 {
335369
return fmt.Errorf("invalid CSRF token")
336370
}
337371

‎internal/auth/service_apikeys.go‎

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,13 @@ func (s *Service) CreateAPIKey(ctx context.Context, userID, name string, permiss
5050
hash := sha256.Sum256([]byte(apiKey))
5151
keyHash := base64.RawURLEncoding.EncodeToString(hash[:])
5252

53-
// Extract key prefix (first 8 chars) for display
54-
keyPrefix := apiKey[:8]
53+
// Extract key prefix (first 8 chars) for display. The key is always 43 chars
54+
// (32 random bytes base64url-encoded), so [:8] is safe today. The guard makes
55+
// the function robust to future changes in key-generation length (03-L2).
56+
keyPrefix := apiKey
57+
if len(apiKey) > 8 {
58+
keyPrefix = apiKey[:8]
59+
}
5560

5661
// Validate permissions - ensure they don't exceed user's permissions
5762
if err := s.validateAPIKeyPermissions(ctx, user, permissions); err != nil {
@@ -269,13 +274,21 @@ func (s *Service) ValidateUserAPIKey(ctx context.Context, apiKey string) (*UserA
269274
return nil, nil, err
270275
}
271276

272-
// Update last used timestamp (async to avoid blocking)
277+
// Update last used timestamp asynchronously to avoid blocking the
278+
// authentication hot path. singleflight.Group ensures at most one
279+
// in-flight DB write per keyID at any moment: subsequent concurrent
280+
// requests for the same key are deduplicated rather than spawning an
281+
// unbounded number of goroutines (DoS amplifier on a revoked key).
282+
keyID := key.ID
273283
go func() {
274-
updateCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
275-
defer cancel()
276-
if err := s.UpdateLastUsed(updateCtx, key.ID); err != nil {
277-
logging.Warnf("Failed to update API key last used timestamp: %v", err)
278-
}
284+
_, _, _ = s.lastUsedSFG.Do(keyID, func() (any, error) {
285+
updateCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
286+
defer cancel()
287+
if err := s.UpdateLastUsed(updateCtx, keyID); err != nil {
288+
logging.Debugf("Failed to update API key last used timestamp for key %s: %v", keyID, err)
289+
}
290+
return nil, nil
291+
})
279292
}()
280293

281294
return key, user, nil

‎internal/auth/service_group.go‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,19 @@ func (s *Service) matchConstraints(permConstraints, reqConstraints *PermissionCo
229229
s.matchPurchaseAmountConstraint(permConstraints.MaxPurchaseAmount, reqConstraints.MaxPurchaseAmount)
230230
}
231231

232-
// matchStringListConstraints checks if two string lists have any overlap
232+
// matchStringListConstraints checks if two string lists have any overlap.
233+
//
234+
// Semantics (03-L6): if either list is empty the constraint is treated as
235+
// satisfied ("no constraint specified = no restriction"). Concretely:
236+
// - empty permList: the permission has no constraint on this dimension
237+
// - empty reqList: the request does not specify this dimension
238+
//
239+
// Both cases return true (match). Only when both lists are non-empty is
240+
// containsAny used to require at least one common element.
241+
//
242+
// Callers that need "a constrained permission must only match an explicit
243+
// request value" should verify reqList is non-empty before calling, or add
244+
// a separate dimension-specific check.
233245
func (s *Service) matchStringListConstraints(permList, reqList []string) bool {
234246
if len(permList) > 0 && len(reqList) > 0 {
235247
return containsAny(permList, reqList)

‎internal/auth/service_helpers.go‎

Lines changed: 31 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,10 @@ package auth
22

33
import (
44
"context"
5+
"crypto/hmac"
56
"crypto/rand"
67
"crypto/sha256"
8+
"encoding/base64"
79
"encoding/hex"
810
"time"
911
)
@@ -16,6 +18,16 @@ func hashSessionToken(token string) string {
1618
return hex.EncodeToString(hash[:])
1719
}
1820

21+
// deriveCSRFToken derives a CSRF token as HMAC-SHA256(csrfKey, rawSessionToken).
22+
// The token is cryptographically bound to the session token and the server-side
23+
// key: an attacker who reads the database cannot forge a CSRF token without the
24+
// key, and cannot replay a CSRF token from another session.
25+
func deriveCSRFToken(csrfKey []byte, rawSessionToken string) string {
26+
mac := hmac.New(sha256.New, csrfKey)
27+
mac.Write([]byte(rawSessionToken))
28+
return hex.EncodeToString(mac.Sum(nil))
29+
}
30+
1931
// createSession creates a new session for a user
2032
func (s *Service) createSession(ctx context.Context, user *User, userAgent, ipAddress string) (*Session, error) {
2133
// Generate a cryptographically random session token
@@ -24,17 +36,20 @@ func (s *Service) createSession(ctx context.Context, user *User, userAgent, ipAd
2436
return nil, err
2537
}
2638

27-
// Hash the token for secure storage
28-
// The raw token is returned to the client; only the hash is stored
39+
// Hash the token for secure storage.
40+
// The raw token is returned to the client; only the hash is stored.
2941
hashedToken := hashSessionToken(rawToken)
3042

31-
// Generate CSRF token for state-changing request protection
32-
csrfToken, err := generateToken()
33-
if err != nil {
34-
return nil, err
35-
}
43+
// Derive the CSRF token as HMAC-SHA256(csrfKey, rawToken).
44+
// The token is bound to this session's raw token and the server-side key,
45+
// so it cannot be forged from a DB read alone. Not stored in the DB
46+
// (the stored value is the MAC itself, kept only for diagnostics/compat).
47+
csrfToken := deriveCSRFToken(s.csrfKey, rawToken)
3648

37-
// Create session with hashed token for storage
49+
// Create session with hashed token for storage.
50+
// The csrf_token column holds the MAC for observability; validation
51+
// always recomputes it from the raw session token rather than trusting
52+
// the stored value, so DB exposure does not yield a usable CSRF token.
3853
storedSession := &Session{
3954
Token: hashedToken, // Store the hash, not the raw token
4055
UserID: user.ID,
@@ -43,7 +58,7 @@ func (s *Service) createSession(ctx context.Context, user *User, userAgent, ipAd
4358
CreatedAt: time.Now(),
4459
UserAgent: userAgent,
4560
IPAddress: ipAddress,
46-
CSRFToken: csrfToken,
61+
CSRFToken: csrfToken, // MAC stored for diagnostics only; not used in validation
4762
}
4863

4964
if err := s.store.CreateSession(ctx, storedSession); err != nil {
@@ -59,20 +74,21 @@ func (s *Service) createSession(ctx context.Context, user *User, userAgent, ipAd
5974
CreatedAt: storedSession.CreatedAt,
6075
UserAgent: userAgent,
6176
IPAddress: ipAddress,
62-
CSRFToken: csrfToken,
77+
CSRFToken: csrfToken, // Client receives the MAC as the CSRF token
6378
}
6479

6580
return clientSession, nil
6681
}
6782

68-
// generateToken generates a cryptographically secure random token
83+
// generateToken generates a cryptographically secure random token by encoding
84+
// 32 random bytes as base64url. Hashing CSPRNG output through SHA-256 adds
85+
// nothing -- the raw bytes already carry 256 bits of entropy (03-N4).
6986
func generateToken() (string, error) {
70-
bytes := make([]byte, 32)
71-
if _, err := rand.Read(bytes); err != nil {
87+
b := make([]byte, 32)
88+
if _, err := rand.Read(b); err != nil {
7289
return "", err
7390
}
74-
hash := sha256.Sum256(bytes)
75-
return hex.EncodeToString(hash[:]), nil
91+
return base64.RawURLEncoding.EncodeToString(b), nil
7692
}
7793

7894
// containsAny checks if any element from requested is in allowed

‎internal/auth/service_mfa.go‎

Lines changed: 32 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -46,21 +46,38 @@ const recoveryCodeLength = 8
4646
// shown to humans once and typed back, so visual clarity matters.
4747
const recoveryCodeAlphabet = "ABCDEFGHJKMNPQRSTUVWXYZ23456789"
4848

49-
// verifyTOTP validates a TOTP code against the secret
50-
// Implements RFC 6238 TOTP with 30-second time steps
51-
// Uses constant-time comparison to prevent timing attacks
49+
// verifyTOTP validates a TOTP code against the secret.
50+
// Implements RFC 6238 TOTP with 30-second time steps.
51+
// Uses constant-time comparison to prevent timing attacks.
52+
//
53+
// Fails closed: returns false immediately when code or secret is empty.
54+
// An empty code can arrive from a missing form field; an empty secret signals
55+
// a data-integrity anomaly (MFA enabled but no secret stored). In both cases
56+
// the correct behavior is to reject, not to accidentally match because
57+
// generateTOTP("","") returns "" and ConstantTimeCompare("","") == 1.
5258
func verifyTOTP(secret, code string) bool {
53-
// Allow for time skew by checking current and adjacent time windows
59+
// Fail closed: reject empty code and empty secret up front.
60+
// generateTOTP returns "" on base32-decode failure; ConstantTimeCompare("","")
61+
// evaluates to 1 and would bypass MFA for any caller that passes code="".
62+
if code == "" {
63+
return false
64+
}
65+
if secret == "" {
66+
return false
67+
}
68+
69+
// Allow for time skew by checking current and adjacent time windows.
5470
currentTime := time.Now().Unix()
5571
timeStep := int64(config.MFATimeStep)
5672

57-
// Check current time step and one step before/after for clock skew tolerance
58-
// Use constant-time comparison and avoid early returns to prevent timing attacks
73+
// Check current time step and one step before/after for clock skew tolerance.
74+
// Use constant-time comparison and avoid early returns to prevent timing attacks.
5975
valid := 0
6076
for _, offset := range []int64{-1, 0, 1} {
6177
counter := (currentTime / timeStep) + offset
6278
expected := generateTOTP(secret, counter)
63-
// Use constant-time comparison - bitwise OR accumulates matches
79+
// generateTOTP returns "" on base32-decode failure; a non-empty code can
80+
// never equal "" so a bad secret produces no match.
6481
if subtle.ConstantTimeCompare([]byte(expected), []byte(code)) == 1 {
6582
valid = 1
6683
}
@@ -207,13 +224,21 @@ func (s *Service) hashRecoveryCode(code string) (string, error) {
207224
// removal is what makes recovery codes single-use; the caller must
208225
// persist the user afterwards via UpdateUser.
209226
//
227+
// Fails closed on empty input: an empty string normalizes to "", which
228+
// bcrypt.CompareHashAndPassword would compare against every stored hash.
229+
// Reject it immediately to avoid leaking which hashes exist.
230+
//
210231
// Iterates over all stored hashes (bcrypt compare is constant-time
211232
// for the matching hash but not across the slice — recovery codes
212233
// are 8 chars of high-entropy alphabet, so timing leakage of which
213234
// slot matched isn't usefully exploitable, but the comparison stays
214235
// branch-free by ORing the bool into a single accumulator).
215236
func (s *Service) consumeRecoveryCode(user *User, entered string) bool {
216237
normalized := normalizeRecoveryCode(entered)
238+
// Fail closed: reject empty input before any bcrypt comparison.
239+
if normalized == "" {
240+
return false
241+
}
217242
matchIdx := -1
218243
for i, hash := range user.MFARecoveryCodes {
219244
if err := bcrypt.CompareHashAndPassword([]byte(hash), []byte(normalized)); err == nil {

0 commit comments

Comments
 (0)