Bugzilla – Full Text Bug Listing
| Summary: | Orion should provide protection from cross-site request forgery | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Erwin Margewitsch <erwin.margewitsch> | ||||
| Component: | Server | Assignee: | Erwin Margewitsch <erwin.margewitsch> | ||||
| Status: | RESOLVED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | ahunter.eclipse, john.arthorne, mamacdon, matthias.sohn, Michael_Rennie, simon_kaegi | ||||
| Version: | unspecified | Flags: | ahunter.eclipse:
iplog+
|
||||
| Target Milestone: | 7.0 | ||||||
| Hardware: | PC | ||||||
| OS: | Mac OS X | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Erwin Margewitsch
Change in the server: https://git.eclipse.org/r/#/c/26730/ Change in the client: https://git.eclipse.org/r/26742 Very cool, thanks for the contribution Erwin! I uploaded new patchsets which are implementing the double submit cookie approach [1] [1]: https://www.owasp.org/index.php/Cross-Site_Request_Forgery_%28CSRF%29_Prevention_Cheat_Sheet#Double_Submit_Cookies Sorry Erwin -- I ran out of time to take this further. Moving to 7.0 (In reply to Simon Kaegi from comment #4) > Sorry Erwin -- I ran out of time to take this further. Moving to 7.0 Do not forget to review this change in Gerrit so we can cleanup the open list for Orion. (In reply to Anthony Hunter from comment #5) > (In reply to Simon Kaegi from comment #4) > > Sorry Erwin -- I ran out of time to take this further. Moving to 7.0 > > Do not forget to review this change in Gerrit so we can cleanup the open > list for Orion. I also rebased and retested the changes on the current master branch. Will the proposed change make it into 7.0? With the newest patchset on the server the XSRFProtectionFilter can be disabled with a configuration property. Change in the server: https://git.eclipse.org/r/#/c/26730/ Please review, I tested Erwin's changes and all looks good for me but we'd like another opinion from one of the Orion gurus. We pushed this fix to master but now users are complaining that things are broken on http://orion.eclipse.org when we pushed the build containing this fix. There are tons of errors in the logs: ERROR o.e.o.s.s.XSRFPreventionFilter - PUT /site/insoo-A on behalf of user 'insoo': missing CSRF token in header. Not sure we are ready to push these changes if there is client adoption required. Self hosting is broken because of this change. You get the error: "User name not specified" (In reply to Anthony Hunter from comment #10) > We pushed this fix to master but now users are complaining that things are > broken on http://orion.eclipse.org when we pushed the build containing this > fix. > > There are tons of errors in the logs: > > ERROR o.e.o.s.s.XSRFPreventionFilter - PUT /site/insoo-A on behalf of user > 'insoo': missing CSRF token in header. > > Not sure we are ready to push these changes if there is client adoption > required. Where can I find the logs? Created attachment 248017 [details]
log file from orion.eclipse.org
Here is the log file from orion.eclipse.org
This change also breaks all the server JUnit tests that use Basic authentication and not the Form based authentication used on orion.eclipse.org. 355 out of 511 tests fail. (In reply to Anthony Hunter from comment #11) > Self hosting is broken because of this change. > > You get the error: > > "User name not specified" When XSRF filter checks the request against the endpoint and exception lists, it didn't account for the fact that in the self hosting case, the path name contains some additional segments: > /hosted/foo/login > /hosted/foo/login/form > /hosted/foo/login/canaddusers > ... I added a check, so those errors should be gone now: http://git.eclipse.org/c/orion/org.eclipse.orion.server.git/commit/?id=d22a780 (In reply to Erwin Margewitsch from comment #8) > With the newest patchset on the server the XSRFProtectionFilter can be > disabled with a configuration property. > > Change in the server: https://git.eclipse.org/r/#/c/26730/ Missed this comment. I have added this property to to the tests so the JUnits will pass: http://git.eclipse.org/c/orion/org.eclipse.orion.server.git/commit/?id=da70ba0d52484ec8dc4367a1b97708d6c882707b One more change: in some servers (*cough* WebSphere *cough*) getCookies() may return null, so we need a check to prevent NPE http://git.eclipse.org/c/orion/org.eclipse.orion.server.git/commit/?id=b8a0469 (In reply to Mark Macdonald from comment #17) > One more change: in some servers (*cough* WebSphere *cough*) getCookies() > may return null, so we need a check to prevent NPE > http://git.eclipse.org/c/orion/org.eclipse.orion.server.git/commit/ > ?id=b8a0469 This NPE problem also occurs when running Orion from a WAR on Tomcat. I think we can close this bug again. Marking RESOLVED. |