Skip to content

Commit

Permalink
web: Deprecate xsrf_cookies (and xsrf_token, etc)
Browse files Browse the repository at this point in the history
This feature is more invasive than using the samesite cookie attribute
but does not provide additional protection, so it is no longer
something that we should recommend.

Now that this feature is deprecated, the open issues related to it
will not be fixed (however, I intend to keep the current code around
indefinitely; there are no plans to remove it).

Closes tornadoweb#865
Closes tornadoweb#2573
Closes tornadoweb#3026
  • Loading branch information
bdarnell committed Feb 1, 2023
1 parent 0f8e10b commit 0558518
Show file tree
Hide file tree
Showing 16 changed files with 105 additions and 32 deletions.
5 changes: 2 additions & 3 deletions demos/blog/blog.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ def __init__(self, db):
template_path=os.path.join(os.path.dirname(__file__), "templates"),
static_path=os.path.join(os.path.dirname(__file__), "static"),
ui_modules={"Entry": EntryModule},
xsrf_cookies=True,
cookie_secret="__TODO:_GENERATE_YOUR_OWN_RANDOM_VALUE_HERE__",
login_url="/auth/login",
debug=True,
Expand Down Expand Up @@ -242,7 +241,7 @@ async def post(self):
self.get_argument("name"),
tornado.escape.to_unicode(hashed_password),
)
self.set_signed_cookie("blogdemo_user", str(author.id))
self.set_signed_cookie("blogdemo_user", str(author.id), samesite="lax")
self.redirect(self.get_argument("next", "/"))


Expand All @@ -269,7 +268,7 @@ async def post(self):
tornado.escape.utf8(author.hashed_password),
)
if password_equal:
self.set_signed_cookie("blogdemo_user", str(author.id))
self.set_signed_cookie("blogdemo_user", str(author.id), samesite="lax")
self.redirect(self.get_argument("next", "/"))
else:
self.render("login.html", error="incorrect password")
Expand Down
1 change: 0 additions & 1 deletion demos/blog/templates/compose.html
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
{% if entry %}
<input type="hidden" name="id" value="{{ entry.id }}"/>
{% end %}
{% module xsrf_form_html() %}
</form>
{% end %}

Expand Down
1 change: 0 additions & 1 deletion demos/blog/templates/create_author.html
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
Email: <input name="email" type="text"><br>
Name: <input name="name" type="text"><br>
Password: <input name="password" type="password"><br>
{% module xsrf_form_html() %}
<input type="submit">
</form>
{% end %}
1 change: 0 additions & 1 deletion demos/blog/templates/login.html
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
<form action="/auth/login" method="POST">
Email: <input name="email" type="text"><br>
Password: <input name="password" type="password"><br>
{% module xsrf_form_html() %}
<input type="submit">
</form>
{% end %}
1 change: 0 additions & 1 deletion demos/chat/chatdemo.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@ async def main():
cookie_secret="__TODO:_GENERATE_YOUR_OWN_RANDOM_VALUE_HERE__",
template_path=os.path.join(os.path.dirname(__file__), "templates"),
static_path=os.path.join(os.path.dirname(__file__), "static"),
xsrf_cookies=True,
debug=options.debug,
)
app.listen(options.port)
Expand Down
2 changes: 0 additions & 2 deletions demos/chat/static/chat.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ function getCookie(name) {
}

