[Proposal] CSRF checks on GET routes

[beeonthego]

The Vulnerabilities on GET Routes

For historical reasons some GET routes in Gitea do more than getting info and actually change state, for example /user/logout, and GET routes with action/:action in it. Current CSRF handler only checks POST routes, and thus leaves those GET routes vulnerable.

One particular route admin/notices/empty should probably be a POST i/o GET route.

Strategies

We are working on a PR to address these vulnerabilities, and would like the community feedback on the best strategy.

Option 1: reuse existing CSRF token + referrer-policy header (WARNING! BAD IDEA, Don't do this. See comments below for reason.)

In this option we will append the existing CSRF token to vulnerable GET routes as a query string, validate the token. A referrer-policy header can be set to same origin to prevent abuse of the token.

This option has less changes to existing code.

Option 2: generate a new JWT token with a key exclusive for this purpose

In this option we will generate a new JWT token and send to the browser via a new cookie. This will have more changes to the code.

Questions

security concerns on reusing csrf token on URL

We are thinking to implement option 1, and have code already done. It has less code changes and hopefully can be merged soon. Are there any concerns on this option 1?

response code or redirect

What should we do if the required token is missing or invalid? Should we redirect to home page, or return 403? Browsers treat 403 error code differently, for example Chrome on Mac displays the following message which may not be what you want.

This site can’t be reached
The web page at https://gitea.on-my-super-domain.dummy/user/logout might be temporarily down or it may have moved permanently to a new web address.
ERR_INVALID_RESPONSE

Potential Impact

Both options require an extra handler on vulnerable routes to check the token in query string, and minor changes in URL in templates. While the PR can include the new token in default templates, this is potentially a breaking change to people using custom templates. The custom templates need to be updated to include the token in generated URL.

[techknowlogick]

This is a dupe ticket. I'm on mobile right now so I can get to it, but it was decided to ensure CSRF on those routes.

When I get to a computer I'll link ticket.

[beeonthego]

Hello,

appreciate the quick response. Before creating this issue, I have searched and read all open and closed issues with the keyword csrf, and haven't found anything similar. (maybe it has been deleted?) This is about CSRF checks AFTER users log in, not on login route. The mischief can be done with a link to the unprotected route, on issue/profile pages on the same domain.

If someone is already working on it, could you share the link to the issue and discussions?

If it is not the same issue, do you mind re-opening the issue?

Gitea (and gogs) has some unconventional use of http verbs, such as performing delete action on GET requests.

[techknowlogick]

It's hard to search for because the security researcher who reported it deleted their account, and because it included some sensitive details removed the details from the ticket. However the it's also been reported through the security@gitea.io email so the owners (and a couple maintainers) have the vulnerability details, and the ticket is kept open as a placeholder until it can be resolved.

I'm back at a computer now and the ticket is #4357

[beeonthego]

I actually have read that issue, which is about unprotected post routes in API. This issue is about the unprotected GET routes of UI in the browser. So I thought these are totally different issues. Do you mind reopening it for community comments so a fix can be decided and submitted soon?

The researcher (cezar??) brought my attention to those CSRF issues. I am especially concerned about them because our application is multi user environment. Not sure why the account has been deleted. I hope the researcher can test and verify the fixes if the researcher is still hanging around.

I have done something about token check in api routes, and have proposed to limit API access to token only in reqToken handler. I will prepare a pull request on that since it is a separate issue. One of the maintainers mentioned that a few API GET routes are used in the user/repo search in dashboard. these routes do not have reqToken handler, and will continue to work without token. But in the long run I think it is a good practice to always supply a token when making requests to api routes. People make less mistakes when the logic is simple.

I have a small team and limited resources, and prefer not to maintain a separate repo, or manually resolve the merge conflicts.

[beeonthego]

Just in case someone will be reading this issue in the future, we have opted for rewriting the code and using a separate key and token exclusively for vulnerable GET routes. it is cheap and fast to generate and verify a token stored in another cookie. The consequences of losing CSRF token for POST requests are huge. It is a risk not worth taking.

[beeonthego]

@cezar97 Thanks for the insight, and welcome back! I am glad you are still here. If you can create a new account using the username of deleted account, you should keep this one. Otherwise another person might use it and pretend to be you.

I have read issue #4337 but couldn't find it when I created this issue to prepare a PR. The content has been deleted and it does not contain csrf or xsrf keyword now. The credit goes to you for reporting the csrf vulnerabilities on GET routes.

Thank you for being patient and persistent. I think Gitea as a community should have responded faster to address the security issues (I am one of the users and I am guilty of this too). Some fixes are easy, for example my PR #4840 only has 3 lines of code. It will probably only take a few minutes for someone already familiar with the history and the code of this project.

PR #4840 needs a change in access policy and test suites. It has failed some of the tests because the tests assume access to API routes with cookie. The same vulnerability exists in Gogs API routes. I have submitted the same PR to Gogs (matching gogs var and file name) and the PR has passed all tests there. I will see which team of maintainers respond to the PR faster. Gitea maintainers may have forgotten the pain of waiting for PR to be merged, the same reason they have forked Gitea from Gogs.

[beeonthego]

@techknowlogick Is someone else already working on these GET routes or changing them to POST routes? If someone is already working on it, I will focus on something else.

[beeonthego]

@cezar97 In one of your issues deleted, you mentioned about the possibility of theft in CSRF tokens designed for POST routes. Developers have to send the tokens to the browser as page content in one way or another ( in http head or body content, or inject into javascript code). When a victim visits a malicious external link, the attacker can use a script to send a GET request to the server, parse the response and obtain the token. Browsers will send the victim's cookie automatically, even if the attacker can not access the cookie. So these tokens are no secrets anyway. It is the browser's same origin policy that prevents the attacker from sending POST requests using stolen csrf token and cookie. What do you think?

For this reason, a CSRF token on GET routes has to be different one, if the page contains important information and can not change to a POST route.

Appreciate your wisdom in this area.

[beeonthego]

Below are my notes on this issue. Hopefully it is helpful to someone reading this issue.

Upon careful examination of the issue, for a CSRF token to work effectively on GET routes, all GET routes of the member area must have a CSRF token. The reason is that if any route is not covered, an attacker can send a GET request to this route using victim's cookie, parse the response, and get all the tokens for GET routes and POST routes.

The current implementation of CSRF token is useless and gives false sense of security, [edit: if Access-Control-Allow-Origin has been set up incorrectly. ]. The real defense is in correct CORS header setting, browser's same origin security policy, and http-only cookie setting.

POST route tokens should work together with GET route tokens to guard against XSS. If there is a POST route token on the page, the page must be protected with a token in the query string.

The routes will have to be POST if they perform some actions and change state. This is the best strategy even if it means breaking changes.

For the same reason, member areas should not display application tokens or other critical information after creating the token. Malicious scripts have unrestricted access to GET routes in members area, using victim's cookie, [edit: if Access-Control-Allow-Origin has been set up incorrectly. ]

Some routes have to be GET routes, for example the redirect URL after successful authentication. If there is important information on this page, append a token to this url before redirect, then verify this token on the server before rendering this page. The token must appear only once.

[beeonthego]

@cezar97 Thank you for the insight. You have been extremely helpful. The discussion is getting deep now. I hope this thread will help someone to understand and implement CSRF properly in their own app in the future.

Regarding cross site GET requests, browsers actually DO SEND the cookie along with the request even if the resource is on another host. I have tested this on Chrome for Mac Version 68.0.3440.106. The developer console displays a warning icon and message "Provisional headers are shown", and it DOES NOT show all the request headers. This is the same whether browser caching is on or off. However when I examine the request headers on nginx proxy, the cookies ARE clearly there in the cross site GET request! Other browsers are probably doing the same. If you are curious, you can test different browsers, check the request headers on server, and share the results.

It seems that browsers do not distinguish whether a GET request is made by entering a link in address bar, clicking on browse history/bookmark, or is initiated from a page on another site. A browser checks CORS policy, and if the request is allowed, it sends cookie along with the request to fetch resources. This is for a better UX.

The implication is that when a victim visits a malicious external site, the site have full access to all the resources available on GET routes. Browsers send the cookie automatically. The malicious site does not have direct access to cookie, but it can take advantage of browsers' behaviour, and steal information or valuable/private code. [edit: modern browsers do not expose the response of certain content type to JavaScript, but json /jsonp may not be covered, and old browsers may not implement this. The api routes accept cookie and returns JSON response, are vulnerable to abuse, and thus need priority fix.]

The only defense is to have a token on all GET routes too, no matter whether the routes perform action or not. [edit: make sure Access-Control-Allow-Origin header is set up properly and restricts access from trusted host only. It is dangerous to set it to a wildcard * and allow access from any hosts. ]

That is why most internet banking sites have ugly URL with long query strings.

Thank you for the comments on browse history and bookmarks. These are good points and I will think about how to handle the response when a token is missing or expired.

[beeonthego]

@cezar97 Thank you very much for the patience and detailed explanations. I do have a better understanding now.

HTML tags of embedded resources (such as img, style, script etc) are not covered by same origin policy. For these tags, browsers send cookies along with cross site GET request without asking. These are the GET routes I referred to before.

You are right, GET routes should not perform action or change state. Changing the routes to POST involves a lot of changes in the front end JavaScript, the templates, and test suites.

Internally we have modified go-macaron/csrf module in the vendor folder and context module, and added an extra token on these GET routes in templates for a quick fix. I will upload the go-macaron/csrf module to my repo, just in case someone will be interested in fixing their app too.

There won't be a PR as eventually the routes should be changed to POST. Changes in vendor folder may never get merged in upstream repo, and thus a PR will never pass the dependency tests.

[Siesh1oo]

@beeonthego : not sure if you have seen my comment in the other issue, but you can do minimally intrusive fixes using jQuery by replacing the GET url with for example

## /action/logout endpoint changed to POST in router, now replace in HTML template:
<a href="action/logout">Logout</a>
## with:
<a href="javascript:$.post('action/logout', { _csrf : '{{.CsrfToken}}' })">Logout</a>
## (assuming the template processor is replacing the CsrfToken variable with the actual token)

This way, a PR would only need to touch router definitions and the HTML render template files.

[lafriks]

@beeonthego but than on the other hand logout will not work without javascript (some people do use gitea without js)

[techknowlogick]

or you could do /logout?csrf=123213123131 and on the logout route if the csrf is not present then present a page that has the logout link with the csrf

[beeonthego]

@Siesh1oo Thanks for sharing the trick. I have read about it in your comments in another issue.

No matter how to implement it, people using custom templates will have to update templates manually. It is a breaking change.

In our app we have custom oAuth2 redirect routes (GET) and have to protect with a token in query string. To reduce the risk of exposing the token by referrer, we use a separate short-life token for such GET requests only, and render a normal page without protected content if the token is missing or expired.

I have uploaded the modification to go-macaron/csrf module under this branch
https://github.com/beeonthego/csrf/tree/extra-token-on-get-action-routes

I am not sure how to proceed. Changes in vendor folder may never be merged in upstream repo.

[Siesh1oo]

@techknowlogick : Technically possible, but be sure to understand the caveats https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet#Disclosure_of_Token_in_URL

[Siesh1oo]

@lafriks wrote:

@beeonthego but than on the other hand logout will not work without javascript (some people do use gitea without js)

When Javascript is disabled, gitea does not even show the logout button (nor any other functional button) -- is gitea supposed to be functional without javascript?

[lafriks]

Oh, you are right logout is not visible :) Than yeah we can do that with JavaScript. I would prefer form with post tho