From f12005b92fa9bb33f082bd50747eb11791605cff Mon Sep 17 00:00:00 2001 From: Donald Stufft Date: Fri, 15 Apr 2016 17:41:35 -0400 Subject: [PATCH 1/2] Only Accept CSRF Tokens in headers or POST bodies Previously `check_csrf_token` would allow passing in a CSRF token in through a the URL of a request. However this is a security issue because a CSRF token must not be allowed to leak, and URLs regularly get copy/pasted or otherwise end up leaking to the outside world. --- docs/narr/sessions.rst | 7 +++---- docs/narr/viewconfig.rst | 2 +- pyramid/session.py | 28 ++++++++++++++++++------- pyramid/tests/test_config/test_views.py | 7 +++++-- pyramid/tests/test_session.py | 9 +++++--- pyramid/tests/test_viewderivers.py | 14 +++++++------ 6 files changed, 43 insertions(+), 24 deletions(-) diff --git a/docs/narr/sessions.rst b/docs/narr/sessions.rst index d66e862585..ad086268b4 100644 --- a/docs/narr/sessions.rst +++ b/docs/narr/sessions.rst @@ -391,8 +391,8 @@ will return ``True``, otherwise it will raise ``HTTPBadRequest``. Optionally, you can specify ``raises=False`` to have the check return ``False`` instead of raising an exception. -By default, it checks for a GET or POST parameter named ``csrf_token`` or a -header named ``X-CSRF-Token``. +By default, it checks for a POST parameter named ``csrf_token`` or a header +named ``X-CSRF-Token``. .. code-block:: python @@ -430,8 +430,7 @@ If ``require_csrf`` is ``True`` but does not explicitly define a token to check, then the token name is pulled from whatever was set in the ``pyramid.require_default_csrf`` setting. Finally, if that setting does not explicitly define a token, then ``csrf_token`` is the token required. This token -name will be required in ``request.params`` which is a combination of the -query string and a submitted form body. +name will be required in ``request.POST`` which is the submitted form body. It is always possible to pass the token in the ``X-CSRF-Token`` header as well. There is currently no way to define an alternate name for this header without diff --git a/docs/narr/viewconfig.rst b/docs/narr/viewconfig.rst index e645185f5f..40db5fbeb7 100644 --- a/docs/narr/viewconfig.rst +++ b/docs/narr/viewconfig.rst @@ -459,7 +459,7 @@ configured view. check name. If CSRF checking is performed, the checked value will be the value of - ``request.params[check_name]``. This value will be compared against the + ``request.POST[check_name]``. This value will be compared against the value of ``request.session.get_csrf_token()``, and the check will pass if these two values are the same. If the check passes, the associated view will be permitted to execute. If the check fails, the associated view will not be diff --git a/pyramid/session.py b/pyramid/session.py index fd7b5f8d58..6136e26a0e 100644 --- a/pyramid/session.py +++ b/pyramid/session.py @@ -106,13 +106,14 @@ def check_csrf_token(request, header='X-CSRF-Token', raises=True): """ Check the CSRF token in the request's session against the value in - ``request.params.get(token)`` or ``request.headers.get(header)``. - If a ``token`` keyword is not supplied to this function, the string - ``csrf_token`` will be used to look up the token in ``request.params``. - If a ``header`` keyword is not supplied to this function, the string - ``X-CSRF-Token`` will be used to look up the token in ``request.headers``. - - If the value supplied by param or by header doesn't match the value + ``request.POST.get(token)`` (if a POST request) or + ``request.headers.get(header)``. If a ``token`` keyword is not supplied to + this function, the string ``csrf_token`` will be used to look up the token + in ``request.POST``. If a ``header`` keyword is not supplied to this + function, the string ``X-CSRF-Token`` will be used to look up the token in + ``request.headers``. + + If the value supplied by post or by header doesn't match the value supplied by ``request.session.get_csrf_token()``, and ``raises`` is ``True``, this function will raise an :exc:`pyramid.exceptions.BadCSRFToken` exception. @@ -128,7 +129,18 @@ def check_csrf_token(request, .. versionadded:: 1.4a2 """ - supplied_token = request.params.get(token, request.headers.get(header, "")) + # If this is a POST/PUT/etc request, then we'll check the body to see if it + # has a token. We explicitly use request.POST here because CSRF tokens + # should never appear in an URL as doing so is a security issue. We also + # explicitly check for request.POST here as we do not support sending form + # encoded data over anything but a request.POST. + supplied_token = request.POST.get(token, "") + + # If we were unable to locate a CSRF token in a request body, then we'll + # check to see if there are any headers that have a value for us. + if supplied_token == "": + supplied_token = request.headers.get(header, "") + expected_token = request.session.get_csrf_token() if strings_differ(bytes_(expected_token), bytes_(supplied_token)): if raises: diff --git a/pyramid/tests/test_config/test_views.py b/pyramid/tests/test_config/test_views.py index 0bf0bd0b39..7be72257d6 100644 --- a/pyramid/tests/test_config/test_views.py +++ b/pyramid/tests/test_config/test_views.py @@ -1502,8 +1502,9 @@ def test_add_view_with_check_csrf_predicates_match(self): self.assertEqual(len(w), 1) wrapper = self._getViewCallable(config) request = self._makeRequest(config) + request.method = "POST" request.session = DummySession({'csrf_token': 'foo'}) - request.params = {'csrf_token': 'foo'} + request.POST = {'csrf_token': 'foo'} request.headers = {} self.assertEqual(wrapper(None, request), 'OK') @@ -1595,7 +1596,7 @@ def view(request): view = self._getViewCallable(config) request = self._makeRequest(config) request.method = 'POST' - request.params = {'st': 'foo'} + request.POST = {'st': 'foo'} request.headers = {} request.session = DummySession({'csrf_token': 'foo'}) self.assertEqual(view(None, request), 'OK') @@ -1609,6 +1610,7 @@ def view(request): view = self._getViewCallable(config) request = self._makeRequest(config) request.method = 'POST' + request.POST = {} request.headers = {'X-CSRF-Token': 'foo'} request.session = DummySession({'csrf_token': 'foo'}) self.assertEqual(view(None, request), 'OK') @@ -1622,6 +1624,7 @@ def view(request): return 'OK' view = self._getViewCallable(config) request = self._makeRequest(config) request.method = 'POST' + request.POST = {} request.headers = {} request.session = DummySession({'csrf_token': 'foo'}) self.assertRaises(BadCSRFToken, lambda: view(None, request)) diff --git a/pyramid/tests/test_session.py b/pyramid/tests/test_session.py index 914d28a832..4ec8f94a49 100644 --- a/pyramid/tests/test_session.py +++ b/pyramid/tests/test_session.py @@ -666,7 +666,8 @@ def _callFUT(self, *args, **kwargs): def test_success_token(self): request = testing.DummyRequest() - request.params['csrf_token'] = request.session.get_csrf_token() + request.method = "POST" + request.POST = {'csrf_token': request.session.get_csrf_token()} self.assertEqual(self._callFUT(request, token='csrf_token'), True) def test_success_header(self): @@ -676,7 +677,8 @@ def test_success_header(self): def test_success_default_token(self): request = testing.DummyRequest() - request.params['csrf_token'] = request.session.get_csrf_token() + request.method = "POST" + request.POST = {'csrf_token': request.session.get_csrf_token()} self.assertEqual(self._callFUT(request), True) def test_success_default_header(self): @@ -698,8 +700,9 @@ def test_failure_no_raises(self): def test_token_differing_types(self): from pyramid.compat import text_ request = testing.DummyRequest() + request.method = "POST" request.session['_csrft_'] = text_('foo') - request.params['csrf_token'] = b'foo' + request.POST = {'csrf_token': b'foo'} self.assertEqual(self._callFUT(request, token='csrf_token'), True) class DummySerializer(object): diff --git a/pyramid/tests/test_viewderivers.py b/pyramid/tests/test_viewderivers.py index c8fbe6f36d..bd5b3f2de2 100644 --- a/pyramid/tests/test_viewderivers.py +++ b/pyramid/tests/test_viewderivers.py @@ -1120,6 +1120,7 @@ def inner_view(request): return response request = self._makeRequest() request.method = 'POST' + request.POST = {} request.session = DummySession({'csrf_token': 'foo'}) request.headers = {'X-CSRF-Token': 'foo'} view = self.config._derive_view(inner_view, require_csrf=True) @@ -1133,7 +1134,7 @@ def inner_view(request): request = self._makeRequest() request.method = 'POST' request.session = DummySession({'csrf_token': 'foo'}) - request.params['DUMMY'] = 'foo' + request.POST = {'DUMMY': 'foo'} view = self.config._derive_view(inner_view, require_csrf='DUMMY') result = view(None, request) self.assertTrue(result is response) @@ -1154,7 +1155,7 @@ def inner_view(request): pass request = self._makeRequest() request.method = 'POST' request.session = DummySession({'csrf_token': 'foo'}) - request.params['DUMMY'] = 'bar' + request.POST = {'DUMMY': 'bar'} view = self.config._derive_view(inner_view, require_csrf='DUMMY') self.assertRaises(BadCSRFToken, lambda: view(None, request)) @@ -1163,6 +1164,7 @@ def test_csrf_view_fails_on_bad_POST_header(self): def inner_view(request): pass request = self._makeRequest() request.method = 'POST' + request.POST = {} request.session = DummySession({'csrf_token': 'foo'}) request.headers = {'X-CSRF-Token': 'bar'} view = self.config._derive_view(inner_view, require_csrf='DUMMY') @@ -1175,7 +1177,7 @@ def inner_view(request): request = self._makeRequest() request.method = 'POST' request.session = DummySession({'csrf_token': 'foo'}) - request.params['csrf_token'] = 'foo' + request.POST = {'csrf_token': 'foo'} self.config.add_settings({'pyramid.require_default_csrf': 'yes'}) view = self.config._derive_view(inner_view) result = view(None, request) @@ -1188,7 +1190,7 @@ def inner_view(request): request = self._makeRequest() request.method = 'POST' request.session = DummySession({'csrf_token': 'foo'}) - request.params['DUMMY'] = 'foo' + request.POST = {'DUMMY': 'foo'} self.config.add_settings({'pyramid.require_default_csrf': 'DUMMY'}) view = self.config._derive_view(inner_view) result = view(None, request) @@ -1214,7 +1216,7 @@ def inner_view(request): request = self._makeRequest() request.method = 'POST' request.session = DummySession({'csrf_token': 'foo'}) - request.params['DUMMY'] = 'foo' + request.POST = {'DUMMY': 'foo'} self.config.add_settings({'pyramid.require_default_csrf': 'yes'}) view = self.config._derive_view(inner_view, require_csrf='DUMMY') result = view(None, request) @@ -1227,7 +1229,7 @@ def inner_view(request): request = self._makeRequest() request.method = 'POST' request.session = DummySession({'csrf_token': 'foo'}) - request.params['DUMMY'] = 'foo' + request.POST = {'DUMMY': 'foo'} self.config.add_settings({'pyramid.require_default_csrf': 'DUMMY'}) view = self.config._derive_view(inner_view, require_csrf=True) result = view(None, request) From 21d5beaed1641e1f50ab1ab3c481b1c8f3ad1173 Mon Sep 17 00:00:00 2001 From: Donald Stufft Date: Fri, 15 Apr 2016 17:59:55 -0400 Subject: [PATCH 2/2] Have Automatic CSRF on all unsafe HTTP methods Instead of only protecting against unsafe POST requests, have the automatic CSRF protect on all methods which are not defined as "safe" by RFC2616. --- docs/narr/sessions.rst | 9 +++++---- docs/narr/viewconfig.rst | 9 +++++---- docs/whatsnew-1.7.rst | 4 ++-- pyramid/tests/test_viewderivers.py | 11 +++++++++++ pyramid/viewderivers.py | 4 +++- 5 files changed, 26 insertions(+), 11 deletions(-) diff --git a/docs/narr/sessions.rst b/docs/narr/sessions.rst index ad086268b4..0e895ff81f 100644 --- a/docs/narr/sessions.rst +++ b/docs/narr/sessions.rst @@ -411,15 +411,16 @@ Checking CSRF Tokens Automatically .. versionadded:: 1.7 -:app:`Pyramid` supports automatically checking CSRF tokens on POST requests. -Any other request may be checked manually. This feature can be turned on -globally for an application using the ``pyramid.require_default_csrf`` setting. +:app:`Pyramid` supports automatically checking CSRF tokens on requests with an +unsafe method as defined by RFC2616. Any other request may be checked manually. +This feature can be turned on globally for an application using the +``pyramid.require_default_csrf`` setting. If the ``pyramid.required_default_csrf`` setting is a :term:`truthy string` or ``True`` then the default CSRF token parameter will be ``csrf_token``. If a different token is desired, it may be passed as the value. Finally, a :term:`falsey string` or ``False`` will turn off automatic CSRF checking -globally on every POST request. +globally on every request. No matter what, CSRF checking may be explicitly enabled or disabled on a per-view basis using the ``require_csrf`` view option. This option is of the diff --git a/docs/narr/viewconfig.rst b/docs/narr/viewconfig.rst index 40db5fbeb7..3b8f0353aa 100644 --- a/docs/narr/viewconfig.rst +++ b/docs/narr/viewconfig.rst @@ -195,10 +195,11 @@ Non-Predicate Arguments ``require_csrf`` - CSRF checks only affect POST requests. Any other request methods will pass - untouched. This option is used in combination with the - ``pyramid.require_default_csrf`` setting to control which request parameters - are checked for CSRF tokens. + CSRF checks will affect any request method that is not defined as a "safe" + method by RFC2616. In pratice this means that GET, HEAD, OPTIONS, and TRACE + methods will pass untouched and all others methods will require CSRF. This + option is used in combination with the ``pyramid.require_default_csrf`` + setting to control which request parameters are checked for CSRF tokens. This feature requires a configured :term:`session factory`. diff --git a/docs/whatsnew-1.7.rst b/docs/whatsnew-1.7.rst index 83ece690e4..b85e65ec19 100644 --- a/docs/whatsnew-1.7.rst +++ b/docs/whatsnew-1.7.rst @@ -39,14 +39,14 @@ Feature Additions to security checks. See https://github.com/Pylons/pyramid/pull/2021 - Added a new setting, ``pyramid.require_default_csrf`` which may be used - to turn on CSRF checks globally for every POST request in the application. + to turn on CSRF checks globally for every request in the application. This should be considered a good default for websites built on Pyramid. It is possible to opt-out of CSRF checks on a per-view basis by setting ``require_csrf=False`` on those views. See :ref:`auto_csrf_checking` and https://github.com/Pylons/pyramid/pull/2413 -- Added a ``require_csrf`` view option which will enforce CSRF checks on POST +- Added a ``require_csrf`` view option which will enforce CSRF checks on requests. If the CSRF check fails a ``BadCSRFToken`` exception will be raised and may be caught by exception views (the default response is a ``400 Bad Request``). This option should be used in place of the deprecated diff --git a/pyramid/tests/test_viewderivers.py b/pyramid/tests/test_viewderivers.py index bd5b3f2de2..4613762f5f 100644 --- a/pyramid/tests/test_viewderivers.py +++ b/pyramid/tests/test_viewderivers.py @@ -1170,6 +1170,17 @@ def inner_view(request): pass view = self.config._derive_view(inner_view, require_csrf='DUMMY') self.assertRaises(BadCSRFToken, lambda: view(None, request)) + def test_csrf_view_fails_on_bad_PUT_header(self): + from pyramid.exceptions import BadCSRFToken + def inner_view(request): pass + request = self._makeRequest() + request.method = 'PUT' + request.POST = {} + request.session = DummySession({'csrf_token': 'foo'}) + request.headers = {'X-CSRF-Token': 'bar'} + view = self.config._derive_view(inner_view, require_csrf='DUMMY') + self.assertRaises(BadCSRFToken, lambda: view(None, request)) + def test_csrf_view_uses_config_setting_truthy(self): response = DummyResponse() def inner_view(request): diff --git a/pyramid/viewderivers.py b/pyramid/viewderivers.py index 41102319d2..e9ff094169 100644 --- a/pyramid/viewderivers.py +++ b/pyramid/viewderivers.py @@ -488,7 +488,9 @@ def csrf_view(view, info): wrapped_view = view if val: def csrf_view(context, request): - if request.method == 'POST': + # Assume that anything not defined as 'safe' by RFC2616 needs + # protection + if request.method not in {"GET", "HEAD", "OPTIONS", "TRACE"}: check_csrf_token(request, val, raises=True) return view(context, request) wrapped_view = csrf_view