CookieStore should use different keys for encryption and MAC #5251

Closed
thaidn opened this Issue Mar 3, 2012 · 3 comments

Comments

Projects
None yet
3 participants

thaidn commented Mar 3, 2012

Please do not use the same key for both encryption and HMAC. Although a reuse of a key for different purposes is a bad practice in general, in some case the mistake might lead to a spectacular attack [1]. I know CookieStore uses HMAC, but why takes the risk? In addition, we want to make sure that if one of the schemes is broken, the key recovered by the attacker does not allow attacking the other.

We also want to avoid the the risk of somebody reusing the same key for unauthenticated encryption, i.e., Joe wants to encrypt his product ID, and he uses AES without HMAC, using config.secret_token as his key. In other words, he has introduced a padding oracle vulnerability [2], which can be used to decrypt cookies even though they use HMAC. This is the same mistake that ASP.NET made [3].

The best way to avoid this is to use a secure key derivation function, which is not very hard to implement, and derive separate keys for different purposes from config.secret_token. Something as simple as

key = SHA256(config.secret_token + "PURPOSE")

would be more secure than the current approach.

I'm willing to help either implement the fix for this issue or review somebody else's code.

Cheers,

Thai.

[1] http://en.wikipedia.org/wiki/CBC-MAC#Using_the_same_key_for_encryption_and_authentication.

[2] http://usenix.org/events/woot10/tech/full_papers/Rizzo.pdf

[3] http://weblogs.asp.net/scottgu/archive/2010/09/18/important-asp-net-security-vulnerability.aspx

thaidn commented Mar 3, 2012

MRI's OpenSSL module has PKCS5.pbkdf2_hmac and PKCS5.pbkdf2_hmac_sha1, which are secure key derivation functions that you can use.

Owner

pixeltrix commented Mar 3, 2012

What encryption? As far as I'm aware CookieStore only signs the data and doesn't encrypt it.

Member

drogus commented Mar 4, 2012

There was a discussion on encrypted store here: #3955 and implementation here: #5034. It seems that it uses only one secret key, so it's a good idea to address that before it's merged.

@drogus drogus closed this Mar 4, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment