From f1226c98c44119261b6e1a5652d32e49eb912a53 Mon Sep 17 00:00:00 2001 From: Nathan Yergler Date: Sun, 4 Sep 2011 18:15:52 -0700 Subject: [PATCH 1/6] Issue 361 Initial implementation of CSRF protection middleware --- mediagoblin/config_spec.ini | 3 + mediagoblin/middleware/__init__.py | 1 + mediagoblin/middleware/csrf.py | 131 +++++++++++++++++++++++++++++ mediagoblin/util.py | 3 + 4 files changed, 138 insertions(+) create mode 100644 mediagoblin/middleware/csrf.py diff --git a/mediagoblin/config_spec.ini b/mediagoblin/config_spec.ini index a0fbde09..8018b243 100644 --- a/mediagoblin/config_spec.ini +++ b/mediagoblin/config_spec.ini @@ -41,6 +41,9 @@ celery_setup_elsewhere = boolean(default=False) # source files for a media file but can also be a HUGE security risk. allow_attachments = boolean(default=False) +# Cookie stuff +secret_key = string(default="Something Super Duper Secrit!") +csrf_cookie_name = string(default='mediagoblin_nonce') [storage:publicstore] base_dir = string(default="%(here)s/user_dev/media/public") diff --git a/mediagoblin/middleware/__init__.py b/mediagoblin/middleware/__init__.py index 586debbf..05325ee5 100644 --- a/mediagoblin/middleware/__init__.py +++ b/mediagoblin/middleware/__init__.py @@ -16,4 +16,5 @@ ENABLED_MIDDLEWARE = ( 'mediagoblin.middleware.noop:NoOpMiddleware', + 'mediagoblin.middleware.csrf:CsrfMiddleware', ) diff --git a/mediagoblin/middleware/csrf.py b/mediagoblin/middleware/csrf.py new file mode 100644 index 00000000..a372d0b5 --- /dev/null +++ b/mediagoblin/middleware/csrf.py @@ -0,0 +1,131 @@ +# GNU MediaGoblin -- federated, autonomous media hosting +# Copyright (C) 2011 MediaGoblin contributors. See AUTHORS. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . + +import hashlib +import random + +from webob.exc import HTTPForbidden +from wtforms import Form, HiddenField, validators + +from mediagoblin import mg_globals + +# Use the system (hardware-based) random number generator if it exists. +# -- this optimization is lifted from Django +if hasattr(random, 'SystemRandom'): + randrange = random.SystemRandom().randrange +else: + randrange = random.randrange + + +class CsrfForm(Form): + """Simple form to handle rendering a CSRF token and confirming it + is included in the POST.""" + + csrf_token = HiddenField("", + [validators.Required()]) + +def render_csrf_form_token(request): + """Render the CSRF token in a format suitable for inclusion in a + form.""" + + form = CsrfForm(csrf_token = request.environ['CSRF_TOKEN']) + + return form.csrf_token + +class CsrfMiddleware(object): + """CSRF Protection Middleware + + Adds a CSRF Cookie to responses and verifies that it is present + and matches the form token for non-safe requests. + """ + + MAX_CSRF_KEY = 2 << 63 + SAFE_HTTP_METHODS = ("GET", "HEAD", "OPTIONS", "TRACE") + + def __init__(self, mg_app): + self.app = mg_app + + def process_request(self, request): + """For non-safe requests, confirm that the tokens are present + and match. + """ + + # get the token from the cookie + try: + request.environ['CSRF_TOKEN'] = \ + request.cookies[mg_globals.app_config['csrf_cookie_name']] + + except KeyError, e: + # if it doesn't exist, make a new one + request.environ['CSRF_TOKEN'] = self._make_token(request) + + # if this is a non-"safe" request (ie, one that could have + # side effects), confirm that the CSRF tokens are present and + # valid + if request.method not in self.SAFE_HTTP_METHODS: + return self.verify_tokens(request) + + def process_response(self, request, response): + """Add the CSRF cookie to the response if needed and set Vary + headers. + """ + + # set the CSRF cookie + response.set_cookie( + mg_globals.app_config['csrf_cookie_name'], + request.environ['CSRF_TOKEN'], + max_age=60*60*24*7*52, path='/', + domain=mg_globals.app_config.get('csrf_cookie_domain', None), + secure=(request.scheme.lower() == 'https'), + httponly=True) + + # update the Vary header + response.vary = (response.vary or []) + ['Cookie'] + + def _make_token(self, request): + """Generate a new token to use for CSRF protection.""" + + return hashlib.md5("%s%s" % + (randrange(0, self.MAX_CSRF_KEY), + mg_globals.app_config['secret_key']) + ).hexdigest() + + def verify_tokens(self, request): + """Verify that the CSRF Cookie exists and that it matches the + form value.""" + + # confirm the cookie token was presented + cookie_token = request.cookies.get( + mg_globals.app_config['csrf_cookie_name'], + None) + + if cookie_token is None: + # the CSRF cookie must be present in the request + return HTTPForbidden() + + # get the form token and confirm it matches + form = CsrfForm(request.POST) + if form.validate(): + form_token = form.csrf_token.data + + if form_token == cookie_token: + # all's well that ends well + return + + # either the tokens didn't match or the form token wasn't + # present; either way, the request is denied + return HTTPForbidden() + diff --git a/mediagoblin/util.py b/mediagoblin/util.py index e391b8b0..bc72f8df 100644 --- a/mediagoblin/util.py +++ b/mediagoblin/util.py @@ -39,6 +39,7 @@ from wtforms.form import Form from mediagoblin import mg_globals from mediagoblin import messages from mediagoblin.db.util import ObjectId +from mediagoblin.middleware.csrf import render_csrf_form_token from itertools import izip, count @@ -125,6 +126,8 @@ def render_template(request, template_path, context): template = request.template_env.get_template( template_path) context['request'] = request + context['csrf_token'] = render_csrf_form_token(request) + rendered = template.render(context) if TESTS_ENABLED: From 0a8a3fc1571100aba3bd3a3dec98f5e9e252780b Mon Sep 17 00:00:00 2001 From: Nathan Yergler Date: Sun, 4 Sep 2011 18:16:03 -0700 Subject: [PATCH 2/6] Issue 361: Include the CSRF token in all forms --- mediagoblin/templates/mediagoblin/auth/login.html | 1 + mediagoblin/templates/mediagoblin/auth/register.html | 1 + mediagoblin/templates/mediagoblin/edit/attachments.html | 1 + mediagoblin/templates/mediagoblin/edit/edit.html | 1 + mediagoblin/templates/mediagoblin/edit/edit_profile.html | 1 + mediagoblin/templates/mediagoblin/submit/start.html | 1 + mediagoblin/templates/mediagoblin/test_submit.html | 1 + mediagoblin/templates/mediagoblin/user_pages/media.html | 1 + .../templates/mediagoblin/user_pages/media_confirm_delete.html | 1 + 9 files changed, 9 insertions(+) diff --git a/mediagoblin/templates/mediagoblin/auth/login.html b/mediagoblin/templates/mediagoblin/auth/login.html index 958cf9ea..1be58560 100644 --- a/mediagoblin/templates/mediagoblin/auth/login.html +++ b/mediagoblin/templates/mediagoblin/auth/login.html @@ -22,6 +22,7 @@ {% block mediagoblin_content %}
+ {{ csrf_token }}

