Roundup Tracker - Issues

Issue 2550690

classification
Title: Inadequate CSRF protection
Type: security Severity: normal
Components: Web interface Versions: 1.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: rouilj Nosy List: ber, eadler, joseph_myers, rouilj, schlatterbeck
Priority: high Keywords:

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:51rouiljsetstatus: open -> closed
resolution: fixed
messages: + msg5953
2017-03-26 20:57:03joseph_myerssetmessages: + msg5948
2017-03-26 03:36:20rouiljsetmessages: + msg5947
2017-03-19 23:07:51rouiljsetmessages: + msg5946
2017-03-19 21:52:03rouiljsetmessages: + msg5945
2017-03-19 17:04:25rouiljsetstatus: new -> open
assignee: rouilj
messages: + msg5944
2016-07-13 00:22:33rouiljsetmessages: + msg5843
2013-10-09 20:31:12rouiljsetnosy: + rouilj
messages: + msg4935
2013-04-08 22:07:19eadlersetnosy: + eadler
2013-02-04 14:53:18bersetmessages: + msg4791
2013-02-04 14:01:29joseph_myerssetmessages: + msg4790
2013-02-04 10:13:25schlatterbecksetmessages: + msg4788
2013-02-04 09:43:44bersetpriority: high
nosy: + schlatterbeck, ber
messages: + msg4786
2011-02-22 20:15:53joseph_myerscreate