Fixes for cookie signing

[GwendolenLynch]

I hit a problem with CSRF tokens being rejected as invalid on both login and Symfony Form submissions.

After a lot of head scratching, I observed that multiple sessions were being created per request and that for some reason the session ID was being invalidated by NativeSessionStorage.

See https://github.com/symfony/symfony/blob/6.1/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php#L169-L172 and the comment above does the job explaining why.

The changes in Symfony were introduced here symfony/symfony#46249 and recently updated here symfony/symfony#46790

tl;dr Using a dot as the value delimiter when signing cookies, and signing session cookies, will cause sessions to fail.

Finally, if signing cookies with an empty value causes an exception. So the second fix just handles that case.

Fixes #154
Fixes #312
Fixes #313

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.63%. Comparing base (489a75d) to head (7cc3a47).
⚠️ Report is 101 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #315      +/-   ##
============================================
+ Coverage     95.74%   96.63%   +0.88%     
- Complexity      433      434       +1     
============================================
  Files            55       55              
  Lines          1409     1959     +550     
============================================
+ Hits           1349     1893     +544     
- Misses           60       66       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[yellow1912]

I'm running into this exact issue. Hope the PR is merged soon.

[Seldaek]

Suggested change

	if ($cookie->getValue() && (true === $this->signedCookieNames || \in_array($cookie->getName(), $this->signedCookieNames, true))) {
	if (null !== $cookie->getValue() && (true === $this->signedCookieNames || \in_array($cookie->getName(), $this->signedCookieNames, true))) {

[Seldaek]

I'm not so sure about this.. On one hand it seems ok as a workaround, although I would like to still have BC here for . separated cookies so old cookies still work after deploying the new code (i.e. it should split using the last dot or comma found in the string if any).

On the other hand, it seems like the root cause is that you are signing session cookies, and

NelmioSecurityBundle/src/EventListener/SignedCookieListener.php

Lines 57 to 61 in 7cc3a47

	if ($this->signer->verifySignedValue($cookie)) {
	$request->cookies->set($name, $this->signer->getVerifiedRawValue($cookie));
	} else {
	$request->cookies->remove($name);
	}

does not update $_COOKIE directly so the NativeSessionHandler does not see the unsigned-cookie value, and it actually uses the signed value as session id which to me sounds still borked.

So IMO we should probably write to $_COOKIE to unsign the cookies as well as writing to the request.. Any opinion here @nicolas-grekas from the Symfony/session perspective?

[Seldaek]

Alternative fix would be to NOT sign the session cookie ever even if * is configured for signed cookies, because really that id should anyway be verified in the session storage to prevent session fixation, so I don't think signing this actually improves anything security wise.

[WebDaMa]

@Seldaek What is the status of this PR? Our company is having the same bug.
If you can provide a todo list, we can work on it.

[gqai]

@WebDaMa also waiting for the PR, specifically the not signing a "null" cookie. We did find a work around in the interim, if you'd like to reach out.

@Seldaek is the only thing currently blocking this the two checks that are failing? Maybe I can take a look at it.

[Seldaek]

Nope it's not only the build, it's also my comments above which were not addressed.

[Seldaek]

https://github.com/nelmio/NelmioSecurityBundle/releases/tag/v3.2.0 has added support for null cookies - but the issue with signing the session cookie might still be current. I am not sure and I'd have to try it to confirm I guess.. But as time is very limited here, can someone tell me if this PR is still needed?

[OdendaalG]

@Seldaek If v3.2.0 resolves the null cookie issue, then it fixes the issues I came across. On the matter of the signing (which I never found to be a bug in the end), I believe that is unresolved but would require a bit of time to solve. Given the lack of time on your part and I think the prevalence, it could go to the back burner. Let me know if I can assist with something but in the mean time, thank you for this.