jQuery.postJSON = function(url, args, callback) {
args._xsrf = getCookie("_xsrf");
$.ajax({url: url, data: $.param(args), dataType: "text", type: "POST",
success: function(response) {
if (callback) callback(eval("(" + response + ")"));
Expand Down Expand Up @@ -90,7 +89,6 @@ var updater = {
cursor: null,

poll: function() {
var args = {"_xsrf": getCookie("_xsrf")};
if (updater.cursor) args.cursor = updater.cursor;
$.ajax({url: "/a/message/updates", type: "POST", dataType: "text",
data: $.param(args), success: updater.onSuccess,
Expand Down
1 change: 0 additions & 1 deletion demos/chat/templates/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
<td style="padding-left:5px">
<input type="submit" value="{{ _("Post") }}">
<input type="hidden" name="next" value="{{ request.path }}">
{% module xsrf_form_html() %}
</td>
</tr>
</table>
Expand Down
3 changes: 1 addition & 2 deletions demos/facebook/facebook.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ def __init__(self):
login_url="/auth/login",
template_path=os.path.join(os.path.dirname(__file__), "templates"),
static_path=os.path.join(os.path.dirname(__file__), "static"),
xsrf_cookies=True,
facebook_api_key=options.facebook_api_key,
facebook_secret=options.facebook_secret,
ui_modules={"Post": PostModule},
Expand Down Expand Up @@ -84,7 +83,7 @@ async def get(self):
client_secret=self.settings["facebook_secret"],
code=self.get_argument("code"),
)
self.set_signed_cookie("fbdemo_user", tornado.escape.json_encode(user))
self.set_signed_cookie("fbdemo_user", tornado.escape.json_encode(user), samesite="lax")
self.redirect(self.get_argument("next", "/"))
return
self.authorize_redirect(
Expand Down
1 change: 0 additions & 1 deletion demos/websocket/chatdemo.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ def __init__(self):
cookie_secret="__TODO:_GENERATE_YOUR_OWN_RANDOM_VALUE_HERE__",
template_path=os.path.join(os.path.dirname(__file__), "templates"),
static_path=os.path.join(os.path.dirname(__file__), "static"),
xsrf_cookies=True,
)
super().__init__(handlers, **settings)

Expand Down
1 change: 0 additions & 1 deletion demos/websocket/templates/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
<td style="padding-left:5px">
<input type="submit" value="{{ _("Post") }}">
<input type="hidden" name="next" value="{{ request.path }}">
{% module xsrf_form_html() %}
</td>
</tr>
</table>
Expand Down
1 change: 0 additions & 1 deletion docs/guide/running.rst
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,6 @@ You can serve static files from Tornado by specifying the
"static_path": os.path.join(os.path.dirname(__file__), "static"),
"cookie_secret": "__TODO:_GENERATE_YOUR_OWN_RANDOM_VALUE_HERE__",
"login_url": "/login",
"xsrf_cookies": True,
}
application = tornado.web.Application([
(r"/", MainHandler),
Expand Down
77 changes: 65 additions & 12 deletions docs/guide/security.rst
Original file line number Diff line number Diff line change
Expand Up @@ -207,23 +207,52 @@ the Google credentials in a cookie for later access:

See the `tornado.auth` module documentation for more details.

.. _xsrf:

Cross-site request forgery protection
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

`Cross-site request
forgery <http://en.wikipedia.org/wiki/Cross-site_request_forgery>`_, or
XSRF, is a common problem for personalized web applications.

The generally accepted solution to prevent XSRF is to cookie every user
with an unpredictable value and include that value as an additional
argument with every form submission on your site. If the cookie and the
value in the form submission do not match, then the request is likely
forged.

Tornado comes with built-in XSRF protection. To include it in your site,
include the application setting ``xsrf_cookies``:
XSRF or CSRF (Tornado uses the acronym XSRF for historical reasons, although
CSRF is generally more commonly used today), is a common problem for
personalized web applications.

The simplest solution to this problem is to set the ``samesite`` attribute to
either ``lax`` or ``strict`` on all cookies used to authenticate the user.
(``lax`` is generally fine for this purpose; ``strict`` would be appropriate
if your application may use the HTTP ``GET`` method for requests that
have side effects). Note that as of January 2023, ``lax`` mode is the default
for some browsers, but not all.

In the web security model, a "site" is broader than an "origin" or "host"
(``example.com`` is a site; ``a.example.com`` and ``b.example.com`` are
hosts within that site). While not technically "cross-site", attacks from
one host to another within a site are relevant in many contexts. Such attacks
may be called
`"same-site request forgery" <https://minimalblue.com/data/papers/USENIX21_can_i_take_your_subdomain.pdf>`_
or "related-domain attacks". Protection against same-site attacks requires
something stronger than a ``samesite`` cookie, such as the
`synchronizer token pattern <https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#synchronizer-token-pattern>`_.
This pattern is somewhat invasive and cannot be implemented within Tornado
as it is a fairly low-level framework, but it may be worth considering at
the application level.

.. _legacy-xsrf:

Legacy XSRF protection
^^^^^^^^^^^^^^^^^^^^^^

Prior to the introduction of the ``samesite`` cookie attribute, Tornado
included an implementation of the
`double-submit cookie <https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#double-submit-cookie>`_
pattern, called ``xsrf_cookies``. This feature provided protection against
XSRF attacks that is equivalent to that provided by the ``samesite`` cookie
attribute, but required modifications to the application to ensure that the
XSRF token was passed whenever it was needed. Since the ``samesite`` cookie
attribute provides equivalent protection with less work, the ``xsrf_cookies``
feature is deprecated.

To enable the ``xsrf_cookies`` feature, use the application setting
``xsrf_cookies=True``:

.. testcode::

Expand Down Expand Up @@ -288,6 +317,30 @@ However, if you support both cookie and non-cookie-based authentication,
it is important that XSRF protection be used whenever the current
request is authenticated with a cookie.

.. _xsrf-deprecation:

Migrating from legacy XSRF protection
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The ``samesite="lax"`` cookie attribute, when applied to *all* cookies used
for authentication, provides protection against XSRF attacks that is
equivalent to Tornado's ``xsrf_cookies`` feature, so that feature is now
deprecated.

If you have an application that uses Tornado's ``xsrf_cookies`` feature
and you want to migrate to the ``samesite`` cookie attribute, follow these
steps:

1. Pass ``samesite="lax"`` (or ``samesite="strict"``) to `.set_signed_cookie`
(or its deprecated alias ``set_secure_cookie``) every time you set a cookie
to be used for user authentication.
2. Deploy your application. Wait until all cookies that might have been set
without the ``samesite`` attribute have expired.
3. Remove the ``xsrf_cookies=True`` application setting from your code,
and all instances of ``xsrf_form_html`` from your templates and code. If
you have JavaScript code that touches the ``_xsrf`` cookie or sets an
``_xsrf`` query parameter, it can be removed as well.

.. _dnsrebinding:

DNS Rebinding
Expand Down
1 change: 0 additions & 1 deletion docs/guide/templates.rst
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,6 @@ Here is a properly internationalized template::
<div>{{ _("Username") }} <input type="text" name="username"/></div>
<div>{{ _("Password") }} <input type="password" name="password"/></div>
<div><input type="submit" value="{{ _("Sign in") }}"/></div>
{% module xsrf_form_html() %}
</form>
</body>
</html>
Expand Down
8 changes: 5 additions & 3 deletions docs/web.rst
Original file line number Diff line number Diff line change
Expand Up @@ -243,16 +243,18 @@
* ``login_url``: The `authenticated` decorator will redirect
to this url if the user is not logged in. Can be further
customized by overriding `RequestHandler.get_login_url`
* ``xsrf_cookies``: If ``True``, :ref:`xsrf` will be enabled.
* ``xsrf_cookies``: If ``True``, :ref:`legacy-xsrf` will be enabled.
This functionality is deprecated as of Tornado 6.3; see
:ref:`xsrf-deprecation` for more.
* ``xsrf_cookie_version``: Controls the version of new XSRF
cookies produced by this server. Should generally be left
at the default (which will always be the highest supported
version), but may be set to a lower value temporarily
during version transitions. New in Tornado 3.2.2, which
introduced XSRF cookie version 2.
introduced XSRF cookie version 2. Deprecated since Tornado 6.3.
* ``xsrf_cookie_kwargs``: May be set to a dictionary of
additional arguments to be passed to `.RequestHandler.set_cookie`
for the XSRF cookie.
for the XSRF cookie. Deprecated since Tornado 6.3.
* ``twitter_consumer_key``, ``twitter_consumer_secret``,
``friendfeed_consumer_key``, ``friendfeed_consumer_secret``,
``google_consumer_key``, ``google_consumer_secret``,
Expand Down
16 changes: 16 additions & 0 deletions tornado/test/web_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
from tornado.simple_httpclient import SimpleAsyncHTTPClient
from tornado.template import DictLoader
from tornado.testing import AsyncHTTPTestCase, AsyncTestCase, ExpectLog, gen_test
from tornado.test.util import ignore_deprecation
from tornado.util import ObjectDict, unicode_type
from tornado.web import (
Application,
Expand Down Expand Up @@ -1721,6 +1722,11 @@ def get_handlers(self):
# explicitly defined error handler and an implicit 404.
return [("/error", ErrorHandler, dict(status_code=417))]

def get_app(self):
# xsrf_cookies is deprecated
with ignore_deprecation():
return super().get_app()

def get_app_kwargs(self):
return dict(xsrf_cookies=True)

Expand Down Expand Up @@ -2728,6 +2734,11 @@ def get(self):
def post(self):
self.write("ok")

def get_app(self):
# xsrf_cookies is deprecated
with ignore_deprecation():
return super().get_app()

def get_app_kwargs(self):
return dict(xsrf_cookies=True)

Expand Down Expand Up @@ -2924,6 +2935,11 @@ class Handler(RequestHandler):
def get(self):
self.write(self.xsrf_token)

def get_app(self):
# xsrf_cookies is deprecated
with ignore_deprecation():
return super().get_app()

def get_app_kwargs(self):
return dict(
xsrf_cookies=True, xsrf_cookie_kwargs=dict(httponly=True, expires_days=2)
Expand Down
17 changes: 16 additions & 1 deletion tornado/web.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ async def main():
import types
import urllib.parse
from urllib.parse import urlencode
import warnings

from tornado.concurrent import Future, future_set_result_unless_cancelled
from tornado import escape
Expand Down Expand Up @@ -1434,7 +1435,7 @@ def get_template_path(self) -> Optional[str]:

@property
def xsrf_token(self) -> bytes:
"""The XSRF-prevention token for the current user/session.
"""The deprecated XSRF-prevention token for the current user/session.
To prevent cross-site request forgery, we set an '_xsrf' cookie
and include the same '_xsrf' value as an argument with all POST
Expand Down Expand Up @@ -1464,6 +1465,10 @@ def xsrf_token(self) -> bytes:
``xsrf_cookie_kwargs=dict(httponly=True, secure=True)``
will set the ``secure`` and ``httponly`` flags on the
``_xsrf`` cookie.
.. deprecated:: 6.3
See :ref:`xsrf-deprecation`.
"""
if not hasattr(self, "_xsrf_token"):
version, token, timestamp = self._get_raw_xsrf_token()
Expand Down Expand Up @@ -1568,6 +1573,10 @@ def check_xsrf_cookie(self) -> None:
.. versionchanged:: 3.2.2
Added support for cookie version 2. Both versions 1 and 2 are
supported.
.. deprecated:: 6.3
See :ref:`xsrf-deprecation`.
"""
# Prior to release 1.1.1, this check was ignored if the HTTP header
# ``X-Requested-With: XMLHTTPRequest`` was present. This exception
Expand Down Expand Up @@ -1601,6 +1610,10 @@ def xsrf_form_html(self) -> str:
xsrf_form_html() %}``
See `check_xsrf_cookie()` above for more information.
.. deprecated:: 6.3
See :ref:`xsrf-deprecation`
"""
return (
'<input type="hidden" name="_xsrf" value="'
Expand Down Expand Up @@ -2104,6 +2117,8 @@ def __init__(
transforms: Optional[List[Type["OutputTransform"]]] = None,
**settings: Any,
) -> None:
if settings.get("xsrf_cookies"):
warnings.warn("xsrf_cookies setting is deprecated", DeprecationWarning)
if transforms is None:
self.transforms = [] # type: List[Type[OutputTransform]]
if settings.get("compress_response") or settings.get("gzip"):
Expand Down

0 comments on commit 0558518

Please sign in to comment.