[#2749] - More fixes and cleanup for cookie authentication (aka remember me)
- Closed
- 21 Feb 2014
- Medium
- Build: staging
- # 2749
- Diff
- Bakual:CleanupRememberMe
- Success The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Issue
There are various issues with the current remember-me feature. However the main one is that the code is spread over three plugins. The remember system plugin, the cookie authentication plugin and the joomla user plugin. This is mainly the case because the needed events are not triggered for each plugin group for sure. While this worked it made the code complicated and also in some cases duplicated that it was getting hard to understand what is going on. And complicate code tends to let bugs slip in. And there are a few of them. For example when using the same browser on different computers you could get unexpected results.
Testing and tl;dr
Make sure the cookie authentication works as expected, that means after closing the browser and reopening you should be logged in again automatically.
After login, the first part (divided by a dot) in the cookie should change and the second part should stay the same.
After logout, the cookie is deleted.
In the #__user_keys table, the hashed token has to change after each login, the series should stay the same.
After logout, the entries are removed.
Proposed Solution and technical stuff
This PR moves all code into the authentication cookie plugin.
The joomla user plugin has no code anymore related to cookie authentication.
The system remember plugin is still needed, but has no code anymore with actually dealing with the authentication. All it does is check for a specific cookie (if no user is logged in already), and if it exists it will initialise the login process without further checks, handing the authentication process to whatever plugin will do it. Yes that means you could exchange the core cookie plugin with a 3rd party one as long as it will use the same cookie name.
The remember system plugin will also make sure authentication plugins are loaded for the onUserAfterLogout event.
Other than that the system plugin will not do anything.
There are also some other changes:
- The plugin parameters and language strings moved from the
rememberplugin to thecookieplugin. I've written an MySQL query to move them during an update so this should be backward compatible. The other database queries would still be needed. - This allowed to remove some unneeded special handling in the application which basically passed the remember parameters as options to the authentication plugins.
- Deprecated methods in JUserHelper which are very specific to the cookie authentication plugin. It is now dealt with directly in the plugin.
- Removed the uastring part in the cookie value. It's the same string as the cookie name and thus only redundant information. It's still stored in the database and is used to validate the cookie, but instead of relying on the cookie value, we now check against the cookie name.
- The token is stored in the database as hash using the same method we use for the password. The reason is that in case someone has access to the database, he can't just read out the token directly.
There are additionally some improvements to how the cookie is authenticated. For that I have to explain the system a bit:
How it works
If the remember-me checkbox is set when an user has successfully logged in, the plugin will create a cookie with the following information:
cookie name: The name of the cookie is created using JUserHelper::getShortHashedUserAgent() which generates a hash based on the Joomla URL and the browser user agent (without version part). It's just a way to generate a unique identifier on a given computer. This name doesn't have to be secure and can in fact easily recalculated.
** cookie value:** The value of the cookie consists of two parts, separated by a dot (.). The first part is the token and the second part is the series. Both are random strings generated using JUserHelper::genRandomPassword(). The token will be replaced after each successful login, making it a one-time-use "password". The series will be the same over the lifespan of the cookie, making it sort of a "user name", it's also unique in the database table.
When that user visits the page again later, the plugin will look for that specific cookie name and load it if present. Then it will check if the series part is valid. After that it will go and verify the token and if successful will look up the username in the database table and log in the user.
When the user logs out, the cookie and all entries for the current series are deleted.
Now the interesting part is what happens when something fails. Obviously when the cookie is not present, nothing happens. When the cookie is present but the series does not match or the value has a wrong format, the cookie will be deleted. If the series matches but the token doesn't validate, we assume an attack and delete all tokens for that user from the database. This case should only happen if the series was either guessed (brute forced) correctly or if the cookie was stolen and used twice (once by attacker and once by victim). This action is logged.
We still will have 2 plugins with frustrating titles (Cookie and Remember Me).
Why not to have single system plugin 'System - Remember Me'?
System plugin can incorporate all triggers from both 'authentication' and 'user' group', hence we will have complete remember me logic in single place.
System plugin can incorporate all triggers from both 'authentication' and 'user' group', hence we will have complete remember me logic in single place.
Authentication plugins work a bit different. The onUserAuthenticate method is only triggered for plugins in the authentication group. It's not triggered for system plugins.
See https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/user/authentication.php#L286 for the code.
So that's why it needs to be an authentication plugin.
On the other hand since authentication plugins aren't loaded as long as there is nothing to authenticate, we need a system plugin which can trigger the login process.
I agree, but in order to login user via cookie we need to check user status and cookie on each page load (like current plugin does in PlgSystemRemember::onAfterInitialise()), otherwise we just won't be able to auto-login user. So system plugin is the only possible solution for cookie-based authentication. Or we can move this logic inside Joomla core, but it's not our choice I hope :)
We can't check cookie presence in onUserAuthenticate event, it's launched only once user does a login, but cookie login is hidden and occurs once a user visits site.
in order to login user via cookie we need to check user status and cookie on each page load (like current plugin does in PlgSystemRemember::onAfterInitialise())
The remember system plugin still does that check. It first checks if the user is not logged in (if (JFactory::getUser()->get('guest'))) and then if a valid cookie is present (if ($this->app->input->cookie->get($cookieName)))
The difference is that currently, this plugin also contains part of the authentication logic, and in my PR it will only trigger the login with $this->app->login(), leaving all the authentication logic to the authentication plugins.
So system plugin is the only possible solution for cookie-based authentication.
The authentication can't be done in the system plugin. As said the event is not triggered in the system group. It's only triggered in the authentication group.
Yes, sure it checks for guest user.
System plugins can use events of ALL groups because 'system' group is always loaded first. That's why I am talking about having single system plugin.
Sorry, no, onUserAuthenticate is really triggered in 'authentication' plugins only... We just need to use all plugins via usual $dispatcher->trigger('onUserAuthenticate'...
Sorry, no, onUserAuthenticate is really triggered in 'authentication' plugins only... We just need to use all plugins via usual $dispatcher->trigger('onUserAuthenticate'...
I guess there is a reason it isn't done with $dispatcher->trigger('onUserAuthenticate', ...) here. We maybe don't want to propagate the users username and password to each loaded plugin and want to restrict authentication to authentication plugins. It's a security related task after all 
It's foolish idea because if required - plugins can inject own instance into JPluginHelper and still get all user credentials. Or just listen to POST. So it's completely safe to change launching of 'onUserAuthenticate' to default code with importing 'authentication' plugins group and next triggering dispatcher.
So it's completely safe to change launching of 'onUserAuthenticate' to default code with importing 'authentication' plugins group and next triggering dispatcher.
It may be safe, it may be not. That would need proper testing on itself and some security reviews as well. You can do a PR to change that if you want. I will not touch it with this PR as it's out of the scope of this PR.
Howdy,
I was playing with this and encountered a couple of issues.
The first is the use of the JUserHelper::getShortHashedUserAgent(). This bases the hash in part on JUri::base() which returns the base URL including protocol. In the case of forcing login to SSL this limits the cookie to only be used over SSL by default. As in our case we have the login and all user profile actions segmented to an SSL protected section of the site. So using the remember me cookie fails on the non SSL protected areas of the site.
What we did was use a hash based on the url without the protocol, so the hostname and script_name and user agent. This provides a consistent hash across http and https sections.
The second issue I had was with the PlgAuthenticationCookie::onUserAfterLogin. I ran into the case where some browsers (safari, webkit based and possibly firefox) do a prefetch on the url even before you hit enter to go to the site. This is causing a valid login via PlgSystemRemember, but the PlgAuthenticationCookie::onUserAfterLogin is attempting to change the login cookies password hash based on a random password. It appears that these browsers are not keeping the result of the cookie change based on the reset when you actually goto the site and sending the original saved cookie password hash instead. So a login failure happens.
Upon looking at this I am questioning in the need to reset the cookies random password at all. Its reseting the password as well as giving the cookie a completely new valid time based on the cookie length again. So you really end up with a cookie that never expires as long as you go back to the page within the valid cookie length time. Im not sure that is the intended behavior. Its a valid one for some cases, but in the case where you want to force the user to re-login ever so often to verify credentials it breaks. Im my option it should probably be an option in remember to choose which method of expiration you want.
One thing I might add is you might think about adding the httpOnly flag on the remember cookie. This will prevent client side script access or manipulation (if the browser supports it) but will still be sent on XHR/Ajax calls as part of the normal headers.
@wonderslug Imho, I think a cookie should not be shared across SSL and non-SSL pages. But that may be a subjective view.
Its reseting the password as well as giving the cookie a completely new valid time based on the cookie length again.
The lifespan issued for the cookie doesn't really reflect the real lifespan. It could be faked anyway. The point of thruth is the lifespan saved in the database and that's why we need a consistent series part over the lifespan of the cookie. You should be forced to login manually again after the lifespan set in the options expires.
As for why we need to change the token each time: If we don't do that, you have no way to detect a hacking attemp.
I ran into the case where some browsers (safari, webkit based and possibly firefox) do a prefetch on the url even before you hit enter to go to the site.
Is it possible to detect that prefetch somehow? I can see that this will cause issues.
One thing I might add is you might think about adding the httpOnly flag on the remember cookie. This will prevent client side script access or manipulation (if the browser supports it) but will still be sent on XHR/Ajax calls as part of the normal headers.
I think that should be possible to do.
| Status | New | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2014-02-21 07:36:40 |
infograf768
-
Tracker:
)
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33068
(no additional information there