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

Problem with latest Symfony update and signed_cookie feature #312

Open
Flo-JB opened this issue Jun 3, 2022 · 5 comments · May be fixed by #315
Open

Problem with latest Symfony update and signed_cookie feature #312

Flo-JB opened this issue Jun 3, 2022 · 5 comments · May be fixed by #315

Comments

@Flo-JB
Copy link

Flo-JB commented Jun 3, 2022

After the latest Symfony update with this pull symfony/symfony#46249 Symfony refreshes the session on each page view because the implemented regex filter from the pull reequest fails because of the "." stored in the session cookie with signed_cookie feature enabled.

It would be easier to fix this one on Symfony side but maybe they keep the restricted check and an update in this library is needed...

@peter17
Copy link

peter17 commented Jun 3, 2022

@Flo-JB may I ask which value you use for signed_cookie.names? Thanks!

For other people with similar problem, it may be possible to exclude the session cookie from the signing process and sign only other cookies, see #154

@Flo-JB
Copy link
Author

Flo-JB commented Jun 3, 2022

@peter17 sure - in this specific project all cookies are included by setting the value to ['*'].
Excluding the session cookie is a possible fix right now. But aren't there any disadvantages regarding security in this case?

I don't see a good generic solution right now without breaking the signing meachism (for example: storing the hash in a separate cookie file instead of the same one)
Adjusting your regex validation isn't a good solution either because this would be just related to this library and basically "wrong" because the validation itself is correct for the session_id

@peter17
Copy link

peter17 commented Jun 3, 2022

I am thinking of an ugly solution that might work:

on src/EventListener/SignedCookieListener.php

replace the following block:

        $names = true === $this->signedCookieNames ? $request->cookies->keys() : $this->signedCookieNames;
        foreach ($names as $name) {
            if ($request->cookies->has($name)) {
                $cookie = $request->cookies->get($name);
                if ($this->signer->verifySignedValue($cookie)) {
                    $request->cookies->set($name, $this->signer->getVerifiedRawValue($cookie));
                } else {
                    $request->cookies->remove($name);
                }
            }
        }

by

        $names = true === $this->signedCookieNames ? $request->cookies->keys() : $this->signedCookieNames;
        foreach ($names as $name) {
            if ($request->cookies->has($name)) {
                $cookie = $request->cookies->get($name);
                if ($this->signer->verifySignedValue($cookie)) {
                    $rawValue = $this->signer->getVerifiedRawValue($cookie);
                    $request->cookies->set($name, $rawValue);
                    if ($name === session_name()) {
                        $_COOKIE[session_name()] = $rawValue;
                    }
                } else {
                    $request->cookies->remove($name);
                    if ($name === session_name()) {
                        unset($_COOKIE[session_name()]);
                    }
                }
            }
        }

I believe it would work because I think this happens before the session_start() call of the NativeSessionStorage. If it didn't then an exception would have been thrown by NativeSessionStorage before I made my fix there.

@Flo-JB Do you have a minimum code to reproduce the issue? Or can you try this on your project? I don't have time now to set up a project and test this... Thanks!

@Flo-JB
Copy link
Author

Flo-JB commented Jun 3, 2022

@peter17
I tested with your code and it works. You are right - you can use the time between onKernelRequest and onKernelResponse to fix the cookie value for your regex check.

With that fix the cookie will be rewritten two times on each page call (surely not the perfect solution) but I think better than the other options.

Another alternative would be more complex - I think of an overridable session_id check service instead of a hardcoded one which could be injected to NativeSessionStorage and could be replaced by the NelmioSecurityBundle.

Big thanks for investigating this issue

@peter17
Copy link

peter17 commented Jun 3, 2022

Good! I'd like the opinion of a maintainer of NelmioSecurityBundle before I make a PR with this change. Maybe they have better ideas or can comment the code above?

Thanks in advance

nicolas-grekas added a commit to symfony/symfony that referenced this issue Jun 19, 2022
…on id" to only validate when files session storage is used (alexpott)

This PR was submitted for the 6.1 branch but it was squashed and merged into the 4.4 branch instead.

Discussion
----------

[HttpFoundation] Update "[Session] Overwrite invalid session id" to only validate when files session storage is used

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fixes #46249
| License       | MIT
| Doc PR        | -

#46249 restricts the allowed characters in a session ID. Unfortunately this broke at least two open source projects the use the Symfony component. See nelmio/NelmioSecurityBundle#312 and https://www.drupal.org/project/drupal/issues/3285696

I think the change is not quite correct. It assumes that the valid characters in a session ID is consistent across all session handlers. It is not. See https://www.php.net/manual/en/function.session-id.php it says:

>Depending on the session handler, not all characters are allowed within the session id. For example, the file session handler only allows characters in the range a-z A-Z 0-9 , (comma) and - (minus)!

So we've limited the characters to the file session handler but we might not be using that.

Commits
-------

12460fa [HttpFoundation] Update "[Session] Overwrite invalid session id" to only validate when files session storage is used
@GwendolenLynch GwendolenLynch linked a pull request Sep 3, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants