MFA Session cookie and MFA resource evations prevention

[peppelinux]

The good @lorenzo4securitas tested some webpath evasion and then we decided to add some additional checks.

This PR

• adds a specialized session / cookie for anything related the MFA
• uses an additional attribute mfa_authenticated to enable additional filters for the protection of the MFA endpoints
• protects the mfa endpoints with a check based on mfa_authenticated, and not just by using the generic django user logged in check; according to the following approach:
  • create: the user can create new 2fa only if he doesn't have already configured a 2fa or if it is authenticated with 2fa. This allows the creation of MFA for user that doesn't have any yet
  • list, delete: the user must be authenticated with 2fa and must have at least a valid 2fa already configured.

[xi]

Thanks for the contribution!

uses an additional attribute mfa_authenticated to enable additional filters for the protection of the MFA endpoints

I have never personally needed this, but it seems to be a common request. So I think this is a useful addition, especially considering your next point.

I am wondering whether we should also provide a MFARequiredMixin to make this easier to use. My current feeling is that we should do one step at a time and only add MFARequiredMixin when someone actually needs it.

This does need some documentation though.

protects the mfa endpoints with a check based on mfa_authenticated, and not just by using the generic django user logged in check

I really like this idea.

My only comment is on the name of the mixin. MFAFilteredDispatcher feels very abstract to me. What do you think about MFARequiredIfExistsMixin.

adds a specialized session / cookie for anything related the MFA

I am really not convinced by this yet:

• It is a breaking change (a new midleware is required)
• It makes setup more complicated
• It duplicates a lot of complicated code from django
• I fear that we miss out on some of django's security guarantees by using a separate session

Maybe I am missing something. Can you explain the benefits of using a separate session for MFA?

In general, it would make my life easier if you could split the changes into multiple commits (one for each of these points and also split out unrelated style changes) and add some tests.

I am looking forward to your response. This is great stuff!

[peppelinux]

hey @xi

• Agreed MFARequiredIfExistsMixin I'll take another week since I'm traveling, consider it done

• regarding "adds a specialized session / cookie"

  • It is a breaking change (a new midleware is required) -> yes it is
  • It makes setup more complicated -> no, adding middlewares is something popular in the django ecosystem
  • It duplicates a lot of complicated code from django -> yes but unfortunately django's SessionStorage is not handy for inheritance
  • I fear that we miss out on some of django's security guarantees by using a separate session -> no, since I brought all the security properties we already have in the default and general settings. My scope is to specialized sessions and cookies for different purpose, because an MFA implementation should be something self-consistent and under its own. In this way we avoid conflicting claims that other middlewares and implementation (even of other mfas systems) might introduce. Having a specialized session for MFA allow us to better focus the improvements of the user's authentication state for specific implementation aspects, as the ones you mentioned in the README

  In general, it would make my life easier if you could split the changes into multiple commits

I was wondering that having all in a single commit would be more easy to read, but I can redo the work if this is necessary. Probably some code linting on my commits will be added as well.

Let's continue our discussion on separating the session, I can also purpose an option for implementers that doesnt want this (default, without breaking changes) with implementers that wants this (me). A simple approach based on getattr(request, 'mfa_session', 'session') would be the way to go and probably here we'll find our agreement

thank you for the quick reply, this is very promising for the next release

[peppelinux]

@xi I have finally applied your code review in this commit

1. MFAFilteredDispatcher renamed to MFARequiredIfExistsMixin
2. adds a specialized session / cookie -> it's now optional. Implementations that just want to continue with the default sessionstorage doesn't need to do any action. A dispatcher detects if a separate storage is used or not.
3. unit tests updated accordingly. The current behaviour with the enforced security is now part of the CI

I look forward for your kindly feedback and the merge of this PR for the next release

[xi]

I had another look at this and I get the feeling that it doesn't actually tackle the heart of the problem. Consider these cases:

• If the user had keys on login, MFA was enforced so the new checks pass. An attacker who gets access to that session has full access.

• If the user has no keys, the new checks still pass. An attacker who gets access to that session could create a new key and thereby lock out the legitimate user (denial of service). These changes do nothing to prevent this.

• If the user had no keys during login, but has created some during the session, the checks would fail. As far as I can see this is the only case in which attackers would actually be restricted: They could not change the existing keys. (You missed the CreateView, so denial of service is still possible, but that can easily be fixed).

  However, a legitimate user would also no longer be able to change their keys. So if a user sets up a key and something goes wrong, they are now locked out of their own account because in order to reset the broken key they would have to login with that broken key.

In summary, I think this does more harm than good.

A better option could be to require re-authentication for managing keys. To prevent users from locking themselves out we could have exceptions for keys that have been created during the current session. I understand that this would be very different from what you implemented. I am also not sure if that would be a good fit for this library. But I am open for ideas!

Let me know what you think and if you want to continue working on this issue. I still think there is a lot of potential, it is just much harder than I first anticipated.

[peppelinux]

thank you for the time spent on this PR @xi. Point by point:

If the user had keys on login, MFA was enforced so the new checks pass. An attacker who gets access to that session has full access.

we could set a timeout on the mfa_authenticated in minutes. Then the session would be updated with the removal of the mfa_authenticated and an attacker that steals a session would be then forced to submit the 2fa. If I got your issue.

