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

kbond · 2017-10-12T13:12:09Z

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 · 2017-10-12T13:21:58Z

src/Symfony/Component/Security/Http/RememberMe/AbstractRememberMeServices.php

-            return new RememberMeToken($user, $this->providerKey, $this->secret);
+            $token = new RememberMeToken($user, $this->providerKey, $this->secret);
+
+            if (!$token->isAuthenticated()) {


Won't the token always be not authenticated because setAuthenticated() is now never called?

yeah, just found that out.... working on a fix

weaverryan · 2017-10-12T13:24:18Z

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 · 2017-10-12T13:47:33Z

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 · 2017-10-12T14:18:23Z

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 · 2017-10-12T14:21:59Z

What about running the user through the UserChecker in RememberMeListener?

weaverryan · 2017-10-12T14:24:54Z

@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 · 2017-10-12T14:25:14Z

Ok, I'll update this PR

kbond · 2017-10-12T14:48:52Z

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

chalasr · 2017-10-12T14:54:37Z

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

weaverryan · 2017-10-12T15:02:50Z

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 · 2017-10-12T15:02:53Z

Ok switched the base branch to 2.7

weaverryan · 2017-10-12T15:04:49Z

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

kbond · 2017-10-12T15:07:39Z

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

weaverryan · 2017-10-12T15:09:53Z

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 · 2017-10-12T15:20:57Z

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.

weaverryan · 2017-10-12T17:06:38Z

👍 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 · 2017-10-13T13:42:55Z

Thank you @kbond.

…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

carsonbot added Status: Needs Review Security Bug BC Break Deprecation labels Oct 12, 2017

kbond changed the base branch from master to 3.4 October 12, 2017 13:13

kbond mentioned this pull request Oct 12, 2017

[Security] logout remember_me user that has changed #24525

Closed

weaverryan reviewed Oct 12, 2017

View reviewed changes

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

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

reject remember-me token if user check fails

fe190b6

kbond changed the base branch from 3.4 to 2.7 October 12, 2017 15:01

nicolas-grekas removed BC Break Deprecation labels Oct 12, 2017

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

nicolas-grekas approved these changes Oct 13, 2017

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 13, 2017

chalasr approved these changes Oct 13, 2017

View reviewed changes

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

This was referenced Oct 30, 2017

Release v3.4.0-BETA2 #24761

Merged

Release v4.0.0-BETA2 #24762

Merged

This was referenced Nov 10, 2017

Release v2.7.36 #24914

Merged

Release v2.8.29 #24915

Merged

Release v3.3.11 #24916

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kbond commented Oct 12, 2017 •

edited

Loading

weaverryan Oct 12, 2017

kbond Oct 12, 2017

weaverryan commented Oct 12, 2017

kbond commented Oct 12, 2017

weaverryan commented Oct 12, 2017

kbond commented Oct 12, 2017

weaverryan commented Oct 12, 2017

kbond commented Oct 12, 2017

kbond commented Oct 12, 2017

chalasr commented Oct 12, 2017

weaverryan commented Oct 12, 2017

kbond commented Oct 12, 2017

weaverryan commented Oct 12, 2017

kbond commented Oct 12, 2017

weaverryan commented Oct 12, 2017

kbond commented Oct 12, 2017

weaverryan commented Oct 12, 2017

fabpot commented Oct 13, 2017

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

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

Conversation

kbond commented Oct 12, 2017 • edited Loading

weaverryan Oct 12, 2017

Choose a reason for hiding this comment

kbond Oct 12, 2017

Choose a reason for hiding this comment

weaverryan commented Oct 12, 2017

kbond commented Oct 12, 2017

weaverryan commented Oct 12, 2017

kbond commented Oct 12, 2017

weaverryan commented Oct 12, 2017

kbond commented Oct 12, 2017

kbond commented Oct 12, 2017

chalasr commented Oct 12, 2017

weaverryan commented Oct 12, 2017

kbond commented Oct 12, 2017

weaverryan commented Oct 12, 2017

kbond commented Oct 12, 2017

weaverryan commented Oct 12, 2017

kbond commented Oct 12, 2017

weaverryan commented Oct 12, 2017

fabpot commented Oct 13, 2017

kbond commented Oct 12, 2017 •

edited

Loading