feat: temporarily lock out local accounts after failed logins

[oc-tmueller]

Fixes OC10-153.

What

Core does not throttle password authentication at all (CWE-307): every entry point which verifies a password against the user backend accepts an unlimited number of attempts, so a local account with a weak password can be brute-forced online at request rate unless an optional app is installed.

This adds a DB backed temporary lockout for accounts of the built-in Database backend. Five failed attempts within 15 minutes lock the account for ten minutes. The lockout always expires on its own — there is no administrative unlock and no account is ever disabled — and a successful login clears the counter immediately. The login form names the remaining time.

External backends are deliberately not tracked: LDAP/AD counts the same attempt itself via badPwdCount, and lockout of an OIDC account belongs to the IdP, so counting here would punish twice.

Where it hooks in

The three places that verify a submitted password, each gated ahead of the credential check so a locked account does not even pay for the password hash:

• Session::loginWithPassword() — web login form, WebDAV and OCS basic auth
• TokenController::generateToken() — app password creation
• OcsController::checkPerson()

Token, auth module and remember-me logins are untouched: they revalidate an existing credential rather than a submitted password, so throttling them would only lock out already authenticated clients.

Configuration

New account_lockout.* parameters, documented in config/config.sample.php:

parameter	default	meaning
account_lockout.enabled	true	master switch
account_lockout.max_attempts	5	failures which trigger the lockout
account_lockout.duration	600	seconds an account stays locked
account_lockout.attempt_window	900	seconds of inactivity after which the counter is forgotten

Notable details

• Lockout keys are lower cased in PHP, so admin, Admin and ADMIN share one budget of attempts regardless of the database collation.
• Login names which do not resolve to an account are tracked in the same key space and produce exactly the same response as a locked existing account, so the lockout cannot be used to probe whether a user name exists. The unlocked read path performs no account lookup at all, so there is no timing difference either.
• Every counter update is a single SQL statement, so requests served by different application servers cannot lose a count; the lockout itself is started by a conditional UPDATE, so concurrent failures cannot extend a running one.
• One request may verify the same credentials twice (login by email is retried with the resolved uid) — that is counted once.
• The login route stays available while an upgrade is pending, so every statement tolerates account_lockouts being absent and simply locks nobody out until the migration has run.
• Deleting an account drops its counter, so a recreated namesake does not inherit it.
• tryBasicAuthLogin() reports a lockout the same way it reports a wrong password, because not every caller of \OC::handleLogin() handles a LoginException. The endpoints which can carry the explanation — the login form and the DAV backend — do not use that method.
• A background job (ExpireLockoutsJob) removes rows which can no longer affect a login decision.

Tests

New tests/lib/Authentication/AccountLockout/AccountLockoutTest.php (13 tests against a real database) covers the threshold, the automatic expiry, the counter decay, the disabled switch, custom thresholds, external backends, the enumeration properties, case variants, the login-by-email double count and concurrent failures. Controller and session level gating is covered in SessionTest, TokenControllerTest, OcsControllerTest and LoginControllerTest.

Verified green locally: tests/lib/User, tests/lib/Authentication, tests/Core, tests/lib/ServerTest.php, tests/lib/SetupTest.php; php-cs-fixer and phpstan clean. The schema migration and the job registration were verified against a real sqlite install.

No acceptance feature needed re-tagging: the scenarios which exceed five failed attempts never follow up with a correct password, and their accounts are deleted per scenario, which clears the counter.

Related

The same change for the 10.16 line: #41806

🤖 Generated with Claude Code

[kw-fscheuer]

Reviewed as a security review against OC10-153. This closes the finding — the lockout is enforced ahead of the credential check on every path that verifies a submitted password, and I could not find a way around it. Approving; everything below is non-blocking.

What I verified

• Gate placement. throwIfLockedOut() precedes checkPassword() in Session::loginWithPassword(), so a locked account never pays for the hash and never reaches an external backend. loginWithPassword() really is the single funnel for the web form, WebDAV/DAV basic auth and OCS.
• Coverage. All three callers that verify a submitted password are gated (Session::loginWithPassword(), TokenController::generateToken(), OcsController::checkPerson()). The remaining checkPassword() call sites — Session.php token revalidation and BasicAuthModule — are revalidation paths, see below.
• Counter logic. restartIfStale / increment / conditional lock compose correctly for the absent, current, stale-window and expired-lockout cases. After expiry the next failure resets to 1 rather than resuming at the threshold, so the budget really is max_attempts per cycle. lock() requiring locked_until IS NULL does prevent concurrent failures from extending a running lockout.
• No enumeration oracle. Tracking unknown login names in the same key space is the right call, and the read path doing no account lookup unless a lockout is already in effect closes the timing side too. Nice.
• Case normalisation in PHP rather than relying on collation, with MAX_KEY_LENGTH matching the column width.
• Storage. DB-backed rather than the distributed cache, which matters because Memcache\Factory degrades to NULL_CACHE when neither memcache.local nor memcache.distributed is configured — a cache-based counter would have been a silent no-op on a stock install.
• Error surfacing. AccountLockedException extends LoginException is the detail that makes this clean: DAV's existing Auth::check() catch turns it into a 401 carrying the message with no change in apps/dav. LoginController::tryLogin() now catches it too, which it had to — an uncaught LoginException there would have rendered an error page instead of the message. Confirmed the message renders escaped on the form via p($message) in core/templates/login.php.
• Migration. Unique index on uid, secondary index on last_fail_at, and uid deliberately wider than users.uid because login names are attacker-supplied. Job registered for both fresh installs and upgrades.
• Upgrade tolerance. Treating TableNotFoundException as "lock nobody out" keeps the login route usable while an upgrade is pending. Good instinct.

