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

CSRF protection for login, logout, and user registration. #4365

Merged
merged 1 commit into from Aug 5, 2017

Conversation

Projects
None yet
5 participants
@jmchilton
Copy link
Member

commented Aug 1, 2017

The approach is to use per-session CSRF tokens - this avoids many complications related to per-form tokens. We generate a sequence of hashes based on session IDs that doesn't follow the same pattern as normal database API IDs by supplying a "kind" parameter to encode_id (we use the same pattern for securing the job files API for Pulsar).

More information on CSRF for login and logout - https://security.stackexchange.com/a/62797 and CSRF in general including per-session tokens (https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet#General_Recommendation%3a_Synchronizer_Token_Pattern).

Ping @erasche, @manabuishii.

@manabuishii

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2017

I think this looks good .

@manabuishii

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2017

This sections describes, potential problem of using GET(URL) .

https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet#Disclosure_of_Token_in_URL

/loglout just logout confirmation page and
it has hidden field for csrf token and using POST method is more safer.

@jmchilton

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2017

Thanks for the link - this is a subtle point about GETs.

CSRF tokens in GET requests are potentially leaked at several locations: browser history, HTTP log files, network appliances that make a point to log the first line of an HTTP request, and Referer headers if the protected site links to an external site.

In this case however, that token is only useful until that that logout link is clicked - at that point it is no longer sensitive - or at least we are assuming it is no longer sensitive. I guess I understand the paranoia of not wanting these token to leak in general for GETs but for logouts specifically I think it is okay?

That said - the logout operation really should be a POST REST-fully speaking anyway. Here is a good line:

For these cases, attempting to retrofit this pattern in existing applications requires significant development time and cost, and as a temporary measure it may be better to pass CSRF tokens in the URL. Once the application has been fixed to respond to HTTP GET and POST verbs correctly, CSRF tokens for GET requests should be turned off.

If we spend more time on CSRF I would like to apply it to say tool shed installation - because that is an entry point that provides shell access to the system - and then next maybe to password modification forms, user and role creation, etc... before reworking logout. How about a create an issue and we track this for longer term development.

@jmchilton jmchilton referenced this pull request Aug 2, 2017

Open

More CSRF Protection #4367

0 of 7 tasks complete
@erasche

This comment has been minimized.

Copy link
Member

commented Aug 2, 2017

I'm used to CSRF tokens changing after every single request. If I'm reading the code correctly, this will only generate one per session? This will give us some protection, but probably isn't perfect long term. 👍 on this though, you have a task list for going forward and we can look into per-form / per-request csrf tokens going forward maybe. (Then again, as we move more and more to api-only, this becomes less of an issue.)

@jmchilton

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2017

@erasche https://security.stackexchange.com/questions/22903/why-refresh-csrf-token-per-form-request

For the reasons already discussed, it is not necessary to generate a new token per request. It brings almost zero security advantage, and it costs you in terms of usability: with only one token valid at once, the user will not be able to navigate the webapp normally. For example if they hit the 'back' button and submit the form with new values, the submission will fail, and likely greet them with some hostile error message. If they try to open a resource in a second tab, they'll find the session randomly breaks in one or both tabs. It is usually not worth maiming your application's usability to satisfy this pointless requirement

I learned about CSRF in the context of per-request tokens also - but in practice it just wouldn't be workable in Galaxy or any single page app I don't think. I think long term per-session tokens are the way to go.

@jmchilton

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2017

Then again, as we move more and more to api-only, this becomes less of an issue.

I don't know that this mitigates anything - as long as we are using the session cookie to authenticate and GET or POST to a page that changes Galaxy state - we should be using per-session CSRF tokens - regardless of if these are API endpoints or more traditional endpoints. I think continuing to use the session cookie (with or without CSRF protection) is a lot more secure than sticking the user's API key in the web app and exposing that extra places also.

So far this CSRF protection is used as needed in a couple endpoints - I think longer term it would be good for the same logic that determines if an API key is present or checks the session cookie if not should be extended to just always ensure there is a CSRF token if there is no API key - but it would require retrofitting dozens of forms probably.

@erasche

This comment has been minimized.

Copy link
Member

commented Aug 2, 2017

yeah, ok, even OWASP is fine with per-session csrf tokens. I'm a bit out of date apparnetly.

I don't know that this mitigates anything - as long as we are using the session cookie to authenticate and GET or POST to a page that changes Galaxy state - we should be using per-session CSRF tokens - regardless of if these are API endpoints or more traditional endpoints. I think continuing to use the session cookie (with or without CSRF protection) is a lot more secure than sticking the user's API key in the web app and exposing that extra places also.

Yes, you're right here. Just my hope that we move to per-session authentication tokens in headers. This would expose us more to XSS issues, but remove the CSRF risks.

So far this CSRF protection is used as needed in a couple endpoints - I think longer term it would be good for the same logic that determines if an API key is present or checks the session cookie if not should be extended to just always ensure there is a CSRF token if there is no API key - but it would require retrofitting dozens of forms probably.

sounds good to me.

@martenson martenson changed the title CSRF protection for login, logout, and user registeration. CSRF protection for login, logout, and user registration. Aug 2, 2017

@martenson

This comment has been minimized.

Copy link
Member

commented Aug 2, 2017

Failing tests look related.

@jmchilton

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2017

@martenson Yeah thanks for spotting that - I've pushed in a fix for that. I'll rebase and re-pack if the tests pass now.

@erasche

This comment has been minimized.

Copy link
Member

commented Aug 4, 2017

@jmchilton if you can fix the conflicts I'll merge this today.

CSRF protection for login, logout, and user registeration.
The approach is to use per-session CSRF tokens - this avoids many complications related to per-form tokens. We generate a sequence of hashes based on session IDs that doesn't follow the same pattern as normal database API IDs by supplying a "kind" parameter to encode_id (we use the same pattern for securing the job files API for Pulsar).

@jmchilton jmchilton force-pushed the jmchilton:csrf branch from 0af0ec0 to b17091a Aug 4, 2017

@jmchilton

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2017

@erasche Rebased and un-conflicted.

@erasche

This comment has been minimized.

Copy link
Member

commented Aug 5, 2017

Rant py27lint locally and looks good.

@erasche erasche merged commit 4effd7d into galaxyproject:dev Aug 5, 2017

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
api test Build finished. 279 tests run, 0 skipped, 0 failed.
Details
framework test Build finished. 150 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 37 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
@erasche

This comment has been minimized.

Copy link
Member

commented Aug 5, 2017

@mvdbeek I received an email, did you remove your comment because it wasn't a bug?

@mvdbeek

This comment has been minimized.

Copy link
Member

commented Aug 5, 2017

Yes, sorry for the noise.

@erasche

This comment has been minimized.

Copy link
Member

commented Aug 5, 2017

No worries! I was quite concerned we'd missed something. Glad to hear false positive :)

@jmchilton jmchilton deleted the jmchilton:csrf branch Aug 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.