Aspnetcore: Consider requiring Antiforgery Validation by default

Created on 30 Oct 2015  Â·  27Comments  Â·  Source: dotnet/aspnetcore

Current convention is that with any form submitted an Anti XSRF Token ist supposed to be sent along and evaluated at server side.

So, instead of requiring the programmer to add countless Html.AntiForgeryToken() calls and ValidateAntiForgeryToken attributes, wouldn't it be an improvement to automatically send an Anti XSRF Token along with each Html.BeginForm() or Ajax.BeginForm() and to evaluate it with each HttpPost and similar?

A configuration option could be added allowing to set whether this should automatically occur or (as in the current MVC/Web API implementation) manually.

affected-medium area-mvc enhancement severity-major

Most helpful comment

Here's our tentative plan for 2.0.0:

• Enable auto anti request forgery validation in the framework by default
• Update authentication support to expose whether the current authentication scheme is persistent or not (see https://github.com/aspnet/Security/issues/1172)
• Update auto anti request forgery to not validate if the authentication scheme is explicitly not persistent (ex Bearer)

After this is done we will do some tweaking with the template to decide on the final design
• Update Web API item template to add an attribute (new AntiforgeryValidationModeAttribute) that explicitly states that auto anti request forgery behavior will be applied
• Consider adding explicit code to the template with the antiforgery behavior even if it’s the default. Prototype and see what it looks like.

All 27 comments

@blowdart Looks like we did this already for the form tag helper. If we turn this one on for HTML helpers by default as well then we would have to figure out what the behavior is when you also specify Html.AntiForgeryToken().

I suggested this about a year ago. Backcompat was the reason given to not do it. Putting it in the form is the easy part, however I'm not so sure about checking it by default on controllers. We'd have to have a [DontValidateAntiForgery] attribute instead, and that feels odd to me. Also now don't have separate API controllers it gets even harder, you don't want checking on those, because you shouldn't be using cookies at that point.

In my opinion it's pretty horribad how we have it right now. An inconsistency among the two (FormTagHelper and Html.BeginForm). We should pick one and make it consistent to avoid confusion. I personally would prefer the default opt-in but I agree [DontValidateAntiForgery] feels awkward.

We could change the behavior of [ValidateAntiForgery] to only apply to form posts. Then it's just any other global filter.

To clarify. We ask users to put this on actions that need validation today instead of adding it globally, because the [ValidateAntiForgery] attribute will throw on actions to which the concept of XSRF doesn't apply.

Thanks for taking the time.

I'm going to follow this thread silently. Just wanted to add to @rynowak 's comment that I would have guessed the ValidateAntiForgery attribute would be applied only to HttpPost (and similar) requests already.

Following the concepts proposed here, I want to add some input.

I love the idea of making this auto :+1:

instead of [DontValidateAntiForgery], i think [ValidateAntiForgery(false)] it's very aligned with the framework and it looks great!

@Html.AntiForgeryToken() can do nothing (backcompat maybe in a shim) or be removed.

and the actual validation can only be executed on form request with application/x-www-form-urlencoded and multipart/form-data headers. (not sure if this is the correct approach to detect when forgery must be validated) as @rynowak suggested with a global filter.

+1 it should be safe by default. It's just a lot of boiler plate that we need to remember to add to forms/post methods.

We've had some more discussion internally about this since posting about it last week. We (as well as @blowdart) really like the idea of things being more secure by default. The one scenario we're concerned about today is API endpoints that don't use antiforgery.

It's a small ask for a browser-based endpoint to do the right thing with antiforgery. Turning this on by default also means turning it on for APIs. We'll need to do the work here aspnet/Antiforgery#17 as well to make this reasonable to API authors. Any concerns about this?

Act now. Ask for forgiveness later.

API endpoints should be using bearer tokens for antixsrf.

As for doing this automatically, comparing to other frameworks: RoR does this automatically with a global config setting.

this might sound a bit daft but what about the following situation:

the problem with making it automatic, is what about when there needs to be external intervention to expand the validation timeout, i.e. a page with videos on it and each time you click a video it checks with the server and then pulls the content from the server, if you made the Anti Forgery Token automatically applied, how would one go about timeout problems after the standard timeout,

i.e. a user stays on the page while doing other stuff for one hour then tries to click the link to popup a video without refreshing the page. This would cause the server to reject the token as its now expired.

"note: this situation is using some form of ajax to refresh content"

@rynowak :
I am surprised that Web API comes into play in regard to XSRF tokens because I am not sure whether MVC and Web API topology can generally be compared in this regard:

While MVC relies on a 1:1 Response-Request cycle, Web API is based on a 0:many cycle. I.e., a POST request can be issued anytime _without any preceding request_ in RESTful Web API while in MVC there is always a single XSRF token "challenge" sent, preceding a single, matching POST request.

So I would suggest to add automatic XSRF tokens to MVC and similer 1:1 technologies only (e.g. WebForms), but not Web API, as Anti Forgery Tokens don't natively apply to Web API right from the beginning.

@david-whitehead

the problem with making it automatic, is what about when there needs to be external intervention to expand the validation timeout

This would only apply to non-idempotent verbs (non-GET/HEAD). If you have this scenario today, and you're doing something non-idempotent on the server, you have an XSRF bug!

If we make this automatic, there will be be an opt-out.

I am surprised that Web API comes into play in regard to XSRF tokens because I am not sure whether MVC and Web API topology can generally be compared in this regard

There is one framework now. We don't have a simple way to distinguish what your expectations are for the usage of your controllers/actions.

In general our recommendation is to not use cookie auth with APIs ever. There's historically been no barrier to using antiforgery with API endpoints (typically in a hybrid MVC+API SPA), or just ignoring antiforgery and having having XSRF bugs.

Making this behavior automatic will impact anyone who's already doing this, and we need to provide an end-to-end for doing the right thing.

@rynowak what are the current concerns now about it making it auto by default?

I understand that API endpoint should not use XSFR. Isn't safe to only apply XSFR for application/x-www-form-urlencoded and multipart/form-data headers or is too much magic ?

Isn't safe to only apply XSFR for application/x-www-form-urlencoded and multipart/form-data headers or is too much magic ?

Users have existing APIs that accept these content types. Recommended/non-recommended we need to understand the impact on user's sites even if the behavior is an improvement.

@rynowak I believe you said you were going to work on this?

Users have existing APIs that accept these content types.

What about an opt-in setting on MvcOptions, perhaps? New apps (or even VS new->project) can set it?

@Eilon @brockallen the droid you're looking for is

c# services .AddMvc() .InitializeTagHelper<FormTagHelper>((helper, _) => helper.Antiforgery = true);

That's great!

Oh, also -- perhaps VS templates, project -> new sets this? This is your chance to show folks how to do it right? Or at least educate people on the existence of it.

Well we want the default to really be on, as opposed to just a template thing. Plus, that's not syntax I'd share with my parents :smile:

So to summarize here:

0). We've added support for antiforgery via header. This is really needed for a first party SPA that uses cookie auth. Sample here: https://github.com/aspnet/Antiforgery/tree/dev/samples/AntiforgerySample

