diff --git a/docs/SECURITY.md b/docs/SECURITY.md index f089d4cc..ee8f21ca 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -749,7 +749,11 @@ The TOTP secret is stored **encrypted at rest** (the store cipher) and recovery **argon2id-hashed**; verification uses the server clock and a constant-time compare over a **configurable clock-skew window** (`[auth].totp_skew_steps`, **default `0` = the current 30 s step only** — strictest replay window, ASVS 6.5.5; set `1`/`2` to restore RFC-6238 ±1 network-delay tolerance, the forward step -clamped to the current step so single-use holds). TOTP is a shared-secret factor — L3 *prefers* +clamped to the current step to avoid a self-inflicted lockout). ⚠️ **Single-use (ASVS 6.5.1) holds only at +the default `0`.** At `totp_skew_steps >= 1` the clamp records a tolerated *future* code against the +current step, leaving that code's own step unspent — so the **same code verifies a second time** once the +clock reaches it. That is the cost of the opt-out, and it is why the default is `0`. TOTP is a +shared-secret factor — L3 *prefers* phishing-resistant factors: **WebAuthn passkeys are the built WP-14b sibling** (next section), and TOTP stays fully supported alongside them (a non-browser client — e.g. the test harness, or CLI/API automation — has no `navigator.credentials`, so TOTP remains its usable second factor). diff --git a/messagefoundry/auth/totp.py b/messagefoundry/auth/totp.py index c0670f77..b59d1c1b 100644 --- a/messagefoundry/auth/totp.py +++ b/messagefoundry/auth/totp.py @@ -109,7 +109,17 @@ def verify_totp_step( tolerated future code (``counter+1``) reports ``counter``, never advancing the single-use high- water mark past the genuinely-current step. Otherwise burning ``counter+1`` would reject the user's own current-step code (a non-greater step) for up to ~30 s — a self-inflicted lockout, not a - bypass. The clamp only lowers the recorded step, so single-use is preserved. + bypass. + + ⚠️ **At ``window >= 1`` the clamp does not preserve single-use — it is what costs it.** Because a + tolerated future code is recorded at ``counter`` rather than at its OWN step, that step is left + unspent, so the SAME code verifies again once the clock reaches ``counter+1``: two successful uses + of one code, which is what ASVS 6.5.1 forbids. Measured — ``verify_totp_step(s, totp(s, now=t+30), + now=t, window=1)`` returns ``step(t)``, and the same code at ``now=t+30`` returns ``step(t)+1``, + strictly greater, so a high-water store accepts both. At the shipped default ``window=0`` + (``[auth].totp_skew_steps``) single-use DOES hold, because a future code is not accepted at all. + The second use is the price of the opt-out, not a property of the clamp. Pinned by + tests/test_totp_window.py::test_optout_lets_one_tolerated_future_code_be_used_twice. """ candidate = code.strip() if len(candidate) != digits or not candidate.isdigit(): diff --git a/tests/test_totp_window.py b/tests/test_totp_window.py index 3d2c7745..520c4484 100644 --- a/tests/test_totp_window.py +++ b/tests/test_totp_window.py @@ -14,7 +14,10 @@ authenticator can still log in) but the returned step is **clamped to the current step**, so consuming a tolerated future code never advances the single-use high-water mark past ``now`` — otherwise the user's own genuine current-step code (a non-greater step) would be rejected for up to ~30 s, a - self-inflicted lockout, not a bypass. The clamp only lowers the recorded step, so single-use holds. + self-inflicted lockout, not a bypass. ⚠️ The clamp does NOT preserve single-use here — it is what + costs it: recording a tolerated future code at ``now`` leaves that code's OWN step unspent, so the + same code verifies again one step later (two successful uses, ASVS 6.5.1). Single-use holds only at + the strict default. See test_optout_lets_one_tolerated_future_code_be_used_twice. These call ``verify_totp_step`` directly with an EXPLICIT ``window`` so both the strict default and the opt-out are pinned regardless of the module-level ``DEFAULT_WINDOW`` (which stays 1 for callers that @@ -99,6 +102,39 @@ def test_optout_fast_clock_future_code_causes_no_self_lockout() -> None: assert genuine is not None and genuine > consumed +def test_optout_lets_one_tolerated_future_code_be_used_twice() -> None: + """The clamp's COST, pinned — one code, two successful uses, at ``totp_skew_steps >= 1``. + + This is the gap the neighbouring tests leave. ``test_single_use_step_is_stable_for_the_same_code`` + replays the same code at the SAME ``now``; ``test_optout_fast_clock...no_self_lockout`` replays a + DIFFERENT (genuine) code at a later ``now``. Nobody replayed the SAME code at a LATER ``now``, which + is where the clamp bites: recording a tolerated ``counter+1`` code at ``counter`` leaves its own step + unspent, so it verifies again when the clock arrives there. A high-water store that rejects a + non-greater step accepts BOTH, because the second resolution is strictly greater. + + Three docstrings previously drew the opposite conclusion from the same true premise ("the clamp only + lowers the recorded step, so single-use is preserved"). It lowers the step, and that is precisely + why the code survives. ASVS 6.5.1 requires TOTPs be "only successfully usable once"; that holds at + the shipped default and not at the opt-out, so the opt-out carries a real cost the docs now name. + """ + t = 5_000 * PERIOD + 5.0 + future = totp.totp(SECRET, now=t + PERIOD) + + first = totp.verify_totp_step(SECRET, future, now=t, window=1) + second = totp.verify_totp_step(SECRET, future, now=t + PERIOD, window=1) + assert first == _step(t) # clamped down, per SEC-014 + assert second == _step(t + PERIOD) # its own step, still unspent + assert second is not None and first is not None and second > first, ( + "the second resolution must be STRICTLY GREATER — that is what makes a high-water store " + "accept the same code a second time" + ) + + # The strict default is the control: the same code cannot be used twice, because the first + # presentation is refused outright rather than clamped. + assert totp.verify_totp_step(SECRET, future, now=t, window=0) is None + assert totp.verify_totp_step(SECRET, future, now=t + PERIOD, window=0) == _step(t + PERIOD) + + def test_single_use_step_is_stable_for_the_same_code_and_now() -> None: # The same code at the same now resolves to the same step both times (a single-use store rejecting a # non-greater step then rejects the replay). Holds under both the strict and the opt-out window.