Make the login-approval attempt cap atomic (LC-608) #565

Merged

longjacksonle merged 1 commit from fix/lc587-atomic-attempt-cap into main

2026-07-20 18:43:53 +02:00

longjacksonle commented

2026-07-20 05:01:18 +02:00

Owner

Closes LC-608. Found while reviewing LC-587 before it ships beyond staging. Same read-then-act shape LC-601 closed on the consume path, one function over.

The defect

login_approval::verify did three separate steps: read the challenge including attempts, compare the submitted code against code_hash, then bump attempts and burn the challenge if the bump crossed MAX_ATTEMPTS. The cap was only consulted against the value read in step one, so concurrent submissions all read the same pre-bump count and every one of them reached the comparison.

The 6-digit code space is 1e6, and the entire security argument for a 6-digit code is "5 tries per emailed code". Under concurrency that bound did not exist.

The fix

One statement that is simultaneously the validity check and the counter:

UPDATE login_approvals SET attempts = attempts + 1
 WHERE id = ? AND consumed_at IS NULL AND expires_at > datetime('now') AND attempts < ?
RETURNING user_id, code_hash, country, device_hash, attempts

No row means unknown, expired, consumed, or out of attempts. A returned row is that caller's reserved slot, so at most MAX_ATTEMPTS comparisons ever happen against one challenge. A correct code also spends a slot, which is harmless because success consumes the challenge outright.

get_valid_login_approval and bump_login_approval_attempts are deleted rather than left available. Both were unused after this change, and leaving them in place leaves the unsafe sequence sitting there for the next caller to reassemble.

Verification, including a wrong turn worth recording

The regression test releases 40 wrong-code submissions through a barrier, then reads attempts back. Every submission that got compared also bumped, so the stored count is the number of guesses the challenge allowed. 40 of 40 before the fix, 5 of 40 after. Confirmed failing against the unpatched code by stashing the two source files.

My first attempt at this test asserted on the wrong observable and reported the opposite conclusion. Counting how many calls return Wrong gives 0 of 40, because by the time each of the 40 read its post-bump counter the value was already past the cap, so they all returned Invalid. That reads exactly like the cap working perfectly, while 40 comparisons were in fact performed. Anyone re-testing this area should measure the counter, not the outcomes.

just check and the full just test suite are green.

Severity

Moderate, not critical. An attacker still needs the challenge token, which means they have already completed SSO as the victim and are guessing a code mailed to the victim's verified address. Concurrency lifted the per-challenge guess count from 5 to however many requests they can land, far below 1e6 in one shot but well above the intended bound, and repeatable across freshly minted challenges.

Noted, not changed

POST /auth/bunyip/approve has no rate limit of its own beyond the per-challenge cap. Much less pressing now that the cap is real, but it is the remaining lever.
The code comparison is a plain == on hex digests rather than constant-time. It compares SHA-256 hashes, not the codes, so the timing signal leaks nothing usable. Recorded so the next reader doesn't have to re-derive it.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BE9k4nWNUhPrjte9BcvASg

