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

Protection against CSRF added #22077

Closed
wants to merge 4 commits into from
Closed

Conversation

pboguslawski
Copy link
Contributor

Details sent to security@gitea.io in messages

Wed, 30 Nov 2022 12:47:33 +0100.
Fri, 2 Dec 2022 19:59:02 +0100

Author-Change-Id: IB#1129006

@yardenshoham yardenshoham added the topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! label Dec 11, 2022
@yardenshoham yardenshoham added this to the 1.19.0 milestone Dec 11, 2022
@@ -1,4 +1,4 @@
// Copyright 2021 The Gitea Authors. All rights reserved.
// Copyright 2022 The Gitea Authors. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, updating the copyright year is kinda useless nowadays, as using git is sufficient to establish who/when the work was done (which is why there is "the gitea authors" and not the detail). That's the opinion of the Linux Foundation ( https://www.linuxfoundation.org/blog/blog/copyright-notices-in-open-source-software-projects , the author is the Vice President of Compliance and Legal ). I also got the same answer from our legal department, even if I haven't been able to convince them to publish something outside our intranet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 21, 2022

// Allow requests not recognized as CSRF.
secFetchSite := strings.ToLower(req.Header.Get("Sec-Fetch-Site"))
if req.Method == "GET" || // GET must not be used for changing state (CSRF resistant).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it also cover HEAD ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added HEAD and OPTIONS also.

@zeripath
Copy link
Contributor

I actually have a feeling that we should not be allowing the ReverseProxy Auth on the API - it's the only consistent way of making the API work

zeripath added a commit to zeripath/gitea that referenced this pull request Dec 22, 2022
Since we changed the /api/v1/ routes to disallow session authentication we also
removed their reliance on CSRF. However, we left the ReverseProxy authentication
here - but this means that POSTs to the API are no longer protected by CSRF.

Now, ReverseProxy authentication is a kind of session authentication, and is
therefore inconsistent with the removal of session from the API.

This PR proposes that we simply remove the ReverseProxy authentication from the
API and therefore users of the API must explicitly use tokens or basic authentication.

Replace go-gitea#22077

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor

I don't fully understand why you have added another dependency library for the cors instead of making our current one work or replacing our current one.

@pboguslawski
Copy link
Contributor Author

pboguslawski commented Dec 22, 2022

I actually have a feeling that we should not be allowing the ReverseProxy Auth on the API - it's the only consistent way of making the API work

What inconsistency do you see in using same auth method (header auth from reverse proxy) for all API calls? Any risks?

I don't fully understand why you have added another dependency library for the cors instead of making our current one work or replacing our current one.

https://github.com/go-chi/cors does not provide OriginAllowed method and it seems even maintainer is not sure why this fork is still for. Existing CSRF code was not analyzed and left untouched; if this PR is accepted - existing token based CSRF stuff should be checked and (if is still required as separate line of defense) may be switched to github.com/rs/cors or removed (if not required when browsers compatible with fetch metadata headers will be required for gitea).

@zeripath
Copy link
Contributor

zeripath commented Dec 22, 2022

I actually have a feeling that we should not be allowing the ReverseProxy Auth on the API - it's the only consistent way of making the API work

What inconsistency you see in using one auth method for all API calls?

For every other authentication mechanism we now require that API calls are separately and explicitly authenticated - this makes ReverseProxy the odd one out.

I don't fully understand why you have added another dependency library for the cors instead of making our current one work or replacing our current one.

https://github.com/go-chi/cors does not provide OriginAllowed method and it seems even maintainer is not sure why this fork is still for. Existing CSRF code was not analyzed and left untouched; if this PR is accepted - existing token based CSRF stuff should be checked and (if is still required as separate line of defense) may be switched to github.com/rs/cors or removed (if not required when browsers compatible with fetch metadata headers will be required for gitea).

This PR means we now have TWO libraries adding CORS headers. We need to use one or the other not both. It's fine to say that we shouldn't be using the go-chi/cors fork but we then we need to not use it.

This PR doesn't appear to add CSRF token checking as far as I can see? I guess that also needs to be added?

@pboguslawski
Copy link
Contributor Author

pboguslawski commented Dec 22, 2022

For every other authentication mechanism we now require that API calls are separately and explicitly authenticated - this makes ReverseProxy the odd one out.

If account A can make change X using "web API" why shouldn't be allowed to make same change X using "API"? Why to "mess" with separate API areas and separate sets of auths for both? Any real risks?

