Tornado: support samesite xsrf cookie

[minrk]

A somewhat more recent approach to CSRF is setting samesite=strict on a cookie. SameSite is handy because it allows the browser to enforce the same-site-ness of a request, without needing application code to send the token separately. SameSite can be used almost everywhere, but not quite. It would be handy to support this strategy of csrf checking (cookie is present and samesite=strict) as equivalent to the current check of token delivery by both cookie and argument.

If I wanted to adopt this strategy now, do you suppose it would be preferable to:

1. create a new samesite cookie for this purpose and implement my own handling, or
2. modify tornado's xsrf handling to attempt to apply samesite restrictions on the cookie

?

Unfortunately, only Python 3.8 has support for setting samesite cookies in the stdlib. This is easily patched (Morsel._reserved['samesite'] = 'SameSite'), but needing a patch is never great.

[bdarnell]

Samesite cookies are new to me, but here are my initial thoughts:

If you send a cookie with samesite=strict to a browser that doesn't support it, the attribute gets silently ignored, right? And there's no way to tell on the server side whether the samesite flag is supported/in use (except for user-agent sniffing). So there's not really a good way to use samesite where available and fall back to the old way.

If you can eliminate the legacy fallback, things get a lot simpler: you can avoid all the xsrf_form_html stuff. In fact, I don't know if you need any explicit XSRF protection at all: just mark your login cookie as samesite, and then you're done. So I think if you're willing to rely on samesite being available, you can just disable tornado's xsrf protection and use samesite instead.

If you're not willing to give up on users of older browsers that don't support samesite, is it worthwhile to support two modes so you can use samesite where available? I'm not sure that it is - if you're not willing to rely solely on samesite I'd probably just stick with old-style XSRF protection. (You can set samesite in xsrf_cookie_kwargs for a little defense-in-depth, but you still need the template html and other parts of application support).

[tugceakin]

This will cause issues after Chrome 80 upgrade in cases where tornado server is running inside a cross origin iframe.

A cookie associated with a cross-site resource at was set without the SameSite attribute. It has been blocked, as Chrome now only delivers cookies with cross-site requests if they are set with SameSite=None and Secure. You can review cookies in developer tools under Application>Storage>Cookies and see more details at https://www.chromestatus.com/feature/5088147346030592 and https://www.chromestatus.com/feature/5633521622188032.

[bdarnell]

If your application requires the use of the SameSite attribute, I think you'll need to either upgrade to python 3.8 or apply the Morsel monkey-patch yourself. I don't think it would be appropriate for Tornado to do this kind of monkey-patch.

If SameSite cookies are becoming more important, consider lobbying python-dev to backport this change to older versions of python, or see if someone will create and maintain a copy of http.cookies as a standalone package (similar to what was once done with unittest2 and backports_abc).

It's also possible for tornado to move away from http.cookies entirely if there is an alternative cookie package available (or as a last resort reimplement the relevant RFCs), But that's a much larger change and wouldn't happen quickly.

[tugceakin]

Thanks Ben. We ended up monkey-patching our student virtual environments at Udacity. Upgrading to 3.8 was not an immediate option, because some of our courses are taught in Python 3.6 which requires a larger scale of content updates.

[jbvsmo]

Or maybe Tornado should just implement its own Morsel class since this is simple and defined on a RFC. This class could also be extensible to support future names instead of how Python handles it. There is nothing special, to be honest, in this class that would require it to enforce a small collection of names

[bdarnell]

There is nothing special, to be honest, in this class that would require it to enforce a small collection of names

I think there are good reasons to enforce that names are on a small list - it helps catch mistakes earlier, and it can handle special formatting requirements (notice that there is special per-field handling of integer arguments to the expires and max-age fields, and the secure and httponly settings are special because they're not encoded as a key-value pair.

The problem is that the web platform evolves at its own pace and it doesn't make sense for these limits to be baked into slow-moving language releases. There must either be a way to say "trust me, i want to use this flag that's not on the list" or the list must be updated more frequently even if this looks like "adding features to old releases".

Moving away from the standard library's cookie module makes sense, but somebody has to do the work to make that happen. And given the non-trivial security risks of anything touching cookie parsing, I wouldn't count on that being ready for production usage by the time the chrome policy change goes into effect, so monkey-patching Morsel is likely to be the most expedient approach.

[jbvsmo]

Thanks @bdarnell, this is actually a good argument. Well, I had monkey patched the std lib anyway but would be good if this can go through some roadmap to be part of tornado at some point.

[stalkerg]

@bdarnell I suppose we can do same as Bottle guys did
https://github.com/bottlepy/bottle/pull/983
or at least we should notify about it in the documentation.

[enolfc]

2911 is related to this. While one can set the SameSite when using py3.8, the cookies are not cleared unless this is also specified.