Skip to content

Commit 67043e3

Browse files
smarcetsmarcet
committed
fix: MFA-enforced accounts leak enumeration via passwordless login guard order
Root cause: UserController::postLogin()'s shouldRequire2FA() guard ran before loginWithOTP() validated the submitted OTP, so a caller without the real code could distinguish MFA-enforced accounts from the rejection message alone - an account-enumeration oracle requiring no credentials. Fix: introduce AuthService::loginWithOTPEnforcing2FA(), which checks shouldRequire2FA() after the OTP is proven valid and before finalizeRedemption()/Auth::login() - so a guessed/invalid code is rejected generically before reaching the account-status branch, and a valid code against an enforced account is rejected before any login side effect (redemption, Auth::login, the Login event / queued PostLoginUser job) fires. loginWithOTP() (used by InteractiveGrantType and TokenService's OAuth2 grants) is unchanged - only UserController::postLogin()'s interactive web login now enforces 2FA at this layer. Avoids a boolean flag parameter (flags-over-objects antipattern) by exposing two explicitly-named public methods delegating to a shared, flag-free resolveOTPUser() helper. Adds a regression test proving an invalid OTP against an MFA-enforced account gets the same generic rejection as any other invalid code.
1 parent ec64f2f commit 67043e3

4 files changed

Lines changed: 110 additions & 32 deletions

File tree

‎app/Http/Controllers/UserController.php‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -591,20 +591,10 @@ public function postLogin()
591591

592592
if ($flow == IAuthService::AuthenticationFlowPasswordless) {
593593

594-
// Passwordless login is single-factor (email access only) and
595-
// must not be usable to satisfy MFA enforcement (SDS idp-mfa.md
596-
// §7.4 / Open Question #3).
597-
$existing_user = $this->auth_service->getUserByUsername($username);
598-
if (!is_null($existing_user) && $existing_user->shouldRequire2FA()) {
599-
throw new AuthenticationException(
600-
"This account requires password and two-factor authentication. Please use the password login option."
601-
);
602-
}
603-
604594
$client = $this->resolveClientFromMemento();
605595

606596
$otpClaim = OAuth2OTP::fromParams($username, $connection, $password);
607-
$this->auth_service->loginWithOTP($otpClaim, $client);
597+
$this->auth_service->loginWithOTPEnforcing2FA($otpClaim, $client);
608598
// A completed login must not leave the OTP screen restorable
609599
// on a later refresh - same identity-leakage concern already
610600
// fixed for the MFA flow's verify2FA()/verify2FARecovery().

‎app/libs/Auth/AuthService.php‎

Lines changed: 75 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -271,38 +271,92 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $
271271
);
272272