{% trans %}Log in{% endtrans %}

{% if login_failed %} diff --git a/mediagoblin/templates/mediagoblin/auth/register.html b/mediagoblin/templates/mediagoblin/auth/register.html index e72b3a52..25b68058 100644 --- a/mediagoblin/templates/mediagoblin/auth/register.html +++ b/mediagoblin/templates/mediagoblin/auth/register.html @@ -26,6 +26,7 @@

{% trans %}Create an account!{% endtrans %}

{{ wtforms_util.render_divs(register_form) }} + {{ csrf_token }}
diff --git a/mediagoblin/templates/mediagoblin/edit/attachments.html b/mediagoblin/templates/mediagoblin/edit/attachments.html index 63b06581..d8b55f58 100644 --- a/mediagoblin/templates/mediagoblin/edit/attachments.html +++ b/mediagoblin/templates/mediagoblin/edit/attachments.html @@ -49,6 +49,7 @@
Cancel + {{ csrf_token }}
diff --git a/mediagoblin/templates/mediagoblin/edit/edit.html b/mediagoblin/templates/mediagoblin/edit/edit.html index 8c4e2efb..b4b3be85 100644 --- a/mediagoblin/templates/mediagoblin/edit/edit.html +++ b/mediagoblin/templates/mediagoblin/edit/edit.html @@ -35,6 +35,7 @@
{% trans %}Cancel{% endtrans %} + {{ csrf_token }}
diff --git a/mediagoblin/templates/mediagoblin/edit/edit_profile.html b/mediagoblin/templates/mediagoblin/edit/edit_profile.html index 464c663d..93b2a792 100644 --- a/mediagoblin/templates/mediagoblin/edit/edit_profile.html +++ b/mediagoblin/templates/mediagoblin/edit/edit_profile.html @@ -33,6 +33,7 @@ {{ wtforms_util.render_divs(form) }}
+ {{ csrf_token }}
diff --git a/mediagoblin/templates/mediagoblin/submit/start.html b/mediagoblin/templates/mediagoblin/submit/start.html index f2e844df..7bc6ff45 100644 --- a/mediagoblin/templates/mediagoblin/submit/start.html +++ b/mediagoblin/templates/mediagoblin/submit/start.html @@ -26,6 +26,7 @@

