From 460d47e80db87fbc710259b34a859948cd3b6362 Mon Sep 17 00:00:00 2001 From: Patrick Hayes Date: Sat, 23 Apr 2016 18:07:51 -0700 Subject: [PATCH 01/13] Add invalidate_corrupt to CookieSession --- beaker/session.py | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/beaker/session.py b/beaker/session.py index cbd76944..adddaf8e 100644 --- a/beaker/session.py +++ b/beaker/session.py @@ -169,7 +169,7 @@ def __init__(self, request, id=None, invalidate_corrupt=False, try: self.load() except Exception as e: - if invalidate_corrupt: + if self.invalidate_corrupt: util.warn( "Invalidating corrupt session %s; " "error was: %s. Set invalidate_corrupt=False " @@ -498,13 +498,19 @@ class CookieSession(Session): :param encrypt_key: The key to use for the local session encryption, if not provided the session will not be encrypted. :param validate_key: The key used to sign the local encrypted session + :param invalidate_corrupt: How to handle corrupt data when loading. When + set to True, then corrupt data will be silently + invalidated and a new session created, + otherwise invalid data will cause an exception. + :type invalidate_corrupt: bool """ def __init__(self, request, key='beaker.session.id', timeout=None, cookie_expires=True, cookie_domain=None, cookie_path='/', encrypt_key=None, validate_key=None, secure=False, httponly=False, data_serializer='pickle', - encrypt_nonce_bits=DEFAULT_NONCE_BITS, **kwargs): + encrypt_nonce_bits=DEFAULT_NONCE_BITS, invalidate_corrupt=False, + **kwargs): if not crypto.has_aes and encrypt_key: raise InvalidCryptoBackendError("No AES library is installed, can't generate " @@ -523,6 +529,7 @@ def __init__(self, request, key='beaker.session.id', timeout=None, self._domain = cookie_domain self._path = cookie_path self.data_serializer = data_serializer + self.invalidate_corrupt = invalidate_corrupt try: cookieheader = request['cookie'] @@ -548,8 +555,15 @@ def __init__(self, request, key='beaker.session.id', timeout=None, cookie_data = self.cookie[self.key].value self.update(self._decrypt_data(cookie_data)) self._path = self.get('_path', '/') - except: - pass + except Exception as e: + if self.invalidate_corrupt: + util.warn( + "Invalidating corrupt session %s; " + "error was: %s. Set invalidate_corrupt=False " + "to propagate this exception." % (self.id, e)) + self.invalidate() + else: + raise if self.timeout is not None: now = time.time() From 634c5cfbaa166b207d983b2aa4d2ca4cef50c011 Mon Sep 17 00:00:00 2001 From: Patrick Hayes Date: Sat, 23 Apr 2016 18:08:18 -0700 Subject: [PATCH 02/13] Don't double catch invalidation errors --- beaker/session.py | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/beaker/session.py b/beaker/session.py index adddaf8e..caf2edd8 100644 --- a/beaker/session.py +++ b/beaker/session.py @@ -279,31 +279,16 @@ def _decrypt_data(self, session_data): """Bas64, decipher, then un-serialize the data for the session dict""" if self.encrypt_key: - try: - __, nonce_b64len = self.encrypt_nonce_size - nonce = session_data[:nonce_b64len] - encrypt_key = crypto.generateCryptoKeys(self.encrypt_key, - self.validate_key + nonce, 1) - payload = b64decode(session_data[nonce_b64len:]) - data = crypto.aesDecrypt(payload, encrypt_key) - except: - # As much as I hate a bare except, we get some insane errors - # here that get tossed when crypto fails, so we raise the - # 'right' exception - if self.invalidate_corrupt: - return None - else: - raise + __, nonce_b64len = self.encrypt_nonce_size + nonce = session_data[:nonce_b64len] + encrypt_key = crypto.generateCryptoKeys(self.encrypt_key, + self.validate_key + nonce, 1) + payload = b64decode(session_data[nonce_b64len:]) + data = crypto.aesDecrypt(payload, encrypt_key) else: data = b64decode(session_data) - try: - return util.deserialize(data, self.data_serializer) - except: - if self.invalidate_corrupt: - return None - else: - raise + return util.deserialize(data, self.data_serializer) def _delete_cookie(self): self.request['set_cookie'] = True From 67ed5710ce4f0e624b6c5566cdbaed3243521095 Mon Sep 17 00:00:00 2001 From: Patrick Hayes Date: Sat, 23 Apr 2016 18:08:37 -0700 Subject: [PATCH 03/13] Update session tests for CookieSession --- tests/test_session.py | 131 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 107 insertions(+), 24 deletions(-) diff --git a/tests/test_session.py b/tests/test_session.py index 926dabbe..131bab74 100644 --- a/tests/test_session.py +++ b/tests/test_session.py @@ -5,10 +5,10 @@ import time import warnings -from nose import SkipTest +from nose import SkipTest, with_setup from beaker.crypto import has_aes -from beaker.session import Session +from beaker.session import CookieSession, Session, SignedCookie from beaker import util @@ -19,15 +19,51 @@ def get_session(**kwargs): return Session({}, **options) -def test_save_load(): +COOKIE_REQUEST = {} +def setup_cookie_request(): + COOKIE_REQUEST.clear() + + +def get_cookie_session(**kwargs): + """A shortcut for creating :class:`CookieSession` instance""" + options = {'validate_key': 'test_key'} + options.update(**kwargs) + if COOKIE_REQUEST.get('set_cookie'): + cookie_out = COOKIE_REQUEST.get('cookie_out') + key = 'beaker.session.id' + cookie_out = cookie_out[cookie_out.index(key) + len(key) + 1:] + cookie_out = cookie_out[:cookie_out.index(';')] + + COOKIE_REQUEST['cookie'] = {key: cookie_out} + return CookieSession(COOKIE_REQUEST, **options) + + +@with_setup(setup_cookie_request) +def test_session(): + for test_case in ( + check_save_load, + check_save_load_encryption, + check_save_load_encryption_cookie, + check_decryption_failure, + check_delete, + check_revert, + check_invalidate, + check_timeout, + ): + for session_getter in (get_session,): + setup_cookie_request() + yield test_case, session_getter + + +def check_save_load(session_getter): """Test if the data is actually persistent across requests""" - session = get_session() + session = session_getter() session[u_('Suomi')] = u_('Kimi Räikkönen') session[u_('Great Britain')] = u_('Jenson Button') session[u_('Deutchland')] = u_('Sebastian Vettel') session.save() - session = get_session(id=session.id) + session = session_getter(id=session.id) assert u_('Suomi') in session assert u_('Great Britain') in session assert u_('Deutchland') in session @@ -37,18 +73,18 @@ def test_save_load(): assert session[u_('Deutchland')] == u_('Sebastian Vettel') -def test_save_load_encryption(): +def check_save_load_encryption(session_getter): """Test if the data is actually persistent across requests""" if not has_aes: raise SkipTest() - session = get_session(encrypt_key='666a19cf7f61c64c', + session = session_getter(encrypt_key='666a19cf7f61c64c', validate_key='hoobermas') session[u_('Suomi')] = u_('Kimi Räikkönen') session[u_('Great Britain')] = u_('Jenson Button') session[u_('Deutchland')] = u_('Sebastian Vettel') session.save() - session = get_session(id=session.id, encrypt_key='666a19cf7f61c64c', + session = session_getter(id=session.id, encrypt_key='666a19cf7f61c64c', validate_key='hoobermas') assert u_('Suomi') in session assert u_('Great Britain') in session @@ -59,26 +95,48 @@ def test_save_load_encryption(): assert session[u_('Deutchland')] == u_('Sebastian Vettel') -def test_decryption_failure(): +def check_save_load_encryption_cookie(session_getter): + """Test if the data is actually persistent across requests""" + if not has_aes: + raise SkipTest() + session = session_getter(encrypt_key='666a19cf7f61c64c', + validate_key='hoobermas') + session[u_('Suomi')] = u_('Kimi Räikkönen') + session[u_('Great Britain')] = u_('Jenson Button') + session[u_('Deutchland')] = u_('Sebastian Vettel') + session.save() + + session = session_getter(id=session.id, encrypt_key='666a19cf7f61c64c', + validate_key='hoobermas') + assert u_('Suomi') in session + assert u_('Great Britain') in session + assert u_('Deutchland') in session + + assert session[u_('Suomi')] == u_('Kimi Räikkönen') + assert session[u_('Great Britain')] == u_('Jenson Button') + assert session[u_('Deutchland')] == u_('Sebastian Vettel') + + +def check_decryption_failure(session_getter): """Test if the data fails without the right keys""" if not has_aes: raise SkipTest() - session = get_session(encrypt_key='666a19cf7f61c64c', + session = session_getter(encrypt_key='666a19cf7f61c64c', validate_key='hoobermas') session[u_('Suomi')] = u_('Kimi Räikkönen') session[u_('Great Britain')] = u_('Jenson Button') session[u_('Deutchland')] = u_('Sebastian Vettel') session.save() - session = get_session(id=session.id, encrypt_key='asfdasdfadsfsadf', + session = session_getter(id=session.id, encrypt_key='asfdasdfadsfsadf', validate_key='hoobermas', invalidate_corrupt=True) assert u_('Suomi') not in session assert u_('Great Britain') not in session -def test_delete(): +def check_delete(session_getter): """Test :meth:`Session.delete`""" - session = get_session() + session = session_getter() session[u_('Suomi')] = u_('Kimi Räikkönen') session[u_('Great Britain')] = u_('Jenson Button') session[u_('Deutchland')] = u_('Sebastian Vettel') @@ -89,15 +147,15 @@ def test_delete(): assert u_('Deutchland') not in session -def test_revert(): +def check_revert(session_getter): """Test :meth:`Session.revert`""" - session = get_session() + session = session_getter() session[u_('Suomi')] = u_('Kimi Räikkönen') session[u_('Great Britain')] = u_('Jenson Button') session[u_('Deutchland')] = u_('Sebastian Vettel') session.save() - session = get_session(id=session.id) + session = session_getter(id=session.id) del session[u_('Suomi')] session[u_('Great Britain')] = u_('Lewis Hamilton') session[u_('Deutchland')] = u_('Michael Schumacher') @@ -110,15 +168,17 @@ def test_revert(): assert u_('España') not in session -def test_invalidate(): +def check_invalidate(session_getter): """Test :meth:`Session.invalidate`""" - session = get_session() + session = session_getter() + session.save() id = session.id created = session.created session[u_('Suomi')] = u_('Kimi Räikkönen') session[u_('Great Britain')] = u_('Jenson Button') session[u_('Deutchland')] = u_('Sebastian Vettel') session.invalidate() + session.save() assert session.id != id assert session.created != created @@ -127,6 +187,7 @@ def test_invalidate(): assert u_('Deutchland') not in session +@with_setup(setup_cookie_request) def test_regenerate_id(): """Test :meth:`Session.regenerate_id`""" # new session & save @@ -163,9 +224,10 @@ def test_regenerate_id(): assert session[u_('foo')] == u_('bar') -def test_timeout(): +def check_timeout(session_getter): """Test if the session times out properly""" - session = get_session(timeout=2) + session = session_getter(timeout=2) + session.save() id = session.id created = session.created session[u_('Suomi')] = u_('Kimi Räikkönen') @@ -173,7 +235,7 @@ def test_timeout(): session[u_('Deutchland')] = u_('Sebastian Vettel') session.save() - session = get_session(id=session.id, timeout=2) + session = session_getter(id=session.id, timeout=2) assert session.id == id assert session.created == created assert session[u_('Suomi')] == u_('Kimi Räikkönen') @@ -181,7 +243,7 @@ def test_timeout(): assert session[u_('Deutchland')] == u_('Sebastian Vettel') time.sleep(2) - session = get_session(id=session.id, timeout=2) + session = session_getter(id=session.id, timeout=2) assert session.id != id assert session.created != created assert u_('Suomi') not in session @@ -189,6 +251,7 @@ def test_timeout(): assert u_('Deutchland') not in session +@with_setup(setup_cookie_request) def test_cookies_enabled(): """ Test if cookies are sent out properly when ``use_cookies`` @@ -239,7 +302,7 @@ def __call__(self, message, category, filename, lineno, file=None, line=None): assert 'httponly' in cookie, cookie warnings.showwarning = orig_sw -def test_cookies_disabled(): +def tes_cookies_disabled(): """ Test that no cookies are sent when ``use_cookies`` is set to ``False`` """ @@ -260,6 +323,7 @@ def test_cookies_disabled(): assert 'cookie_out' not in session.request +@with_setup(setup_cookie_request) def test_file_based_replace_optimization(): """Test the file-based backend with session, which includes the 'replace' optimization. @@ -305,6 +369,7 @@ def test_file_based_replace_optimization(): session.namespace.do_close() +@with_setup(setup_cookie_request) def test_invalidate_corrupt(): session = get_session(use_cookies=False, type='file', data_dir='./cache') @@ -316,7 +381,7 @@ def test_invalidate_corrupt(): f.close() util.assert_raises( - pickle.UnpicklingError, + (pickle.UnpicklingError, EOFError, TypeError), get_session, use_cookies=False, type='file', data_dir='./cache', id=session.id @@ -326,3 +391,21 @@ def test_invalidate_corrupt(): invalidate_corrupt=True, data_dir='./cache', id=session.id) assert "foo" not in dict(session) + + +@with_setup(setup_cookie_request) +def test_invalidate_corrupt_cookie(): + session = get_cookie_session() + session['foo'] = 'bar' + session.save() + + COOKIE_REQUEST['cookie_out'] = ' beaker.session.id=fakecookie; Path=/' + + util.assert_raises( + (pickle.UnpicklingError, EOFError, TypeError), + get_cookie_session, + id=session.id + ) + + session = get_cookie_session(invalidate_corrupt=True, id=session.id) + assert "foo" not in dict(session) From 090c6f07d3741e7e3af5b91cda7693fb4b17174c Mon Sep 17 00:00:00 2001 From: Patrick Hayes Date: Sat, 23 Apr 2016 18:12:35 -0700 Subject: [PATCH 04/13] Remove duplicate test --- tests/test_session.py | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/tests/test_session.py b/tests/test_session.py index 131bab74..87b8ecf9 100644 --- a/tests/test_session.py +++ b/tests/test_session.py @@ -43,7 +43,6 @@ def test_session(): for test_case in ( check_save_load, check_save_load_encryption, - check_save_load_encryption_cookie, check_decryption_failure, check_delete, check_revert, @@ -95,28 +94,6 @@ def check_save_load_encryption(session_getter): assert session[u_('Deutchland')] == u_('Sebastian Vettel') -def check_save_load_encryption_cookie(session_getter): - """Test if the data is actually persistent across requests""" - if not has_aes: - raise SkipTest() - session = session_getter(encrypt_key='666a19cf7f61c64c', - validate_key='hoobermas') - session[u_('Suomi')] = u_('Kimi Räikkönen') - session[u_('Great Britain')] = u_('Jenson Button') - session[u_('Deutchland')] = u_('Sebastian Vettel') - session.save() - - session = session_getter(id=session.id, encrypt_key='666a19cf7f61c64c', - validate_key='hoobermas') - assert u_('Suomi') in session - assert u_('Great Britain') in session - assert u_('Deutchland') in session - - assert session[u_('Suomi')] == u_('Kimi Räikkönen') - assert session[u_('Great Britain')] == u_('Jenson Button') - assert session[u_('Deutchland')] == u_('Sebastian Vettel') - - def check_decryption_failure(session_getter): """Test if the data fails without the right keys""" if not has_aes: From fd4c4bf3d227678ef8e0dfaa468f38678c81b76b Mon Sep 17 00:00:00 2001 From: Patrick Hayes Date: Sat, 23 Apr 2016 18:56:46 -0700 Subject: [PATCH 05/13] Add additional error type --- tests/test_session.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/test_session.py b/tests/test_session.py index 87b8ecf9..538de01a 100644 --- a/tests/test_session.py +++ b/tests/test_session.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- from beaker._compat import u_, pickle +import binascii import sys import time import warnings @@ -8,7 +9,7 @@ from nose import SkipTest, with_setup from beaker.crypto import has_aes -from beaker.session import CookieSession, Session, SignedCookie +from beaker.session import CookieSession, Session from beaker import util @@ -358,7 +359,7 @@ def test_invalidate_corrupt(): f.close() util.assert_raises( - (pickle.UnpicklingError, EOFError, TypeError), + (pickle.UnpicklingError, EOFError, TypeError, binascii.Error), get_session, use_cookies=False, type='file', data_dir='./cache', id=session.id @@ -379,7 +380,7 @@ def test_invalidate_corrupt_cookie(): COOKIE_REQUEST['cookie_out'] = ' beaker.session.id=fakecookie; Path=/' util.assert_raises( - (pickle.UnpicklingError, EOFError, TypeError), + (pickle.UnpicklingError, EOFError, TypeError, binascii.Error), get_cookie_session, id=session.id ) From 577974d8d9a26c6e8b93898265d193f712ebc0b1 Mon Sep 17 00:00:00 2001 From: Patrick Hayes Date: Sat, 23 Apr 2016 19:32:39 -0700 Subject: [PATCH 06/13] Fix cookie handling --- tests/test_session.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/tests/test_session.py b/tests/test_session.py index 538de01a..a686e27d 100644 --- a/tests/test_session.py +++ b/tests/test_session.py @@ -30,12 +30,7 @@ def get_cookie_session(**kwargs): options = {'validate_key': 'test_key'} options.update(**kwargs) if COOKIE_REQUEST.get('set_cookie'): - cookie_out = COOKIE_REQUEST.get('cookie_out') - key = 'beaker.session.id' - cookie_out = cookie_out[cookie_out.index(key) + len(key) + 1:] - cookie_out = cookie_out[:cookie_out.index(';')] - - COOKIE_REQUEST['cookie'] = {key: cookie_out} + COOKIE_REQUEST['cookie'] = COOKIE_REQUEST.get('cookie_out') return CookieSession(COOKIE_REQUEST, **options) From ce47a03b0f62fe93aa619f47ddf04eea034a2a3b Mon Sep 17 00:00:00 2001 From: Patrick Hayes Date: Sun, 24 Apr 2016 11:34:20 -0700 Subject: [PATCH 07/13] Propagate invalidate_corrupt to signature validation I set invalidate_corrupt to default to True inside SignedCookie for backwards compatibility reasons (if any client code instantiates SignedCookie directly). But this will extend the handling of invalidate_corrupt to a case it did not previously handle. Before this, if invalidate_corrupt was False, it was still possible to silently invalidate the session if the signature was invalid. --- beaker/session.py | 37 ++++++++++++++++++++++++++++++------- tests/test_session.py | 33 +++++++++++++++++++++++++++------ 2 files changed, 57 insertions(+), 13 deletions(-) diff --git a/beaker/session.py b/beaker/session.py index caf2edd8..ba0b8453 100644 --- a/beaker/session.py +++ b/beaker/session.py @@ -44,8 +44,9 @@ def _session_id(): class SignedCookie(SimpleCookie): """Extends python cookie to give digital signature support""" - def __init__(self, secret, input=None): + def __init__(self, secret, input=None, invalidate_corrupt=True): self.secret = secret.encode('UTF-8') + self.invalidate_corrupt = invalidate_corrupt http_cookies.BaseCookie.__init__(self, input) def value_decode(self, val): @@ -56,13 +57,19 @@ def value_decode(self, val): invalid_bits = 0 input_sig = val[:40] if len(sig) != len(input_sig): - return None, val + if self.invalidate_corrupt: + return None, val + else: + raise BeakerException("Invalid signature") for a, b in zip(sig, input_sig): invalid_bits += a != b if invalid_bits: - return None, val + if self.invalidate_corrupt: + return None, val + else: + raise BeakerException("Invalid signature") else: return val[40:], val @@ -152,9 +159,17 @@ def __init__(self, request, id=None, invalidate_corrupt=False, cookieheader = request.get('cookie', '') if secret: try: - self.cookie = SignedCookie(secret, input=cookieheader) + self.cookie = SignedCookie( + secret, + input=cookieheader, + invalidate_corrupt=self.invalidate_corrupt, + ) except http_cookies.CookieError: - self.cookie = SignedCookie(secret, input=None) + self.cookie = SignedCookie( + secret, + input=None, + invalidate_corrupt=self.invalidate_corrupt, + ) else: self.cookie = SimpleCookie(input=cookieheader) @@ -526,9 +541,17 @@ def __init__(self, request, key='beaker.session.id', timeout=None, "Session.") try: - self.cookie = SignedCookie(validate_key, input=cookieheader) + self.cookie = SignedCookie( + validate_key, + input=cookieheader, + invalidate_corrupt=self.invalidate_corrupt, + ) except http_cookies.CookieError: - self.cookie = SignedCookie(validate_key, input=None) + self.cookie = SignedCookie( + validate_key, + input=None, + invalidate_corrupt=self.invalidate_corrupt, + ) self['_id'] = _session_id() self.is_new = True diff --git a/tests/test_session.py b/tests/test_session.py index a686e27d..6586a0ff 100644 --- a/tests/test_session.py +++ b/tests/test_session.py @@ -9,6 +9,7 @@ from nose import SkipTest, with_setup from beaker.crypto import has_aes +from beaker.exceptions import BeakerException from beaker.session import CookieSession, Session from beaker import util @@ -367,18 +368,38 @@ def test_invalidate_corrupt(): @with_setup(setup_cookie_request) -def test_invalidate_corrupt_cookie(): - session = get_cookie_session() +def test_invalidate_invalid_signed_cookie(): + kwargs = {'validate_key': 'test_key', 'encrypt_key': 'encrypt'} + session = get_cookie_session(**kwargs) session['foo'] = 'bar' session.save() - COOKIE_REQUEST['cookie_out'] = ' beaker.session.id=fakecookie; Path=/' + COOKIE_REQUEST['cookie_out'] = ( + COOKIE_REQUEST['cookie_out'][:20] + + 'aaaaa' + + COOKIE_REQUEST['cookie_out'][25:] + ) util.assert_raises( - (pickle.UnpicklingError, EOFError, TypeError, binascii.Error), + BeakerException, get_cookie_session, - id=session.id + id=session.id, + invalidate_corrupt=False, + ) + + +@with_setup(setup_cookie_request) +def test_invalidate_invalid_signed_cookie_invalidate_corrupt(): + kwargs = {'validate_key': 'test_key', 'encrypt_key': 'encrypt'} + session = get_cookie_session(**kwargs) + session['foo'] = 'bar' + session.save() + + COOKIE_REQUEST['cookie_out'] = ( + COOKIE_REQUEST['cookie_out'][:20] + + 'aaaaa' + + COOKIE_REQUEST['cookie_out'][25:] ) - session = get_cookie_session(invalidate_corrupt=True, id=session.id) + session = get_cookie_session(id=session.id, invalidate_corrupt=True, **kwargs) assert "foo" not in dict(session) From 8cc89b1df6cec068dcd63d316651fdf327baa356 Mon Sep 17 00:00:00 2001 From: Patrick Hayes Date: Sun, 24 Apr 2016 12:05:38 -0700 Subject: [PATCH 08/13] Treat empty cookies as unset --- beaker/session.py | 3 +++ tests/test_session.py | 12 ++++++++++++ 2 files changed, 15 insertions(+) diff --git a/beaker/session.py b/beaker/session.py index ba0b8453..a9c63da6 100644 --- a/beaker/session.py +++ b/beaker/session.py @@ -51,6 +51,9 @@ def __init__(self, secret, input=None, invalidate_corrupt=True): def value_decode(self, val): val = val.strip('"') + if not val: + return None, val + sig = HMAC.new(self.secret, val[40:].encode('utf-8'), SHA1).hexdigest() # Avoid timing attacks diff --git a/tests/test_session.py b/tests/test_session.py index 6586a0ff..6deb9495 100644 --- a/tests/test_session.py +++ b/tests/test_session.py @@ -367,6 +367,18 @@ def test_invalidate_corrupt(): assert "foo" not in dict(session) +@with_setup(setup_cookie_request) +def test_invalidate_empty_cookie(): + kwargs = {'validate_key': 'test_key', 'encrypt_key': 'encrypt'} + session = get_cookie_session(**kwargs) + session['foo'] = 'bar' + session.save() + + COOKIE_REQUEST['cookie_out'] = ' beaker.session.id=' + session = get_cookie_session(id=session.id, invalidate_corrupt=False, **kwargs) + assert "foo" not in dict(session) + + @with_setup(setup_cookie_request) def test_invalidate_invalid_signed_cookie(): kwargs = {'validate_key': 'test_key', 'encrypt_key': 'encrypt'} From b7aaf48431ab293157d8539feb85f9d29301d34c Mon Sep 17 00:00:00 2001 From: Patrick Hayes Date: Sun, 24 Apr 2016 12:31:48 -0700 Subject: [PATCH 09/13] Bring back missing test cases --- tests/test_session.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_session.py b/tests/test_session.py index 6deb9495..dfe74483 100644 --- a/tests/test_session.py +++ b/tests/test_session.py @@ -46,7 +46,7 @@ def test_session(): check_invalidate, check_timeout, ): - for session_getter in (get_session,): + for session_getter in (get_session, get_cookie_session,): setup_cookie_request() yield test_case, session_getter From ec0e3f50a43fbcaf30491b126b4ed6dbcd96cb0c Mon Sep 17 00:00:00 2001 From: Patrick Hayes Date: Sun, 24 Apr 2016 12:31:58 -0700 Subject: [PATCH 10/13] Fix invalid signature handling Previous implementation would throw invalid signature errors on unrelated cookies, which is undesirable This changes the interface of SignedCookie a bit which could affect external callers. Any recommendations on this? We could also make a new ValidatingSignedCookie class or something that does this and deprecate the old one. --- beaker/docs/modules/session.rst | 3 +- beaker/session.py | 49 +++++++++++++++++++-------------- tests/test_session.py | 12 ++++++++ 3 files changed, 42 insertions(+), 22 deletions(-) diff --git a/beaker/docs/modules/session.rst b/beaker/docs/modules/session.rst index 05aab40a..b9246329 100644 --- a/beaker/docs/modules/session.rst +++ b/beaker/docs/modules/session.rst @@ -1,4 +1,4 @@ -:mod:`beaker.session` -- Session classes +:mod:`beaker.session` -- Session classes ======================================== .. automodule:: beaker.session @@ -13,3 +13,4 @@ Module Contents .. autoclass:: SessionObject :members: persist, get_by_id, accessed .. autoclass:: SignedCookie +.. autodata:: InvalidSignature diff --git a/beaker/session.py b/beaker/session.py index a9c63da6..a19dc8bd 100644 --- a/beaker/session.py +++ b/beaker/session.py @@ -9,7 +9,19 @@ from beaker.exceptions import BeakerException, InvalidCryptoBackendError from beaker.cookie import SimpleCookie -__all__ = ['SignedCookie', 'Session'] +__all__ = ['SignedCookie', 'Session', 'InvalidSignature'] + + +class _InvalidSignatureType(object): + """Returned from SignedCookie when the value's signature was invalid.""" + def __nonzero__(self): + return False + + def __bool__(self): + return False + + +InvalidSignature = _InvalidSignatureType() try: @@ -42,11 +54,10 @@ def _session_id(): return raw_id.replace('+', '-').replace('/', '_').rstrip('=') -class SignedCookie(SimpleCookie): +class _SignedCookie(SimpleCookie): """Extends python cookie to give digital signature support""" - def __init__(self, secret, input=None, invalidate_corrupt=True): + def __init__(self, secret, input=None): self.secret = secret.encode('UTF-8') - self.invalidate_corrupt = invalidate_corrupt http_cookies.BaseCookie.__init__(self, input) def value_decode(self, val): @@ -60,19 +71,13 @@ def value_decode(self, val): invalid_bits = 0 input_sig = val[:40] if len(sig) != len(input_sig): - if self.invalidate_corrupt: - return None, val - else: - raise BeakerException("Invalid signature") + return InvalidSignature, val for a, b in zip(sig, input_sig): invalid_bits += a != b if invalid_bits: - if self.invalidate_corrupt: - return None, val - else: - raise BeakerException("Invalid signature") + return InvalidSignature, val else: return val[40:], val @@ -162,22 +167,24 @@ def __init__(self, request, id=None, invalidate_corrupt=False, cookieheader = request.get('cookie', '') if secret: try: - self.cookie = SignedCookie( + self.cookie = _SignedCookie( secret, input=cookieheader, - invalidate_corrupt=self.invalidate_corrupt, ) except http_cookies.CookieError: - self.cookie = SignedCookie( + self.cookie = _SignedCookie( secret, input=None, - invalidate_corrupt=self.invalidate_corrupt, ) else: self.cookie = SimpleCookie(input=cookieheader) if not self.id and self.key in self.cookie: - self.id = self.cookie[self.key].value + cookie_data = self.cookie[self.key].value + # Should we check invalidate_corrupt here? + if cookie_data is InvalidSignature: + cookie_data = None + self.id = cookie_data self.is_new = self.id is None if self.is_new: @@ -544,16 +551,14 @@ def __init__(self, request, key='beaker.session.id', timeout=None, "Session.") try: - self.cookie = SignedCookie( + self.cookie = _SignedCookie( validate_key, input=cookieheader, - invalidate_corrupt=self.invalidate_corrupt, ) except http_cookies.CookieError: - self.cookie = SignedCookie( + self.cookie = _SignedCookie( validate_key, input=None, - invalidate_corrupt=self.invalidate_corrupt, ) self['_id'] = _session_id() @@ -564,6 +569,8 @@ def __init__(self, request, key='beaker.session.id', timeout=None, self.is_new = False try: cookie_data = self.cookie[self.key].value + if cookie_data is InvalidSignature: + raise BeakerException("Invalid signature") self.update(self._decrypt_data(cookie_data)) self._path = self.get('_path', '/') except Exception as e: diff --git a/tests/test_session.py b/tests/test_session.py index dfe74483..1a2aa304 100644 --- a/tests/test_session.py +++ b/tests/test_session.py @@ -379,6 +379,18 @@ def test_invalidate_empty_cookie(): assert "foo" not in dict(session) +@with_setup(setup_cookie_request) +def test_unrelated_cookie(): + kwargs = {'validate_key': 'test_key', 'encrypt_key': 'encrypt'} + session = get_cookie_session(**kwargs) + session['foo'] = 'bar' + session.save() + + COOKIE_REQUEST['cookie_out'] = COOKIE_REQUEST['cookie_out'] + '; some.other=cookie' + session = get_cookie_session(id=session.id, invalidate_corrupt=False, **kwargs) + assert "foo" in dict(session) + + @with_setup(setup_cookie_request) def test_invalidate_invalid_signed_cookie(): kwargs = {'validate_key': 'test_key', 'encrypt_key': 'encrypt'} From ff0e73d7fdf1371b421b78c9b007d3891aff3d4d Mon Sep 17 00:00:00 2001 From: Patrick Hayes Date: Sun, 24 Apr 2016 12:54:26 -0700 Subject: [PATCH 11/13] Oops, didnt mean to push renamed class --- beaker/session.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/beaker/session.py b/beaker/session.py index a19dc8bd..f79f585f 100644 --- a/beaker/session.py +++ b/beaker/session.py @@ -54,7 +54,7 @@ def _session_id(): return raw_id.replace('+', '-').replace('/', '_').rstrip('=') -class _SignedCookie(SimpleCookie): +class SignedCookie(SimpleCookie): """Extends python cookie to give digital signature support""" def __init__(self, secret, input=None): self.secret = secret.encode('UTF-8') @@ -167,12 +167,12 @@ def __init__(self, request, id=None, invalidate_corrupt=False, cookieheader = request.get('cookie', '') if secret: try: - self.cookie = _SignedCookie( + self.cookie = SignedCookie( secret, input=cookieheader, ) except http_cookies.CookieError: - self.cookie = _SignedCookie( + self.cookie = SignedCookie( secret, input=None, ) @@ -551,12 +551,12 @@ def __init__(self, request, key='beaker.session.id', timeout=None, "Session.") try: - self.cookie = _SignedCookie( + self.cookie = SignedCookie( validate_key, input=cookieheader, ) except http_cookies.CookieError: - self.cookie = _SignedCookie( + self.cookie = SignedCookie( validate_key, input=None, ) From fed68c256b5e11031766657b0a0c1cb69d603e50 Mon Sep 17 00:00:00 2001 From: Patrick Hayes Date: Wed, 14 Sep 2016 15:10:50 -0700 Subject: [PATCH 12/13] Update method name --- tests/test_session.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_session.py b/tests/test_session.py index 1a2aa304..6250a423 100644 --- a/tests/test_session.py +++ b/tests/test_session.py @@ -276,7 +276,7 @@ def __call__(self, message, category, filename, lineno, file=None, line=None): assert 'httponly' in cookie, cookie warnings.showwarning = orig_sw -def tes_cookies_disabled(): +def test_cookies_disabled(): """ Test that no cookies are sent when ``use_cookies`` is set to ``False`` """ From 03ea0978e8098e633a9b9ef924629b74c5aebac0 Mon Sep 17 00:00:00 2001 From: Patrick Hayes Date: Thu, 15 Sep 2016 10:20:00 -0700 Subject: [PATCH 13/13] typo --- tests/test_session.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_session.py b/tests/test_session.py index 6c46cc44..45448d55 100644 --- a/tests/test_session.py +++ b/tests/test_session.py @@ -13,7 +13,7 @@ from beaker.container import MemoryNamespaceManager from beaker.crypto import has_aes from beaker.exceptions import BeakerException -from beaker.session import CookieSession, Session, SesssionObject +from beaker.session import CookieSession, Session, SessionObject from beaker.util import assert_raises