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

Merging #315 (5de2c59) into master (aaf3742) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #315      +/-   ##
============================================
+ Coverage     95.69%   95.74%   +0.04%     
- Complexity      426      434       +8     
============================================
  Files            55       55              
  Lines          1395     1409      +14     
============================================
+ Hits           1335     1349      +14     
  Misses           60       60              

Impacted Files	Coverage Δ
src/ContentSecurityPolicy/PolicyManager.php	95.23% <100.00%> (ø)
src/DependencyInjection/Configuration.php	99.23% <100.00%> (+0.04%)	⬆️
src/EventListener/SignedCookieListener.php	100.00% <100.00%> (ø)
src/Signer.php	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[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.