"two-factor-authenticate" remains accessible in the middle of a login flow

[pcsegal]

Hi,

Regarding the moment after clicking "Sign In" and before completing the 2FA form:

As addressed by issue #8, I know that going to any page other than "two-factor-authenticate" takes the user out of this intermediate state (by removing the "allauth_2fa_user_id" session key).

However, as long as I stay within the "two-factor-authenticate" page, it will remain in that state until the session expires. So, I can, for example, close the page, then reopen it several days later and the 2FA form will still be there waiting for the same user to type the token.

It seems like a behavior that could be potentially exploited. Should there be a mechanism against that? Maybe the session expiry time could be set to a small value, like 5 minutes, when reaching that state, then reset to a longer value only after the flow is completed?

Thank you in advance.

[clokep]

Yes, that does seem pretty weird. I could think of a situation like the following:

• User attempts to log-in with username/password.
• User gets to the 2FA page and realizes they don't have their phone (or whatever TOTP device they use).
• User closes tab.

On a "shared" computer, someone else could re-open the page and enter the token without knowing the original password.

I think it is an edge-case, but is unexpected behavior. Expiring the session quickly is a probably a reasonable solution.

[greyhare]

So I've spent two days hunting down a bug related to this one. The behavior was:

1. Go to sign in page.
2. Enter username and password.
3. Redirected to 2FA prompt.
4. Enter 2FA (TOTP) code.
5. Redirected to sign in page.
6. Repeat from 2.

I managed to track it down to AllauthTwoFactorMiddleware.process_request() deleting allauth_2fa_user_id from the session. And I just found out why; this is what the runserver output typically looks like for that flow:

[04/Nov/2019 09:36:04] "GET /accounts/login/ HTTP/1.1" 200 20822
[04/Nov/2019 09:36:04] "GET /static/CACHE/css/cookies.968d7e16bfd2.css HTTP/1.1" 200 720
[04/Nov/2019 09:36:04] "GET /static/CACHE/css/project.06d33650b1ef.css HTTP/1.1" 200 378
[04/Nov/2019 09:36:04] "GET /static/CACHE/css/mwm.42f545798894.css HTTP/1.1" 200 121339
[04/Nov/2019 09:36:32] "POST /accounts/login/ HTTP/1.1" 302 0
[04/Nov/2019 09:36:33] "GET /accounts/two-factor-authenticate HTTP/1.1" 200 19551
[04/Nov/2019 09:36:33] "GET /static/CACHE/css/project.06d33650b1ef.css HTTP/1.1" 200 378
[04/Nov/2019 09:36:33] "GET /static/CACHE/css/cookies.968d7e16bfd2.css HTTP/1.1" 200 720
[04/Nov/2019 09:36:33] "GET /static/CACHE/css/mwm.42f545798894.css HTTP/1.1" 200 121339
[04/Nov/2019 09:36:33] "GET /apple-touch-icon.png HTTP/1.1" 302 0
[04/Nov/2019 09:36:58] "POST /accounts/two-factor-authenticate HTTP/1.1" 302 0
[04/Nov/2019 09:36:58] "GET /accounts/login/ HTTP/1.1" 200 20822
[04/Nov/2019 09:36:58] "GET /static/CACHE/css/project.06d33650b1ef.css HTTP/1.1" 200 378
[04/Nov/2019 09:36:58] "GET /static/CACHE/css/cookies.968d7e16bfd2.css HTTP/1.1" 200 720
[04/Nov/2019 09:36:58] "GET /static/CACHE/css/mwm.42f545798894.css HTTP/1.1" 200 121339

That GET /apple-touch-icon.png is what's wiping the key. Why is that GET there at all? It's a redirect to the static path; because of legacy reasons, browsers expect your icons at the root level. So I have a bunch of path(..., RedirectView.as_view(url=...)) entries in my site urlpatterns.

I'm not sure what the problem with "they're half-logged-in" is that requires no other HTTP activity at all until the POST /accounts/two-factor-authenticate step.

I get the "On a "shared" computer, someone else could re-open the page and enter the token without knowing the original password" case from the prior comment, but that trick works one and only if the bad guy has control of the TOTP device. In my case it's a non-issue, because my session cookie expiration is set to 0 so the cookie is dead once the tab is closed. I guess someone can restore the cookie by reopening the tab from history?

So I'm not sure how to fix this (I can replace AllauthTwoFactorMiddleware with my own code, but the question is what that code should be), but the current behavior is broken for me. It seems the "half-logged-in" state needs better definition. Maybe it needs to be passed a list of "safe" URL name patterns (which don't wipe the login) in a setting?

[greyhare]

Looking through django-otp, there's a OTPAuthenticationFormMixin in forms that could put everything on the login page and possibly fix this, but I'm at a loss as to where to mix it in.

