search

isaacphysics / isaac-app

Isaac Physics historic front-end project, based on Angular JS
https://isaacphysics.org
MIT License
6 stars 1 forks source link

Cross-Origin Protection #892

Open jsharkey13 opened 6 years ago

jsharkey13 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

(That CSRF paper, for reference: http://ieeexplore.ieee.org/stamp/stamp.jsp?tp=&arnumber=7961990&isnumber=7961936&tag=1)