{% trans %}Submit yer media{% endtrans %}

{{ wtforms_util.render_divs(submit_form) }}
+ {{ csrf_token }}
diff --git a/mediagoblin/templates/mediagoblin/test_submit.html b/mediagoblin/templates/mediagoblin/test_submit.html index 78b88ae8..190b9ac3 100644 --- a/mediagoblin/templates/mediagoblin/test_submit.html +++ b/mediagoblin/templates/mediagoblin/test_submit.html @@ -26,6 +26,7 @@ + {{ csrf_token }} diff --git a/mediagoblin/templates/mediagoblin/user_pages/media.html b/mediagoblin/templates/mediagoblin/user_pages/media.html index 442bef6d..433f74dc 100644 --- a/mediagoblin/templates/mediagoblin/user_pages/media.html +++ b/mediagoblin/templates/mediagoblin/user_pages/media.html @@ -72,6 +72,7 @@ {{ wtforms_util.render_divs(comment_form) }}
+ {{ csrf_token }}
{% endif %} diff --git a/mediagoblin/templates/mediagoblin/user_pages/media_confirm_delete.html b/mediagoblin/templates/mediagoblin/user_pages/media_confirm_delete.html index 48fbc3b0..3acf802b 100644 --- a/mediagoblin/templates/mediagoblin/user_pages/media_confirm_delete.html +++ b/mediagoblin/templates/mediagoblin/user_pages/media_confirm_delete.html @@ -42,6 +42,7 @@ {{ wtforms_util.render_divs(form) }}
+ {{ csrf_token }}
From 5d2abe45b2bae9111d4f1bda645b53414d2b240d Mon Sep 17 00:00:00 2001 From: Nathan Yergler Date: Sat, 1 Oct 2011 12:48:43 -0700 Subject: [PATCH 3/6] PEP8-ification. --- mediagoblin/middleware/csrf.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/mediagoblin/middleware/csrf.py b/mediagoblin/middleware/csrf.py index a372d0b5..68ece6d3 100644 --- a/mediagoblin/middleware/csrf.py +++ b/mediagoblin/middleware/csrf.py @@ -34,17 +34,19 @@ class CsrfForm(Form): """Simple form to handle rendering a CSRF token and confirming it is included in the POST.""" - csrf_token = HiddenField("", + csrf_token = HiddenField("", [validators.Required()]) + def render_csrf_form_token(request): """Render the CSRF token in a format suitable for inclusion in a form.""" - form = CsrfForm(csrf_token = request.environ['CSRF_TOKEN']) + form = CsrfForm(csrf_token=request.environ['CSRF_TOKEN']) return form.csrf_token + class CsrfMiddleware(object): """CSRF Protection Middleware @@ -87,7 +89,8 @@ class CsrfMiddleware(object): response.set_cookie( mg_globals.app_config['csrf_cookie_name'], request.environ['CSRF_TOKEN'], - max_age=60*60*24*7*52, path='/', + max_age=60 * 60 * 24 * 7 * 52, + path='/', domain=mg_globals.app_config.get('csrf_cookie_domain', None), secure=(request.scheme.lower() == 'https'), httponly=True) @@ -98,10 +101,9 @@ class CsrfMiddleware(object): def _make_token(self, request): """Generate a new token to use for CSRF protection.""" - return hashlib.md5("%s%s" % - (randrange(0, self.MAX_CSRF_KEY), - mg_globals.app_config['secret_key']) - ).hexdigest() + return hashlib.md5("%s%s" % + (randrange(0, self.MAX_CSRF_KEY), + mg_globals.app_config['secret_key'])).hexdigest() def verify_tokens(self, request): """Verify that the CSRF Cookie exists and that it matches the @@ -109,7 +111,7 @@ class CsrfMiddleware(object): # confirm the cookie token was presented cookie_token = request.cookies.get( - mg_globals.app_config['csrf_cookie_name'], + mg_globals.app_config['csrf_cookie_name'], None) if cookie_token is None: @@ -128,4 +130,3 @@ class CsrfMiddleware(object): # either the tokens didn't match or the form token wasn't # present; either way, the request is denied return HTTPForbidden() - From 7e694e5fd858aeaea7eb7e9a9062b36d17a3b7f7 Mon Sep 17 00:00:00 2001 From: Nathan Yergler Date: Sat, 1 Oct 2011 13:13:14 -0700 Subject: [PATCH 4/6] #361: Don't test for CSRF token if we're running unit tests. --- mediagoblin/middleware/csrf.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/mediagoblin/middleware/csrf.py b/mediagoblin/middleware/csrf.py index 68ece6d3..d41bcd87 100644 --- a/mediagoblin/middleware/csrf.py +++ b/mediagoblin/middleware/csrf.py @@ -77,7 +77,10 @@ class CsrfMiddleware(object): # if this is a non-"safe" request (ie, one that could have # side effects), confirm that the CSRF tokens are present and # valid - if request.method not in self.SAFE_HTTP_METHODS: + if request.method not in self.SAFE_HTTP_METHODS \ + and ('gmg.verify_csrf' in request.environ or + 'paste.testing' not in request.environ): + return self.verify_tokens(request) def process_response(self, request, response): From 4f475d3024f689c1c461dc26bd679dfb514a46ef Mon Sep 17 00:00:00 2001 From: Nathan Yergler Date: Sat, 1 Oct 2011 14:21:02 -0700 Subject: [PATCH 5/6] #361 Unit tests for CSRF Middleware --- mediagoblin/tests/test_csrf_middleware.py | 69 +++++++++++++++++++++++ 1 file changed, 69 insertions(+) create mode 100644 mediagoblin/tests/test_csrf_middleware.py diff --git a/mediagoblin/tests/test_csrf_middleware.py b/mediagoblin/tests/test_csrf_middleware.py new file mode 100644 index 00000000..cf03fe58 --- /dev/null +++ b/mediagoblin/tests/test_csrf_middleware.py @@ -0,0 +1,69 @@ +# GNU MediaGoblin -- federated, autonomous media hosting +# Copyright (C) 2011 MediaGoblin contributors. See AUTHORS. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . + +import urlparse +import datetime + +from nose.tools import assert_equal + +from mediagoblin.tests.tools import setup_fresh_app +from mediagoblin import mg_globals + + +@setup_fresh_app +def test_csrf_cookie_set(test_app): + + # get login page + response = test_app.get('/auth/login/') + + # assert that the mediagoblin nonce cookie has been set + assert 'Set-Cookie' in response.headers + assert 'mediagoblin_nonce' in response.cookies_set + + # assert that we're also sending a vary header + assert response.headers.get('Vary', False) == 'Cookie' + + +@setup_fresh_app +def test_csrf_token_must_match(test_app): + + # construct a request with no cookie or form token + assert test_app.post('/auth/login/', + extra_environ={'gmg.verify_csrf': True}, + expect_errors=True).status_int == 403 + + # construct a request with a cookie, but no form token + assert test_app.post('/auth/login/', + headers={'Cookie': str('%s=foo; ' % + mg_globals.app_config['csrf_cookie_name'])}, + extra_environ={'gmg.verify_csrf': True}, + expect_errors=True).status_int == 403 + + # if both the cookie and form token are provided, they must match + assert test_app.post('/auth/login/', + {'csrf_token': 'blarf'}, + headers={'Cookie': str('%s=foo; ' % + mg_globals.app_config['csrf_cookie_name'])}, + extra_environ={'gmg.verify_csrf': True}, + expect_errors=True).\ + status_int == 403 + + assert test_app.post('/auth/login/', + {'csrf_token': 'foo'}, + headers={'Cookie': str('%s=foo; ' % + mg_globals.app_config['csrf_cookie_name'])}, + extra_environ={'gmg.verify_csrf': True}).\ + status_int == 200 From 9202e5a1e15183b134fa15c4e1290dea8ed2acbe Mon Sep 17 00:00:00 2001 From: Nathan Yergler Date: Sat, 1 Oct 2011 14:24:49 -0700 Subject: [PATCH 6/6] #361: Removing additional secret key, per CW's request. --- mediagoblin/config_spec.ini | 1 - mediagoblin/middleware/csrf.py | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/mediagoblin/config_spec.ini b/mediagoblin/config_spec.ini index 37fe7130..298a6951 100644 --- a/mediagoblin/config_spec.ini +++ b/mediagoblin/config_spec.ini @@ -42,7 +42,6 @@ celery_setup_elsewhere = boolean(default=False) allow_attachments = boolean(default=False) # Cookie stuff -secret_key = string(default="Something Super Duper Secrit!") csrf_cookie_name = string(default='mediagoblin_nonce') [storage:publicstore] diff --git a/mediagoblin/middleware/csrf.py b/mediagoblin/middleware/csrf.py index d41bcd87..44b799d5 100644 --- a/mediagoblin/middleware/csrf.py +++ b/mediagoblin/middleware/csrf.py @@ -106,7 +106,7 @@ class CsrfMiddleware(object): return hashlib.md5("%s%s" % (randrange(0, self.MAX_CSRF_KEY), - mg_globals.app_config['secret_key'])).hexdigest() + randrange(0, self.MAX_CSRF_KEY))).hexdigest() def verify_tokens(self, request): """Verify that the CSRF Cookie exists and that it matches the