1). We're going to add a 'smart' antiforgery attribute that you can apply as a global filter for sites using cookie/persistent auth. This will be part of the template for MVC: https://github.com/aspnet/Mvc/pull/3784

2). We're going to add an 'opt-out' for antiforgery that you can apply as a filter https://github.com/aspnet/Mvc/pull/3784

3). We're going to create a sample for using conventions to build a mixed site serving browsers, 1st party SPA APIs, and 3rd party APIs. This will demonstrate how to configure different auth policies and antiforgery policies in a cross-cutting way using attributes + conventions

4). We want to change BeginForm and the form TagHelper to do the right thing to include an antiforgery token by default (for some definition of default).

The end result is that if you're building an MVC app, you're doing the right thing with antiforgery by default.

x). Should we add an HtmlHelper/TagHelper for a <meta /> tag with antiforgery?
y). Should we add a filter for an antiforgery-via-cookie (like the sample)

Yay!

@rynowak What is the remaining work that this issue is still tracking?

@danroth27 - changed the title.

The remaining work here is to identify a way that we can validate an antiforgery token when required - by default.

We also need to have a way to opt out of antiforgery validation for specific authentication schemes (like Bearer).

Here's our tentative plan for 2.0.0:

• Enable auto anti request forgery validation in the framework by default
• Update authentication support to expose whether the current authentication scheme is persistent or not (see https://github.com/aspnet/Security/issues/1172)
• Update auto anti request forgery to not validate if the authentication scheme is explicitly not persistent (ex Bearer)

After this is done we will do some tweaking with the template to decide on the final design
• Update Web API item template to add an attribute (new AntiforgeryValidationModeAttribute) that explicitly states that auto anti request forgery behavior will be applied
• Consider adding explicit code to the template with the antiforgery behavior even if it’s the default. Prototype and see what it looks like.

Will reconsider in 2.1.0.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

guardrex picture guardrex  Â·  3Comments

Pixel-Lord picture Pixel-Lord  Â·  3Comments

farhadibehnam picture farhadibehnam  Â·  3Comments

rbanks54 picture rbanks54  Â·  3Comments

aurokk picture aurokk  Â·  3Comments