No guarantee of session isolation with Symfony

[mathieudz]

Currently PHP PM relies on Symfony to close the sessions properly. I already had two instances where that did not happen:

• a bug where the session storage changed serializing method (PHP vs. igbinary) and thus crashed
• a bundle (SchebTwoFactorBundle) that reads the session "on finish request", effectively reopening the session after SessionListener closed it.

Of course, these bugs need to be solved, but they should not be able to cause security issues. Luckily due to extensive use of session bound CSRF-like tokens, the website was hardly usable anymore and no harm was done. Otherwise, sessions would have leaked to other users, e.g. a regular user could have used an admin's session.

I suggest there should be some kind of session cleanup after each request. The only way to do that seems to be the save() method on the session.

[acasademont]

@mathieudz this is one of the dangers of switching to a long running process like react-php/php-pm that manages more than one request. Developers need to be fully aware that the global state can't be trusted anymore.

Not sure php-pm can (or should) do something about it. We've had some recent discussions about how far should we (php-pm) try to go in fixing weird stuff caused by the long-running processes. How would this be best addressed?

[mathieudz]

I agree this is an inherent problem, but the security consequences of this specific case are huge. I would check the session service after each request and if it's open, close it (by saving it). In the former case it would crash (which is good), in the second case it would close the session properly. I have an event listener now that does it in my own code, so I don't need any change in PHP PM, but I think it would be a good idea for PHP PM to do such a basic check.

[mathieudz]

FYI, this is my workaround:

    public function onFinishRequest(FinishRequestEvent $event): void
    {
        $request = $event->getRequest();
        if ($request->hasSession() && $request->getSession()->isStarted()) {
            $this->logger->warning('Session not closed when processing request. Closing it now.');
            $request->getSession()->save();
        }
    }

[mathieudz]

After upgrade to Symfony 5.1, sessions are again leaking despite my workaround.

[acasademont]

@mathieudz would be great to have a repro test-case, I'm using php-pm in a stateless REST api, so no sessions on our end to see what might be happening.

[mathieudz]

I'm still trying to figure out what causes the leak. The only way to reproduce it currently is to switch production to v5.1 and wait a few hours until it occurs (and all hell breaks loose).

[mathieudz]

I'm back on 5.1 and the problem does not occur anymore. No way to reproduce it anymore. Still it would be a good thing if PHP PM were able to check if a session is still open after a request and do something about it (close the session, stop the worker, ...) These issues are really very hard to debug.