Bug #34964

FE Session record is never removed, even if no session data left

Added by Steffen Müller over 9 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
-
Target version:
-
Start date:
2012-03-17
Due date:
% Done:

100%

Estimated time:
TYPO3 Version:
4.5
PHP Version:
Tags:
Complexity:
Is Regression:
Sprint Focus:

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

34964_4-5.diff (2.74 KB) 34964_4-5.diff Steffen Müller, 2012-03-17 15:52
T3X_demo34964-0_0_0-z-201203171606.t3x (94.5 KB) T3X_demo34964-0_0_0-z-201203171606.t3x Steffen Müller, 2012-03-17 16:12

Related issues

Related to TYPO3 Core - Bug #45570: fe_session Data Change external payment checkoutClosed2013-02-18

Actions
Related to TYPO3 Core - Bug #45578: storeSessionData not working anymore with 4.5.23Rejected2013-02-18

Actions
Related to TYPO3 Core - Bug #45708: feuserauth storeSessionData fails to save Data to DB in "ses"-modeClosed2013-02-21

Actions
Related to TYPO3 Core - Bug #53598: Select/Delete fe_sessions twice per requestClosedAlexander Opitz2013-11-13

Actions
#1

Updated by Gerrit Code Review over 9 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

#2

Updated by Steffen Müller over 9 years ago

Added also a patch for TYPO3 4.5

#3

Updated by Steffen Müller over 9 years ago

The example code above is taken from 4.5, but the problem still exists in master.

#4

Updated by Steffen Müller over 9 years ago

Demo extension added.
Just install and add plugin to page.

  1. Hit "Reload page" multiple times to see the cookie changing.
  2. Hit "Add and show a Flashmessage" to add flash message.
  3. Hit "Reload page" again multiple times to see the cookie is NOT changing anymore.

Apply patch and replay above steps.

#5

Updated by Steffen Müller over 9 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';
#6

Updated by Philipp Gampe over 9 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?

#7

Updated by Helmut Hummel over 9 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?

#8

Updated by Steffen Müller over 9 years ago

Indeed, you are right. I will investigate and write a post as soon as I found the reason for this.

#9

Updated by Steffen Müller over 9 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.

#10

Updated by Gerrit Code Review over 9 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

#11

Updated by Steffen Müller over 9 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.

#12

Updated by Gerrit Code Review over 9 years ago

Patch set 3 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9719

#13

Updated by Steffen Müller over 9 years ago

Ok. Now the patch is finally complete and the demo extension should work, too.

#14

Updated by Gerrit Code Review over 9 years ago

Patch set 4 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9719

#15

Updated by Gerrit Code Review about 9 years ago

Patch set 5 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9719

#16

Updated by Gerrit Code Review over 8 years ago

Patch set 6 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9719

#17

Updated by Gerrit Code Review over 8 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

#18

Updated by Gerrit Code Review over 8 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

#19

Updated by Gerrit Code Review over 8 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

#20

Updated by Gerrit Code Review over 8 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

#21

Updated by Steffen Müller over 8 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100
#22

Updated by Johannes Goslar over 8 years ago

The 4.5 patch seems to have an error: http://forge.typo3.org/issues/45578

#23

Updated by Elliot Sawyer about 8 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

#24

Updated by Benni Mack over 2 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF