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

V4.2.2 CSRF and/vs anti-automation #795

Closed
elarlang opened this issue May 30, 2020 · 53 comments
Closed

V4.2.2 CSRF and/vs anti-automation #795

elarlang opened this issue May 30, 2020 · 53 comments
Assignees
Milestone

Comments

@elarlang
Copy link
Collaborator

V4.2.2

Current:

V4.2.2 Verify that the application or framework enforces a strong anti-CSRF mechanism to protect authenticated functionality, and effective anti-automation or anti-CSRF protects unauthenticated functionality.

  • Anti-automation and CSRF are different problems and require different solutions for defense / Anti-automation is not what anti-CSRF protection (requirement) should address.
  • Anti-automation is covered with requirements in "V11.1 Business Logic Security Requirements"

No clear proposals yet, I would like to know first, why this anti-automation part is in this requirement?

@jmanico
Copy link
Member

jmanico commented May 31, 2020 via email

@elarlang
Copy link
Collaborator Author

For non-auth, CSRF defense is important.

Another example - there is some defense "too many requests from one IP and traffic from IP will be blocked", for instance: authentication attempts. So it's important to have CSRF defense on login in also, that I can not force someone else's browser to make wrong attempts and to get blocked.

Anti-automation - I think we should have separate subcategory for Anti-automation under business logic. But as it is not urgent yet, I haven't made any issue for this yet. I'll do it after ASVS v4.0.2 is out. I think you current example (2) is also covered by V11 category, like:

V11.1.2 Verify the application will only process business logic flows with all steps being processed in realistic human time, i.e. transactions are not submitted too quickly.

But how to improve current V4.2.2? Very robust and simple could be:

V4.2.2 Verify that the application or framework enforces a strong defense against Cross-Site Request Forgery (CSRF) attacks.

Questions:

  • do we need "or framework" part?
  • do we need to point out separately, that yes, for non-auth users it's also makes sense to provide defense against CSRF?

@jmanico
Copy link
Member

jmanico commented May 31, 2020 via email

@elarlang
Copy link
Collaborator Author

Proposal:

V4.2.2 Verify that the application enforces a strong defense against Cross-Site Request Forgery (CSRF) attacks for both, authenticated and not authenticated users.

Language check/correction needed.

@jmanico
Copy link
Member

jmanico commented May 31, 2020 via email

@elarlang
Copy link
Collaborator Author

elarlang commented Jun 1, 2020

... and how to put it to the requirement?

If you say "sensitive feature" we should also define "what is sensitive feature", otherwise it's endless discussion "is this feature sensitive enough".

Defense against CSRF is required when not safe HTTP method is used:

And here, precondition is - HTTP methods are used like they are meant to be - no data changes with safe HTTP methods.

@jmanico
Copy link
Member

jmanico commented Jun 1, 2020 via email

@elarlang
Copy link
Collaborator Author

elarlang commented Jun 1, 2020

@jmanico, I leave this proposal fine-tuning to you :)

Also, I'lle take here requirement from "V13.2 RESTful Web Service Verification Requirements" subcategory, as there are also no mention of sensitive action / transaction or something like that. Maybe this one needs some improvement as well.

V13.2.3 Verify that RESTful web services that utilize cookies are protected from Cross-Site Request Forgery via the use of at least one or more of the following: triple or double submit cookie pattern (see references), CSRF nonces, or Origin request header checks.

@jmanico
Copy link
Member

jmanico commented Jun 1, 2020 via email

@tghosth tghosth added this to the 4.1 milestone Jun 2, 2020
@elarlang
Copy link
Collaborator Author

elarlang commented Jun 2, 2020

@tghosth , why milestone v4.1?

@tghosth
Copy link
Collaborator

tghosth commented Jun 10, 2020

@tghosth , why milestone v4.1?

Sounds like a non-cosmetic change to a requirement so too extensive for a 4.0.2

@tghosth
Copy link
Collaborator

tghosth commented Jun 18, 2020

Hi @jmanico whilst you are working on this, please can you try and resolve the potential duplication betwee v4.4.2 and v13.2.4 as @elarlang mentioned in #809

@jmanico
Copy link
Member

jmanico commented Jun 18, 2020 via email

@elarlang
Copy link
Collaborator Author

Takeaway from #876 - CSRF defense is not clear authorization problem from an application point of view, it's defense "is user's browser authorized to do the request". This requirement (merged with V13.2.4) should belong to the same category like CORS/ACAO. Maybe need for clear new category to send clear message, which requirements exists for taking away or lowering impact for attack vectors via victim browser.

@jmanico
Copy link
Member

jmanico commented Mar 11, 2021

This is very hard to wordsmith but I am trying to address it now.

@jmanico
Copy link
Member

jmanico commented Mar 11, 2021

I propose:

V4.2.2 Verify that the application or framework enforces a strong anti-CSRF mechanism to protect authenticated or sensitive public functionality.

I realize "sensitive public functionality" is not perfect but I am open to suggestion to fix this. I think "sensitive functionality" is too hard to define at this level.

@jmanico
Copy link
Member

jmanico commented Mar 11, 2021

