Skip to content

Commit c19f19a

Browse files
smarcetsmarcet
authored
fix: MFA-enforced accounts leak enumeration via passwordless login guard order (#154)
* 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. * test: assert the passwordless rejection is indistinguishable, not just unworded testInvalidOtpAgainstEnforcedUserDoesNotLeakMFAStatus only asserted that the flash message lacked the phrase "two-factor authentication". Enumeration is about distinguishability, so that assertion still passed if a future enforced-only branch leaked a differently worded message. It now drives the same invalid code through the same endpoint for the MFA-enforced admin and for a non-enforced control user, and asserts the two rejections are byte-identical. The stale flash is cleared between requests so the comparison cannot read a value against itself. Verified by mutation: restoring the pre-fix guard order with reworded text fails the new assertSame and would have passed the old substring assertion. Also records why testEnforcedUserCannotBypassMFAViaPasswordlessLogin adds no separate "no login side effect" assertions. Both candidates were tried and removed as change detectors that cannot fail: the DB-visible effects (OTP redemption, sibling revocation) are rolled back by the AuthService transaction regardless of where the guard sits, and the session-visible one (Auth::login() and the Login event queuing PostLoginUser) is already caught first by the existing Auth::check() assertion. Both confirmed by mutation. Full file green: OK (58 tests, 380 assertions).
1 parent e72bc80 commit c19f19a

4 files changed

Lines changed: 147 additions & 32 deletions

File tree

‎app/Http/Controllers/UserController.php‎

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

614614
if ($flow == IAuthService::AuthenticationFlowPasswordless) {
615615

616-
// Passwordless login is single-factor (email access only) and
617-
// must not be usable to satisfy MFA enforcement (SDS idp-mfa.md
618-
// §7.4 / Open Question #3).
619-
$existing_user = $this->auth_service->getUserByUsername($username);
620-
if (!is_null($existing_user) && $existing_user->shouldRequire2FA()) {
621-
throw new AuthenticationException(
622-
"This account requires password and two-factor authentication. Please use the password login option."
623-
);
624-
}
625-
626616
$client = $this->resolveClientFromMemento();
627617

628618
$otpClaim = OAuth2OTP::fromParams($username, $connection, $password);
629-
$this->auth_service->loginWithOTP($otpClaim, $client);
619+
$this->auth_service->loginWithOTPEnforcing2FA($otpClaim, $client);
630620
// A completed login must not leave the OTP screen restorable
631621
// on a later refresh - same identity-leakage concern already
632622
// 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
@@ -272,38 +272,92 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $
272272
);
273273

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

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

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

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

299319
$this->finalizeRedemption($otp, $user, $client);
300-
301320
Auth::login($user, $remember);
302-
Log::debug(sprintf("AuthService::loginWithOTP user %s logged in.", $user->getId()));
321+
Log::debug(sprintf("AuthService::loginWithOTPEnforcing2FA user %s logged in.", $user->getId()));
303322
return $otp;
304323
});
305324
}
306325

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

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

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

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

96108
/**
97109
* @param string $username

‎tests/TwoFactorLoginFlowTest.php‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,15 @@ public function testNonAdminWithoutMFALogsInNormally(): void
193193

194194
public function testEnforcedUserCannotBypassMFAViaPasswordlessLogin(): void
195195
{
196+
// No separate "no login side effect" assertions here on purpose: the
197+
// DB-visible ones (OTP redemption, sibling revocation) are unreachable
198+
// by construction, because the guard throws inside the AuthService
199+
// transaction and DoctrineTransactionService rolls the whole closure
200+
// back - moving the guard below finalizeRedemption() leaves the OTP
201+
// un-redeemed all the same. The session-visible one (Auth::login() and
202+
// the Login event that queues PostLoginUser) is already covered by the
203+
// Auth::check() assertion below, which is what fails first if the guard
204+
// is moved below Auth::login(). Both were verified by mutation.
196205
$this->emitOTP(self::ADMIN_EMAIL);
197206
$code = $this->latestOtpCode(self::ADMIN_EMAIL);
198207

@@ -213,6 +222,56 @@ public function testEnforcedUserCannotBypassMFAViaPasswordlessLogin(): void
213222
$this->assertSame('otp', Session::get('flow'), 'a reload must land back on the OTP screen, not silently fall back to password');
214223
}
215224

225+
public function testInvalidOtpAgainstEnforcedUserDoesNotLeakMFAStatus(): void
226+
{
227+
// Regression guard: the enforcement check used to run BEFORE the OTP was
228+
// validated, so an attacker submitting a garbage/guessed code against an
229+
// MFA-enforced account's email got the distinguishing "requires password
230+
// and two-factor authentication" message without ever proving control of
231+
// the inbox - an account-enumeration oracle. The check must now only be
232+
// reachable after loginWithOTPEnforcing2FA() has proven the code valid,
233+
// so an invalid code gets the same generic rejection for any account.
234+
//
235+
// Enumeration is about DISTINGUISHABILITY, not about one phrase: merely
236+
// asserting the absence of "two-factor authentication" would still pass
237+
// if some future enforced-only branch leaked a *differently* worded
238+
// message. So the enforced account's rejection is compared byte-for-byte
239+
// against a non-enforced control driven through the same endpoint with
240+
// the same bad code.
241+
$control_email = $this->createPlainUser();
242+
243+
$this->emitOTP(self::ADMIN_EMAIL);
244+
$this->postLoginOTP(self::ADMIN_EMAIL, 'not-the-real-code');
245+
246+
$this->assertFalse(Auth::check(), 'an invalid code must never authenticate');
247+
$this->assertResponseStatus(302);
248+
$enforced_notice = Session::get('flash_notice');
249+
$this->assertNotNull($enforced_notice, 'the enforced account must get a flashed rejection');
250+
251+
// Cleared so the control's assertion cannot silently read the enforced
252+
// account's leftover flash and compare a value against itself.
253+
Session::forget('flash_notice');
254+
255+
$this->emitOTP($control_email);
256+
$this->postLoginOTP($control_email, 'not-the-real-code');
257+
258+
$this->assertFalse(Auth::check(), 'an invalid code must never authenticate the control account either');
259+
$this->assertResponseStatus(302);
260+
$control_notice = Session::get('flash_notice');
261+
$this->assertNotNull($control_notice, 'the control account must get a flashed rejection');
262+
263+
$this->assertSame(
264+
$control_notice,
265+
$enforced_notice,
266+
'an invalid code must produce an identical rejection for an enforced and a non-enforced account - any difference is an enumeration oracle'
267+
);
268+
$this->assertStringNotContainsString(
269+
'two-factor authentication',
270+
$enforced_notice,
271+
'an invalid code must not leak that the account is MFA-enforced'
272+
);
273+
}
274+
216275
public function testNonEnforcedUserStillLogsInViaPasswordlessLogin(): void
217276
{
218277
$email = $this->createPlainUser();

0 commit comments

Comments
 (0)