Skip to content

Harden code verification: input validation, bounded drift window, and replay protection support (RFC 6238 §5.2) - #12

Open
Rodots wants to merge 5 commits into
Vectorface:masterfrom
Rodots:fix/verify-code-hardening
Open

Harden code verification: input validation, bounded drift window, and replay protection support (RFC 6238 §5.2)#12
Rodots wants to merge 5 commits into
Vectorface:masterfrom
Rodots:fix/verify-code-hardening

Conversation

@Rodots

@Rodots Rodots commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR hardens verifyCode() / setCodeLength() against a few subtle but practical
security issues, without changing behavior for any currently-valid usage. All changes
are covered by new unit tests (+94 lines, full suite green).

Problems addressed

1. verifyCode() hardcodes a 6-digit check, silently breaking 8-digit configurations (bug + security)

verifyCode() rejects any code whose length is not exactly 6:

if (strlen($code) != 6) {
    return false;
}

But the library supports setCodeLength(8). In that configuration getCode() produces
8-digit codes while verifyCode() rejects all of them — an integrator who configured
longer (stronger) codes gets a verifier that never succeeds, and the natural "fix" on
their side is to roll their own comparison, losing the hash_equals() timing-safe path.

Fix: validate against the configured $this->_codeLength, and additionally require
ctype_digit($code) so non-numeric input is rejected before any HMAC work.

2. setCodeLength() accepts values that produce broken or trivially brute-forceable codes

Any int was accepted. Values < 6 produce codes weaker than RFC 4226 permits (§4 R4:
"The value displayed on the token MUST be easily read … at least a six-digit code");
values > 9 overflow the modulo table and previously failed obscurely at generation time.

Fix: setCodeLength() now throws InvalidArgumentException outside the 6–8 range
(the range Google Authenticator-compatible apps actually support).

3. Unbounded $discrepancy allows arbitrarily large validation windows

verifyCode($secret, $code, PHP_INT_MAX) would happily loop over an astronomically large
window. A caller passing an unvalidated, attacker-influenced discrepancy value would
massively inflate the brute-force acceptance window (each extra slice adds ~2×10⁻⁶
acceptance probability per guess for 6-digit codes) and burn CPU on HMAC computations.

Fix: $discrepancy is bounded to 0–60 slices (±30 minutes), throwing
InvalidArgumentException beyond that. RFC 6238 §5.2 recommends "at most one time step"
of transmission delay allowance; 60 is already a generous ceiling for clock-skew use cases.

4. No way to implement RFC 6238 §5.2 replay protection

RFC 6238 §5.2 requires that a verifier "MUST NOT accept the second attempt of the OTP
after the successful validation". Callers currently can't implement this because
verifyCode() doesn't report which time slice matched — a code remains valid for the
whole discrepancy window, so an attacker who observes a code (shoulder-surfing, phishing
proxy, logs) can replay it.

Fix: an optional by-reference out-parameter:

public function verifyCode(string $secret, string $code, int $discrepancy = 1, ?int &$matchedTimeSlice = null): bool

On success it receives the matched slice; integrators persist the last accepted slice per
user and reject any match with slice <= lastAccepted. Usage is documented in the README.

Backward compatibility

  • No signature breaks: the new parameter is optional and by-ref; existing call sites are unaffected.
  • 6-digit flows behave identically (aside from now also rejecting non-numeric strings, which could never match anyway).
  • setCodeLength()/verifyCode() now throw on inputs that previously produced silently broken or dangerous behavior — this only surfaces in code that was already misconfigured.

Testing

  • New tests: code-length range enforcement, 8-digit verify round-trip, non-numeric code rejection, discrepancy bounds, matched-time-slice reporting.
  • Existing suite (incl. RFC 6238 Appendix B vectors) passes unchanged: composer test → OK.

@jdpanderson

Copy link
Copy Markdown
Member
  1. verifyCode() hardcodes a 6-digit check, silently breaking 8-digit configurations (bug + security)
  2. setCodeLength() accepts values that produce broken or trivially brute-forceable codes

These are worthwhile fixes.

  1. Unbounded $discrepancy allows arbitrarily large validation windows

Please remove this change. I don't want to dictate how someone can use this library; It's totally fine if someone wants to detect clock drift by burning CPU time.

  1. No way to implement RFC 6238 §5.2 replay protection

Please remove this change. This is outside the scope of this library. RFC 6238 Section 5.2 specifies that validation systems must enforce a one-time-only use policy for each generated code: the secret/timeslice combination can't be re-used once it yields a successful authentication. This library does not have a datastore nor does it maintain state, so it cannot enforce this in any meaningful way. The higher level application will have to implement this policy.

The changes are otherwise quite long, the diff touches files/content that aren't relevant to the PR's changes. If you want to re-submit this, please tell your AI agent to make surgical changes only and to be concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants