Skip to content

[Security] logout remember_me user that has changed #24525

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.

Sign up for GitHub

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

Jump to bottom
Closed
kbond opened this issue Oct 11, 2017 · 7 comments
Closed

[Security] logout remember_me user that has changed #24525

kbond opened this issue Oct 11, 2017 · 7 comments
Labels
Bug Security Status: Needs Review

Comments

@kbond
Copy link
Member

kbond commented Oct 11, 2017

Q A
Bug report? yes?
Feature request? yes?
BC Break report? no
RFC? yes
Symfony version 3.4.0

With #23882, a session token is logged out when the user has changed but not if you have a remember me token. When visiting a page when logged in after your account has changed, you lose the session token but fallback to an authenticated RememberMeToken.

Thoughts on how we can fix this?
@kbond kbond mentioned this issue Oct 11, 2017
Closed
@linaori
Copy link
Contributor

linaori commented Oct 12, 2017

How exactly is the user changed?

@kbond
Copy link
Member Author

kbond commented Oct 12, 2017 •
edited
Loading

The user is disabled. I am using the EqualtableInterface

@kbond kbond mentioned this issue Oct 12, 2017
Merged
@kbond
Copy link
Member Author

kbond commented Oct 12, 2017

I attempted a fix in PR #24536. I think it might be a BC break so I could use some help to eliminate that.

@kbond
Copy link
Member Author

kbond commented Oct 12, 2017

I ended up going a different direction with #24536. The user in RememberMeToken cannot be compared as there is no user to compare with. I ended up adding a missing UserCheckerInterface::checkPostAuth() call in RememberMeAuthenticationProvider::authenticate()

fabpot added a commit that referenced this issue Oct 13, 2017
@fabpot
bug #24536 [Security] Reject remember-me token if UserCheckerInterfac…
4ebd60f 
…e::checkPostAuth() fails (kbond)

This PR was merged into the 2.7 branch.

Discussion
----------

[Security] Reject remember-me token if UserCheckerInterface::checkPostAuth() fails

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #24525
| License       | MIT
| Doc PR        | -

I think this is a security hole - a user can remain logged in with a remember me cookie even though they can no longer pass `UserCheckInterface::checkPostAuth()` (could be disabled).

This is a small BC break but shouldn't be an issue as I think it is a bug. I don't think this requires a BC layer but if so, I can add.

Commits
-------

fe190b6 reject remember-me token if user check fails
@fabpot fabpot closed this as completed Oct 13, 2017
@egonolieux
Copy link

Maybe I am doing something wrong as this should be fixed, but I still have this problem using the EqualtableInterface in version 3.4.4 having the logout_on_user_change option set to true.

In my isEqualTo method I compare the user's email address, password and roles. If I change any of these values in the database, the session gets invalidated but the user remains logged in because of the remember me token:

[2018-03-01 01:43:10] request.INFO: Matched route "account". {"route":"account","route_parameters":{"_controller":"AppBundle\\Controller\\AccountController::accountAction","_route":"account"},"request_uri":"https://example.com/account","method":"GET"} []
[2018-03-01 01:43:10] security.DEBUG: Read existing security token from the session. {"key":"_security_main","token_class":"Symfony\\Component\\Security\\Core\\Authentication\\Token\\RememberMeToken"} []
[2018-03-01 01:43:10] security.DEBUG: Token was deauthenticated after trying to refresh it. {"username":"user@email.com","provider":"Symfony\\Bridge\\Doctrine\\Security\\User\\EntityUserProvider"} []
[2018-03-01 01:43:10] security.DEBUG: Remember-me cookie detected. [] []
[2018-03-01 01:43:10] security.INFO: Remember-me cookie accepted. [] []
[2018-03-01 01:43:10] security.DEBUG: Populated the token storage with a remember-me token. [] []
[2018-03-01 01:43:10] security.DEBUG: Stored the security token in the session. {"key":"_security_main"} []

My security config is pretty basic:

security:

    encoders:
        AppBundle\Entity\User:
            algorithm: bcrypt
            cost: 12

    firewalls:
        api:
            pattern: ^/api
            stateless: true
            anonymous: ~
            http_basic:
                provider: user_entity_provider
                realm: API
        content:
            pattern: ^/content
            security: false
        dev:
            pattern: ^/_profiler
            security: false
        main:
            provider: user_entity_provider
            stateless: false
            anonymous: ~
            logout_on_user_change: true
            form_login:
                login_path: /account/sign-in
                check_path: /account/sign-in
                default_target_path: /
                csrf_token_generator: security.csrf.token_manager
            logout: 
                path: /account/sign-out
                target: /
            remember_me:
                name: REMEMBERME
                secret: '%secret%'
                lifetime: 31536000 # 1 year.

    providers:
        user_entity_provider:
            entity:
                class: AppBundle:User
                property: emailAddress

@chalasr
Copy link
Member

chalasr commented Mar 2, 2018

@egonolieux please open a new issue if you think that you found a bug, comments on closed tickets aren't tracked.

@egonolieux egonolieux mentioned this issue Mar 2, 2018
Closed
@egonolieux
Copy link

@chalasr sorry for that, I was not sure to post it here or to open a new issue ;). #26379

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Security Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@fabpot @kbond @linaori @egonolieux @chalasr