Skip to content
This repository was archived by the owner on May 9, 2022. It is now read-only.

Cross-Origin Protection #892

Open
jsharkey13 opened this issue Oct 17, 2017 · 4 comments
Open

Cross-Origin Protection #892

jsharkey13 opened this issue Oct 17, 2017 · 4 comments

Comments

@jsharkey13
Copy link
Member

jsharkey13 commented Oct 17, 2017

After adding logging of Referer and Origin headers, it is already clear that many browsers do not add Origin headers to same-origin POST requests.

The original authors of the RFC that led to the header suggest that Origin headers which are missing or null, or on an allowed whitelist, should be treated as acceptable; but that requests with a mismatched Origin should be blocked. This affords users with a modern safe browser that does send the headers properly a better degree of protection from CSRF and XSS attacks. We should respond to requests with invalid origins with the CrossSiteRequestForgeryException and a 403 Forbidden.

I don't know if we also want to use the Referer header in cases where the POST has no Origin? And by POST I really mean all state changing requests.

This obviously affords no protection against scripts outside of browsers where headers can be spoofed, but that's not CSRF or XSS.

@jsharkey13
Copy link
Member Author

Raising an exception would make a mess of the code, and require checks in many places. I'm going to do what we do if the cookie has been edited or altered by hand and is invalid and just return no user, which just returns a 401 Unauthorized and is at least consistent with the explicit CSRF checks on SSO stuff.

jsharkey13 added a commit to isaacphysics/isaac-api that referenced this issue Oct 18, 2017
# We rely on "secure;HttpOnly" cookies to prevent cross-site GET requests, and use
# this Origin header checking to protect against cross-site POST and DELETE. Browsers
# will prevent XHR JSON requests by using a pre-flight check, but form POSTs definitely
# work and can be protected against this way.
# Relevant to isaacphysics/isaac-app#892
@jsharkey13
Copy link
Member Author

We also don't protect registration this way, since it's not an authenticated request. I don't know how best to extract the checking code; to check authenticated requests it wants to be in UserAuthenticationManager.java as it is now, but registration requests never touch this class (only UsersFacade.java and UserAccountManager.java). I could make it into a utility class, like RequestIPExtractor.java became, but I think it needs to throw the CrossSiteRequestForgeryException which is in the [...].auth.exceptions subpackage . . .

@mlt47
Copy link
Contributor

mlt47 commented Oct 19, 2017

As your example and the paper last week mentioned that Cross Site Request Forgery can happen pre-auth, I would say it is sensible to move the Exception form the auth.exceptions subpackage (to which one I don't know off the top of my head).

@jsharkey13
Copy link
Member Author

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants