From 9573bdca55ddc5488066d3af525e41ed1d872ea6 Mon Sep 17 00:00:00 2001 From: NavaL Date: Wed, 24 Feb 2016 16:33:20 +0100 Subject: Backend and frontend protection against csrf attacks: - root resources changes the csrf token cookie everytime it is loaded, in particular during the intestitial load during login - it will also add that cookie on single user mode - initialize will still load all resources - but they you cant access them if the csrf token do not match - all ajax calls needs to add the token to the header - non ajax get requests do not need xsrf token validation - non ajax post will have to send the token in as a form input or in the content Issue #612 --- service/pixelated/resources/__init__.py | 4 ++ service/pixelated/resources/login_resource.py | 3 +- service/pixelated/resources/root_resource.py | 69 +++++++++++++----- .../test/integration/test_retrieve_attachment.py | 17 +++++ .../test/support/integration/app_test_client.py | 20 +++--- service/test/support/test_helper.py | 13 +++- service/test/unit/resources/test_root_resource.py | 82 +++++++++++++++++++++- web-ui/app/js/helpers/browser.js | 9 ++- web-ui/app/js/helpers/monitored_ajax.js | 10 +-- web-ui/app/js/services/mail_service.js | 2 +- web-ui/test/spec/helpers/browser.spec.js | 12 ++++ .../test/spec/helpers/monitored_ajax_call.spec.js | 24 ++++++- 12 files changed, 225 insertions(+), 40 deletions(-) create mode 100644 web-ui/test/spec/helpers/browser.spec.js diff --git a/service/pixelated/resources/__init__.py b/service/pixelated/resources/__init__.py index 14ecac86..469c8bc8 100644 --- a/service/pixelated/resources/__init__.py +++ b/service/pixelated/resources/__init__.py @@ -99,3 +99,7 @@ class UnAuthorizedResource(Resource): def render_GET(self, request): request.setResponseCode(UNAUTHORIZED) return "Unauthorized!" + + def render_POST(self, request): + request.setResponseCode(UNAUTHORIZED) + return "Unauthorized!" diff --git a/service/pixelated/resources/login_resource.py b/service/pixelated/resources/login_resource.py index f1d9c1e3..aca266cf 100644 --- a/service/pixelated/resources/login_resource.py +++ b/service/pixelated/resources/login_resource.py @@ -20,14 +20,13 @@ from xml.sax import SAXParseException from twisted.cred import credentials from twisted.internet import defer +from twisted.python.filepath import FilePath from twisted.web import util -from twisted.web.error import FlattenerError from twisted.web.http import UNAUTHORIZED, OK from twisted.web.resource import IResource, NoResource from twisted.web.server import NOT_DONE_YET from twisted.web.static import File from twisted.web.template import Element, XMLFile, renderElement, renderer -from twisted.python.filepath import FilePath from pixelated.adapter.welcome_mail import add_welcome_mail from pixelated.resources import BaseResource, UnAuthorizedResource, IPixelatedSession diff --git a/service/pixelated/resources/root_resource.py b/service/pixelated/resources/root_resource.py index 6e619951..2c32ab0c 100644 --- a/service/pixelated/resources/root_resource.py +++ b/service/pixelated/resources/root_resource.py @@ -13,11 +13,12 @@ # # You should have received a copy of the GNU Affero General Public License # along with Pixelated. If not, see . - +import hashlib +import json import os from string import Template -from pixelated.resources import BaseResource +from pixelated.resources import BaseResource, UnAuthorizedResource from pixelated.resources.attachments_resource import AttachmentsResource from pixelated.resources.contacts_resource import ContactsResource from pixelated.resources.features_resource import FeaturesResource @@ -29,22 +30,22 @@ from pixelated.resources.mail_resource import MailResource from pixelated.resources.mails_resource import MailsResource from pixelated.resources.tags_resource import TagsResource from pixelated.resources.keys_resource import KeysResource -from twisted.web.resource import Resource from twisted.web.static import File +CSRF_TOKEN_LENGTH = 32 MODE_STARTUP = 1 MODE_RUNNING = 2 class RootResource(BaseResource): - def __init__(self, services_factory): BaseResource.__init__(self, services_factory) self._startup_assets_folder = self._get_startup_folder() self._static_folder = self._get_static_folder() self._html_template = open(os.path.join(self._static_folder, 'index.html')).read() self._services_factory = services_factory + self.child_resources = ChildResourcesMap() self._startup_mode() def _startup_mode(self): @@ -54,21 +55,39 @@ class RootResource(BaseResource): def getChild(self, path, request): if path == '': return self - return Resource.getChild(self, path, request) + if self._is_xsrf_valid(request): + return self.child_resources.get(path) + return UnAuthorizedResource() + + def _is_xsrf_valid(self, request): + xsrf_token = request.getCookie('XSRF-TOKEN') + + ajax_request = (request.getHeader('x-requested-with') == 'XMLHttpRequest') + if ajax_request: + xsrf_header = request.getHeader('x-xsrf-token') + return xsrf_header and xsrf_header == xsrf_token + + get_request = (request.method == 'GET') + if get_request: + return True + + csrf_input = request.args.get('csrftoken', [None])[0] or json.loads(request.content.read()).get('csrftoken', [None])[0] + return csrf_input and csrf_input == xsrf_token def initialize(self, portal=None, disclaimer_banner=None): - self.putChild('assets', File(self._static_folder)) - self.putChild('keys', KeysResource(self._services_factory)) - self.putChild(AttachmentsResource.BASE_URL, AttachmentsResource(self._services_factory)) - self.putChild('contacts', ContactsResource(self._services_factory)) - self.putChild('features', FeaturesResource(portal)) - self.putChild('tags', TagsResource(self._services_factory)) - self.putChild('mails', MailsResource(self._services_factory)) - self.putChild('mail', MailResource(self._services_factory)) - self.putChild('feedback', FeedbackResource(self._services_factory)) - self.putChild('user-settings', UserSettingsResource(self._services_factory)) - self.putChild(LoginResource.BASE_URL, LoginResource(self._services_factory, portal, disclaimer_banner=disclaimer_banner)) - self.putChild(LogoutResource.BASE_URL, LogoutResource(self._services_factory)) + self.child_resources.add('assets', File(self._static_folder)) + self.child_resources.add('keys', KeysResource(self._services_factory)) + self.child_resources.add(AttachmentsResource.BASE_URL, AttachmentsResource(self._services_factory)) + self.child_resources.add('contacts', ContactsResource(self._services_factory)) + self.child_resources.add('features', FeaturesResource(portal)) + self.child_resources.add('tags', TagsResource(self._services_factory)) + self.child_resources.add('mails', MailsResource(self._services_factory)) + self.child_resources.add('mail', MailResource(self._services_factory)) + self.child_resources.add('feedback', FeedbackResource(self._services_factory)) + self.child_resources.add('user-settings', UserSettingsResource(self._services_factory)) + self.child_resources.add(LoginResource.BASE_URL, + LoginResource(self._services_factory, portal, disclaimer_banner=disclaimer_banner)) + self.child_resources.add(LogoutResource.BASE_URL, LogoutResource(self._services_factory)) self._mode = MODE_RUNNING @@ -89,10 +108,26 @@ class RootResource(BaseResource): def _is_starting(self): return self._mode == MODE_STARTUP + def _add_csrf_cookie(self, request): + csrf_token = hashlib.sha256(os.urandom(CSRF_TOKEN_LENGTH)).hexdigest() + request.addCookie('XSRF-TOKEN', csrf_token) + def render_GET(self, request): + self._add_csrf_cookie(request) if self._is_starting(): return open(os.path.join(self._startup_assets_folder, 'Interstitial.html')).read() else: account_email = self.mail_service(request).account_email response = Template(self._html_template).safe_substitute(account_email=account_email) return str(response) + + +class ChildResourcesMap(object): + def __init__(self): + self._registry = {} + + def add(self, path, resource): + self._registry[path] = resource + + def get(self, path): + return self._registry.get(path) diff --git a/service/test/integration/test_retrieve_attachment.py b/service/test/integration/test_retrieve_attachment.py index 4aaeadc2..31c8c5df 100644 --- a/service/test/integration/test_retrieve_attachment.py +++ b/service/test/integration/test_retrieve_attachment.py @@ -43,6 +43,23 @@ class RetrieveAttachmentTest(SoledadTestBase): self.assertEquals(expected_content_disposition, req.outgoingHeaders['content-disposition']) self.assertEquals(expected_content_type, req.outgoingHeaders['content-type']) + @defer.inlineCallbacks + def test_should_retrieve_attachment_even_if_xsrf_token_not_passed(self): + attachment_id, input_mail = self._create_mail_with_attachment() + yield self.mail_store.add_mail('INBOX', input_mail.as_string()) + + requested_filename = "file name with space" + expected_content_type = 'text/plain' + expected_content_disposition = 'attachment; filename="file name with space"' + + attachment, req = yield self.get_attachment(attachment_id, 'base64', filename=requested_filename, + content_type=expected_content_type, ajax=False, csrf='mismatched token') + + self.assertEqual(200, req.code) + self.assertEquals('pretend to be binary attachment data', attachment) + self.assertEquals(expected_content_disposition, req.outgoingHeaders['content-disposition']) + self.assertEquals(expected_content_type, req.outgoingHeaders['content-type']) + def _create_mail_with_attachment(self): input_mail = MIMEMultipart() input_mail.attach(MIMEText(u'a utf8 message', _charset='utf-8')) diff --git a/service/test/support/integration/app_test_client.py b/service/test/support/integration/app_test_client.py index 8ab58397..a2360a4e 100644 --- a/service/test/support/integration/app_test_client.py +++ b/service/test/support/integration/app_test_client.py @@ -244,22 +244,22 @@ class AppTestClient(object): time.sleep(1) return lambda: process.terminate() - def get(self, path, get_args='', as_json=True): - request = request_mock(path) + def get(self, path, get_args='', as_json=True, ajax=True, csrf='token'): + request = request_mock(path, ajax=ajax, csrf=csrf) request.args = get_args return self._render(request, as_json) - def post(self, path, body='', headers=None): + def post(self, path, body='', headers=None, ajax=True, csrf='token'): headers = headers or {'Content-Type': 'application/json'} - request = request_mock(path=path, method="POST", body=body, headers=headers) + request = request_mock(path=path, method="POST", body=body, headers=headers, ajax=ajax, csrf=csrf) return self._render(request) - def put(self, path, body): - request = request_mock(path=path, method="PUT", body=body, headers={'Content-Type': ['application/json']}) + def put(self, path, body, ajax=True, csrf='token'): + request = request_mock(path=path, method="PUT", body=body, headers={'Content-Type': ['application/json']}, ajax=ajax, csrf=csrf) return self._render(request) - def delete(self, path, body=""): - request = request_mock(path=path, body=body, headers={'Content-Type': ['application/json']}, method="DELETE") + def delete(self, path, body="", ajax=True, csrf='token'): + request = request_mock(path=path, body=body, headers={'Content-Type': ['application/json']}, method="DELETE", ajax=ajax, csrf=csrf) return self._render(request) @defer.inlineCallbacks @@ -322,13 +322,13 @@ class AppTestClient(object): defer.returnValue(mails) @defer.inlineCallbacks - def get_attachment(self, ident, encoding, filename=None, content_type=None): + def get_attachment(self, ident, encoding, filename=None, content_type=None, ajax=True, csrf='token'): params = {'encoding': [encoding]} if filename: params['filename'] = [filename] if content_type: params['content_type'] = [content_type] - deferred_result, req = self.get("/attachment/%s" % ident, params, as_json=False) + deferred_result, req = self.get("/attachment/%s" % ident, params, as_json=False, ajax=ajax, csrf=csrf) res = yield deferred_result defer.returnValue((res, req)) diff --git a/service/test/support/test_helper.py b/service/test/support/test_helper.py index 77c74407..640baf6f 100644 --- a/service/test/support/test_helper.py +++ b/service/test/support/test_helper.py @@ -88,6 +88,7 @@ class PixRequestMock(DummyRequest): DummyRequest.__init__(self, path) self.content = None self.code = None + self.cookies = {} def getWrittenData(self): if len(self.written): @@ -97,8 +98,14 @@ class PixRequestMock(DummyRequest): self.setResponseCode(302) self.setHeader(b"location", url) + def addCookie(self, key, value): + self.cookies[key] = value -def request_mock(path='', method='GET', body='', headers={}): + def getCookie(self, key): + return self.cookies.get(key) + + +def request_mock(path='', method='GET', body='', headers={}, ajax=True, csrf='token'): dummy = PixRequestMock(path.split('/')) for name, val in headers.iteritems(): dummy.headers[name.lower()] = val @@ -108,5 +115,9 @@ def request_mock(path='', method='GET', body='', headers={}): else: for key, val in body.items(): dummy.addArg(key, val) + if ajax: + dummy.headers['x-requested-with'] = 'XMLHttpRequest' + dummy.headers['x-xsrf-token'] = csrf + dummy.addCookie('XSRF-TOKEN', csrf) return dummy diff --git a/service/test/unit/resources/test_root_resource.py b/service/test/unit/resources/test_root_resource.py index 3b0846ee..53481f56 100644 --- a/service/test/unit/resources/test_root_resource.py +++ b/service/test/unit/resources/test_root_resource.py @@ -1,11 +1,13 @@ import unittest import re + +from mock import MagicMock, patch from mockito import mock, when, any as ANY from pixelated.application import UserAgentMode from test.unit.resources import DummySite from twisted.web.test.requesthelper import DummyRequest -from pixelated.resources.root_resource import RootResource +from pixelated.resources.root_resource import RootResource, MODE_STARTUP, MODE_RUNNING class TestRootResource(unittest.TestCase): @@ -25,9 +27,11 @@ class TestRootResource(unittest.TestCase): root_resource._html_template = "$account_email" root_resource._mode = root_resource self.web = DummySite(root_resource) + self.root_resource = root_resource def test_render_GET_should_template_account_email(self): request = DummyRequest(['']) + request.addCookie = lambda key, value: 'stubbed' d = self.web.get(request) @@ -38,3 +42,79 @@ class TestRootResource(unittest.TestCase): d.addCallback(assert_response) return d + + def _test_should_renew_xsrf_cookie(self): + request = DummyRequest(['']) + request.addCookie = MagicMock() + generated_csrf_token = 'csrf_token' + mock_sha = MagicMock() + mock_sha.hexdigest = MagicMock(return_value=generated_csrf_token) + + with patch('hashlib.sha256', return_value=mock_sha): + d = self.web.get(request) + + def assert_csrf_cookie(_): + request.addCookie.assert_called_once_with('XSRF-TOKEN', generated_csrf_token) + + d.addCallback(assert_csrf_cookie) + return d + + def test_should_renew_xsrf_cookie_on_startup_mode(self): + self.root_resource._mode = MODE_STARTUP + self._test_should_renew_xsrf_cookie() + + def test_should_renew_xsrf_cookie_on_running_mode(self): + self.root_resource._mode = MODE_RUNNING + self._test_should_renew_xsrf_cookie() + + def _mock_ajax_csrf(self, request, csrf_token): + request.headers['x-requested-with'] = 'XMLHttpRequest' + request.headers['x-xsrf-token'] = csrf_token + + def test_should_unauthorize_child_resource_ajax_requests_when_csrf_mismatch(self): + request = DummyRequest(['/child']) + self._mock_ajax_csrf(request, 'stubbed csrf token') + + request.getCookie = MagicMock(return_value='mismatched csrf token') + + d = self.web.get(request) + + def assert_unauthorized(_): + self.assertEqual(401, request.responseCode) + self.assertEqual("Unauthorized!", request.written[0]) + + d.addCallback(assert_unauthorized) + return d + + def test_should_authorize_child_resource_non_ajax_GET_requests(self): + request = DummyRequest(['features']) + + request.getCookie = MagicMock(return_value='irrelevant -- stubbed') + self.root_resource.initialize() + + d = self.web.get(request) + + def assert_unauthorized(_): + self.assertEqual(200, request.code) + + d.addCallback(assert_unauthorized) + return d + + def test_should_unauthorize_child_resource_non_ajax_POST_requests_when_csrf_input_mismatch(self): + request = DummyRequest(['mails']) + request.method = 'POST' + request.addArg('csrftoken', 'some csrf token') + mock_content = MagicMock() + mock_content.read = MagicMock(return_value={}) + request.content = mock_content + + request.getCookie = MagicMock(return_value='mismatched csrf token') + + d = self.web.get(request) + + def assert_unauthorized(_): + self.assertEqual(401, request.responseCode) + self.assertEqual("Unauthorized!", request.written[0]) + + d.addCallback(assert_unauthorized) + return d diff --git a/web-ui/app/js/helpers/browser.js b/web-ui/app/js/helpers/browser.js index e5be6667..23aa79c8 100644 --- a/web-ui/app/js/helpers/browser.js +++ b/web-ui/app/js/helpers/browser.js @@ -23,7 +23,14 @@ define([], function () { window.location.replace(url); } + function getCookie(name) { + var value = '; ' + document.cookie; + var parts = value.split('; ' + name + '='); + if (parts.length === 2) return parts.pop().split(';').shift(); + } + return { - redirect: redirect + redirect: redirect, + getCookie: getCookie }; }); diff --git a/web-ui/app/js/helpers/monitored_ajax.js b/web-ui/app/js/helpers/monitored_ajax.js index 1cb720de..dc182d58 100644 --- a/web-ui/app/js/helpers/monitored_ajax.js +++ b/web-ui/app/js/helpers/monitored_ajax.js @@ -36,6 +36,8 @@ define(['page/events', 'views/i18n', 'helpers/browser'], function (events, i18n, } }; + config.headers = {'X-XSRF-TOKEN': browser.getCookie('XSRF-TOKEN')}; + var originalComplete = config.complete; config.complete = function () { if (originalComplete) { @@ -46,15 +48,15 @@ define(['page/events', 'views/i18n', 'helpers/browser'], function (events, i18n, return $.ajax(url, config).fail(function (xmlhttprequest, textstatus, message) { if (!config.skipErrorMessage) { var msg = (xmlhttprequest.responseJSON && xmlhttprequest.responseJSON.message) || - messages[textstatus] || - 'unexpected problem while talking to server'; - on.trigger(document, events.ui.userAlerts.displayMessage, {message: i18n(msg)}); + messages[textstatus] || + 'unexpected problem while talking to server'; + on.trigger(document, events.ui.userAlerts.displayMessage, {message: i18n(msg), class: 'error'}); } if (xmlhttprequest.status === 302) { var redirectUrl = xmlhttprequest.getResponseHeader('Location'); browser.redirect(redirectUrl); - }else if (xmlhttprequest.status === 401) { + } else if (xmlhttprequest.status === 401) { browser.redirect('/'); } diff --git a/web-ui/app/js/services/mail_service.js b/web-ui/app/js/services/mail_service.js index a63d517e..412451cb 100644 --- a/web-ui/app/js/services/mail_service.js +++ b/web-ui/app/js/services/mail_service.js @@ -246,7 +246,7 @@ define( this.trigger(document, events.mails.available, _.merge({tag: this.attr.currentTag, forSearch: this.attr.lastQuery }, this.parseMails(data))); }.bind(this)) .fail(function () { - this.trigger(document, events.ui.userAlerts.displayMessage, { message: i18n('Could not fetch messages') }); + this.trigger(document, events.ui.userAlerts.displayMessage, { message: i18n('Could not fetch messages'), class: 'error' }); }.bind(this)); }; diff --git a/web-ui/test/spec/helpers/browser.spec.js b/web-ui/test/spec/helpers/browser.spec.js new file mode 100644 index 00000000..5b740da8 --- /dev/null +++ b/web-ui/test/spec/helpers/browser.spec.js @@ -0,0 +1,12 @@ +define(['helpers/browser'], function (browser) { + 'use strict'; + + describe('browser ', function() { + it('gets cookie', function() { + document.cookie = 'TWISTED_SESSION=ff895ffc45a4ce140bfc5dda6c61d232; i18next=en-us'; + expect(browser.getCookie('TWISTED_SESSION')).toEqual('ff895ffc45a4ce140bfc5dda6c61d232'); + expect(browser.getCookie('i18next')).toEqual('en-us'); + }); + + }); +}); diff --git a/web-ui/test/spec/helpers/monitored_ajax_call.spec.js b/web-ui/test/spec/helpers/monitored_ajax_call.spec.js index 972ca3ae..c0d55198 100644 --- a/web-ui/test/spec/helpers/monitored_ajax_call.spec.js +++ b/web-ui/test/spec/helpers/monitored_ajax_call.spec.js @@ -1,6 +1,24 @@ define(['helpers/monitored_ajax'], function (monitoredAjax) { 'use strict'; describe('monitoredAjaxCall', function () { + + describe('default configs', function () { + + it('should always attach the xsrf token in the header', function () { + var component = { trigger: function () {}}; + var d = $.Deferred(); + spyOn($, 'ajax').and.returnValue(d); + document.cookie = 'XSRF-TOKEN=ff895ffc45a4ce140bfc5dda6c61d232; i18next=en-us'; + var anyUrl = '/'; + + monitoredAjax(component, anyUrl, {}); + + expect($.ajax.calls.mostRecent().args[1].headers).toEqual({ 'X-XSRF-TOKEN' : 'ff895ffc45a4ce140bfc5dda6c61d232' }); + + }); + + }); + describe('when dealing with errors', function () { _.each( @@ -19,7 +37,7 @@ define(['helpers/monitored_ajax'], function (monitoredAjax) { d.reject({ responseJSON: {}}, errorType, ''); expect(component.trigger).toHaveBeenCalledWith(document, Pixelated.events.ui.userAlerts.displayMessage, - { message: errorMessage }); + { message: errorMessage, class: 'error' }); }); }); @@ -33,7 +51,7 @@ define(['helpers/monitored_ajax'], function (monitoredAjax) { d.reject({ responseJSON: { message: 'Server Message'}}, 'error', ''); expect(component.trigger).toHaveBeenCalledWith(document, Pixelated.events.ui.userAlerts.displayMessage, - { message: 'Server Message' }); + { message: 'Server Message', class: 'error' }); }); }); @@ -76,4 +94,4 @@ define(['helpers/monitored_ajax'], function (monitoredAjax) { }); }); -}); \ No newline at end of file +}); -- cgit v1.2.3