-
-
Notifications
You must be signed in to change notification settings - Fork 679
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
Clarify CSRF requirement 50.4.1 #2481
Comments
Notes:
|
Yes, "Where cross-origin requests are used, this should always trigger a CORS preflight request first." does not feel right as a consequence. |
So I would clarify to: "[MODIFIED, MOVED FROM 4.2.2, MERGED FROM 13.2.3] Verify that the application defends against Cross-Site Request Forgery (CSRF) attacks to protect authenticated or sensitive public functionality. This should use the development framework's built-in anti-CSRF functionality or CSRF tokens, along with additional defense-in-depth measures such as validating the origin header. Where cross-origin requests are used for this functionality, this should always trigger a CORS preflight request first." Can we think of a situation where a cross-origin request for "authenticated or sensitive public functionality" should not trigger a pre-flight? |
I don't like the requirement nor the proposal. It takes some time to list all the issues. That's why it is assigned to me with status "awaiting proposal". |
So, some ideas to take into account. Problems:
Proposals (to take into account):
v4.0.3 Requirements to cover:
Additionally, v5.0.0 requirements to cover:
|
I agree. I don't like this term very much (and I don't like "XSS" much either for the same kind of reasons). However, this is the accepted wording. It is difficult to depart from it and mint your own wording when the whole community is using this "CSRF". FWIW, I think a better wording would be "cross origin request forgery" but it is not used at all anywhere.
I kind-of like that 😄. I'm wondering if this is clear enough though. There might be other kind of client-side request forgery (for example, client-side cross-protocol confusion attacks, ALPACA). |
Offtopic:
Probably you can find also from here (ASVS repo) in issues discussions how much I don't like the XSS. The XSS even worse than CSRF. CSRF is "half ok", because the "request forgery" part still explains what it is. But XSS is also used in "Same-Origin HTML injection" situation, which means not a single word from cross-site scripting is actually true, and this is insane nonsense. Ontopic:
I think "this is the used term", not that much maybe "the accepted term" anymore. At the end, the term Cross-Site Request Forgery is incorrect and there is no reason to use it. The same with XSS - we have it mentioned only in one requirement in the context of impact. The "Cross-Site" is used also in "XSSI" name.
I would not limit it to cross-origin either. It can be also same-origin - for example, an attacker can use only HTML injection (classical outcome from sanitized HTML, you can still use |
@elarlang I see your point but can you make a concrete wording suggestion please. |
Agreed XSS is not a good term. All three of the words can be wrong.
Many folks use “web UI content injection”, “content injection” or similar as a more accurate term.
|
Addressing proposals from @elarlang :
Cross Site Request Forgery, Session Riding CSRF and XSRF are the common terms and acronyms. If we stray beyond these, can we please cite them along side the new term? Maybe session riding is more accurate than cross site request forgery or request forgery? But overall I do not think this is a big problem. Most of the industry recognizes these legacy terms. The generic term "request forgery" is too generic IMO, it can incorrectly refer to SSRF or Clickjacking.
I like the direction of this. But I think it's more than state changing functionality. I think it's any functionality that would cause damage if the request was forged, regardless of the verb or state changing effect.
I think this is a really good idea to add in. Thumbs up on this one. |
Previously I proposed to use client-side request forgery to be "paired" with SSRF, but one of them is making the request and another is accepting the request:
The "Client" is also vague, and in the situation of SSRF to happen, the vulnerable application itself is a client to make the request. So maybe browser-based request forgery is a more correct or self-explaining term to use. ProposalProposal is provided with different parts to have more precise feedback.
Part 1 - the definition:
Part 2 - the clarification the functionality to defend
Part 3 - safe HTTP methods for data changes (merge 13.6.2)
Part 4 - CORS spotlight (merge 13.2.5)
Part 5 - terminology note Too long to add into the requirement, probably part of the chapter text:
|
We also have a requirement for safe-methods:
The main goal is the same, it is a kind of pre-condition to have a defense against browser-based request forgery attacks and it does not have its own separate risk. The choice here is, to merge everything into one monster requirement, or to have some more specific separate requirements to address separate related and underlying problems, such as using safe methods for state changes or CORS-preflight downgrade to CORS-safelisted. |
So what is the rough wording suggestion please here @elarlang? |
Rough wording is concat(part1, part2, part3, part4, part-x) - written as separate parts to have more precise feedback and finetuning for different parts. Updated proposal in #2481 (comment) |
Regarding,
Maybe we need to focus on state changing or resource intensive functionality. Not sure I know what sensitive functionality means in this that isn't included by one of those other definitions.
Not convinced there is anything else. I would propose the following text: "Verify that requests to state-changing or resource intensive functionality are checked to ensure they originate from the application itself, to defend against browser-based request forgery attacks (commonly known as cross-site request forgery (CSRF)). Additionally, this functionality must not be invoked using safe HTTP methods such as HEAD, OPTIONS, or GET. This precaution will ensure that cross-origin requests to this functionality will trigger a preflight request." Comments on the proposal:
V50.4 Browser Origin SeparationWhen accepting a request on the server side, we need to be sure it is initiated by the application itself and not in some way spoofed by a malicious party. Key security mechanisms here include browser security policies like Same Origin Policy for JavaScript and also SameSite logic for cookies. The most common and well known attack vector is known as cross-site request forgery (CSRF), but note that the definition of site (effective Top Level Domain + 1) makes the term CSRF outdated and incorrect. The scope limitation to cross-site is not valid, as the attack may come from 'same-site but cross-origin' or from 'same-site and same-origin' scope. |
As we included CORS, it may be some other trusted party that uses the API
The proposal lost spotlight for the non-authenticated actions, such as authentication.
If you don't validate the previously/my proposed part 4, you can bypass this new/your proposed part 4. |
I think the following requirement covers parts 1, 2 and 4 above: "Verify that requests to state-changing, security sensitive, or resource intensive functionality includes a mechanism to prevent browser-based request forgery attacks, commonly known as cross-site request forgery (CSRF). Ideally the functionality should only accept content-types which would trigger a CORS preflight and enforce an allow list of 'Origin' headers. Alternatively, another CSRF prevention mechanism may be used." It covers the "write" case here where the "read" case is covered by 50.3.6. I think the following requirement covers part "Verify that requests to state-changing, security sensitive, or resource intensive functionality use appropriate HTTP methods including POST, PATCH, DELETE or PUT." Part 5 will be covered in chapter text but we have a note that we wanted to mention authentication as a specific "write" case there. |
TLDR:
Disclaimer - in total I have spent (too) many hours to put this together. I expect any comments to be technically validated before proposing anything else, just so that I do not have further time to explain how HTTP or CORS works. It also means, that every detail I proposed, is there for a reason, and if an alternative proposal is going to lose some detail, it must be validated and explained reason to do so. Separate requirementsI'm going to propose (to keep) separate requirements instead of merging everything to one monster-anti-csrf-requirement, just because the impact is a "CSRF". If the CORS-preflight can be downgraded to CORS-safelisted request, the impact is a "CSRF" but it is a separate problem to solve compared to "missing tokens" or "making changes with navigation request". In the same way, we have a separate requirement for each technical situation on how to reach HTML or JavaScript execution (so-called XSS). If we could use one requirement "verify that user-controlled input is not executed as HTML or JavaScript" we could lose too many details and it could not provide anything practical anymore. For example, we can not put together problems related to dynamic document building (HTML or JavaScript injection), using an incorrect method to display the data (HTML or JavaScript execution), using an incorrect Using separate requirements also aligns with agreed principles such as "One problem per requirement", "different test-cases into different requirements", and "a separate requirement to highlight really widespread problems". Listed requirements are not duplicates, they just have a common impact. From the impact point of view, those (theoretically) can be merged into one monster requirement but with the cost of actionable details. Part 2, 5 - Terminology, Functionality, scope, technical limitationLimitation for request forgery requirements - it affects only if there
Previously proposed:
It should be covered in the chapter text. The chapter text must include also "Part 5" - the explanation, of why "CSRF" is not a technically correct term to use. Part 4I start from Part 4, to get the scope reduced for the others. The scope and the point to cover - if using API and/or CORS - accept only API and/or CORS calls. Applies only to APIs that are used via browser using JavaScript (XHR or fetch) Previously (#2481 (comment)) proposed as part 4:
Test-Cases:
Expected defense:
In other words: in case it is possible to skip pre-flight request and downgrade it to CORS-safelisted request, the expected defense is not to ask "CSRF tokens", but to fix the set of headers. It means it should be a separate requirement - it is a separate technical problem, it expects a different defense than "ask tokens". It should cover current v5.0 requirements:
The meaning for the requirement:
Part 3 - Accept only safe methods - current 13.6.2 (#2492).When simplified, it needs to achieve that:
Test-cases (simplified):
Expected defense (simplified):
Previous proposal (simplified):
When not simplified, we need to take into account:
That means, although state-changing or sensitive functionality is called using an HTTP safe method, it is not vulnerable to "CSRF" just because of that. Instead of "use HTTP safe methods" we need to ask "the functionality must not be triggered with navigation request or resource loading request, e.g. loading an image". Navigation request here is the most critical because cookies with It should cover current v5.0 requirements:
The proposal has "negative wording", but let's start with that.
This requirement is a pre-condition to have any defense against browser-based request forgery. The expected defense is not to check and validate tokens, the expected defense is to have a correct architecture in place. Part 1Previous proposal (with updated wording by Josh):
It should cover current v5.0 requirements:
A pre-condition:
Test-Cases:
Expected defense:
Proposal with some limitations and direction compared to the previous one:
Scope - this requirement basically handles the situation if an HTTP request using the POST method is made
So the requirement may also start with:
Before any comments - please validate your comment against this post before submitting it. . o O ( Where do I send the bill for writing this post? ) |
#thiscommentshouldhavebeenablogpost 🙃🤣 |
Please can you add your proposals to the tl;dr together with tags to help understand which current requirements they are modifying. |
In the current situation, the long post was (forced to be) written to explain and address all the situations, different test cases, and expected developments together and point out, how they don't overlap. I recommend taking the time and analyze it. Understanding it is a pre-condition to make any decision based on the provided information. So I don't think those 3 proposed requirements should be taken out of context and then start answering the questions out of context that are actually already written in the post. |
Ok so as I understand it, your proposed requirements are: Part 4: V50.4.3 - [MODIFIED, MOVED FROM 13.2.5, SPLIT FROM 14.5.3] Verify that if the application uses and relies on CORS-preflight mechanism, it is not possible to call the functionality with a CORS-safelisted request. It may require validating the 'Origin' and 'Content-Type' request headers or using an extra header field in the communication that is not a CORS-safelisted request header field. Part 3: V13.6.2 - [MODIFIED, MOVED FROM 13.2.1] Verify that state-changing or resource-demanding functionality is not executed for a navigation request or is called an incorrect destination (e.g. export data functionality is called as an image from the browser). Not accepting safe HTTP methods such as HEAD, OPTIONS, or GET and Sec-Fetch-* request headers can be used for validation. It requires special attention if the application does not distinguish URL parameters and message body parameters. Part 1: 50.4.1 - [MODIFIED, MOVED FROM 4.2.2, MERGED FROM 13.2.3] Verify that requests to state-changing or resource intensive functionality are checked to ensure they originate from the application itself. For example, by using and validating tokens or extra HTTP headers that are not CORS-safelisted request-headers. This is to defend against browser-based request forgery attacks, commonly known as cross-site request forgery (CSRF). Correct? If so, I can then go and analyse your argumentation whilst clearly understanding your end goal from your argumentation. |
Relations to current requirements that will be covered with new proposal, were listed in my post into each part. So I think the question was answered already. How correct tags are pre-condition to analyze the concept "one monster requirement" vs "3 separate requirements" is not understandable for me. What is the correct tag also depends on the requirement. I listed at the moment current v5.0.be requirements, if we want to have correct tags, we need to analyze the content of a v4.0.3 requirement compared to new proposed requirement. At this stage, I don't see the reason to put time into it. Just for an example, 13.2.3 from v4.0.3 is basically covered by 2 separate requirements. Overall, requirements to cover was provided in #2481 (comment) and 13.6.2 was linked later in #2481 (comment) |
Summary of discussion These are ok: Part 4: "V50.4.3 - [MODIFIED, MOVED FROM 13.2.5, SPLIT FROM 14.5.3] Verify that, if the application relies on the CORS preflight mechanism to prevent disallowed cross-origin use of sensitive functionality, it is not possible to call the functionality with a CORS-safelisted request. This may require checking the values of the 'Origin' and 'Content-Type' request headers or using an extra header field that is not CORS-safelisted." Add a note to chapter text that this applies to apps that are designed to be accessed cross origin but also those which are not but instead preflight is used as a security mechanism. Part 1: "50.4.1 - [MODIFIED, MOVED FROM 4.2.2, MERGED FROM 13.2.3] Verify that CORS-safelisted requests to sensitive functionality are checked to ensure that they originate from the application itself. This may be done by using and validating anti-forgery tokens or requiring extra HTTP headers that are not CORS-safelisted request-headers. This is to defend against browser-based request forgery attacks, commonly known as cross-site request forgery (CSRF)." Add anti-forgery tokens to glossary? This needs more thought: Part 3: V13.6.2 - [MODIFIED, MOVED FROM 13.2.1] Verify that sensitive functionality is not executed for a navigation request or is called an incorrect destination (e.g. export data functionality is called as an image from the browser). Not accepting safe HTTP methods such as HEAD, OPTIONS, or GET and Sec-Fetch-* request headers can be used for validation. It requires special attention if the application does not distinguish URL parameters and message body parameters. |
Looking again at Part 3: This is the current state: "V13.6.2 - [MODIFIED, MOVED FROM 13.2.1] Verify that HTTP requests using the HEAD, OPTIONS, TRACE or GET verb do not modify any backend data structure or perform any state-changing actions. These requests are safe methods and should therefore not have any side effects." I previously said (here) that I thought this should be merged into a separate CSRF requirement. I am prepared to accept that it needs to stay separate and also include the subtleties of the version proposed above. I therefore propose the following @elarlang : "V13.6.2 - [MODIFIED, MOVED FROM 13.2.1] Verify that calls to sensitive functionality use appropriate HTTP methods such as POST, PUT, PATCH or DELETE, and not methods defined by the HTTP specification as "safe" such as HEAD, OPTIONS, or GET. Alternatively, strict validation of the Sec-Fetch-* can be used to ensure that the request did not originate from an inappropriate cross-origin call, a navigation request, or a resource load (such as an image source). This is particularly important if the application does not distinguish between URL parameters and message body parameters." What do you think? |
Proposed middle-part:
Attempt to turn it into positive requirement:
If to keep the first option, "such as an image source" needs to be updated to say that "executing functionality being loaded as image resource is not accepted if not intended". |
I don't understand why this level of detail is necessary? We have already said this is about calling sensitive functionality. When would we ever expect this from a resource load? |
Images and dynamically creating images can be called as an image resource and it is expected functionality. |
"V13.6.2 - [MODIFIED, MOVED FROM 13.2.1] Verify that calls to sensitive functionality use appropriate HTTP methods such as POST, PUT, PATCH or DELETE, and not methods defined by the HTTP specification as "safe" such as HEAD, OPTIONS, or GET. Alternatively, strict validation of the Sec-Fetch-* can be used to ensure that the request did not originate from an inappropriate cross-origin call, a navigation request, or a resource load (such as an image source) where this is not expected. This is particularly important if the application does not distinguish between URL parameters and message body parameters." Good enough? 👆 |
One more finetuning:
|
Ok so I think we are there, I think all these requirements now belong in V50.4 so I would propose as follows: V50.4 Browser Origin SeparationWhen accepting a request on the server side, we need to be sure it is initiated by the application itself or by a trusted party and has not been forged by an attacker. The keywords here are browser security policies like Same Origin Policy for JavaScript and also SameSite logic for cookies. Another common protection is the CORS preflight mechanism. This mechanism will be critical for endpoints designed to be called from a different origin, but it can also be a useful request forgery prevention mechanism for endpoints which are not designed to be called from a different origin. The category should contain requirements with ideas:
V13V13.2 Web Services
V13.6 HTTP Request Header ValidationDelete 13.6.2 and move the two reqs after it up by one number Appendix A: Glossary
|
Let me know if you are good with this @elarlang and I will open a PR and make notes on the other relevant issues |
The chapter text should contain the explanation what kind of functionality must be protected and it is valid for authenticated and non-authenticated users. I need to rethink the modification tags, but you can start with the PR. |
Current text:
We also have:
Points to change:
Potential wording:
[MODIFIED, MOVED FROM 4.2.2, MERGED FROM 13.2.3] Verify that the application defends against Cross-Site Request Forgery (CSRF) attacks to protect authenticated or sensitive public functionality. This should use the development framework's built-in anti-CSRF functionality or CSRF tokens, along with additional defense-in-depth measures such as validating the origin header. Where cross-origin requests are used, this should always trigger a CORS preflight request first.
The text was updated successfully, but these errors were encountered: