Contrib modules can bypass TFA authentication

3410444-event-sub-user-auth is a work in progress to start discussion. This branch provides an event subscriber to catch when bypass_tfa_auth_for_user is utilized and no token validation has occurred.

An alternative we might want to evaluate is if we can perform a check in hook_user_login(). Perhaps a combination of the two options might provide sufficient interlocking.

Updating title to better reflect that this is related to installing contrib modules leads to TFA accidentally being bypassed (as oppose to intentionally disabling/bypassing TFA)

Linking related core issue for adding better MFA support into the login process since in context its ultimately the adhoc nature that causes issues.

hook_user_login() is a no-go. It isn't always called after authentication. This can be seen with the http_basic authentication method. While we don't expect http_basic to have the same faults as the form its best not to leave it as a possibility.

I did find a AccountEvents::SET_USER event that is dispatched whenever AccountProxy::setAccount() is called. I am working on converting the solution in 3410444-event-sub-user-auth to to this new event. This event happens sooner which could allow us to depend more on core to handle responses. In the case of the form based we can destroy the exception before hook_user_login() is even called and throw an exception to end code execution. In the case of http_basic (and other similar methods) we could trigger before any code action could occur.

Looking at AccountEvents::SET_USER and I'm realizing that users being able to be set without going through TFA is inherently also the cause of issues such as 📌 SA-CONTRIB-2023-030 and 2.x Active .

I've been investigate if we can use this for more than just protecting where the forms are modified and instead make it more holistic having the security system defend more generally.

AccountEvents::SET_USER is called:

From user_login_finalize() when starting a session for a user. This occurs when logging in through the forms, using password reset.
Hook_user_login() is called later from inside user_login_finalize().

From the AccountSwticher Service (AccountSwitcher::switchTo):
Used when when opening up an impersonation (single request) to allow code to run as one user while not logging out of the first user.

AuthenticationSubscriber::onKernelRequestAuthenticate():
This is done on every request. Prior to calling the subscriber obtains the current user from

• Session based authentication grabs the UID from $session->get('uid);
• http_basic authentication provider decodes the username/password before calling the UserAuth service. Auth methods validating passwords will follow similar methods
• Any other authentication provider that validates the information provided, without calling UserAuth

Those method calling the UserAuth service are easy to work with, we could set a flag and know the UserAuth was called and the request is inherently validated through TFA. That covers user_login_finalize() and onKernalRequestAUthenticate for http_basic and similar.

onKernelRequestAuthenticate() for session based authentication could be handled by setting a value iset n hook_user_login() to validate the value isn't changed outside of TFA knowledge (avoiding $session->set('uid', SOME_UID);)

AccountSwitcher can be exmpeted with a decorator indicating that the request is part of a UserSwitch event. There could be an argument for denying these requests, perhaps that could be a followup for a feature where more discussion on the benefits/risk of doing so.

onKernelRequestAuthenticate() allowing user/password authentication (tokens) while blocking others is the complex one. This, after some deeper thought, can also be solved with a (dynamic) decorator on the methods that are permitted to bypass TFA (such as a token based auth method). This would require some manual configuration by site admins, however it can be limited to a simple configuration parameter and for many sites would require no action.

I have most of the code for this already built as POC, it just needs cleanup and finalized testing to be sure no scenarios are missed. Addtionaly it likely needs documentation improvements to go with it.

Uploaded a first version of this so that I can step back for a few days and give it another look with a fresh set of eyes.

This includes some decent technical documentation, though I'm realizing as I type that I didn't document in an admin friendly section so will need work for that.

At the moment this will break one time login links. I consider that a proof that this is actually fixing our root level problem in TFA. We can fix the one time login links in 📌 Use an EventSubscriber to process one time login links Needs work .

Looks like one of the tests runs well in my local lab but not in GitLab, will need to investigate options.

Did some cleanup, mostly around enabling strict types and using static data providers with some minor spelling/wording changes.

If i had to split this up into chunks to make it smaller it would look roughly like:

-- Add flag to complete session login.
src/Form/EntryForm.php
src/Form/TfaLoginForm.php
tests/src/Unit/Form/TfaEntryFormTest.php
tests/src/Unit/Form/TfaLoginFormTest.php
tests/src/Unit/TfaModuleTest.php
tfa.module

-- Add flag on complete User.Auth login.
src/TfaUserAuth.php
tests/src/Unit/TfaUserAuthTest.php

-- Add Auth Decorators
src/Authentication/Provider/TfaAuthDecorator.php
src/Authentication/Provider/TfaChallengeAuthDecorator.php
src/Compiler/TfaAuthDecoratorCompiler.php
src/TfaServiceProvider.php
tests/src/Unit/Authentication/TfaAuthDecoratorTest.php
tests/src/Unit/Authentication/TfaChallengeAuthDecoratorTest.php
tests/src/Unit/Compiler/TfaAuthDecoratorCompilerTest.php
tests/src/Unit/TfaServiceProviderTest.php

-- Add flag for user.switcher
src/TfaAccountSwitcher.php
tests/src/Unit/TfaAccountSwitcherTest.php

-- Enforce TFA completed.
docs/SUMMARY.md
docs/configuration/SUMMARY.md
docs/configuration/exempt-auth-provider.md
docs/development/SUMMARY.md
docs/technical/SUMMARY.md
docs/technical/set-user-protection.md
src/EventSubscriber/TfaUserSetSubscriber.php
src/Exceptions/TfaAccessDenied.php
tests/src/Unit/EventSubscriber/TfaUserSetSubscriberTest.php
tests/src/Functional/TfaTestBase.php

Automatically closed - issue fixed for 2 weeks with no activity.

@cmlara

i see the fix commit "on 2.x", followed by the auto-close, here.

(1) will the fix be backported to 8.x branch?
(2) is there a list of known-to-have-issues modules, that are at this risk?
(3) should (new) production be yet moved to 2x (still -alpha, or -dev)?

(1) will the fix be backported to 8.x branch?

Fundamentally in my opinion these are API breaking changes (combination of no 'well defined' API defined in 1.x, and being major operation workflow changes). I can't see a way to cleanly backport these to 1.x

(2) is there a list of known-to-have-issues modules, that are at this risk?

I do not have the resources to audit the entire contrib ecosystem. I believe all the modules know at the time of the issue are in the summary or linked issues, however there are likely more.

For 1.x: In general if the module edits the login form, alters the login form workflow, or allows authentication outside the login form it should be considered a risk that needs to be manually validated.

(3) should (new) production be yet moved to 2x (still -alpha, or -dev)?

My personal suggestion would be to use an external SSO provider with MFA if possible.

2.x is nowhere near stable or ready for production and I believe still has a few loose ends needed on know security issues in 1.x that need to be implemented differently in 2.x. I'm a bit surprised to see a higher level maintainer pushed an alpha3/alpha4 with some major know issues present.

If you can't use an external SSO, 1.x is likely better than nothing, however it comes with significant caveats that we keep finding holes in the implementation that just can not be properly fixed without major branch changes. We bandage up many of the known exploits, however the root fault is generally still there waiting for the next "oh we didn't expect that" incident.

thx for the writeup
does seem that 2x isn't quite alpha even; appreciate the clarification.

1.x does install, and appears to work . but the 'holes', good enuf for dev, won't do for prod.

not a fan of third party security -- might poke at this to see which local SSO solution, if any, might work.
and, how to integrate it into D11 ...