If the user has no keys, the new checks still pass. An attacker who gets access to that session could create a new key and thereby lock out the legitimate user (denial of service). These changes do nothing to prevent this.

this leds to the credential provisioning. Does an admin of the platform be forced to create a code and a recovery code as well to be provided to new users OR the user must create its 2fa on the first access (if MFA is mandatory). This touches UX strategies, I would keep this out of the scope of this PR for now.

However, a legitimate user would also no longer be able to change their keys. So if a user sets up a key and something goes wrong, they are now locked out of their own account because in order to reset the broken key they would have to login with that broken key.

This leds to security vs. UX. This procedure may led to the security protocol for the users locked out. If you agree we can add an option to enable the additional check proposed by this PR or not. From the current perspective this PR should be considered to avoid an attacker to login and remove the 2fa with the sole submission of username/password.

The case of the user locked out should be resolved with a ticker or an additional security protocol, not technical but administrative. This may depend on the internal policies and provisioning mechanisms.

A better option could be to require re-authentication for managing keys.

A collegue of mine has suggested exactly this. Even this has its security vs UX cons. We can do that, we must decide how. do we assume that the user must submit also the 2fa as the actual PR introduces? If yes, we can leave this PR as it is and add the timeout on the mfa_authenticated. This would ask the 2fa only on the users that wants to list/delete the 2fas.

To prevent users from locking themselves out we could have exceptions for keys that have been created during the current session.

ok, can we get this PR merged on a dev branch and continue the developments in this direction? We can add an additional identifier in the mfa_session with the pks of the 2fa created during the current session. When you say "exception for keys" what do you exactly mean?

I still think there is a lot of potential, it is just much harder than I first anticipated.

I fully agree with you, we're facing the security vs. UX issues that MFA brings with itself. I would merge this PR as it is stable and, without tagging a new release, create the user stories in the form of issues, to have a clear approach and analysis on the UX and on the security impacts we might have.

[xi]

this PR should be considered to avoid an attacker to login and remove the 2fa with the sole submission of username/password

But that is already impossible. If a key exists, it must be used during login. Sure, some security-in-depth would be nice to prevent issues like CVE-2022-24857. But if an attacker can bypass MFA during login they gain nothing from removing the keys. They can already log in.

I would merge this PR as it is stable and, without tagging a new release, create the user stories in the form of issues, to have a clear approach and analysis on the UX and on the security impacts we might have.

To be clear: I will not merge this pull request. As I see it, the alternatives that we discussed would share very little code with this approach. So I see no benefit in merging this.

Here is a rough outline of what we need instead:

• Store the timestamp of the last MFA authentication in the session
• Provide a view to re-authenticate without a new login
• Provide a mixin that redirects to the re-authentication view if the authentication is too old
• Figure out how to protect the key management views that actually improves security and prevents users from locking themselves out of their own accounts (ideally without new options)

[peppelinux]

I want to use this library and give my support for the improvement of the security of the solution. This is my priority (the what). Then I have to work with you for the implementation of the solution (the how).

The What (problem statement)

scenario A An adversary can get logged in with username and password and in front of the 2fa submission form can bypass this stage making a http request to the mfa listing endpoint, then the adversary can delete one or more 2fa and get back to the previous pages or redo the login. After having submitted username and password the adversary gets logged in.

scenario B following the steps of the scenario A, the adversary creates a brand new 2fa making the user (victim) locked out.

How (solution statement)

this PR introduces an additional checks for the user profiles with 2fa enabled: the user can list and delete her/his 2fa onlt if authenticated with a 2fa. Otherwise, if the user doesn't have created yet a 2fa he/she can create a 2fa.

Roadmap (actionable agreement and task to be done)

The task marked with [x] are agreed and well understood. The unmarked ones requires further discussion/understanding to get developed and then done.

• [x] Store the timestamp of the last MFA authentication in the session
  • comment: yes, I can do this in this current PR if possibile.
• [ ] Provide a view to re-authenticate without a new login
  • concern: how to authenticate without a new login? Do you mean a passwordless approach? for which scope/purpose?
• [ ] Provide a mixin that redirects to the re-authentication view if the authentication is too old
  • concern: this would force an authenticated user to be re-authenticated, this could be perceived like an UX issue.
• [ ] Figure out how to protect the key management views that actually improves security and prevents users from locking themselves out of their own accounts (ideally without new options)
  • comment: the user is requested to create a recovery code to prevent the lock out (as many security system implements). Users that doesn't are then would be forced to open an assistance ticket. This is an implementation choice, while I assume that this choice is out of the scope of this library.

@xi let's continue working together for an agreement, time is our allied. Thank you for your time and attention to this PR, we can do it together.

[peppelinux]

thank you @Qadeem411 for you kindly revision and approval, I just have updated this PR with the resolution of the merge conflicts

[xi]

scenario A An adversary can get logged in with username and password and in front of the 2fa submission form can bypass this stage making a http request to the mfa listing endpoint, then the adversary can delete one or more 2fa and get back to the previous pages or redo the login. After having submitted username and password the adversary gets logged in.

Can you provide a working proof of concept how an attacker would bypass the two factor authentication? If that were possible, that would be a major security issue. But so far I cannot see how that would be possible.

[kushaldas]

Can we get this merged soon please?