Update session ids on update #16814
Update session ids on update #16814
Conversation
sgiehl
added
not-in-changelog
sgiehl
force-pushed
the
fixsessionupdate
branch
from
1cb0190
to
ee72027
Compare
core/Updates/4.0.0-b1.php
Outdated
| $sessions = Db::fetchAll('SELECT id from ' . Common::prefixTable('session')); | ||
|
|
||
| foreach ($sessions as $session) { | ||
| $migrations[] = $this->migration->db->boundSql( |
There was a problem hiding this comment.
this would actually print sessionId hashes on the screen... of course they are hashed but it may still not be ideal. Matomo may be in that state for a while and people could see them ... it's very low risk though that it 1) becomes visible and 2) someone either has the salt or tries to brute force them (eg a sys admin who is updating Matomo could have the salt but no access to the DB and now could technically brute force the original session id) . If someone was to downgrade later to Matomo 3 then it become an actual sessionId though.
I wonder should we maybe only do this for session Ids that are > 190 characters? In all other cases we could simply log out users? and these we could even simply delete?
Or we simply execute the query in the background similar to the delete from session.
There was a problem hiding this comment.
in any case we should not do this for any 128 character session as this indicates the sessionId might be already hashed
…ashed yet, no longer execute query on get migrations
| @@ -252,6 +257,16 @@ public function getMigrations(Updater $updater) | |||
|
|
|||
| public function doUpdate(Updater $updater) | |||
| { | |||
| $salt = SettingsPiwik::getSalt(); | |||
There was a problem hiding this comment.
we only execute it as part of doUpdate... which should be fine. in worst case someone gets logged out
|
Note: this test fails but also does in 4.x https://travis-ci.com/github/matomo-org/matomo/jobs/449214044#L1438 |
Description:
Instead of updating every single session we could also try
UPDATE session SET id=SHA2(id, 512), but that method might be unavailable if mysql is not configured for SSL, causing the update to fail maybe. So guess it's better this way.fixes #16798
Review