From b9279ae488500fb669e9a46324adee21040692f5 Mon Sep 17 00:00:00 2001 From: Michael Schlenker Date: Wed, 7 Jun 2017 19:39:11 +0200 Subject: [PATCH 1/2] Improve cookie crypto for CookieDealer Added per cookie IV's to the CookieDealers encryption handling. This fixes #363. Also restyled the encrypt and MAC construction for cookie security to use a more modern AEAD approach. In this case it is AES-SIV (RFC 5297), which has the nice property to be a bit resistant to IV reuse. --- CHANGELOG.md | 4 + src/oic/utils/aes.py | 83 +++++++++++++++++ src/oic/utils/http_util.py | 177 +++++++++++++++++++++++++++---------- tests/test_aes.py | 42 +++++++++ tests/test_http_util.py | 38 +++++++- 5 files changed, 295 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bfcc55959..7a73c1f99 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,8 +13,12 @@ The format is based on the [KeepAChangeLog] project. ### Fixed - [#369]: The AuthnEvent object is now serialized to JSON for the session. +### Security +- [#363]: Fixed IV reuse for CookieDealer class. Replaced the encrypt-then-mac construction with a proper AEAD (AES-SIV). + [#324]: https://github.com/OpenIDC/pyoidc/pull/324 [#369]: https://github.com/OpenIDC/pyoidc/pull/369 +[#363]: https://github.com/OpenIDC/pyoidc/issue/363 ## 0.10.0.1 [UNRELEASED] diff --git a/src/oic/utils/aes.py b/src/oic/utils/aes.py index 2bd534ad4..ce3c3a1e1 100644 --- a/src/oic/utils/aes.py +++ b/src/oic/utils/aes.py @@ -7,7 +7,9 @@ from Cryptodome import Random from Cryptodome.Cipher import AES +from six import binary_type from six import indexbytes +from six import text_type __author__ = 'rolandh' @@ -105,6 +107,87 @@ def decrypt(key, msg, iv=None, padding="PKCS#7", b64dec=True): return res.decode("utf-8") +class AEAD(object): + """ + Authenticated Encryption with Associated Data Wrapper + + This does encrypts and does an integrity check in one + operation, so you do not need to combine HMAC + encryption + yourself. + + :param key: The key to use for encryption. + :type key: bytes + :param iv: The initialization vector. + :type iv: bytes + :param mode: One of the AEAD available modes. + + Your key and initialization vectors should be created from random bytes + of sufficient length. + + For the default SIV mode, you need one of: + + - 256-bit key, 128-bit IV to use AES-128 + - 384-bit key, 192-bit IV to use AES-192 + - 512-bit key, 256-bit IV to use AES-256 + + """ + def __init__(self, key, iv, mode=AES.MODE_SIV): + assert isinstance(key, binary_type) + assert isinstance(iv, binary_type) + self.key = key + self.mode = mode + self.iv = iv + self.kernel = AES.new(self.key, self.mode, self.iv) + + def add_associated_data(self, data): + """ + Add data to include in the MAC + + This data is protected by the MAC but not encrypted. + + :param data: data to add in the MAC calculation + :type data: bytes + """ + if isinstance(data, text_type): + data = data.encode('utf-8') + self.kernel.update(data) + + def encrypt_and_tag(self, cleardata): + """ + Encrypt the given data + + Encrypts the given data and returns the encrypted + data and the MAC to later verify and decrypt the data. + + :param cleardata: data to encrypt + :type cleardata: bytes + + :returns: 2-tuple of encrypted data and MAC + """ + assert isinstance(cleardata, binary_type) + return self.kernel.encrypt_and_digest(cleardata) + + def decrypt_and_verify(self, cipherdata, tag): + """ + Decrypt and verify + + Checks the integrity against the tag and decrypts the + data. Any associated data used during encryption + needs to be added before calling this too. + + :param cipherdata: The encrypted data + :type cipherdata: bytes + :param tag: The MAC tag + :type tag: bytes + """ + assert isinstance(cipherdata, binary_type) + assert isinstance(tag, binary_type) + try: + return self.kernel.decrypt_and_verify(cipherdata, tag) + except ValueError: + raise AESError("Failed to verify data") + + if __name__ == "__main__": key_ = "1234523451234545" # 16 byte key # Iff padded the message doesn't have to be multiple of 16 in length diff --git a/src/oic/utils/http_util.py b/src/oic/utils/http_util.py index e5aea0d35..3ef68d34d 100644 --- a/src/oic/utils/http_util.py +++ b/src/oic/utils/http_util.py @@ -1,22 +1,25 @@ from future.backports.http.cookies import SimpleCookie from future.backports.urllib.parse import quote +import base64 import cgi import hashlib import hmac import logging +import os import time from jwkest import as_unicode from six import PY2 +from six import binary_type from six import text_type from oic import rndstr from oic.exception import ImproperlyConfigured from oic.exception import UnsupportedMethod from oic.utils import time_util -from oic.utils.aes import decrypt -from oic.utils.aes import encrypt +from oic.utils.aes import AEAD +from oic.utils.aes import AESError __author__ = 'rohe0002' @@ -262,34 +265,110 @@ def _expiration(timeout, time_format=None): return time_util.in_a_while(minutes=timeout, time_format=time_format) -def cookie_signature(seed, *parts): - """Generates a cookie signature.""" - sha1 = hmac.new(seed, digestmod=hashlib.sha1) +def cookie_signature(key, *parts): + """Generates a cookie signature. + + :param key: The HMAC key to use. + :type key: bytes + :param parts: List of parts to include in the MAC + :type parts: list of bytes or strings + :returns: hexdigest of the HMAC + """ + assert isinstance(key, binary_type) + sha1 = hmac.new(key, digestmod=hashlib.sha1) for part in parts: if part: - sha1.update(part) - return sha1.hexdigest() + if isinstance(part, text_type): + sha1.update(part.encode('utf-8')) + else: + sha1.update(part) + return text_type(sha1.hexdigest()) -def make_cookie(name, load, seed, expire=0, domain="", path="", timestamp=""): +def verify_cookie_signature(sig, key, *parts): + """Constant time verifier for signatures + + :param sig: The signature hexdigest to check + :type sig: text_type + :param key: The HMAC key to use. + :type key: bytes + :param parts: List of parts to include in the MAC + :type parts: list of bytes or strings + :raises: `InvalidCookieSign` when the signature is wrong + """ + assert isinstance(sig, text_type) + return hmac.compare_digest(sig, cookie_signature(key, *parts)) + + +def _make_hashed_key(parts, hashfunc='sha256'): + """ + Construct a key via hashing the parts + + If the parts do not have enough entropy of their + own, this doesn't help. + + The size of the hash digest determines the size. + """ + h = hashlib.new(hashfunc) + for part in parts: + if isinstance(part, text_type): + part = part.encode('utf-8') + if part: + h.update(part) + return h.digest() + + +def make_cookie(name, load, seed, expire=0, domain="", path="", timestamp="", + enc_key=None): """ Create and return a cookie :param name: Cookie name :param load: Cookie load - :param seed: A seed for the HMAC function + :param seed: A seed key for the HMAC function :param expire: Number of minutes before this cookie goes stale :param domain: The domain of the cookie :param path: The path specification for the cookie :param timestamp: A time stamp + :param enc_key: The key to use for cookie encryption. :return: A tuple to be added to headers """ cookie = SimpleCookie() if not timestamp: timestamp = str(int(time.time())) - signature = cookie_signature(seed, load.encode("utf-8"), - timestamp.encode("utf-8")) - cookie[name] = "|".join([load, timestamp, signature]) + + bytes_load = load.encode("utf-8") + bytes_timestamp = timestamp.encode("utf-8") + + # If we have an encryption key, we use an AEAD cipher instead of + # building our own encrypt-and-mac scheme badly. + if enc_key: + # Make sure the key is 256-bit long, for AES-128-SIV + # + # This should go away once we push the keysize requirements up + # to the top level APIs. + key = _make_hashed_key((enc_key, seed)) + + # Random 128-Bit IV + iv = os.urandom(16) + + crypt = AEAD(key, iv) + + # timestamp does not need to be encrypted, just MAC'ed, + # so we add it to 'Associated Data' only. + crypt.add_associated_data(bytes_timestamp) + + ciphertext, tag = crypt.encrypt_and_tag(bytes_load) + cookie_payload = [bytes_timestamp, + base64.b64encode(iv), + base64.b64encode(ciphertext), + base64.b64encode(tag)] + else: + cookie_payload = [ + bytes_load, bytes_timestamp, + cookie_signature(seed, load, timestamp).encode('utf-8')] + + cookie[name] = (b"|".join(cookie_payload)).decode('utf-8') if path: cookie[name]["path"] = path if domain: @@ -301,7 +380,7 @@ def make_cookie(name, load, seed, expire=0, domain="", path="", timestamp=""): return tuple(cookie.output().split(": ", 1)) -def parse_cookie(name, seed, kaka): +def parse_cookie(name, seed, kaka, enc_key=None): """Parses and verifies a cookie value :param seed: A seed used for the HMAC signature @@ -310,24 +389,40 @@ def parse_cookie(name, seed, kaka): """ if not kaka: return None - if isinstance(seed, text_type): - seed = seed.encode("utf-8") cookie_obj = SimpleCookie(text_type(kaka)) morsel = cookie_obj.get(name) + if isinstance(seed, text_type): + seed = seed.encode('utf-8') + if morsel: parts = morsel.value.split("|") - if len(parts) != 3: - return None - # verify the cookie signature - sig = cookie_signature(seed, parts[0].encode("utf-8"), - parts[1].encode("utf-8")) - if sig != parts[2]: - raise InvalidCookieSign() - - try: - return parts[0].strip(), parts[1] - except KeyError: + if len(parts) == 3: + # verify the cookie signature + cleartext, timestamp, sig = parts + if not verify_cookie_signature(sig, seed, cleartext, timestamp): + raise InvalidCookieSign() + return cleartext, timestamp + elif len(parts) == 4: + # encrypted and signed + timestamp = parts[0] + iv = base64.b64decode(parts[1]) + ciphertext = base64.b64decode(parts[2]) + tag = base64.b64decode(parts[3]) + + # Make sure the key is 32-Bytes long + key = _make_hashed_key((enc_key, seed)) + + crypt = AEAD(key, iv) + # timestamp does not need to be encrypted, just MAC'ed, + # so we add it to 'Associated Data' only. + crypt.add_associated_data(timestamp.encode('utf-8')) + try: + cleartext = crypt.decrypt_and_verify(ciphertext, tag) + except AESError: + raise InvalidCookieSign() + return cleartext.decode('utf-8'), timestamp + else: return None else: return None @@ -435,7 +530,6 @@ def __init__(self, srv, ttl=5): self.init_srv(srv) # minutes before the interaction should be completed self.cookie_ttl = ttl # N minutes - self.pad_chr = " " def init_srv(self, srv): if not srv: @@ -447,9 +541,8 @@ def init_srv(self, srv): msg = "CookieDealer.srv.symkey cannot be an empty value" raise ImproperlyConfigured(msg) - for param in ["seed", "iv"]: - if not getattr(srv, param, None): - setattr(srv, param, rndstr().encode("utf-8")) + if not getattr(srv, 'seed', None): + setattr(srv, 'seed', rndstr().encode("utf-8")) def delete_cookie(self, cookie_name=None): if cookie_name is None: @@ -470,16 +563,10 @@ def create_cookie(self, value, typ, cookie_name=None, ttl=-1, kill=False): except TypeError: _msg = "::".join([value[0], timestamp, typ]) - if self.srv.symkey: - # Pad the message to be multiples of 16 bytes in length - lm = len(_msg) - _msg = _msg.ljust(lm + 16 - lm % 16, self.pad_chr) - info = encrypt(self.srv.symkey, _msg, self.srv.iv).decode("utf-8") - else: - info = _msg - cookie = make_cookie(cookie_name, info, self.srv.seed, + cookie = make_cookie(cookie_name, _msg, self.srv.seed, expire=ttl, domain="", path="", - timestamp=timestamp) + timestamp=timestamp, + enc_key=self.srv.symkey) if PY2: return str(cookie[0]), str(cookie[1]) else: @@ -501,18 +588,12 @@ def get_cookie_value(self, cookie=None, cookie_name=None): else: try: info, timestamp = parse_cookie(cookie_name, - self.srv.seed, cookie) + self.srv.seed, cookie, + self.srv.symkey) except (TypeError, AssertionError): return None else: - if self.srv.symkey: - txt = decrypt(self.srv.symkey, info, self.srv.iv) - # strip spaces at the end - txt = txt.rstrip(self.pad_chr) - else: - txt = info - - value, _ts, typ = txt.split("::") + value, _ts, typ = info.split("::") if timestamp == _ts: return value, _ts, typ return None diff --git a/tests/test_aes.py b/tests/test_aes.py index d2f75a8ac..856ccd311 100644 --- a/tests/test_aes.py +++ b/tests/test_aes.py @@ -1,5 +1,9 @@ import os +import pytest + +from oic.utils.aes import AEAD +from oic.utils.aes import AESError from oic.utils.aes import decrypt from oic.utils.aes import encrypt @@ -16,3 +20,41 @@ def test_encrypt_decrypt(): encrypted_msg = encrypt(key_, msg_, 0) txt = decrypt(key_, encrypted_msg, 0) assert txt == msg_ + + +def test_AEAD_good(): + key = os.urandom(32) + iv = os.urandom(16) + cleartext = b"secret sauce" + extra = ["some", "extra", "data"] + k = AEAD(key, iv) + for d in extra: + k.add_associated_data(d) + ciphertext, tag = k.encrypt_and_tag(cleartext) + + # get a fresh AEAD object + c = AEAD(key, iv) + for d in extra: + c.add_associated_data(d) + cleartext2 = c.decrypt_and_verify(ciphertext, tag) + assert cleartext2 == cleartext + + +def test_AEAD_bad_aad(): + key = os.urandom(32) + iv = os.urandom(16) + cleartext = b"secret sauce" + extra = ["some", "extra", "data"] + k = AEAD(key, iv) + for d in extra: + k.add_associated_data(d) + ciphertext, tag = k.encrypt_and_tag(cleartext) + + # get a fresh AEAD object + c = AEAD(key, iv) + # skip one aad item, MAC is wrong now + for d in extra[:1]: + c.add_associated_data(d) + + with pytest.raises(AESError): + c.decrypt_and_verify(ciphertext, tag) diff --git a/tests/test_http_util.py b/tests/test_http_util.py index 8e9d6ee35..0e8abcd27 100644 --- a/tests/test_http_util.py +++ b/tests/test_http_util.py @@ -4,11 +4,14 @@ from oic.exception import ImproperlyConfigured from oic.utils.http_util import CookieDealer +from oic.utils.http_util import InvalidCookieSign from oic.utils.http_util import Response from oic.utils.http_util import cookie_parts +from oic.utils.http_util import cookie_signature from oic.utils.http_util import getpath from oic.utils.http_util import geturl from oic.utils.http_util import parse_cookie +from oic.utils.http_util import verify_cookie_signature __author__ = 'roland' @@ -43,7 +46,7 @@ def start_response(status, headers): def cookie_dealer(): class DummyServer(): def __init__(self): - self.symkey = "0123456789012345" + self.symkey = b"0123456789012345" return CookieDealer(DummyServer()) @@ -81,6 +84,21 @@ def __init__(self): assert expected_msg in str(err.value) +def test_cookie_signature(): + key = b'1234567890abcdef' + parts = ['abc', 'def'] + sig = cookie_signature(key, *parts) + assert verify_cookie_signature(sig, key, *parts) + + +def test_broken_cookie_signature(): + key = b'1234567890abcdef' + parts = ['abc', 'def'] + sig = cookie_signature(key, *parts) + parts.reverse() + assert not verify_cookie_signature(sig, key, *parts) + + def test_parse_cookie(): kaka = ('pyoidc=bjmc::1463043535::upm|' '1463043535|18a201305fa15a96ce4048e1fbb03f7715f86499') @@ -90,6 +108,24 @@ def test_parse_cookie(): assert result == ('bjmc::1463043535::upm', '1463043535') +def test_parse_manipulated_cookie_payload(): + kaka = ('pyoidc=bjmc::1463043536::upm|' + '1463043535|18a201305fa15a96ce4048e1fbb03f7715f86499') + seed = '' + name = 'pyoidc' + with pytest.raises(InvalidCookieSign): + parse_cookie(name, seed, kaka) + + +def test_parse_manipulated_cookie_timestamp(): + kaka = ('pyoidc=bjmc::1463043535::upm|' + '1463043537|18a201305fa15a96ce4048e1fbb03f7715f86499') + seed = '' + name = 'pyoidc' + with pytest.raises(InvalidCookieSign): + parse_cookie(name, seed, kaka) + + def test_cookie_parts(): name = 'pyoidc' kaka = ('pyoidc=bjmc::1463043535::upm|' From 3453ed30756aa02766392637376f7e84f552e17b Mon Sep 17 00:00:00 2001 From: Michael Schlenker Date: Thu, 8 Jun 2017 17:01:12 +0200 Subject: [PATCH 2/2] Addes some more docstrings and test fixtures --- src/oic/utils/aes.py | 2 +- src/oic/utils/http_util.py | 95 +++++++++++++++++++++++--------------- tests/test_aes.py | 33 ++++++++----- 3 files changed, 81 insertions(+), 49 deletions(-) diff --git a/src/oic/utils/aes.py b/src/oic/utils/aes.py index ce3c3a1e1..f11f24f59 100644 --- a/src/oic/utils/aes.py +++ b/src/oic/utils/aes.py @@ -111,7 +111,7 @@ class AEAD(object): """ Authenticated Encryption with Associated Data Wrapper - This does encrypts and does an integrity check in one + This does encryption and integrity check in one operation, so you do not need to combine HMAC + encryption yourself. diff --git a/src/oic/utils/http_util.py b/src/oic/utils/http_util.py index 3ef68d34d..d83993e3d 100644 --- a/src/oic/utils/http_util.py +++ b/src/oic/utils/http_util.py @@ -323,14 +323,32 @@ def make_cookie(name, load, seed, expire=0, domain="", path="", timestamp="", """ Create and return a cookie + The cookie is secured against tampering. + + If you only provide a `seed`, a HMAC gets added to the cookies value + and this is checked, when the cookie is parsed again. + + If you provide both `seed` and `enc_key`, the cookie gets protected + by using AEAD encryption. This provides both a MAC over the whole cookie + and encrypts the `load` in a single step. + + The `seed` and `enc_key` parameters should be byte strings of at least + 16 bytes length each. Those are used as cryptographic keys. + :param name: Cookie name + :type name: text :param load: Cookie load + :type load: text :param seed: A seed key for the HMAC function + :type seed: byte string :param expire: Number of minutes before this cookie goes stale + :type expire: int :param domain: The domain of the cookie :param path: The path specification for the cookie :param timestamp: A time stamp + :type timestamp: text :param enc_key: The key to use for cookie encryption. + :type enc_key: byte string :return: A tuple to be added to headers """ cookie = SimpleCookie() @@ -340,8 +358,6 @@ def make_cookie(name, load, seed, expire=0, domain="", path="", timestamp="", bytes_load = load.encode("utf-8") bytes_timestamp = timestamp.encode("utf-8") - # If we have an encryption key, we use an AEAD cipher instead of - # building our own encrypt-and-mac scheme badly. if enc_key: # Make sure the key is 256-bit long, for AES-128-SIV # @@ -383,49 +399,56 @@ def make_cookie(name, load, seed, expire=0, domain="", path="", timestamp="", def parse_cookie(name, seed, kaka, enc_key=None): """Parses and verifies a cookie value - :param seed: A seed used for the HMAC signature + Parses a cookie created by `make_cookie` and verifies + it has not been tampered with. + + You need to provide the same `seed` and `enc_key` + used when creating the cookie, otherwise the verification + fails. See `make_cookie` for details about the verification. + + :param seed: A seed key used for the HMAC signature + :type seed: bytes :param kaka: The cookie - :return: A tuple consisting of (payload, timestamp) + :param enc_key: The encryption key used. + :type enc_key: bytes or None + :raises InvalidCookieSign: When verification fails. + :return: A tuple consisting of (payload, timestamp) or None if parsing fails """ if not kaka: return None - cookie_obj = SimpleCookie(text_type(kaka)) - morsel = cookie_obj.get(name) if isinstance(seed, text_type): seed = seed.encode('utf-8') - if morsel: - parts = morsel.value.split("|") - if len(parts) == 3: - # verify the cookie signature - cleartext, timestamp, sig = parts - if not verify_cookie_signature(sig, seed, cleartext, timestamp): - raise InvalidCookieSign() - return cleartext, timestamp - elif len(parts) == 4: - # encrypted and signed - timestamp = parts[0] - iv = base64.b64decode(parts[1]) - ciphertext = base64.b64decode(parts[2]) - tag = base64.b64decode(parts[3]) - - # Make sure the key is 32-Bytes long - key = _make_hashed_key((enc_key, seed)) - - crypt = AEAD(key, iv) - # timestamp does not need to be encrypted, just MAC'ed, - # so we add it to 'Associated Data' only. - crypt.add_associated_data(timestamp.encode('utf-8')) - try: - cleartext = crypt.decrypt_and_verify(ciphertext, tag) - except AESError: - raise InvalidCookieSign() - return cleartext.decode('utf-8'), timestamp - else: - return None - else: + parts = cookie_parts(name, kaka) + if parts is None: return None + elif len(parts) == 3: + # verify the cookie signature + cleartext, timestamp, sig = parts + if not verify_cookie_signature(sig, seed, cleartext, timestamp): + raise InvalidCookieSign() + return cleartext, timestamp + elif len(parts) == 4: + # encrypted and signed + timestamp = parts[0] + iv = base64.b64decode(parts[1]) + ciphertext = base64.b64decode(parts[2]) + tag = base64.b64decode(parts[3]) + + # Make sure the key is 32-Bytes long + key = _make_hashed_key((enc_key, seed)) + + crypt = AEAD(key, iv) + # timestamp does not need to be encrypted, just MAC'ed, + # so we add it to 'Associated Data' only. + crypt.add_associated_data(timestamp.encode('utf-8')) + try: + cleartext = crypt.decrypt_and_verify(ciphertext, tag) + except AESError: + raise InvalidCookieSign() + return cleartext.decode('utf-8'), timestamp + return None def cookie_parts(name, kaka): diff --git a/tests/test_aes.py b/tests/test_aes.py index 856ccd311..52552a1ef 100644 --- a/tests/test_aes.py +++ b/tests/test_aes.py @@ -22,36 +22,45 @@ def test_encrypt_decrypt(): assert txt == msg_ -def test_AEAD_good(): - key = os.urandom(32) - iv = os.urandom(16) - cleartext = b"secret sauce" +@pytest.fixture +def aead_key(): + return os.urandom(32) + + +@pytest.fixture +def aead_iv(): + return os.urandom(16) + + +@pytest.fixture +def cleartext(): + return b"secret sauce" + + +def test_AEAD_good(aead_key, aead_iv, cleartext): extra = ["some", "extra", "data"] - k = AEAD(key, iv) + k = AEAD(aead_key, aead_iv) for d in extra: k.add_associated_data(d) ciphertext, tag = k.encrypt_and_tag(cleartext) # get a fresh AEAD object - c = AEAD(key, iv) + c = AEAD(aead_key, aead_iv) for d in extra: c.add_associated_data(d) cleartext2 = c.decrypt_and_verify(ciphertext, tag) assert cleartext2 == cleartext -def test_AEAD_bad_aad(): - key = os.urandom(32) - iv = os.urandom(16) - cleartext = b"secret sauce" +def test_AEAD_bad_aad(aead_key, aead_iv, cleartext): extra = ["some", "extra", "data"] - k = AEAD(key, iv) + k = AEAD(aead_key, aead_iv) for d in extra: k.add_associated_data(d) ciphertext, tag = k.encrypt_and_tag(cleartext) # get a fresh AEAD object - c = AEAD(key, iv) + c = AEAD(aead_key, aead_iv) # skip one aad item, MAC is wrong now for d in extra[:1]: c.add_associated_data(d)