If separation is required - why not to introduce user permissions to access given API area (i.e. "[x] access to web", "[x] access to API) as alternative to application tokens (which should be optional)?

This PR means we now have TWO libraries adding CORS headers.

Code in this PR does not handle CORS requests/reponses it only uses CORS stuff to decide if requests are valid during CSRF validation. If this PR is accepted - existing CORS may be switched to github.com/rs/cors to avoid using two CORS libs (should not hurt as temporary solution).

@zeripath
Copy link
Contributor

If account A can make change X using "web API" why shouldn't be allowed to make same change X using "API"? Why to "mess" with separate API areas and separate sets of auths for both? Any real risks?

I didn't complete agree with the decision to do this - but it was done. ReverseProxy currently stands as the odd one out.

The idea is that you have to authenticate explicitly for the API separately from the UI.

If separation is required - why not to introduce user permissions to access given API area (i.e. "[x] access to web", "[x] access to API) as alternative to application tokens (which should be optional)?

Or use tokens that we already have.

This PR means we now have TWO libraries adding CORS headers.

Code in this PR does not handle CORS requests/reponses it only uses CORS stuff to decide if requests are valid during CSRF validation. If this PR is accepted - existing CORS may be switched to github.com/rs/cors to avoid using two CORS libs (should not hurt as temporary solution).

That's not the way to do things, it won't get done and we'll end up with 2 slightly different CORS libraries. The go-chi/cors library should be replaced in this PR. I won't approve this PR without that although I'm not going to block it.

@lunny lunny closed this in #22219 Dec 27, 2022
lunny pushed a commit that referenced this pull request Dec 27, 2022
Since we changed the /api/v1/ routes to disallow session authentication
we also removed their reliance on CSRF. However, we left the
ReverseProxy authentication here - but this means that POSTs to the API
are no longer protected by CSRF.

Now, ReverseProxy authentication is a kind of session authentication,
and is therefore inconsistent with the removal of session from the API.

This PR proposes that we simply remove the ReverseProxy authentication
from the API and therefore users of the API must explicitly use tokens
or basic authentication.

Replace #22077
Close #22221 
Close #22077 

Signed-off-by: Andrew Thornton <art27@cantab.net>
lunny pushed a commit to lunny/gitea that referenced this pull request Dec 27, 2022
Since we changed the /api/v1/ routes to disallow session authentication
we also removed their reliance on CSRF. However, we left the
ReverseProxy authentication here - but this means that POSTs to the API
are no longer protected by CSRF.

Now, ReverseProxy authentication is a kind of session authentication,
and is therefore inconsistent with the removal of session from the API.

This PR proposes that we simply remove the ReverseProxy authentication
from the API and therefore users of the API must explicitly use tokens
or basic authentication.

Replace go-gitea#22077
Close go-gitea#22221 
Close go-gitea#22077 

Signed-off-by: Andrew Thornton <art27@cantab.net>
lunny pushed a commit to lunny/gitea that referenced this pull request Dec 27, 2022
Since we changed the /api/v1/ routes to disallow session authentication
we also removed their reliance on CSRF. However, we left the
ReverseProxy authentication here - but this means that POSTs to the API
are no longer protected by CSRF.

Now, ReverseProxy authentication is a kind of session authentication,
and is therefore inconsistent with the removal of session from the API.

This PR proposes that we simply remove the ReverseProxy authentication
from the API and therefore users of the API must explicitly use tokens
or basic authentication.

Replace go-gitea#22077
Close go-gitea#22221 
Close go-gitea#22077 

Signed-off-by: Andrew Thornton <art27@cantab.net>
KN4CK3R pushed a commit that referenced this pull request Dec 27, 2022
backport from #22219

Since we changed the /api/v1/ routes to disallow session authentication
we also removed their reliance on CSRF. However, we left the
ReverseProxy authentication here - but this means that POSTs to the API
are no longer protected by CSRF.

Now, ReverseProxy authentication is a kind of session authentication,
and is therefore inconsistent with the removal of session from the API.

This PR proposes that we simply remove the ReverseProxy authentication
from the API and therefore users of the API must explicitly use tokens
or basic authentication.

Replace #22077
Close #22221 
Close #22077 

Signed-off-by: Andrew Thornton <art27@cantab.net>

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: zeripath <art27@cantab.net>
lafriks pushed a commit that referenced this pull request Dec 30, 2022
backport #22219

Since we changed the /api/v1/ routes to disallow session authentication
we also removed their reliance on CSRF. However, we left the
ReverseProxy authentication here - but this means that POSTs to the API
are no longer protected by CSRF.

Now, ReverseProxy authentication is a kind of session authentication,
and is therefore inconsistent with the removal of session from the API.

This PR proposes that we simply remove the ReverseProxy authentication
from the API and therefore users of the API must explicitly use tokens
or basic authentication.

Replace #22077
Close #22221 
Close #22077 

Signed-off-by: Andrew Thornton <art27@cantab.net>

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: zeripath <art27@cantab.net>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. topic/security Something leaks user information or is otherwise vulnerable. Should be fixed!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants