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.

Sign up for GitHub

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

Jump to bottom

Which CSRF token validation is correct? #6735

Open
antheus-s opened this issue Jun 26, 2023 · 15 comments
Open

Which CSRF token validation is correct? #6735

antheus-s opened this issue Jun 26, 2023 · 15 comments
Assignees
@ismcagdas
Milestone
v9.3

Comments

@antheus-s
Copy link
Contributor

antheus-s commented Jun 26, 2023 •
edited

We migrated from .NET Framework to .NET 6, but still use AngularJS with Razor pages and cookie authentication. Our login page is a plain Razor page that is provided by our AccountController. We noticed that when we use the AbpAutoValidateAntiforgeryTokenAttribute as filter for our controllers, that it does not work on the login page if you delete the CSRF cookies, which also allows for CSRF attacks if you login from a different domain that contains a login form like this one:

<html>
  <body>
    <form action="https://localhost:443322/Account/Login?returnUrl=" method="POST">
      <input type="hidden" name="tenancyName" value="tenancyName" />
      <input type="hidden" name="usernameOrEmailAddress" value="username" />
      <input type="hidden" name="password" value="password" />
      <input type="hidden" name="returnUrlHash" value="" />
      <input type="submit" value="Submit request" />
    </form>
  </body>
</html>

If we use the AutoValidateAntiforgeryTokenAttribute from .NET, as a replacement for the AbpAutoValidateAntiforgeryTokenAttribute from ABP, it all works properly.

Based on the documentation, we only need the .NET attribute, yet in the MVC Startup class, I see this:

//MVC
services.AddControllersWithViews(
    options => { options.Filters.Add(new AbpAutoValidateAntiforgeryTokenAttribute()); }
)...

If I use both, I am able to skip CSRF validation.
What is the correct implementation?

Info:

  • Your Abp package version. 7.3.0
  • Your base framework: .NET 6.
@ismcagdas
Copy link
Member

AbpAutoValidateAntiforgeryTokenAttribute should be used. I think we should update the documentation.

@antheus-s
Copy link
Contributor Author

antheus-s commented Jun 28, 2023 •
edited

When we implement the AbpAutoValidateAntiforgeryTokenAttribute, and an attacker POSTS to our form from an external site, there is no cookie and the validation will be skipped due to the following code in the AbpAutoValidateAntiforgeryTokenAuthorizationFilter:

//No need to validate if antiforgery cookie is not sent.
//That means the request is sent from a non-browser client.
//See https://github.com/aspnet/Antiforgery/issues/115
if (!context.HttpContext.Request.Cookies.ContainsKey(_antiforgeryOptions.Cookie.Name))
{
    return false;
}

Based on the comment, we expect the cookie to always exist and determine that the client is a non-browser client if the cookie does not exist. This is unreliable since a malicious web client might not send the cookie, thus skipping the validation.

Is this a regression issue that allows for CSRF attacks because the CSRF validation is skipped due to this code?

@ismcagdas
Copy link
Member

This is unreliable since a malicious web client might not send the cookie, thus skipping the validation.

So, this can be done using a tool like Postman as well. But, this is how it is designed.

@antheus-s
Copy link
Contributor Author

Could you explain to me how this protects against XSRF attacks?
Because that would mean that there are two situations if an external site would contain a script that would POST to an endpoint of an ABP app:

  1. We do not have a cookie, so we skip validation.
  2. We do have the cookie because we visited the site earlier (the XSRF-Token cookie is a session cookie), and it is automatically sent to the API when submitting the form from the malicious website, which makes the request valid.

Thanks in advance.

@ismcagdas
Copy link
Member

You can read https://aspnetboilerplate.com/Pages/Documents/XSRF-CSRF-Protection, especially https://aspnetboilerplate.com/Pages/Documents/XSRF-CSRF-Protection#non-browser-clients

@antheus-s
Copy link
Contributor Author

We have read the documentation already, as referenced in my initial question, but it doesn't answer my question.

Unless we use a token that is part of the form, our endpoint is not protected, because the cookie based validation uses a session cookie that is either correct or does not exist, which in both situations will pass validation.

This looks to me like the XSRF-protection does not work properly, unless I'm missing something?

@ismcagdas
Copy link
Member

because the cookie based validation uses a session cookie that is either correct or does not exist

This could happen if 3rd party website can delete the anti forgery cookie. Normally this shouldn't be possible.

@antheus-s
Copy link
Contributor Author

antheus-s commented Jul 5, 2023 •
edited

We don't need to manually delete the token for this attack to bypass validation, since it is a session cookie. It won't exist anymore if the browser was closed since the last visit to the web app. If we did not close the browser after the token was created, it will exist and be automatically added to the request, which will be valid since the cookie was created in the web client. Both situations would pass the validation.

@ismcagdas
Copy link
Member

Do you mean the authentication cookie or anti-forgery cookie ?

@antheus-s
Copy link
Contributor Author

I mean the antiforgerytoken.

@ismcagdas
Copy link
Member

@antheus-s sorry for my questions but I'm trying to understand the case :)

It won't exist anymore if the browser was closed since the last visit to the web app

If the browser is closed, you need to visit the page again, isn't that right ? In that case, a new cookie will be generated. Or maybe I totally misunderstand the case here.

@antheus-s
Copy link
Contributor Author

@ismcagdas, no problem of course. Good communication is essential. :P

Non-browser clients presumption

In the current implementation of the AbpAutoValidateAntiforgeryTokenAuthorizationFilter, we skip validation if no CSRF token cookie is sent in the request, because we expect this situation to only occur with non-browser clients.

The problem with this presumption is that it is not correct for all situations. If we go to a malicious site with a new browser session, meaning the browser was fully closed and is reopened, and we send a request from the malicious site to a public API endpoint, we do not send the cookie because it does not exist yet until the user has visited the web app. This would make CSRF attacks, such as Login CSRF attacks, possible because we skip validation if the cookie does not exist yet.

An issue with storing the token in a session cookie

If we visit the web app, the CSRF token cookie will be created. If the user leaves the web app, but keeps their browser open, the cookie will stay alive since the browser session has not ended yet. If we now visit a malicious site and they execute a request to our web app from their domain, it means that the cookie will be sent in the request from the malicious domain, for example utilizing a hidden form that is submitted from JavaScript or a Fetch API request with credentials included, which would also make CSRF attacks possible because the validation would pass. This would also work with protected endpoints if cookie based authentication is used and the user logged into the app in this browser session.

My question

The things I describe above have let me to believe that CSRF is not working properly.

@antheus-s
Copy link
Contributor Author

@ismcagdas Is this being investigated? For our ISO certification, we need to know whether this is an issue that needs to be picked up.

@ismcagdas ismcagdas added this to the v9.0 milestone Sep 21, 2023
@ismcagdas
Copy link
Member

@antheus-s we didn't have time to investigate it, sorry. Do you have any suggestion how this should work ? Maybe it can help us to understand the problem in a better way.

@antheus-s
Copy link
Contributor Author

The issue is related to ABP solutions that use cookie based authentication.

In my previous comment, I describe that the assumption "No need to validate if antiforgery cookie is not sent. That means the request is sent from a non-browser client." is incorrect.

Currently, for example, the user could be logged in from an external site (Login CSRF attack), because as long as the user has not yet visited the app page, the cookie won't exist, meaning that CSRF validation will be skipped. If the user has already visited the app page in their current browser session, then the cookie would be still be valid, meaning that CSRF attacks would still be possible. This issue is caused by the fact that the cookies that are sent in a request are based on the URL of the request, not the current site. So if https://evil.com sends a request to our https://app.abp.com/login endpoint, the validation will pass, because the existing CSRF token cookie will be added to the request. Using some simple JavaScript, we could submit a form on https://evil.com, just by visiting the page, without the user ever noticing, which would have impact on their session in our app.

To summarize, we only have protection for situations where someone specifically creates their own cookie with an incorrect token.

I suggest that we remove the code that skips validation if the cookie is absent and therefore force non-browser clients to use a different endpoint, which can be made accessible by using the IgnoreAntiforgeryTokenAttribute.

In our solution, we made it possible to share protected endpoints by using middleware to enable both token based and cookie based authentication, based on whether a token has been provided. But in my opinion, this is beyond the scope of ABP.

@ismcagdas ismcagdas self-assigned this Oct 12, 2023
@ismcagdas ismcagdas modified the milestones: v9.0, v9.1 Nov 15, 2023
@ismcagdas ismcagdas modified the milestones: v9.1, v9.2 Feb 1, 2024
@ismcagdas ismcagdas modified the milestones: v9.2, v9.3 Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ismcagdas ismcagdas

Labels
None yet
Projects
None yet
Milestone
v9.3
Development

No branches or pull requests
2 participants
@ismcagdas @antheus-s