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

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

Merged
merged 1 commit into from Oct 13, 2017

Conversation

Projects
None yet
6 participants
@kbond
Contributor

kbond commented Oct 12, 2017

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.

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan Oct 12, 2017

Member

I wonder if the fix should be that when you are logged in via logout_on_user_change, that somehow we clear the remember_me cookie (similar to what we must do when the user actually logs out manually) ? It seems that if we are logged out via logout_on_user_change, then the AbstractRememberMeServices really has no way to know whether or not this logout occurred because of an expired session cookie or because the user had changed on a previous request.

Member

weaverryan commented Oct 12, 2017

I wonder if the fix should be that when you are logged in via logout_on_user_change, that somehow we clear the remember_me cookie (similar to what we must do when the user actually logs out manually) ? It seems that if we are logged out via logout_on_user_change, then the AbstractRememberMeServices really has no way to know whether or not this logout occurred because of an expired session cookie or because the user had changed on a previous request.

@kbond

This comment has been minimized.

Show comment
Hide comment
@kbond

kbond Oct 12, 2017

Contributor

Yeah, I jumped the gun on this PR... Discovered the remember me service has no way of knowing if the user has changed because the user isn't serialized.

We could clear the remember me cookie as part of the logout_on_user_change logic but what about if the session is cleared and the user is only being authenticated by the RememberMeListener - a changed user could still be authenticated.

Contributor

kbond commented Oct 12, 2017

Yeah, I jumped the gun on this PR... Discovered the remember me service has no way of knowing if the user has changed because the user isn't serialized.

We could clear the remember me cookie as part of the logout_on_user_change logic but what about if the session is cleared and the user is only being authenticated by the RememberMeListener - a changed user could still be authenticated.

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan Oct 12, 2017

Member

We could clear the remember me cookie as part of the logout_on_user_change logic but what about if the session is cleared and the user is only being authenticated by the RememberMeListener - a changed user could still be authenticated.

Hmm... but this seems like a separate issue, right? Right now, if my session expires, and I am "saved" by RememberMe, but my user has actually changed, then I will still be logged in. Aka, this is already an issue with remember me and unrelated, right? I don't think the "remember me" token could ever know if the user was changed... because (by its very nature), it is being activated because there is no user in the session... and so nothing to compare with. So is that fixable, or just the nature of remember_me? i.e. if you use remember_me, you're giving anyone with this cookie a "free pass" to login until that remember_me cookie expires (if you use the default way that remember_me works, where you just decode the cookie and load the user).

Member

weaverryan commented Oct 12, 2017

We could clear the remember me cookie as part of the logout_on_user_change logic but what about if the session is cleared and the user is only being authenticated by the RememberMeListener - a changed user could still be authenticated.

Hmm... but this seems like a separate issue, right? Right now, if my session expires, and I am "saved" by RememberMe, but my user has actually changed, then I will still be logged in. Aka, this is already an issue with remember me and unrelated, right? I don't think the "remember me" token could ever know if the user was changed... because (by its very nature), it is being activated because there is no user in the session... and so nothing to compare with. So is that fixable, or just the nature of remember_me? i.e. if you use remember_me, you're giving anyone with this cookie a "free pass" to login until that remember_me cookie expires (if you use the default way that remember_me works, where you just decode the cookie and load the user).

@kbond

This comment has been minimized.

Show comment
Hide comment
@kbond

kbond Oct 12, 2017

Contributor

What about running the user through the UserChecker in RememberMeListener?

Contributor

kbond commented Oct 12, 2017

What about running the user through the UserChecker in RememberMeListener?

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan Oct 12, 2017

Member

@kbond Hmm, indeed, that's a good idea. Let's try it. We may need a BC layer, but maybe not - it smells like a bug that a "disabled" user could become logged in thanks to their remember_me token.

Member

weaverryan commented Oct 12, 2017

@kbond Hmm, indeed, that's a good idea. Let's try it. We may need a BC layer, but maybe not - it smells like a bug that a "disabled" user could become logged in thanks to their remember_me token.

@kbond

This comment has been minimized.

Show comment
Hide comment
@kbond

kbond Oct 12, 2017

Contributor

Ok, I'll update this PR

Contributor

kbond commented Oct 12, 2017

Ok, I'll update this PR

@kbond kbond changed the title from [Security] Remember me user changed to [Security] Reject remember-me token is UserCheckerInterface::checkPostAuth() fails Oct 12, 2017

@kbond kbond changed the title from [Security] Reject remember-me token is UserCheckerInterface::checkPostAuth() fails to [Security] Reject remember-me token if UserCheckerInterface::checkPostAuth() fails Oct 12, 2017

@kbond

This comment has been minimized.

Show comment
Hide comment
@kbond

kbond Oct 12, 2017

Contributor

This ended up being a pretty simple fix. RememberMeAuthenticationProvider::authenticate() was only running the user through UserCheckerInterface::checkPreAuth() and not checkPostAuth().

Contributor

kbond commented Oct 12, 2017

This ended up being a pretty simple fix. RememberMeAuthenticationProvider::authenticate() was only running the user through UserCheckerInterface::checkPreAuth() and not checkPostAuth().

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Oct 12, 2017

Member

checkPreAuth was added as a bug fix in #11058.
I think this should be done on 2.7.

Member

chalasr commented Oct 12, 2017

checkPreAuth was added as a bug fix in #11058.
I think this should be done on 2.7.

@kbond kbond changed the base branch from 3.4 to 2.7 Oct 12, 2017

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan Oct 12, 2017

Member

Here are some details on why that old change was made: https://github.com/symfony/symfony/pull/11058/files#r18096092. Specifically, checkPostAuth() was actually removed from this at one point... so let's understand the full situation.

Member

weaverryan commented Oct 12, 2017

Here are some details on why that old change was made: https://github.com/symfony/symfony/pull/11058/files#r18096092. Specifically, checkPostAuth() was actually removed from this at one point... so let's understand the full situation.

@kbond

This comment has been minimized.

Show comment
Hide comment
@kbond

kbond Oct 12, 2017

Contributor

Ok switched the base branch to 2.7

Contributor

kbond commented Oct 12, 2017

Ok switched the base branch to 2.7

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan Oct 12, 2017

Member

Yea... to say more, if the User was disabled (as you mentioned #24525 (comment)), that would be caught in checkPreAuth()... right?

Member

weaverryan commented Oct 12, 2017

Yea... to say more, if the User was disabled (as you mentioned #24525 (comment)), that would be caught in checkPreAuth()... right?

@kbond

This comment has been minimized.

Show comment
Hide comment
@kbond

kbond Oct 12, 2017

Contributor

I do my disabled user check in checkPostAuth() because of a tiny security leak (#13994)

Contributor

kbond commented Oct 12, 2017

I do my disabled user check in checkPostAuth() because of a tiny security leak (#13994)

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan Oct 12, 2017

Member

Well, that's a good enough reason for me. Just to verify, with this patch, do you get the desired effects if you try in your real app?

Member

weaverryan commented Oct 12, 2017

Well, that's a good enough reason for me. Just to verify, with this patch, do you get the desired effects if you try in your real app?

@kbond

This comment has been minimized.

Show comment
Hide comment
@kbond

kbond Oct 12, 2017

Contributor

Yes, my real app works as expected.

I think if you have different behavior based on an exception thrown in checkPostAuth() it would still be possible, just not when authenticating with a remember me token. The user would have to login again through the form to initiate that behavior.

Contributor

kbond commented Oct 12, 2017

Yes, my real app works as expected.

I think if you have different behavior based on an exception thrown in checkPostAuth() it would still be possible, just not when authenticating with a remember me token. The user would have to login again through the form to initiate that behavior.

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Oct 12, 2017

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan Oct 12, 2017

Member

👍 from me. This is a slight behavior change, but it's fixing a (security) bug imo: you should never want a remembe_me cookie to authenticate a user that fails the user checker (the default UserChecker::postAuth() checks for $user->isCredentialsNonExpired().

The distinction between pre-auth checks (checks that are done before checking credentials, and so are so errors are exposed to the user) and post-auth checks is not relevant here: both should be checked.

Member

weaverryan commented Oct 12, 2017

👍 from me. This is a slight behavior change, but it's fixing a (security) bug imo: you should never want a remembe_me cookie to authenticate a user that fails the user checker (the default UserChecker::postAuth() checks for $user->isCredentialsNonExpired().

The distinction between pre-auth checks (checks that are done before checking credentials, and so are so errors are exposed to the user) and post-auth checks is not relevant here: both should be checked.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Oct 13, 2017

Member

Thank you @kbond.

Member

fabpot commented Oct 13, 2017

Thank you @kbond.

@fabpot fabpot merged commit fe190b6 into symfony:2.7 Oct 13, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Oct 13, 2017

bug #24536 [Security] Reject remember-me token if UserCheckerInterfac…
…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

This was referenced Oct 30, 2017

@fabpot fabpot referenced this pull request Nov 10, 2017

Merged

Release v2.7.36 #24914

This was referenced Nov 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment