Add CSRF protection

[rnewson]

If the request parameter csrf is set to true when successfully
acquiring a session cookie from _session an additional cookie
(Csrf-token) is returned. All requests that send this new cookie
must also send a header (X-Csrf-Token) with the same value. If the
cookie is sent and the header is missing or different, a 403 response
is generated.

Note that the CSRF token is signed by the server so tampering is
detected and also results in a 403 response.

closes COUCHDB-2762

[robertkowalski]

it would be cool to have an integration test that proves that it works and protects against regressions

[rnewson]

I was speaking with @kxepal and agreed to post the code so we could figure out how to test it.

[kxepal]

Can we have CSRF cookie name configurable to avoid possible collisions with other web services that also defined CSRF protection? I cannot name any that uses the same "Csrf-token" value (Django have "csrftoken", Pyramid - "csrf_token"), but collision probability rate is still could be high here.

Or may be use "CouchDB-CSRF-Token" name by default - unlikely it will cause any problems.

[rnewson]

CouchDB-CSRF and X-CouchDB-CSRF sound fine to me.

[kxepal]

mmm...so we here

1. Enable CSRF protection only by user request, not for the whole server. What means, that it will be easy to forget to activate that protection.
2. CSRF is not applied for Basic and OAuth authed users and those that authed using other methods as they don't POST /_session. They could only GET it, but it's not mandatory.

What are your thoughts about these cases?

[rnewson]

If we don't make it optional then every client will be broken until they add this custom header.

On basic auth, I tend to think of cookie auth being used by browsers/ui's and basic auth being used by programs. CSRF is only an issue for browsers; direct api interactions aren't vulnerable.

[rnewson]

I meant https://en.wikipedia.org/wiki/Cross-site_request_forgery#Cookie-to-Header_Token earlier, sorry for confusion.

[rnewson]

if we now understand the general mechanism, my remaining questions are;

1. unless the admin sets allow_persistent_cookies the x-couchdb-csrf cookie is only lost when the browser is restarted or cookies are manually cleared
2. if allow_persistent_cookies is true, the x-couchdb-csrf cookie will expire. the auth session cookie will expire at the same time, unless it's been renewed (which happens automatically), so this would seem to lock the user out eventually.

The core parts are sound, I think. The generation of the csrf cookie and the way we validate that the cookie and header match and are valid. What's not quite right is the way you get the csrf cookie in the first place.

Thoughts?

[rnewson]

I've revised this and removed the coupling with the session cookie.

A client that does not currently have a CouchDB-CSRF cookie can add X-CouchDB-CSRF: true to their request and will receive one. They must then send the cookie's value in the X-CouchDB-CSRF header for their request to succeed.

The CSRF token check occurs before authentication so you can acquire the CSRF token immediately (perhaps when fetching the welcome message from /).

The token will expire from time to time (same duration as the session cookie but without the automatic extension), so clients should do broadly this;

if (hasCookie("CouchDB-CSRF")) {
setRequestHeader("X-CouchDB-CSRF", cookieValue("CouchDB-CSRF");
} else {
setRequestHeader("X-CouchDB-CSRF", "true");
}

[kxepal]

I like new logic much more. Good improvements!

However, your questions are still valid: if CSRF cookie expires, how it will get prolonged? Especially, since auth cookie renews automagicaly when it ttl comes to the end.

[rnewson]

Because the client will be testing if they have a CSRF cookie (in order to know whether to set the CSRF header). It will vanish, so they will set X-CouchDB-CSRF:true on the next request to get a new one.

[kxepal]

I see. This could work.

[rnewson]

hm, I'm thinking of adding automatic refresh, like we do for session cookie, to prevent even that little gap where there's no CSRF token at expiration.

[kxepal]

I think such refresh could be useful and making CSRF tokens expire is even better.

[rnewson]

So I've added the refresh.When a cookie is more than halfway through its lifetime (which defaults to an hour) a replacement cookie is sent.

Additionally, if you send the CSRF header but not the cookie, that's a 403 Forbidden. This helps clients stay safe (they won't think that just sending the header and getting a non-error response means that they are protected by the CSRF mechanism).

[rnewson]

The list of tests we need;

1. a request with no cookie and with X-CouchDB-CSRF header set to true results in Set-Cookie response header for a cookie named CouchDB-CSRF.
2. a request that has a CouchDB-CSRF cookie but not X-CouchDB-CSRF request header is rejected with a 403.
3. a request that has a CouchDB-CSRF cookie and a X-CouchDB-CSRF request header, but that do not match each other, is rejected with a 403.
4. a request that has a CouchDB-CSRF cookie and a X-CouchDB-CSRF request header, that do match each other, is accepted.
5. a request that has an expired CouchDB-CSRF cookie and aX-CouchDB-CSRF` request header, that do match each other, is rejected with a 403.
6. a request that has no CouchDB-CSRF cookie and aX-CouchDB-CSRF` request header, that do match each other, is rejected with a 403.
7. a request that has a CouchDB-CSRF cookie and a X-CouchDB-CSRF request header, that do match each other, made more than halfway through the cookie's lifetime, is accepted and a new CouchDB-CSRF is returned (with a full lifetime).

[kxepal]

How about to catch a case if we receive malformed cookie here? While HTTP 500 because of badmatch or badarg is Erlang way, but more user friendly will be to be HTTP 400 here.

[rnewson]

sure.

[rnewson]

Another small enhancement, if the cookie is invalid (corrupt or if the secret value was changed) then we clear the cookie, allowing the client to recover without waiting for expiration (which might be 59 minutes away!)

[kxepal]

You do decode_cookie/1 in validate/1 above where you handle invalid result, but here you don't. Sure, it's not logically possible to get badmatch here, but may be move check expiration to validate function to avoid decoding same cookie twice?

[kxepal]

Sorry for double comment posting. GH eventually put my comments to your fork commit, not PR. Not sure how this happens. Also, check code for tabs: you have space-tabs mix a bit.

[rnewson]

turns out a lot of kits don't delete cookie if you set an expiration date in the past, so instead we send a fresh cookie if we get sent an invalid one.

[kxepal]

Is there a case for this branch? validate/1 calls in handle_request_int which will throw bad_request if Csrf is invalid while I see headers/1 is only called after it when response gets prepared where Csrf may be undefined or valid cookie as otherwise it will not pass validation. Or I miss something?

[rnewson]

Ok, I've stopped editing now, was just tidying a few things up.

[kxepal]

Perfect! Like latest changes about malformed and config section change. Have only notes about styling is you don't mind.

[kxepal]

It's the only place where header and cookie words not capitalized.

[rnewson]

@kxepal +1?

[kxepal]

@rnewson +1!