Key reuse is potentially dangerous

nelhage commented 5 years ago

flask-oidc creates two JWS signers using the Flask secret key:

https://github.com/puiterwijk/flask-oidc/blob/68767920a5419e45ba5e725bf758587f01a2a265/flask_oidc/__init__.py#L179-L183

In addition, Flask itself creates a signer [using the same key].(https://github.com/pallets/flask/blob/7e714bd28b6e96d82b2848b48cf8ff48b517b09b/flask/sessions.py#L330-L332)

In general, it's unsafe to reuse a cryptographic key for multiple purposes. The proper construction is to use a KDF like HKDF to derive separate keys for each application.

I'm not sure if this is actually exploitable in this case, but if it is, one shape would look like:

Because each of these signers is signing with an HMAC over the same key, the output of any one of them could be fed back into another, which will then accept the signature, since it's signed using the same key.
This potentially means that an attacker could e.g. swap the session cookie and the OIDC_ID_TOKEN_COOKIE_NAME cookie, which is certainly surprising at best.

I haven't verified that the outputs are sufficiently compatible for the attack to actually work, but ideally the code wouldn't rely on details of slightly-differing output formats for an important security property.

alex commented 5 years ago

FWIW, if you wanted to use pyca/cryptography to resolve this you could change these lines to:

self.extra_data_serializer = JSONWebSignatureSerializer(
    HKDF(
        algorithm=hashes.SHA256(),
        length=32,
        salt=None,
        info="extra-data-serializer",
        backend=default_backend(),
    ).derive(app.config['SECRET_KEY'])
)
 self.cookie_serializer = TimedJSONWebSignatureSerializer( 
    HKDF(
        algorithm=hashes.SHA256(),
        length=32,
        salt=None,
        info="cookie-serializer",
        backend=default_backend(),
    ).derive(app.config['SECRET_KEY'])
)

This will protect against the attack @nelhage describes.

https://cryptography.io/en/latest/hazmat/primitives/key-derivation-functions/#cryptography.hazmat.primitives.kdf.hkdf.HKDF

nelhage commented 5 years ago

Looking further, it looks like itsdangerous has a built-in solution for this: The salt parameter to signers

I believe you can just change the lines to

        self.extra_data_serializer = JSONWebSignatureSerializer(
            app.config['SECRET_KEY'], salt='flask-oidc-extra-data-serializer')
        self.cookie_serializer = TimedJSONWebSignatureSerializer(
            app.config['SECRET_KEY'], salt='flask-oidc-cookie-data-serializer')

I don't have time to test that change locally but I'll send a PR when I get a chance to.

alex commented 5 years ago

That seems way simpler than my HKDF solution :-)

puiterwijk / flask-oidc

Key reuse is potentially dangerous #68