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

[Security] Credentials check action ignores scheme (https) requirement #22097

Open
MacDada opened this issue Mar 21, 2017 · 13 comments

Comments

Projects
None yet
8 participants
@MacDada
Copy link
Contributor

commented Mar 21, 2017

Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 2.8

My auth credentials check action:

	/**
	 * @Route("/login_check", schemes={"https"}, name="user_login_check")
	 * @Method({"POST"})
	 */
	public function loginCheckAction()
	{
		throw new \BadMethodCallException('Firewall should have done stuff, not this action!');
	}

As we can see, I require secure connection to be used. The login form creates a proper url (with https instead of http), but it's not actually required. I can edit the url in form and successfully log in without https.

For https to be actually required, I had to add this to my security.yml config:

security:
    access_control:
        - { path: ^/login_check, requires_channel: https }

To sum up the error: I would assume that route configuration should be enough to require https. It's not. Firewall configuration must be applied as well.

@kherrera-ebsco

This comment has been minimized.

Copy link

commented Mar 21, 2017

I think this is more of a configuration "gotcha" than an actual bug. One configuration is specifically for route matching, while the other configuration is specifically for security.

@MacDada

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2017

I think this is more of a configuration "gotcha" than an actual bug.

@kherrera-ebsco Call it whatever you like. When I require https, I expect the action not work without https.

If I hadn't manually tested it, I'd have security issue (and I guess a lot of people don't know they do have one).

@linaori

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2017

Now I might say something weird... But why not put everything on https and be done with it?

@MacDada

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2017

But why not put everything on https and be done with it?

@iltar We're gonna do it in time. But until then, I need to require ssl just in some actions (authentication for starters).

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Mar 23, 2017

Someone should check if this only happens when defining routes as annotations (then, it's a problem of https://github.com/sensiolabs/SensioFrameworkExtraBundle) or if this also happens in any other format (then it's a routing/frameworkbundle problem).

@MacDada

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2017

Someone should check if this only happens when defining routes as annotations (then, it's a problem of https://github.com/sensiolabs/SensioFrameworkExtraBundle) or if this also happens in any other format (then it's a routing/frameworkbundle problem).

@javiereguiluz It's not a problem with annotations:

// Controller, no annotations
public function loginCheckAction()
{
	throw new \BadMethodCallException();
}
# routing.yml
user_login_check:
    path: '/login_check'
    schemes: ['HTTPS']
    methods: ['POST']

Still, HTTPS is not enforced.

@jsamouh

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2017

@MacDada just for 2.8 ? I tried to symfony master and I have not the problem

@chalasr

This comment has been minimized.

Copy link
Member

commented Apr 24, 2017

I confirm that the check_path's schemes requirement is not considered, authentication listeners use HttpUtils::checkRequestPath() which compares the check path with the request pathinfo only.

@chalasr

This comment has been minimized.

Copy link
Member

commented Apr 24, 2017

Status: reviewed

@modiamir

This comment has been minimized.

Copy link

commented Jun 4, 2018

I am going to make a fix for this issue as my first try to contribute in Symfony.

@modiamir

This comment has been minimized.

Copy link

commented Jun 6, 2018

In HttpUtils::checkRequestPath when RequestMatcherInterface::matchRequest is been called, if scheme is been matched the returned value is an associative array containing these keys: _controller, _locale, _route, and if the scheme is not been matched return value is a associative array containing _controller, path, permanent, scheme, httpPort, httpsPortand _route. Also in this case value of _controller is Symfony\Bundle\FrameworkBundle\Controller\RedirectController::urlRedirectAction.

I think in HttpUtils::checkRequestPath by checking some of above keys, we can verify that the scheme is valid or not. Is that good solution?

@chalasr chalasr added the Security label Jun 6, 2018

@chalasr

This comment has been minimized.

Copy link
Member

commented Jun 6, 2018

I think I would not change the behavior of checkRequestPath() since it does its job which is to compare the path, not the scheme. An @internal checkRequestScheme() instead?

@modiamir

This comment has been minimized.

Copy link

commented Jun 6, 2018

checkRequestPath() uses a urlMatcher service by calling its matchRequest or match method which in that method scheme is being checked. So checkRequestPath's job is not only checking path. The scheme is checking in deeper level and we have its result in $parameters variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.