Skip to content

feat: temporarily lock out local accounts after failed logins - #41806

Open
oc-tmueller wants to merge 1 commit into
10.16from
fix/oc10-153-account-lockout
Open

feat: temporarily lock out local accounts after failed logins#41806
oc-tmueller wants to merge 1 commit into
10.16from
fix/oc10-153-account-lockout

Conversation

@oc-tmueller

Copy link
Copy Markdown

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 master line: #41805

🤖 Generated with Claude Code

Core did not throttle password authentication at all (CWE-307), so a local
account with a weak password could be brute-forced online at request rate
unless an optional app was installed. Every password verifying entry point
was an unlimited oracle.

Adds a DB backed 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 - no administrative unlock exists and
no account is ever disabled - and a successful login clears the counter
immediately. All four values are configurable via `account_lockout.*`.

Gated at the three entry points which verify a password against the user
backend, ahead of the credential check so a locked account does not even pay
for the hash:

  - Session::loginWithPassword(), covering the 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.

Accounts of an external backend are not tracked - LDAP/AD counts the same
attempt itself via badPwdCount and OIDC lockout belongs to the IdP, so
counting twice would punish twice. Login names which do not resolve to any
account are tracked under the same key space and produce exactly the same
response as a locked existing account, otherwise the lockout would answer
whether a user name exists. The unlocked read path performs no account
lookup at all, so there is no timing difference either.

Notable details:

  - Keys are lower cased in PHP, so `admin`, `Admin` and `ADMIN` share one
    budget of attempts regardless of the collation of the database.
  - Every counter update is a single statement, so requests served by
    different application servers cannot lose a count, and the lockout 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 the table being absent and simply does not lock
    anybody out until the migration has run.
  - Deleting an account drops its counter, so a recreated namesake does not
    inherit it.
  - tryBasicAuthLogin() reports a lockout like a wrong password because not
    every caller of \OC::handleLogin() handles a LoginException; the login
    form and the DAV backend, which can carry the explanation, do not use
    that method.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
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