Two things that look like defects but are not

Recording these so the next reviewer doesn't spend time on them:

1. createFunction('fail_count + 1') is not a Postgres/Oracle portability bug. The hardcoded backticks look MySQL-only, but this is core's documented idiom — see the createFunction() docblock in lib/private/DB/QueryBuilder/QueryBuilder.php, and the same atomic-increment pattern already in Files/Cache/Propagator.php and Lock/DBLockingProvider.php.

2. BasicAuthModule is not an unthrottled bypass, though it reads PHP_AUTH_USER/PHP_AUTH_PW straight from the request and calls checkPassword() — which reads like a submitted password rather than the revalidation the description claims. It isn't reachable pre-auth: the built-in module is only yielded by getAuthModules(true), whose sole caller is verifyAuthHeaders(), and every call site of that gates on isLoggedIn() first (apps/dav/lib/Connector/Sabre/Auth.php, two sites in lib/kernel.php, and lib/private/legacy/api.php). tryAuthModuleLogin() passes false, which excludes it. The rationale in the description holds — it's just worth stating why rather than asserting it.

Non-blocking suggestions

1. An account with an email address gets two budgets — ~10 attempts per window instead of 5.

$failureRecorded suppresses the second key of a request, so a request submitting the email locks only the email key and leaves the uid key at zero. An attacker can spend max_attempts on victim@example.com and max_attempts again on victim. Not a bypass — the control still bounds the attack — but it doubles the configured allowance. Telling detail: clearFailures($login, $uid) already takes both keys, so the dual-key nature was handled on the success path but not the failure path.

Keying the failure on the resolved uid when the login resolves would fix it:

$user = $this->userManager->get($login);
$key = $this->keyFor($user !== null ? $user->getUID() : $login);

with the same resolution in getRemainingLockTime() so the gate reads the same key. Note that reintroduces an account lookup on the read path, which is the thing you deliberately avoided for timing reasons — so it needs care, and possibly only resolving on the write path while recording both keys.

2. $failureRecorded is per-request rather than per-key.

Correct today, since the only double-verify in one request is the email retry for the same account. But a future caller checking two different accounts in one request would get the second attempt for free, silently. Cheap to make robust:

/** @var array<string,true> */
private $failuresRecorded = [];
// ...
if (isset($this->failuresRecorded[$key])) { return; }
$this->failuresRecorded[$key] = true;

3. Parallel requests can burst slightly past the threshold.

The gate rejects only once locked_until is set, so concurrent requests that all read locked_until = null before any reaches lock() each get a guess. The window is small because lock() runs immediately after the increment in the same request, and this is a standard trade-off — but if you want to tighten it, also refusing when the stored fail_count has already reached the threshold costs nothing extra on the existing read.

4. new TimeFactory() in Server.php — the TimeFactory service and its ITimeFactory alias are registered a few lines above; $c->query('TimeFactory') would be consistent and keeps the clock substitutable.

One documentation point

The description says the lockout "cannot be used to probe whether a user name exists" — accurate, and the implementation earns it. But because isTracked() returns true for unknown names and false for accounts of an external backend, six failures lock "local or non-existent" and never lock an LDAP/OIDC account, so the behaviour can be used to probe whether a name is backed by an external IdP. That's largely inherent to scoping this to local accounts — you can't lock LDAP accounts without double-punishing them — and leaking backend type rather than existence is clearly the better of the two trade-offs. Worth stating explicitly in the description and admin docs so it's a recorded decision rather than an implied guarantee.

On the 10.16 backport

Since the 10.x line is out of support, #41806 isn't required from a security standpoint — your call whether to still land it.

Nice piece of work: the ordering, the enumeration parity, the LoginException inheritance and the pending-upgrade tolerance are all the non-obvious details, and they're all right.

[phil-davis]

Implementing anything like this means that there is a DoS vector that can be used by an attacker that knows some (or all) the actual usernames on the system. They can keep sending invalid requests with usernames that they know, and effectively lock those users out from accessing the system. The lockout starts, the real user can't successfully do anything for 10 minutes, the attacker keeps sending invalid attempts so that within a few seconds of the lockout ending, the account is locked again.

That is a problem/feature of any auto-lockout system at this level. (otherwise you have to have the equivalent of some whole front-end "CloudFlare" kind of thing that checks where the requests are coming from and... to try to distinguish what are reasonably-real requests vs attack requests)

[phil-davis]

I think that this is mostly true. For a real user who has had their account locked out, when they successfully login after the lockout has expired, then the lockout row in the database table will be deleted.

If someone spams an API endpoint with invalid requests for each of thousands of usernames, then the lockout table with have thousands of records. There will not be any later real login attempts for those usernames, so they do need to be cleaned up by this job.

Or do I miss something?

[phil-davis]

The way this is implemented, I can, for example:

• send an invalid attempt for user "alice" once every 12 minutes (at 0, 12, 24, 36 and 48 minutes)
• that 5th attempt at 48 minutes will lock the account of "alice" for 10 minutes

Within any 15-minute window there was never more than 2 failed login attempts, the 5 attempts were across a 48-minute window.

I suppose it is OK to do it that way. Some attempts that seem to be very slow still trigger a lockout every hour or so. The alternative would be to remember the actual time of every failed attempt so that old attempts could be removed from the accumulated count when they age beyond the attempt window. That would be significant overhead to keep track - both computing all the time intervals and the extra database activity to store and retrieve that data.