phpBB • Free and Open Source Forum Software

I discovered this bug because I use Google banners on my site. GoogleAdsense uses a bot to retrieve and analyze all pages that contain the banners.

Imagine the following scenario:

- Bob logs in but doesn't check 'log me in automatically'
- Bob retrieves the page index.php?sid=f4ad2e010a398c8bc94b91b0dfd5f235 which contains a google banner
- Google notices that it doesn't know that page and spiders the page
- Google doesn't have a valid session (in a cookie or in the URL) so a session_create() is called
- session_create() sees that there is 'sid=f4ad2e010a398c8bc94b91b0dfd5f235' in the URL, sees that this session_id exists in the DB and updates the DB
- The GoogleBot's session is created with Bob's session id (overwriting Bob's session)
- Bob is suddenly logged out
- Bob complains
- I get annoyed
- I file this bug Smile

It seems to be trivial to fix, that is, if there isn't a really good reason to recycle SIDs.

http://phpbb.cvs.sourceforge.net/phpbb/ ... iew=markup

removing this if statement should do the trick:

Code: Select all: if (!$this->session_id || !$db->sql_query($sql) || !$db->sql_affectedrows())

Just to make sure you understand this correctly: There is no session hijacking taking place but only "Bob"'s session is deleted (he is logged out).

To say something about the suggested fix:

I'd propose regenerating the session_id and inserting a new session row whenever that bit of code is reached for security reasons (just to make sure), which is about what Bart suggests.

If the UPDATE query is supposed to stay there, then it has to get an additional check so that only sessions belonging to anonymous users will be overwritten (so a user who logs in keeps the same session id he had as a guest) However not regenerating session_ids after gaining additional power on a website is generally considered bad practice.

As noted to Nils last night, there is a purpose to this code - it's to prevent the sessions table getting filled up with old sessions from guests that have subsequently logged in (or that's the theory from what I can see in the code), so the fix Bart suggests would have undesirable effects in terms of the session table. Of course another user being able to force a log out in this way is a bug, keeping the size of the table down (and thus the confusion of users about phantom users) is what we should be doing