Issue 2550690
Created on 2011-02-22 20:15 by joseph_myers, last changed 2017-04-06 01:58 by rouilj.
Messages | |||
---|---|---|---|
msg4246 | Author: [hidden] (joseph_myers) | Date: 2011-02-22 20:15 | |
Roundup's web interface checks that requests modifying the database come via HTTP POST. This is not sufficient protection against cross-site request forgery attacks; a user's browser can easily be induced by a hostile site to send an arbitrary POST request, through Javascript or otherwise. In addition to the POST check, all those actions checking for POST should also check for a secret token in a hidden form field, linked in the database to the session ID. (Because of incompatibility with existing templates it may not be possible to do this unconditionally, but I think security against CSRF should be the default.) The POST check is still useful, as it makes it unlikely for the secret token to appear in URLs (since it wouldn't work there) and URLs are liable to leak (in Referers etc.). It's useful for the secret token to be a separate random token rather than a copy of the session ID, so that if the contents of a form are leaked (e.g. if the user sends of copy of the page to someone) that doesn't by itself enable impersonating the user. |
|||
msg4786 | Author: [hidden] (ber) | Date: 2013-02-04 09:43 | |
Joseph, thanks for creating the issue. Can you point to a good explanation how this protection can work? Thanks, Bernhard |
|||
msg4788 | Author: [hidden] (schlatterbeck) | Date: 2013-02-04 10:13 | |
On Mon, Feb 04, 2013 at 09:43:44AM +0000, Bernhard Reiter wrote: Thanks for putting me on the nosy list. I think I can offer some guidance on how to implement this but currently don't have too much time myself... And I agree this should be high prio. Ralf |
|||
msg4790 | Author: [hidden] (joseph_myers) | Date: 2013-02-04 14:01 | |
https://en.wikipedia.org/wiki/Cross-site_request_forgery#Prevention https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet (for example) discuss various approaches used to protect against CSRF. For Roundup I still think the right approach is a secret token used in all forms (and it should not be possible to compute the session ID from this token, so that having a leaked copy of a form doesn't allow a third party to generate a cookie to impersonate the user). |
|||
msg4791 | Author: [hidden] (ber) | Date: 2013-02-04 14:53 | |
Joseph, thanks for the hints! |
|||
msg4935 | Author: [hidden] (rouilj) | Date: 2013-10-09 20:31 | |
Also note that the CSRF token may need to be a single use token. So as soon as a post is made with the token, it gets masked/rewritten and is no longer valid. This protects again BREACH types of attacks against the token. http://breachattack.com/ which recommends the following mitigation methods: Disabling HTTP compression Separating secrets from user input Randomizing secrets per request Masking secrets (effectively randomizing by XORing with a random secret per request) Protecting vulnerable pages with CSRF Length hiding (by adding random number of bytes to the responses) Rate-limiting the requests |
|||
msg5843 | Author: [hidden] (rouilj) | Date: 2016-07-13 00:22 | |
I wonder if we could use the one time key mechanism used for password resets here as well. For each form we generate a template function that produces: <hidden name="otk" value="....." > and change RetireAction, RestoreAction, EditCSVAction, EditItemAction::handler, NewItemAction::handler, RegisterActon::hander, (maybe SearchAction) to verify the otk against what is stored int he session database. Would this basic idea work? |
|||
msg5944 | Author: [hidden] (rouilj) | Date: 2017-03-19 17:04 | |
The first attempts to fix this have been pushed to the repo. I still have to invalidate the token if it is used in a get request so the token can't be replayed before it times out. However people should not be using the token in a get request. But I also know people will. (I may even have accidently made that mistake converting the roundup supplied trackers.) see https://sourceforge.net/p/roundup/code/ci/47bd81998ddc9de40f8a0f97f90b84863a21b93f/ for the final csrf patch. |
|||
msg5945 | Author: [hidden] (rouilj) | Date: 2017-03-19 21:52 | |
In message <1489943065.8.0.450565123539.issue2550690@psf.upfronthosting.co.za>, John Rouillard writes: >I still have to invalidate the token if it is used in a >get request so the token can't be replayed before it >times out. > >However people should not be using the token in a get request. >But I also know people will. Any use of a token now destroys the token when when not using POST, PUT or DELETE. So GET, HEAD etc. all invalidate the used token. Tests for replay attacks using the token with a POST or GET have been added to the suite. Also there was a bug in setting token lifetimes and purging expired tokens. That has been fixed as well and tests updated. |
|||
msg5946 | Author: [hidden] (rouilj) | Date: 2017-03-19 23:07 | |
I also implemented support for SameSite authentication cookies. Easier then the rest of the CSRF defenses. See CHANGES.txt and upgrading.txt. |
|||
msg5947 | Author: [hidden] (rouilj) | Date: 2017-03-26 03:36 | |
Joseph, have you had a chance to look at the changes? If implemented properly do they address your concerns? |
|||
msg5948 | Author: [hidden] (joseph_myers) | Date: 2017-03-26 20:57 | |
Without having reviewed the details of the implementation, the general approach seems reasonable. I don't expect to test the changes until after the next release is out. |
|||
msg5953 | Author: [hidden] (rouilj) | Date: 2017-04-06 01:58 | |
I released a new implementation of this using exceptions rather then returning booleans. This fixes the last known issues with my implementation by returning proper xmlrpc fault responses if a header check fails when using the xmlrpc interface. I have been testing it over the past week or so on my local trackers and it seems to be working as intended. Since it looks like nobody will be able to spend time testing this I am going to close it as done well enough for now. |
History | |||
---|---|---|---|
Date | User | Action | Args |
2017-04-06 01:58:51 | rouilj | set | status: open -> closed resolution: fixed messages: + msg5953 |
2017-03-26 20:57:03 | joseph_myers | set | messages: + msg5948 |
2017-03-26 03:36:20 | rouilj | set | messages: + msg5947 |
2017-03-19 23:07:51 | rouilj | set | messages: + msg5946 |
2017-03-19 21:52:03 | rouilj | set | messages: + msg5945 |
2017-03-19 17:04:25 | rouilj | set | status: new -> open assignee: rouilj messages: + msg5944 |
2016-07-13 00:22:33 | rouilj | set | messages: + msg5843 |
2013-10-09 20:31:12 | rouilj | set | nosy:
+ rouilj messages: + msg4935 |
2013-04-08 22:07:19 | eadler | set | nosy: + eadler |
2013-02-04 14:53:18 | ber | set | messages: + msg4791 |
2013-02-04 14:01:29 | joseph_myers | set | messages: + msg4790 |
2013-02-04 10:13:25 | schlatterbeck | set | messages: + msg4788 |
2013-02-04 09:43:44 | ber | set | priority: high nosy: + schlatterbeck, ber messages: + msg4786 |
2011-02-22 20:15:53 | joseph_myers | create |