273273
// TX-C: resolve or create user, finalize, login
274-
return $this->tx_service->transaction(function () use ($otp, $otpClaim, $client, $remember) {
274+
return $this->tx_service->transaction(function () use ($otp, $client, $remember) {
275+
$user = $this->resolveOTPUser($otp);
276+
$this->finalizeRedemption($otp, $user, $client);
277+
Auth::login($user, $remember);
278+
Log::debug(sprintf("AuthService::loginWithOTP user %s logged in.", $user->getId()));
279+
return $otp;
280+
});
281+
}
275282

276-
$user = $this->getUserByUsername($otp->getUserName());
283+
/**
284+
* @param OAuth2OTP $otpClaim
285+
* @param Client|null $client
286+
* @param bool $remember
287+
* @return OAuth2OTP|null
288+
* @throws Exception
289+
*/
290+
public function loginWithOTPEnforcing2FA(OAuth2OTP $otpClaim, ?Client $client = null, bool $remember = false): ?OAuth2OTP
291+
{
292+
Log::debug(sprintf("AuthService::loginWithOTPEnforcing2FA otp %s user %s", $otpClaim->getValue(), $otpClaim->getUserName()));
277293

278-
if (is_null($user)) {
279-
Log::debug(sprintf("AuthService::loginWithOTP user %s does not exist; auto-registering.", $otp->getUserName()));
280-
$user = $this->auth_user_service->registerUser(
281-
[
282-
'email' => $otp->getUserName(),
283-
'email_verified' => true,
284-
'send_email_verified_notice' => false,
285-
'active' => true,
286-
],
287-
$otp
288-
);
289-
} else if ($user->isActive()) {
290-
$user->verifyEmail(false);
291-
}
294+
$otp = $this->findAndValidateOTP(
295+
$otpClaim->getValue(),
296+
$otpClaim->getUserName(),
297+
$otpClaim->getConnection(),
298+
$otpClaim->getScope(),
299+
$client
300+
);
292301

293-
if (!$user->canLogin()) {
294-
Log::warning(sprintf("AuthService::loginWithOTP user %s cannot login (not active).", $user->getId()));
295-
throw new AuthenticationException("We are sorry, your username or password does not match an existing record.");
302+
// TX-C: resolve or create user, enforce 2FA, finalize, login
303+
return $this->tx_service->transaction(function () use ($otp, $client, $remember) {
304+
$user = $this->resolveOTPUser($otp);
305+
306+
// Passwordless login is single-factor (email access only) and must not
307+
// be usable to satisfy MFA enforcement (SDS idp-mfa.md §7.4 / Open
308+
// Question #3). Checked here - after the OTP is proven valid, before
309+
// finalizeRedemption()/Auth::login() - so neither a guessed code nor a
310+
// rejected valid code ever triggers a login side effect (redemption,
311+
// Auth::login, the Login event / queued PostLoginUser job).
312+
if ($user->shouldRequire2FA()) {
313+
throw new AuthenticationException(
314+
"This account requires password and two-factor authentication. Please use the password login option."
315+
);
296316
}
297317

298318
$this->finalizeRedemption($otp, $user, $client);
299-
300319
Auth::login($user, $remember);
301-
Log::debug(sprintf("AuthService::loginWithOTP user %s logged in.", $user->getId()));
320+
Log::debug(sprintf("AuthService::loginWithOTPEnforcing2FA user %s logged in.", $user->getId()));
302321
return $otp;
303322
});
304323
}
305324

325+
/**
326+
* Resolves the user for an already-validated passwordless OTP, auto-registering
327+
* a brand-new email if needed. Does not finalize redemption or log in - callers
328+
* decide that (and whether to enforce 2FA first).
329+
* @param OAuth2OTP $otp
330+
* @return User
331+
* @throws AuthenticationException
332+
*/
333+
private function resolveOTPUser(OAuth2OTP $otp): User
334+
{
335+
$user = $this->getUserByUsername($otp->getUserName());
336+
337+
if (is_null($user)) {
338+
Log::debug(sprintf("AuthService::resolveOTPUser user %s does not exist; auto-registering.", $otp->getUserName()));
339+
$user = $this->auth_user_service->registerUser(
340+
[
341+
'email' => $otp->getUserName(),
342+
'email_verified' => true,
343+
'send_email_verified_notice' => false,
344+
'active' => true,
345+
],
346+
$otp
347+
);
348+
} else if ($user->isActive()) {
349+
$user->verifyEmail(false);
350+
}
351+
352+
if (!$user->canLogin()) {
353+
Log::warning(sprintf("AuthService::resolveOTPUser user %s cannot login (not active).", $user->getId()));
354+
throw new AuthenticationException("We are sorry, your username or password does not match an existing record.");
355+
}
356+
357+
return $user;
358+
}
359+
306360
/**
307361
* Verifies an OTP against an already-authenticated session user (MFA primitive).
308362
*

‎app/libs/Utils/Services/IAuthService.php‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,18 @@ public function loginUser(User $user, bool $remember): void;
9191
*/
9292
public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $remember = false): ?OAuth2OTP;
9393

94+
/**
95+
* Same as loginWithOTP(), but rejects the login when the resolved user has
96+
* MFA enforced - passwordless (email-only) proof is not sufficient for an
97+
* account that requires two-factor authentication.
98+
* @param OAuth2OTP $otpClaim
99+
* @param Client|null $client
100+
* @param bool $remember
101+
* @return OAuth2OTP|null
102+
* @throws AuthenticationException
103+
*/
104+
public function loginWithOTPEnforcing2FA(OAuth2OTP $otpClaim, ?Client $client = null, bool $remember = false): ?OAuth2OTP;
105+
94106

95107
/**
96108
* @param string $username

‎tests/TwoFactorLoginFlowTest.php‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,28 @@ public function testEnforcedUserCannotBypassMFAViaPasswordlessLogin(): void
213213
$this->assertSame('otp', Session::get('flow'), 'a reload must land back on the OTP screen, not silently fall back to password');
214214
}
215215

216+
public function testInvalidOtpAgainstEnforcedUserDoesNotLeakMFAStatus(): void
217+
{
218+
// Regression guard: the enforcement check used to run BEFORE the OTP was
219+
// validated, so an attacker submitting a garbage/guessed code against an
220+
// MFA-enforced account's email got the distinguishing "requires password
221+
// and two-factor authentication" message without ever proving control of
222+
// the inbox - an account-enumeration oracle. The check must now only be
223+
// reachable after loginWithOTPEnforcing2FA() has proven the code valid,
224+
// so an invalid code gets the same generic rejection for any account.
225+
$this->emitOTP(self::ADMIN_EMAIL);
226+
227+
$response = $this->postLoginOTP(self::ADMIN_EMAIL, 'not-the-real-code');
228+
229+
$this->assertFalse(Auth::check(), 'an invalid code must never authenticate');
230+
$this->assertResponseStatus(302);
231+
$this->assertStringNotContainsString(
232+
'two-factor authentication',
233+
Session::get('flash_notice'),
234+
'an invalid code must not leak that the account is MFA-enforced'
235+
);
236+
}
237+
216238
public function testNonEnforcedUserStillLogsInViaPasswordlessLogin(): void
217239
{
218240
$email = $this->createPlainUser();

0 commit comments

Comments
 (0)