Closes LC-608. Found while reviewing LC-587 before it ships beyond staging. Same read-then-act shape LC-601 closed on the consume path, one function over. ## The defect `login_approval::verify` did three separate steps: read the challenge including `attempts`, compare the submitted code against `code_hash`, then bump `attempts` and burn the challenge if the bump crossed `MAX_ATTEMPTS`. The cap was only consulted against the value read in step one, so concurrent submissions all read the same pre-bump count and every one of them reached the comparison. The 6-digit code space is 1e6, and the entire security argument for a 6-digit code is "5 tries per emailed code". Under concurrency that bound did not exist. ## The fix One statement that is simultaneously the validity check and the counter: ```sql UPDATE login_approvals SET attempts = attempts + 1 WHERE id = ? AND consumed_at IS NULL AND expires_at > datetime('now') AND attempts < ? RETURNING user_id, code_hash, country, device_hash, attempts ``` No row means unknown, expired, consumed, or out of attempts. A returned row is that caller's reserved slot, so at most `MAX_ATTEMPTS` comparisons ever happen against one challenge. A correct code also spends a slot, which is harmless because success consumes the challenge outright. `get_valid_login_approval` and `bump_login_approval_attempts` are deleted rather than left available. Both were unused after this change, and leaving them in place leaves the unsafe sequence sitting there for the next caller to reassemble. ## Verification, including a wrong turn worth recording The regression test releases 40 wrong-code submissions through a barrier, then reads `attempts` back. Every submission that got compared also bumped, so the stored count *is* the number of guesses the challenge allowed. **40 of 40 before the fix, 5 of 40 after.** Confirmed failing against the unpatched code by stashing the two source files. My first attempt at this test asserted on the wrong observable and reported the opposite conclusion. Counting how many calls return `Wrong` gives **0 of 40**, because by the time each of the 40 read its post-bump counter the value was already past the cap, so they all returned `Invalid`. That reads exactly like the cap working perfectly, while 40 comparisons were in fact performed. Anyone re-testing this area should measure the counter, not the outcomes. `just check` and the full `just test` suite are green. ## Severity Moderate, not critical. An attacker still needs the challenge token, which means they have already completed SSO as the victim and are guessing a code mailed to the victim's verified address. Concurrency lifted the per-challenge guess count from 5 to however many requests they can land, far below 1e6 in one shot but well above the intended bound, and repeatable across freshly minted challenges. ## Noted, not changed - `POST /auth/bunyip/approve` has no rate limit of its own beyond the per-challenge cap. Much less pressing now that the cap is real, but it is the remaining lever. - The code comparison is a plain `==` on hex digests rather than constant-time. It compares SHA-256 hashes, not the codes, so the timing signal leaks nothing usable. Recorded so the next reader doesn't have to re-derive it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01BE9k4nWNUhPrjte9BcvASg

longjacksonle added 1 commit

2026-07-20 05:01:18 +02:00

fix(auth): make the login-approval attempt cap atomic (LC-608)

check-secrets / Nosey parker (push) Successful in 4s

Details

check-secrets / Kingfisher (push) Successful in 10s

Details

check-secrets / TruffleHog (push) Successful in 10s

Details

check-secrets / Nosey parker (pull_request) Successful in 6s

Details

check-secrets / TruffleHog (pull_request) Successful in 9s

Details

check-secrets / Kingfisher (pull_request) Successful in 11s

Details

Check / clippy + fmt + tests (pull_request) Successful in 5m18s

Details

Create release / Create release from merged PR (pull_request) Has been skipped

Details

cc3f58358b

`verify` read the challenge, compared the submitted code, then bumped `attempts` and burned the challenge if the bump crossed the cap. The cap was only ever checked against the value read in the first step, so concurrent submissions all read the same pre-bump count and every one of them reached the comparison. The 6-digit code space is 1e6 and the security argument for it is "5 tries per emailed code"; under concurrency that bound did not exist. Same read-then-act shape LC-601 closed on the consume path, one function over.

The claim is now a single conditional UPDATE that is both the validity check and the counter, returning the row only when it reserved a slot. No row means unknown, expired, consumed, or out of attempts. A correct code also spends a slot, which is harmless since success consumes the challenge outright. `get_valid_login_approval` and `bump_login_approval_attempts` are removed rather than left in place, so the unsafe sequence cannot be reassembled by the next caller.

The regression test releases 40 wrong-code submissions through a barrier and then reads `attempts` back, since every submission that got compared also bumped: the stored count is the number of guesses the challenge allowed. 40 of 40 before, 5 of 40 after.

Worth recording that the obvious metric is misleading. Counting how many calls return `Wrong` reports 0, because by the time each of the 40 reads its post-bump counter the value is already past the cap, so they all return `Invalid`. That reads like the cap working perfectly while 40 comparisons were performed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BE9k4nWNUhPrjte9BcvASg