aspnet / Antiforgery

[Archived] AntiForgery token feature for ASP.NET Core. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
78 stars 40 forks source link

Make HTTP method configurable #172

Closed jchannon closed 6 years ago

jchannon commented 6 years ago

Currently Antiforgery returns true when validating a request if the method is:

"GET" "HEAD" "OPTIONS" "TRACE"

Would you be open to making these configurable in that if a user wants GET requests to be validated they can set that in the options?

rynowak commented 6 years ago

I think it's a reasonably request for us to make this customizable. I'm curious though, why do you want to validate GET?

We generally don't care about validating GET because if you're doing state changes on a GET the you've already got problems.

jchannon commented 6 years ago

I've just thought it through and I'm not sure it will help me in what I was thinking of using it for.

We've had a "security team" say that you can log into the app, copy the cookie value, logout then use that cookie value to initiate a curl request and get data to be a problem. Me pointing out that with https turned on the only way you can get that cookie is if you are sat at the users desk or compromised their machine and at that point have much bigger issues is completely lost on them and so now I have to add state lookups per request to satisfy their flawed pen test.

I was thinking that doing a check on GET requests might help me but it won't.

On 14 December 2017 at 17:24, Ryan Nowak notifications@github.com wrote:

I think it's a reasonably request for us to make this customizable. I'm curious though, why do you want to validate GET?

We generally don't care about validating GET because if you're doing state changes on a GET the you've already got problems.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/aspnet/Antiforgery/issues/172#issuecomment-351779273, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGapre7g19spMDGe6_7ThULf-128ooyks5tAVm2gaJpZM4RB3jL .

rynowak commented 6 years ago

The antiforgery library is a mitigation against CSRF attacks - which emphasis on the CS which stands for cross-site. It is meant to protect your users from another site getting their client to send a post to your site and modify data using their credentials. It's not general purpose access control.

@blowdart @Tratcher do you care to respond to this comment:

We've had a "security team" say that you can log into the app, copy the cookie value, logout then use that cookie value to initiate a curl request and get data to be a problem

Generally yes, if you can get a users authN cookie, then you can do anything you want as that user. You could also work around CSRF easily. I'm not sure if our logout system has any built in protections for that.

I have to add state lookups per request to satisfy their flawed pent test.

If a malicious person has your authN cookie they could work around that as well.

jchannon commented 6 years ago

Yup understand. Shame I have to put in place something that adds no security value and slows my app down for the sake of ticking a box

On Thu, 14 Dec 2017 at 17:51, Ryan Nowak notifications@github.com wrote:

The antiforgery library is a mitigation against CSRF attacks - which emphasis on the CS which stands for cross-site. It is meant to protect your users from another site getting their client to send a post to your site and modify data using their credentials. It's not general purpose access control.

@blowdart https://github.com/blowdart @Tratcher https://github.com/tratcher do you care to respond to this comment:

We've had a "security team" say that you can log into the app, copy the cookie value, logout then use that cookie value to initiate a curl request and get data to be a problem

Generally yes, if you can get a users authN cookie, then you can do anything you want as that user. You could also work around CSRF easily.

I have to add state lookups per request to satisfy their flawed pent test.

If a malicious person has your authN cookie they could work around that as well.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/aspnet/Antiforgery/issues/172#issuecomment-351786681, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGapkn-kN4zJbP_Cv69qXQ-v9jXuPS8ks5tAWA8gaJpZM4RB3jL .

blowdart commented 6 years ago

You know your security team sucks :)

You could take the approach identity does, embed a guid in your auth cookie, and keep the corresponding value against the user. Then when the user logs out you change the guid, and check the value in the cookie on every request, but yea, it's going to be slow.

Honestly I'd push back against your security team as you have done (no surprises there) and point out if they're running malware on a device you have bigger problems than logging into your app, but I am guessing this is an external team from a consultancy, and of course, management, because they're paying them, will believe them first.

If you want, I could email you my "official" thoughts to use as evidence :)

jchannon commented 6 years ago

My email address is jchannon@vqcomms.com 😄

On Thu, 14 Dec 2017 at 18:05, Barry Dorrans notifications@github.com wrote:

You know your security team sucks :)

You could take the approach identity does, embed a guid in your auth cookie, and keep the corresponding value against the user. Then when the user logs out you change the guid, and check the value in the cookie on every request, but yea, it's going to be slow.

Honestly I'd push back against your security team as you have done (no surprises there) and point out if they're running malware on a device you have bigger problems than logging into your app, but I am guessing this is an external team from a consultancy, and of course, management, because they're paying them, will believe them first.

If you want, I could email you my "official" thoughts to use as evidence :)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/aspnet/Antiforgery/issues/172#issuecomment-351790192, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGaptv641E9j0wSYw8mP1lw9GiA9ZJHks5tAWNagaJpZM4RB3jL .