I agree CSRF does not belong in the access control section.

@jmanico
Copy link
Member

jmanico commented Mar 11, 2021

Ok I suggest we delete 4.2.2 and 13.2.3 and add a new requirement in the CORS section as advised which states:

Verify that the application or framework enforces an anti-CSRF mechanism to protect authenticated or sensitive public functionality using at least one or more of the following: double submit cookie pattern, CSRF nonces or SameSite cookies.

@elarlang
Copy link
Collaborator Author

Last proposal (merged 4.2.2 and 13.2.3):

Verify that the application or framework enforces an anti-CSRF mechanism to protect authenticated or sensitive public functionality using at least one or more of the following: double submit cookie pattern, CSRF nonces or SameSite cookies.

Current 13.2.3:

Verify that RESTful web services that utilize cookies are protected from Cross-Site Request Forgery via the use of at least one or more of the following: double submit cookie pattern, CSRF nonces, or Origin request header checks.

  • Is there clear reason, why new proposal does not contain Origin request header check?
  • I like the style in 13.2.3 where CSRF is written out: Cross-Site Request Forgery

For category change I'll open separate issue in future.

@jmanico
Copy link
Member

jmanico commented Mar 21, 2021

I like the change you propose. Looks good!

@jmanico
Copy link
Member

jmanico commented Mar 22, 2021

Is there clear reason, why new proposal does not contain Origin request header check?

This is not really needed, the other defense are more robust and not all browsers send the Origin header with each request.

I like the style in 13.2.3 where CSRF is written out: Cross-Site Request Forgery

I agree!

@elarlang
Copy link
Collaborator Author

elarlang commented Mar 22, 2021

Current:

  • V4.2.2 Verify that the application or framework enforces a strong anti-CSRF mechanism to protect authenticated functionality, and effective anti-automation or anti-CSRF protects unauthenticated functionality.
  • V13.2.3 Verify that RESTful web services that utilize cookies are protected from Cross-Site Request Forgery via the use of at least one or more of the following: double submit cookie pattern, CSRF nonces, or Origin request header checks.

Proposal 1 - current 4.2.2:

  • Label: [MODIFIED, MERGED 13.2.2]
  • Category: V4.2
    • Not correct, will be changed with separate issue
  • Id: 4.2.2
  • Requirement: Verify that the application or framework enforces defense against Cross-Site Request Forgery (CSRF) attacks to protect authenticated or sensitive public functionality using at least one or more of the following: double submit cookie pattern, CSRF nonces or SameSite cookies.
    • Changed: text merged from 4.2.2 and 13.2.3, anti-automation part is removed, as it's covered by (current) V11.1.* requirements
  • Levels: 1, 2, 3
    • (not changed)
  • CWE-352
    • (not changed)

Proposal 2 - current 13.2.3:

  • Label: [DELETED, MERGED TO 4.2.2]

@Sjord
Copy link
Contributor

Sjord commented Mar 23, 2021

Verify that the application or framework enforces defense against Cross-Site Request Forgery (CSRF) attacks to protect authenticated or sensitive public functionality using at least one or more of the following: double submit cookie pattern, CSRF nonces or SameSite cookies.

"enforces defense" kan perhaps be shortened to "defends".

Some people think that SameSite cookies are not sufficient against CSRF, and that a server-side solution is always needed. I disagree, but that may be something to consider.

CSRF nonces

The term "nonce" suggests that every CSRF token is used once, but I think CSRF tokens that remain the same throughout the session are also acceptable.

sensitive public functionality

I have a hard time thinking of a CSRF attack on a public endpoint. Do you have an example in mind?

@elarlang
Copy link
Collaborator Author

elarlang commented Mar 23, 2021

I have a hard time thinking of a CSRF attack on a public endpoint. Do you have an example in mind?

Please read 2nd and 3rd comment in this thread.

Agree with shortening to "defends".

nonce vs token - in practice, it probably does not matter - if an attacker can read out victim's token, it's probably possible to use nonce from that situation as well.

@jmanico
Copy link
Member

jmanico commented Mar 23, 2021

Some people think that SameSite cookies are not sufficient against CSRF, and that a server-side solution is always needed. I disagree, but that may be something to consider.

This science is clear there. If you only support browsers that have SameSite support (and you enforce this policy in code and reject old browsers) then same site cookies are sufficient. Be wary of wonky IE11 support and bugs in recent versions of Safari. Frankly, I would not depend on SameSite alone... yet.

https://caniuse.com/?search=samesite

@jmanico
Copy link
Member

jmanico commented Apr 8, 2021

Hi @jmanico, above you say that you would not yet trust same site cookies alone but this updated requirement appears to do just that because it gives same site cookies as one of the possible options to implement. Do we need to roll this back?

Josh you are right. SameSite cookies really are not enough. Avi Douglen, Neil Madden and Daniel Fett "schooled" me over twitter and they are right.

So SameSite is sufficient for CSRF defense but only if (1) cookies are the session transport mechanism and (2) older browsers are not supported.

We still need other defense methods for non-cookie based session management like network only AuthN, HTTP Digest/Basic and other edge case issues.

@elarlang
Copy link
Collaborator Author

