Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for cookie signing #315

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

GwendolenLynch
Copy link

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
Copy link

codecov-commenter commented Sep 3, 2022

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
Copy link

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

@@ -72,7 +72,7 @@ public function onKernelResponse(ResponseEvent $e): void
$response = $e->getResponse();

foreach ($response->headers->getCookies() as $cookie) {
if (true === $this->signedCookieNames || \in_array($cookie->getName(), $this->signedCookieNames, true)) {
if ($cookie->getValue() && (true === $this->signedCookieNames || \in_array($cookie->getName(), $this->signedCookieNames, true))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))) {

@@ -75,7 +75,7 @@ private function generateSignature(string $value): string
*/
private function splitSignatureFromSignedValue(string $signedValue): array
{
$pos = strrpos($signedValue, '.');
$pos = strrpos($signedValue, ',');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

WebDaMa commented Apr 27, 2023

@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
Copy link

gqai commented Dec 14, 2023

@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
Copy link
Member

Seldaek commented Dec 14, 2023

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

@Seldaek
Copy link
Member

Seldaek commented Jul 1, 2024

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants