Skip to content

fix(security): stop the user password-reset endpoint from identifying admins - #46

Open
nicdavidson wants to merge 1 commit into
developfrom
security/pwreset-admin-enumeration
Open

fix(security): stop the user password-reset endpoint from identifying admins#46
nicdavidson wants to merge 1 commit into
developfrom
security/pwreset-admin-enumeration

Conversation

@nicdavidson

Copy link
Copy Markdown
Contributor

Low key. No public write-up until this and df-system#62 are both released.

The oracle

POST /api/v2/user/password?reset=true takes no authentication and answered
differently depending on what kind of account the address belonged to. Observed
on a running 7.7.0 instance (df-user 0.17.2, df-system 0.6.5):

Address Response
unknown 404 The supplied email was not found in the system.
registered admin 401 You are not authorized to reset/change password for the account admin@dreamfactory.com
registered ordinary user 200 {"success":true}

Three distinguishable answers. An unauthenticated caller can take a list of
addresses and sort it into admins, ordinary users, and non-accounts. The 401
also echoes the address back, which confirms the account exists and that it is
privileged in a single request.

df-system#62 collapses the unknown case into the success envelope. It does not
touch this one, so the admin oracle survives that fix. Hence a separate PR in
this repo.

The change

Admin accounts still cannot be reset through this endpoint. The refusal is just
no longer audible. Password::passwordReset() returns the same envelope an
ordinary reset produces and does nothing else: no email, no confirm_code
write, no state change on the admin row. An attacker cannot use it to churn
admin confirmation codes either.

isAllowed() keeps throwing for the confirmation-code and security-answer
flows, which already require a secret, but no longer echoes the address.

Verified on a running instance

Patched the vendor copy on a live 7.7.0 container and exercised it:

ADMIN : {"success":true} [200]
USER  : {"success":true} [200]

Byte-identical. Only the ordinary user received mail in mailpit; the admin
received nothing. Admin password reset through its own endpoint,
POST /api/v2/system/admin/password, still returns {"success": true} and
still delivers, so the legitimate path is unaffected. Admin login with the
existing password still works.

Merge note

df-system#62 unifies UserPasswordResource::passwordReset()'s success envelope
to ['success' => true, 'security_question' => null]. The early return added
here must match that byte for byte or the oracle reopens on the shape of the
response instead of its status code. There is a comment on the line saying so.

Both fixes also need a tagged release and a composer bump to reach customers.
The released builds still have the original behaviour.

… admins

POST /api/v2/user/password?reset=true answered differently depending on
what kind of account the address belonged to, so an unauthenticated
caller could sort a list of emails into admins and everyone else:

  admin address   -> 401 "You are not authorized to reset/change password
                     for the account admin@example.com"
  ordinary user   -> 200 {"success":true}

The 401 also echoed the address back, confirming both that the account
exists and that it is privileged.

Admin accounts must not be resettable through this endpoint, but the
refusal does not have to be audible. Password::passwordReset() now
returns the same envelope an ordinary reset produces and sends nothing:
no email, no confirm_code write, no state change on the admin row.

Verified against a running 7.7.0 instance. Admin and ordinary addresses
now return byte-identical responses:

  ADMIN : {"success":true} [200]
  USER  : {"success":true} [200]

and only the ordinary user receives mail. Resetting an admin password
through its own endpoint, POST /api/v2/system/admin/password, is
unaffected and still delivers.

Also drops the address from the UnauthorizedException in isAllowed(),
which is still reached from the confirmation-code and security-answer
flows.

Note for whoever merges this alongside df-system#62: that PR unifies
UserPasswordResource::passwordReset()'s envelope to include
'security_question' => null. The early return here must match it
byte-for-byte or the oracle reopens. There is a comment on the line.
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.

1 participant