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

@@ -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 than 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]
 

src/oic/utils/aes.py

@@ -7,7 +7,7 @@
 
 from Cryptodome import Random
 from Cryptodome.Cipher import AES
-from six import indexbytes
+from six import indexbytes, binary_type, text_type
 
 __author__ = 'rolandh'
 
@@ -105,6 +105,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

src/oic/utils/http_util.py

@@ -1,22 +1,24 @@
 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 text_type
+from six import text_type, binary_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, AESError
 
 __author__ = 'rohe0002'
 
@@ -262,34 +264,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 verify_cookie_signature(sig, key, *parts):
+    """Constant time verifier for signatures
 
-def make_cookie(name, load, seed, expire=0, domain="", path="", timestamp=""):
+       :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 +379,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 +388,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 +529,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 +540,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 +562,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 +587,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