Bug #34964
closed
FE Session record is never removed, even if no session data left
100%
Description
One of the security features in TYPO3 is a changing FE cookie IDs for each request. This mechanism is paused when session data is saved. The problem is that there is no proper way to remove session data. Instead, entries are saved without valid data. So even if there is no more session data, the cookie stays unchanged, which undermines a security feature.
Flashmessages demonstrate the issue. Once a flashmessages was set in session data, it never gets removed. Even if it was delivered and flushed.
Invalidation of FE session data happens in tslib_feuserauth::storeSessionData()
...
$insertFields = array (
'hash' => $this->id,
'content' => serialize($this->sesData),
'tstamp' => $GLOBALS['EXEC_TIME'],
);
$this->removeSessionData();
$GLOBALS['TYPO3_DB']->exec_INSERTquery('fe_session_data', $insertFields);
...
$this->sesData is NULL, but serialize($this->sesData) results in 'N;'.
However, even if it was NULL, the session data is never removed. There is no check for empty data.
Solution is to check for $data===NULL in setKey(), then unset the key and check for empty $this->sesData before writing to DB.
Files
Updated by Gerrit Code Review over 12 years ago
- Status changed from New to Under Review
Patch set 1 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9719
Updated by Steffen Müller over 12 years ago
- File 34964_4-5.diff 34964_4-5.diff added
Added also a patch for TYPO3 4.5
Updated by Steffen Müller over 12 years ago
The example code above is taken from 4.5, but the problem still exists in master.
Updated by Steffen Müller over 12 years ago
Demo extension added.
Just install and add plugin to page.
- Hit "Reload page" multiple times to see the cookie changing.
- Hit "Add and show a Flashmessage" to add flash message.
- Hit "Reload page" again multiple times to see the cookie is NOT changing anymore.
Apply patch and replay above steps.
Updated by Steffen Müller over 12 years ago
If you try to reproduce with server and client on localhost, you have to add this line to localconf.php:
$TYPO3_CONF_VARS['SYS']['devIPmask'] = '254.254.254.254';
Updated by Philipp Gampe over 12 years ago
With and without your patch, the displayed cookie is never changing unless I close the browser.
How do you make the cookie change?
Updated by Helmut Hummel over 12 years ago
- Status changed from Under Review to Needs Feedback
There's no benefit in changing the session id on every request. This is just a side effect of the session fixation prevention in combination with the currently implemented frontend session data logic.
It would be absolutely fine to have the same session id over several requests, especially when it's an unauthorized session, if it can be guaranteed that it is an id generated by TYPO3.
Just in contrast I would prefer to keep one id for unauthorized sessions and only regenerate it when a user logs in.
I think your patch would not harm, but I also do not see any benefits. Can you elaborate your point?
Updated by Steffen Müller over 12 years ago
Indeed, you are right. I will investigate and write a post as soon as I found the reason for this.
Updated by Steffen Müller over 12 years ago
Helmut, thanks for your feedback.
I was just wondering that flashmessages save bogus data to fe_session_data and that it changes behavior of cookie regeneration.
Updated by Gerrit Code Review over 12 years ago
- Status changed from Needs Feedback to Under Review
Patch set 2 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9719
Updated by Steffen Müller over 12 years ago
I uploaded a new patch which also add a test for empty data in setKey()
I still have to investigate why the demo extension is not working.
Updated by Gerrit Code Review over 12 years ago
Patch set 3 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9719
Updated by Steffen Müller over 12 years ago
Ok. Now the patch is finally complete and the demo extension should work, too.
Updated by Gerrit Code Review over 12 years ago
Patch set 4 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9719
Updated by Gerrit Code Review over 12 years ago
Patch set 5 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9719
Updated by Gerrit Code Review about 12 years ago
Patch set 6 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9719
Updated by Gerrit Code Review almost 12 years ago
Patch set 1 for branch TYPO3_6-0 has been pushed to the review server.
It is available at http://review.typo3.org/16952
Updated by Gerrit Code Review almost 12 years ago
Patch set 1 for branch TYPO3_4-7 has been pushed to the review server.
It is available at http://review.typo3.org/16954
Updated by Gerrit Code Review almost 12 years ago
Patch set 1 for branch TYPO3_4-6 has been pushed to the review server.
It is available at http://review.typo3.org/16955
Updated by Gerrit Code Review almost 12 years ago
Patch set 1 for branch TYPO3_4-5 has been pushed to the review server.
It is available at http://review.typo3.org/16956
Updated by Steffen Müller almost 12 years ago
- Status changed from Under Review to Resolved
- % Done changed from 0 to 100
Applied in changeset b4a4cdd09679a0c34ec121fb18e8eafe0408449c.
Updated by Johannes Goslar almost 12 years ago
The 4.5 patch seems to have an error: http://forge.typo3.org/issues/45578
Updated by Elliot Sawyer over 11 years ago
This issue was not included in 4.5.25 because it changes the behaviour. See by Oliver Hader's comment here: http://forge.typo3.org/issues/45570#note-7
Updated by