Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure device has not been logged out #461

Conversation

patrickomeara
Copy link
Contributor

This adds a middleware to check that the password hash in session is the same as the current users password, and logs the user out if not.

This fixes a security issue where an attacker can keep sending requests to an API using the sanctum auth after the password has been updated.

Scenario:
If a user is logged in in two different browsers, they change their password in one, the web session is invalidated in the other, but requests can still be sent via the sanctum cookie auth to the API.

refs #142

@patrickomeara patrickomeara force-pushed the ensure-device-has-not-been-logged-out branch from 0abee2e to 1f97c6b Compare August 10, 2023 12:49
This adds a middleware to check that the password has in session is the same as the current users password.

This fixes a security issue where an attacker can keep sending requests to an API using the sanctum auth after the password has been changed.
@patrickomeara patrickomeara force-pushed the ensure-device-has-not-been-logged-out branch from 1f97c6b to 9392ec9 Compare August 10, 2023 12:51
@taylorotwell
Copy link
Member

Can you not just use the already existing AuthenticateSession middleware?

https://laravel.com/docs/10.x/authentication#invalidating-sessions-on-other-devices

@patrickomeara
Copy link
Contributor Author

Hi @taylorotwell

From what I can see, adding auth.session after auth:sanctum uses the RequestGuard which doesn't have a viaRemember method.

Method Illuminate\Auth\RequestGuard::viaRemember does not exist.

There is also a fatal call to logoutCurrentDevice

Method Illuminate\Auth\RequestGuard::logoutCurrentDevice does not exist.

EnsureDeviceHasNotBeenLoggedOut is a stripped down version of AuthenticateSession that handles only the passsword hash check and logging out if it doesn't match. It rightfully doesn't handle the remember me nor the storing of the password hash.

As it's quite difficult to test for this I have set up a test repo that has both the auth.session middleware branch and the new middleware branch. There are setup instructions in the README there.

Related issues
laravel/framework#40988
https://stackoverflow.com/questions/65623617/logoutotherdevices-isnt-working-with-laravel-sanctum/70469518#70469518
#142

@taylorotwell
Copy link
Member

taylorotwell commented Aug 11, 2023

@patrickomeara using this middleware - how does the password hash get stored in the session in the first place? Is it stored by AuthenticateSession during the original "web" request?

Furthermore, what are the breaking change implications if we tag this on a patch release and people are not already using AuthenticateSession?

* this means the original request had `auth.session` middleware
@patrickomeara patrickomeara force-pushed the ensure-device-has-not-been-logged-out branch from 91e363d to fc7c77e Compare August 11, 2023 23:11
@patrickomeara
Copy link
Contributor Author

patrickomeara commented Aug 11, 2023

Yes the original web request stores the password hash in the session. This is the existing mechanism that logs out other sessions on password change.

This newly added Sanctum-only middleware will log out the sanctum sessions as well.

I've added a check so if the original request isn't using AuthenticateSession the middleware continues. This change will make a patch release backward compatible.

@patrickomeara
Copy link
Contributor Author

I've updated the test repo to use this branch of sanctum.

@patrickomeara
Copy link
Contributor Author

I'm converting this to draft as I've found a condition where the guard changes from web to sanctum when changing the password via a sanctum request and logs the current session out.

@patrickomeara
Copy link
Contributor Author

Closing, as laravel/framework#48056 is a better approach to this problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants