Problem/Motivation
If a user sends us a session cookie that contains a session ID that we do not have in our database we should throw away their session id and give them a new one. But we don't.
Steps to reproduce
- Install Drupal 8
- Install a module that adds some information to the session for anonymous users (outdated but e.g. Sessions Everywhere, https://www.drupal.org/sandbox/kscheirer/2629288)
- Visit site as an anonymous user, check sessions table
- Truncate sessions table
- Visit site again, same session id
- Truncate sessions table
- Modify session value and site again. modified session id stored and kept.
Proposed resolution
If a user comes back with an sid that we don't have in the sessions table, we should create a new session ID for them and send that cookie back to them.
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#21 | 2631220-card-session-fixation-D7.patch | 1.84 KB | greggles |
#9 | 2631220-session-fixation-9.patch | 1.2 KB | kscheirer |
Issue fork drupal-2631220
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
kscheirerComment #3
kscheirerComment #5
kscheirerThe number of fails looks unrelated? Testing again.
Comment #7
cilefen CreditAttribution: cilefen commentedLogins are failing in the tests.
Comment #8
kscheirerComment #9
kscheirerThis patch has better naming. Thank you @cilefen for diagnosing the problem.
Comment #10
tstoecklerSo this returns NULL if
$sid
is empty. We should update the docs or (preferably, IMO) fix the code to also returnFALSE
in that case.Minor, but to me it's a bit weird to fetch the timestamp in SQL and then cast it to bool in PHP. What about a
timestamp <> 0
condition?Comment #11
znerol CreditAttribution: znerol commentedThe SessionManager should have no db-dependency at all. Regrettably we failed to remove all of them before release. So if possible move that to somewhere else.
Also we should check whether Symfony is affected and if so, fix the bug there.
Comment #12
znerol CreditAttribution: znerol commentedI did a bit more research on the subject of unauthenticated session fixation attacks. The OWASP Session Management Cheat Sheet points out that...
Regrettably, the cheat-sheet does not mention unauthenticated cases. However there are quite some scenarios where unauthenticated session fixation might pose a problem. Citing from the linked blog post:
Comment #13
gregglesRe #12, I agree it's not 100% clear what the attack scenario is or what the vulnerability is, that's why this is in public rather than being handled in the private tracker (I created an issue in the private tracker and the team consensus was that it could be public).
One good reason for this patch is just that sites without it will end up with invalid sids in their sessions storage. The number of invalid sids depends on the likelihood of the site to get requests with invalid sids, but...let's say it can pretty easily number into the millions for a site with a long-lived session cookie.
Another good reason is just data validity and avoiding collisions. If a user passes a Drupal site a sid that is not in the sid storage, there's no way to guarantee it's a good value. Drupal works hard to create a random sid and we shouldn't let a user create a bad one for themselves.
Comment #14
znerol CreditAttribution: znerol commentedOh, the attack scenario is rather detailed in the second link I've posted. But the provided patch does not mitigate it. Given a multistep form with summary screen at the end. The attacker generates a session (e.g. by partially filling out the form) and fixates the sid in the users browser. While the victim is filling out the form, the attacker collects private data of the victim.
Regarding user chosen sids: We actively remove empty sessions for unauthenticated users. Therefore if you just manually add a generated session cookie to the browser, then the cookie is supposed to be removed at the first request to the site unless the site sets session data.
Comment #15
gregglesI would say that 1 attack scenario is detailed and that this code doesn't address that scenario. IMO Drupal cannot know which events are worthwhile to change the session identifier to avoid that kind of attack and the responsibility is on the site builder to do that.
In my experience that does not happen. I think this code fixes this bug.
Comment #18
marvil07 CreditAttribution: marvil07 as a volunteer commentedPatch still applies correctly, and based on comment #15 marking as RTBC.
Comment #19
alexpottThis looks like a testable bug - to ensure we don't break it in the future.
Comment #21
gregglesHere is a Drupal 7 patch.
Comment #29
neclimdul#2238561: Use the default PHP session ID instead of generating a custom one broke this. Started to re-roll but that moved all the session ID hashes and almost all the database logic out of the manager and adding it back smelled a bit. I wonder how PHP/symfony handle this. Is there some subtle changes we can make to the handler to just trigger their handling of this? For example, the ::read documentation is "If nothing was read, it must return false." so we aren't following the interface and maybe that where they'd normally regenerate a bad session?
Comment #30
neclimdulactually, trying to recreate this on a clean 9.4.x site the cookie is getting deleted. So... maybe fixed? Someone else want to confirm?
Comment #31
neclimdul#14/#15 suddenly make more sense. I was seeing the cleanup of the empty sessions not actually cleaning up invalid sessions. The summary talks about "gives sessions to anonymous users" but its actually the process of adding something _to_ the session that triggers the behavior.
Comment #32
neclimdulSo the previous approach works but it also looks like one of the reasons this is happening because we aren't really taking full advantage of the php session management because we aren't implementing the poorly named SessionUpdateTimestampHandlerInterface. SessionUpdateTimestampHandlerInterface::validateId works like I thought returning false from read might have worked if session.use_strict_mode is enable.
Also interesting, Symfony has code for this we just aren't using it for some reason. I took a pass at adopting that code and with some manual testing it seems to work. 2 Changes where needed
One thing to note, there is a manual cookie mangling in the ::destroy method of Symfony's base class that I bypassed. I'm not sure why they're handling it there but we didn't do that and it seems like we must be handling that in some other way. Probably deserves some review though.
There are some BC issues with this approach.
Comment #34
andypostComment #35
andypostAccording to #1561866-98: Add support for built-in PHP session upload progress upload progress may need separate cookie
Comment #36
andypostand new MR for 11.x