DarkGhostHunter / Laraguard

"On-premises 2FA Authentication for all your users out-of-the-box
MIT License
266 stars 24 forks source link

Credentials in hidden fields #46

Closed mtveerman closed 3 years ago

mtveerman commented 4 years ago

Storing the credentials in a hidden field feels like a security issue. Basically, it happens here:

https://github.com/DarkGhostHunter/Laraguard/blob/master/src/Listeners/EnforceTwoFactorAuth.php#L112

The way the package is setup (implementing listeners, non evasive) doesn't really allow for a different way I think. I like the non invasiveness, but aren't there options to avoid the hidden fields, or at least limit security issues?

For example:

menufinderafrica commented 3 years ago

I remember emailing similar suggestions based on README guidance on how to report security issues.

  1. I was thinking option 3 above marking as success and then using a middleware to handle 2FA on all routes except /login, /reset, /2fa-recovery etc. obviously.
DarkGhostHunter commented 3 years ago

I'll use sessions to capture the credentials instead of reflashing them, and a no-cache control in the response header to avoid being cached.

I could try to use login() directly once I get the user ID and store it in the session, so I wouldn't need the credentials again being reflashed.

DarkGhostHunter commented 3 years ago

After giving it a lot of thought, it's impossible unless a big rewrite it's done.

The main reason is that it captures the login procedure from the request, meaning, the action the form submits to. The form must have the credentials otherwise the guard won't validate the user, thus, the Validated event won't fire. This is one of the main reasons you can add the TOTP input at any login page and call it a day.

The best I can do is to set a header for no-cache.

DarkGhostHunter commented 3 years ago

As a follow up, if you need top security, you can still create your own 2FA procedure with this package, by disabling the listeners.