elarlang commented Apr 8, 2021

Have not analyzed it enough but the idea is - maybe we should just drop all technical recommendations as those depends on technologies used in application:

Verify that the application defends against Cross-Site Request Forgery (CSRF) attacks to protect authenticated or sensitive public functionality.

There are so many different ways to achieve that. If we remove SameSite from this requirement, but in reality application uses cookie-based session token with samesite=strict, then in practice the defense is there.

@tghosth
Copy link
Collaborator

tghosth commented Apr 9, 2021

@elarlang @jmanico can we say to follow the guidance in the cheat sheet?

https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html

Obviously we would include the reference of the bottom of the page (if it is not there already) and not in the body of the requirement. 😃

@elarlang
Copy link
Collaborator Author

elarlang commented Apr 9, 2021

@tghosth
Copy link
Collaborator

tghosth commented Apr 9, 2021

Ok so can you modify the wording of the requirement so it says that instead of allowing samesite as a standalone option

@jmanico
Copy link
Member

jmanico commented Apr 9, 2021 via email

@elarlang
Copy link
Collaborator Author

Not sure now what is the expectation? I think requirement text should not say "follow cheat sheet", it can be written somewhere in "category description". Requirement text itself should be clean, so my proposal is to clean up all technical advises/requirements:

Verify that the application defends against Cross-Site Request Forgery (CSRF) attacks to protect authenticated or sensitive public functionality.

@jmanico
Copy link
Member

jmanico commented Apr 12, 2021

I would like to avoid any requirement pointing the a cheatsheet - they requirements should be self contained.

I am a bit lost. What more needs to happen? I want to make sure:

  1. Anti-Automation and CSRF are not enough
  2. Stateless rest apps use double-cookie submit or similar defense that is appropriate for statelessness (nonce patterns that use session are not)
  3. SameSite is addressed but as defense In Addition to other defenses
  4. Address non-cookie CSRF scenarios

And I am a bit lost in this thread! :)

@elarlang
Copy link
Collaborator Author

Proposal without "technical advices", go or no go?

Verify that the application defends against Cross-Site Request Forgery (CSRF) attacks to protect authenticated or sensitive public functionality.

@jmanico
Copy link
Member

jmanico commented Apr 20, 2021

I think we need to give advice here.

tghosth added a commit that referenced this issue Apr 21, 2021
Changed the wording to match the cheatsheet
@tghosth tghosth mentioned this issue Apr 21, 2021
@tghosth
Copy link
Collaborator

tghosth commented Apr 21, 2021

I created a PR based on how I understand the cheatsheet's summary https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#introduction

@jmanico @elarlang please look

@elarlang
Copy link
Collaborator Author

I can not see benefits over just dropping all this recommendation part - it does not advice anything.

It recommends to use framework CSRF defense, but what if this one is weak? Then by this requirement application is ok (as it uses framework solution like required), but actually it's not. Most-likely framework built-in defense is used anyway.

Same with "additional defense in depth measures" - it does not give any information. For me it feels better without those filler-extras.

If we actually want to give advises, I think we can not handle it with one requirement and we need context-specific requirements. Personally I don't like this direction, because it's maybe not realistic to cover all tech combinations.

It's possible to achieve defense against CSRF attacks so many different ways - if it's done in whatever way, it's done. It's up to pen-tester to verify that or to prove it not strong enough.

@tghosth
Copy link
Collaborator

tghosth commented Apr 22, 2021

@elarlang so you prefer your option from above?

Verify that the application defends against Cross-Site Request Forgery (CSRF) attacks to protect authenticated or sensitive public functionality.

@elarlang
Copy link
Collaborator Author

elarlang commented Apr 22, 2021

Yes.

to be clear: I'm not against giving technical advises but I think there are way too many different ways to achieve that to be able to cover them with one requirement.

@jmanico
Copy link
Member

jmanico commented Apr 22, 2021 via email

@tghosth
Copy link
Collaborator

tghosth commented Apr 23, 2021

I tend to agree with Jim that we have to say something. @elarlang to you have any other comments on what Jim wrote here?

@elarlang
Copy link
Collaborator Author

In addition, consider additional defense in depth measures such as SameSite cookies or authentication.

This part is covered with requirement in cookie section and it's not relevant for application which does not use cookies.

@elarlang
Copy link
Collaborator Author

From this issue perspective - my goal was to separate anti-csrf and anti-automation requirements and merge 2 different anti-csrf requirements to one requirement, I think those are achieved.

@tghosth
Copy link
Collaborator

tghosth commented Apr 28, 2021

@elarlang, is CSRF relevant for applications which do not use cookies...? :)

@elarlang
Copy link
Collaborator Author

elarlang commented Apr 28, 2021

non-auth functionality, basic authentication

(picked from #795 (comment))

@jmanico
Copy link
Member

jmanico commented Apr 28, 2021 via email

@tghosth
Copy link
Collaborator

tghosth commented May 12, 2021

Ah yes, sorry I missed that bit. #795 no longer mentions cookies thing so I think we are good to merge and close this thread, thanks all :)

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

No branches or pull requests

4 participants