[clokep]

Looking through django-otp, there's a OTPAuthenticationFormMixin in forms that could put everything on the login page and possibly fix this, but I'm at a loss as to where to mix it in.

Since the login forms aren't provided by django-allauth-2fa that could be hard. It could always be documented, of course.

Is there a reduced set of steps to reproduce this, by the way?

[greyhare]

I think the minimum set of steps to reproduce would be something like:

1. Set up a minimal Django project with django-allauth-2fa and its dependents. Leave the socialaccount stuff out of allauth. Also set it up to use runserver and to serve its own static data, e.g. STATIC_ROOT = '/static/'. This last bit is part of the trigger for the bug.
2. Create your superuser.
3. Add some graphics and stuff to the basic page template that's used for all your pages. A stie logo in the menu bar, for example. Also add some favicon files. This is the second part of the trigger.
4. Fire up runserver, create a new user, and set up Google Authenticator (or equivalent) TOTP for it.
5. Logout superuser and login your new user.
6. After the username/password page and before the enter token page, you'll see several accesses to /static/. These are the accesses that discard the session data that the username/password step created.
7. Entering the TOTP token sends you back to the username/password page, since the session data was discarded.

[greyhare]

The /static/ stuff might not be the trigger. Safari, at least, looks for /apple-touch-icon.png, though there are equivalent Android and other files that can trigger it the same way.

Here's the part of my urls.py that handles all the favicons:

from django.conf import settings
from django.urls import include, path, reverse
from django.conf.urls.static import static
from django.contrib import admin, sitemaps
from django.contrib.sitemaps.views import sitemap
from django.views.generic import TemplateView, RedirectView

# ...

def favicon_path(name, pattern=None):
    target = name if pattern is None else pattern
    url = f'{settings.STATIC_URL}images/favicons/{target}'
    return path(name, RedirectView.as_view(url=url))

# ...

urlpatterns = [
    # ...

    path("accounts/", include("allauth_2fa.urls")),
    path("accounts/", include("allauth.urls")),

    # ...

    # Favicons
    favicon_path('android-chrome-<dims>.png', 'android-chrome-%(dims)s.png'),
    favicon_path('apple-touch-icon<dims>.png', 'apple-touch-icon%(dims)s.png'),
    favicon_path('apple-touch-icon.png', 'apple-touch-icon.png'),
    favicon_path('browserconfig.xml'),
    favicon_path('favicon-16x16.png'),
    favicon_path('favicon-32x32.png'),
    favicon_path('favicon.ico'),
    favicon_path('mstile<dims>.png', 'mstile%(dims)s.png'),
    favicon_path('safari-pinned-tab.svg'),
    favicon_path('site.webmanifest'),
] + static(
    settings.MEDIA_URL, document_root=settings.MEDIA_ROOT
)

[greyhare]

As noted last November, it's AllauthTwoFactorMiddleware.process_request() making the mistake in line 23:

        if not match.url_name or not match.url_name.startswith(
                'two-factor-authenticate'):

There needs to be a check here for "harmless" paths that shouldn't reset the login flow. The question is how to get these URLs to this test:

• a setting with a list of additional "startswith" patterns for the URL name,
• a setting with a list of regular expressions,
• a function (or path to a function) to be called to provide the additional test, or
• require the module user to wrap or monkeypatch this middleware with that test (requiring resolve() to be called twice).

[SoulRaven]

i have the same BUG, but with

• javascript-catalog
• and my own javascript-settings.

We must find a solution to ignore specific views from the check. Because some view are legit and part of the process.

The middleware is loaded when the TwoFactorAuthenticate view is loaded, no normally that others view are loaded in the process.

[greyhare]

If a developer would tell me which of the four options I gave were acceptable, I could provide a pull request.

But I'm not going to go to that effort just to have it ignored.

[SoulRaven]

for me the more easy is to have like an exception list. one is to have some "internal utility views" and another is to have a full functional view redirect. In first instance, the "utility views" are not redirected and load directly

[SoulRaven]

most fast solution with exception list

def process_request(self, request):
        match = resolve(request.path)
        except_list = getattr(settings, 'EXCEPT_VIEWS_FROM_CHECK', ())
        except_list += ('two-factor-authenticate',)
        if not match.url_name or not match.url_name.startswith(except_list):
            try:
                del request.session['allauth_2fa_user_id']
            except KeyError:
                log.info(match.url_name)

[greyhare]

@soulraven Use the < > button in the toolbar to mark off code blocks. I can't read your comment.

Also, I'm not sure "must" is the word you wanted, and can't tell what you really meant.

[SoulRaven]

@soulraven Use the < > button in the toolbar to mark off code blocks. I can't read your comment.

Also, I'm not sure "must" is the word you wanted, and can't tell what you really meant.

fix