ring-clojure / ring

Clojure HTTP server abstraction
MIT License
3.73k stars 518 forks source link

The AES / CBC algorithm used in the cookie session store _might_ be insecure #485

Closed kamituel closed 10 months ago

kamituel commented 10 months ago

The ring.middleware.session.cookie uses AES in the CBC mode. This particular algorithm is known to be susceptible to padding oracle attacks in certain contexts.

For this reason some projects have been switching from CBC to GCM mode (example). In TLS 1.3 too support for CBC was dropped altogether.

I'm not qualified to judge for certain whether using AES-CBC poses a security risk in ring.middleware.session.cookie. However, maybe it would be worthwile to switch to a different algorithm (maybe AES-GCM)?

And/or it would also be nice if options passed to cookie-store included a choice of algorithms. Or another way of providing a custom cryptography implementation.

weavejester commented 10 months ago

The encrypted cookie is protected by an associated SHA256 HMAC. My understanding is that this prevents a padded oracle attack, or any form of plaintext oracle attack, as an attacker cannot change the encrypted data without invalidating the associated HMAC.

kamituel commented 10 months ago

This makes sense. I'm not experienced enough in cryptography to be 100% sure, but as far as I understand this particular attack vector, yes, HMAC over the whole cookie should eliminate the risk.

I still think that it'd be nice if the middleware would allow for more flexibility in terms of cryptography used. I might be 99.99% sure that AES-CBC with HMAC is sufficient, but if given a choice, I'd happily use AES-GCM.

Other possible advantages:

Would you be open to a PR that would allow for more flexibility?

Maybe a protocol like this?

(defprotocol CookieStoreEncryption
  (encrypt [key data])
  (decrypt [key data])

For backward compatibility, the current implementation would then implement this protocol. E.g. encrypt would do AES-CBC with HMAC. But it could be overwritten like:

(ring.middleware.session.cookie/cookie-store
  {:key ...some bytes...
   :encryption-impl my-own-encryption})

I'd be happy to provide a PR like this, if the approach is sound in your opinion.

weavejester commented 10 months ago

My inclination in this case is not to allow customization. Rather, I think there's a benefit to focusing on having a single correct way of producing a secure cookie. This removes the possibility of creating a security vulnerability through misconfiguration.

However, as the SessionStore protocol is available, people are free to create third-party session stores using whatever encryption algorithm they wish.