8000 In auto mode, `cookie_secure` may not be set properly in a secure environment if session starts too soon · Issue #40221 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

In auto mode, cookie_secure may not be set properly in a secure environment if session starts too soon #40221

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.

Sign up for GitHub

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

Jump to bottom
Closed
tamcy opened this issue Feb 17, 2021 · 5 comments · Fixed by #40231
Closed

In auto mode, cookie_secure may not be set properly in a secure environment if session starts too soon #40221

tamcy opened this issue Feb 17, 2021 · 5 comments · Fixed by #40231
Labels
Bug Status: Needs Review

Comments

@tamcy
Copy link
Contributor
tamcy commented Feb 17, 2021 •
edited
Loading

Symfony version(s) affected: 4.4.19

Description

Recently I stumbled upon an issue that PHP isn't writing my cookie as "secure" in the output even when I

  1. am accessing the website through HTTPS;
  2. have session.cookie_secure explicitly set to "On" in php.ini, and
  3. have cookie_secure set to auto in Symfony 4.4 (which is the default).

I found this weird, because

  1. this seems to only affect the PHP session ID cookie but not those written by the sendHeaders() method in Symfony's Response object, so obviously Symfony knows it is a secure connection,
  2. this doesn't happens all time - in my setup it only affects the frontend but not the backend, and
  3. the problem goes away if I explicitly set cookie_secure to true in framework.yaml.

After several hours of debugging, I have tracked down the cause. Here's how it happens:

  1. When NativeSessionStorage is created, it calls ini_set() to replace the session-related configurations with those defined in the framework config. The values are written as-is, so when cookie_secure is set to auto in framework.yaml, this auto will be passed to ini_set. Here's the problem - auto is not a valid value for this configuration, and will probably be treated as Off or False. Which is why having session.cookie_secure set to "On" in php.ini won't help in my situation.
  2. Fortunately, this invalid value auto normally won't stay forever. The problem is fixed by Symfony\Component\HttpKernel\EventListener\SessionListener, which is activated when the framework's cookie_secure is set to auto. SessionListener checks if the request is a secure one and if yes, it sets the cookie_secure PHP option to true, thus overriding the previous incorrect value that will be intepretated as false.

Here is the tricky part. The reason the session ID cookie isn't properly written as secure even when the website is accessed through HTTPS is because in the frontend, I have an Authenticator that checks for a specific session key in the supports method. This method is executed so early in the request cycle such that the session is started before SessionListener is run. When this happens, it is too late for SessionListener to set the cookie_secure PHP option to true because NativeSessionStorage's setOption() will exit immediately if the session has been started. Now the auto value will stay till the end, which I believe will be treated as false by PHP, so the PHPSESSID's cookie will not have the secure attribute.

I don't see any warning that one should not access the session in the guard authenticator's supports() method. In fact I can find suggestion of "only return true in supports() if the user is on the correct URL and if that session key exists" in the comment section of a SymfonyCasts page, which is exactly what I was doing (the website I work on contains a frontend and backend which operate with different authenticators, and I need a bridge between them, thus the need for checking a particular session value). So I believe I am not doing something unwise.

I understand that I may have stumbled upon an edge case, and probably not much people will be affected by this. Also it's not difficult to work around this problem, I just need to defer the session checking until getUser() is called. Or I can just explicitly set cookie_secure to true to avoid this glitch. But since the consequence is security related (cookie_secure flag is not turned on even when it should), I think it would be better for me to report this issue anyway and mark it as a bug report first and let your team decide what to do with it.

Thank you!

Update 19 Feb

After some more tests, it looks like the problem is probably a more wide-spreaded one. session.cookie_secure isn't set properly (i.e. stays in "auto") even for the following controller code in a newly set up Symfony 4.4 and 5.2 project:

<?php

namespace App\Controller;

use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpFoundation\Session\SessionInterface;
use Symfony\Component\Routing\Annotation\Route;

class MainController
{
    private $session;

    public function __construct(SessionInterface $session)
    {
        $this->session = $session;
    }

    /**
     * @Route("/")
     */
    public function index(): Response
    {
        $this->session->set('foo', 'bar');

        return new Response(ini_get('session.cookie_secure'));
    }
}
@tamcy tamcy added the Bug label Feb 17, 2021
@nicolas-grekas
Copy link
Member

I have an Authenticator that checks for a specific session key in the supports method. This method is executed so early in the request cycle such that the session is started before SessionListener is run.

I suppose you can't register your authenticator after SessionListener?

How would you recommend fixing this? Would you mind sending a PR (on branch 4.4)?

@stof
Copy link
Member
stof commented Feb 17, 2021

I think fixing that would require changing the way the auto value of cookie_secure is managed:

  • avoid setting auto as a value for the ini setting in the NativeSessionStorage initialization (if we don't have the final value, keeping the existing ini value is a saner behavior than overriding it with an invalid value IMO)
  • ensuring that SessionListener resolves the auto value by the time the SessionListener runs, and not by the time the getSession() method is called in the Request session factory callback

@stof
Copy link
Member
stof commented Feb 17, 2021

the SessionListener runs at priority 128, which means that it already runs way before the guard authenticator (the firewall runs at priority 8)

@tamcy tamcy mentioned this issue Feb 18, 2021
Merged
@tamcy
Copy link
Contributor Author
tamcy commented Feb 18, 2021

Thanks @stof for the suggestion. I am not very familiar with the internals, so I honestly I don't have any concrete idea on working on a fix.

I have done what @stof has suggested in the PR. The code works fine, there is just one thing that bothers me. In the PR I am hard-coding to skip the 'auto' value in HttpFoundation's NativeSessionStorage, which I am not sure if this is a good solution because HttpFoundation can work independently, and this 'auto' value sounds a bit Symfony specific to me. Should I manipulate the default configuration that is passed to the constructor of NativeSessionStorage in the DI code instead? (I am not sure if this is possible, and if yes I'd probably need help on this.)

nicolas-grekas added a commit that referenced this issue Feb 25, 2021
@nicolas-grekas
bug #40231 [HttpKernel] Configure session.cookie_secure earlier (ta…
87aeb8d 
…mcy)

This PR was merged into the 4.4 branch.

Discussion
----------

[HttpKernel] Configure `session.cookie_secure` earlier

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #40221
| License       | MIT
| Doc PR        | N/A

This PR does what @stof had suggested in #40221, allow me to quote him directly:

> 1. avoid setting auto as a value for the ini setting in the NativeSessionStorage initialization
> 2. ensuring that SessionListener resolves the auto value by the time the SessionListener runs, and not by the time the getSession() method is called in the Request session factory callback

Commits
-------

e82918c [HttpKernel] Configure `session.cookie_secure` earlier
@antedom
Copy link
antedom commented Mar 22, 2022

Setting secure_cookie: true if you using https requests, otherwise set false (for local development and http). I had similar problem where for each request new session would be created, running fine on local but on test env there was a problem. In SessionListener check if request is secure would never change secure_cookie to true (if in config is set to auto) because container does not hold session_storage service anymore ($this->container->get('session_storage')). Also don't forget to clear session storage on your browser after update.

Symfony version 5.3.16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[HttpFoundation][HttpKernel] Configure session.cookie_secure earlier tamcy/symfony
5 participants
@nicolas-grekas @stof @tamcy @carsonbot @antedom
0