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

Add security.csrf_auto_token option to add CSRF token automatically #1974

Merged
merged 1 commit into from
Feb 10, 2016

Conversation

kenjis
Copy link
Contributor

@kenjis kenjis commented Feb 10, 2016

This PR adds security.csrf_auto_token.
If you set it true, CSRF token is added automatically when you call Form::open().

…en()

Signed-off-by: Kenji Suzuki <kenji.uui@gmail.com>
WanWizard added a commit that referenced this pull request Feb 10, 2016
Add security.csrf_auto_token option to add CSRF token automatically
@WanWizard WanWizard merged commit f560115 into fuel:1.8/develop Feb 10, 2016
@kenjis
Copy link
Contributor Author

kenjis commented Feb 11, 2016

Thank you for merging.
We need to update docs and config.php. I sent PR for docs: fuel/docs#712

By the way, why the default value for security.csrf_autoload is true in the below?
https://github.com/fuel/core/blob/1.8/develop/classes/security.php#L56

I think the default value is false: https://github.com/fuel/core/blob/1.8/develop/config/config.php#L153
But the docs says true: https://github.com/fuel/docs/blob/1.8/develop/classes/security.html#L159-L161

All of them should be false, right?

@WanWizard
Copy link
Member

From a security point of view, you would say the default should be true, because that forces you to have csrf protection on every form. Problem with that is that currently every POST is checked, so all REST calls will fail too, unless they contain a token, which is not likely.

I did a quick history check, it seems it has always been false in the config, so for BC reasons I'd say all of them should be false. And a solution for REST calls must be added to the security class.

kenjis added a commit to kenjis/fuel-core that referenced this pull request Feb 11, 2016
Signed-off-by: Kenji Suzuki <kenji.uui@gmail.com>

See fuel#1974 (comment)
kenjis added a commit to kenjis/fuel-docs that referenced this pull request Feb 11, 2016
@kenjis
Copy link
Contributor Author

kenjis commented Feb 11, 2016

From a security point of view, you would say the default should be true

Yeah, I agree with you!
But for now, all of them should be false for BC.

I sent PRs to fix:

kenjis added a commit to kenjis/fuel-core that referenced this pull request Feb 11, 2016
Signed-off-by: Kenji Suzuki <kenji.uui@gmail.com>

See fuel#1974
@kenjis
Copy link
Contributor Author

kenjis commented Feb 14, 2016

Problem with that is that currently every POST is checked, so all REST calls will fail too, unless they contain a token, which is not likely.

Do we add ignore URI list like this?
https://gist.github.com/vzool/87b13a3d0c8e168a3587

@WanWizard
Copy link
Member

Would probably do it, but not very elegant.

Thinking about it, I think when you enable automatic checking, you intend to check on every POST, and you as a developer should make sure every POST contains a token. If you don't intend to do that, don't enable auto checking, but check manually, for example in the before() of a controller.

@kenjis
Copy link
Contributor Author

kenjis commented Feb 15, 2016

If we have a site which has web apis, we may not use tokens on the apis.
But we may want to enable automatic checking on every POST to normal web pages.

I don't think manual checking is safer. Because if a developer forgets to implements checking, the site would be vulnerable. If we have a lot of forms or important actions, it costs much to confirm every important action has checking logic.

In my opinion, we should enable automatic checking always.

@WanWizard
Copy link
Member

I think there two different conditions:

  1. Do we automatically check the CSRF token on every POST?
    2 If yes, do we also check posts to a REST API?

Currently, we don't do neither by default because of BC reasons. In the app, we can do 1. by enabling csrf_autoload in the config, but there is no option to omit POSTS to a REST API if it is enabled.

Perhaps we should introduce two new config keys, something like csrf_validate_form_posts and csrf_validate_rest_posts And if they don't exist, fallback to the csrf_autoload value for both, and if that not exist fallback to false.

This way you can define the behavior for both cases independently, and have BC covered.

@kenjis
Copy link
Contributor Author

kenjis commented Feb 16, 2016

How do we know whether REST posts or not?

@WanWizard
Copy link
Member

is_ajax() ?

@kenjis
Copy link
Contributor Author

kenjis commented Feb 18, 2016

  1. Every Web API does not expect Ajax calls. For example, API like GitHub API.
  2. I'm not sure attackers can't make a victim send Ajax requests to cross domains. XHR2 could send cross domain requests.

@WanWizard
Copy link
Member

I know that, but I don't think we can come up with something that fits all possible scenario's. If you have a specific environment, you need specific checks.

So I'm looking for something that follows the 80/20 rule, and covers the majority of the use-cases.

@kenjis
Copy link
Contributor Author

kenjis commented Feb 21, 2016

I opened a issue for this new functionality: #1981
Because I think we need to think how to implement it more.

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 this pull request may close these issues.

2 participants