User tests: Successful: Unsuccessful:
When session time was expired and we open site page, it getting data from expired session data which store in DB, so page has an old form token, and, after, expired data deletes from db here https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/application/cms.php#L758-L768, so next any pages have new token and, when we call ajax from page with old token, we get invalid token message, because user data session was recreated with new token.
Status | New | ⇒ | Pending |
Labels |
Added:
?
|
At this moment expired session data use for start session anyway, i just fix it for ajax requests, which must use the same token.
In thats code https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/application/cms.php#L750-L776 session starting with an expired data from DB and after was deleted and created again with the same session data, but token was changed, because its random generated.
User session will deletes when expired time for 'remember me' flag. I not see unsecure in my code, because its related with 'remember me' plugin and should be continue user session data here.
The code you highlighted has nothing to do with the actual session data other than the $session->start()
call. That's all related to a bunch of basically useless extraneous metadata. The session API correctly determines that a session has expired and deals with the mitigation/deletion of data as appropriate.
If the remember me plugin requires this hack to work, then the changes need to be made in the plugin as that is the place modifying the session and user authentication behaviors. The place that you have made the change introduces a security concern into the database session handler which enables ALL sessions with expired data to be resumed.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-01-04 17:28:36 |
Closed_By | ⇒ | brianteeman |
Closing for the reasons resistance above. It is never going to be accepted as it opens an security hole
@mbabker actualy session will delete for any user in the same hightlight code, when another any user come in site. So your notice which enables ALL sessions with expired data to be resumed
not true, its not deletes just if native user come in and 'remember me' flag still not expired in user cookie.
And as i say before $session->start()
use it anyway with the completley the same useless extraneous metadata
.
I see situation: if we use old session data, then it still correct, so session time should have current time. Actualy it do the same now, just recreated data again, so token was changed too.
No!
First, the highlighted code is a requirement because of the extra metadata stored by the application regardless of the session store you're using and basically performs the same function as the PHP session API's gc()
method. It ensures that even if you're using a non-database session handler, the session table is cleaned up of what should be dead session records. It's crap code, I've been on a crusade against it for months, but frankly without dropping several end user features that have little to no value in 2016 it isn't going anywhere. Set your session handler to anything but the database and see all that extra data that's still written to the database because of JApplicationCms::checkSession()
.
Your proposal, as I stated previously, introduces a security hole into the database session handler. It introduces the possibility to resume in full an expired session. See https://www.owasp.org/index.php/Session_Management_Cheat_Sheet#Session_Expiration for further guidance on how expired sessions should be handled.
Even with the remember me flag enabled, once the session expires, it would not be resumed. Rather, a new session is created automatically based on valid data to create the remember me token and user cookie. It does not in any way re-use the user's previous session data.
The behavior you are seeing with the invalid CSRF token is therefore fully valid and expected.
So all unsecure what i have in my code - the same form token usage or you see something more?
The existing behavior is correct. This hack will introduce a security issue of allowing an expired session's data to be considered valid.