encrypted cookie jar #5034

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
10 participants
Contributor

hmcfletch commented Feb 14, 2012

related to #3955

hmcfletch added some commits Dec 5, 2011

@hmcfletch hmcfletch signed on PermanentCookieJar taken care of in CookieJar 308c0a7
@hmcfletch hmcfletch EncrytedCookieJar
Adding an encrypted cookie jar and associated tests. SignedCookieJar
and EncryptedCookieJar are subclasses of SecretCookieJar, which takes
care of ensuring the secret it good enough.
32feaa3
@hmcfletch hmcfletch EncryptedCookieJar
encrypted cookie data, refractor encrypted and signed cookies jars to
base off of a secret cookie jar to consolidate secret token validation
1b57610
@hmcfletch hmcfletch EncryptedCookieJar tests
mirroring all SignedCookieJar tests
d8fb51f
@hmcfletch hmcfletch Merge branch 'master' of github.com:hmcfletch/rails
Conflicts:
	actionpack/lib/action_dispatch/middleware/cookies.rb
	actionpack/test/dispatch/cookies_test.rb
0454248
Member

josevalim commented Feb 14, 2012

This looks good. Could you please also add a CHANGELOG entry? /cc @NZKoz @jeremy

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Feb 14, 2012

actionpack/lib/action_dispatch/middleware/cookies.rb
def method_missing(method, *arguments, &block)
@parent_jar.send(method, *arguments, &block)
end
end
- class SignedCookieJar < CookieJar #:nodoc:
+ # Base class for the signed and enrypted cookie jar.
@carlosantoniodasilva

carlosantoniodasilva Feb 14, 2012

Owner

There is a typo: encrypted

Owner

jeremy commented Feb 14, 2012

Very nice, Les! Could you rebase and squash these commits?

Contributor

hmcfletch commented Feb 14, 2012

Oh man... I will attempt to rebase... Please to not hold me responsible if I accidentally delete Minnesota during in the process.

Contributor

hmcfletch commented Feb 14, 2012

Upon further inspection, my newness to git shines through. While working on this pull request, I merged rails/master into my branch along the way to keep up to date instead of rebasing my changes. It seems that this will mess with my ability to rebase easily since I have a few of the earlier commits were from a good while a go. Not really sure the best way to go about remedying this situation to keep the logs clean. Any help would be appreciated.

Member

josevalim commented Feb 14, 2012

We may have a oneliner for this, but the simplest way I can suggest is to create a new branch from rails master and cherry-pick commits:

git remote add rails https://josevalim@github.com/rails/rails.git
git fetch rails
git checkout rails/master
git checkout -b my_new_branch

After you create this new branch from master, you can cherry-pick the commits from this pull-request branch. At the end you will have a clean slate to rebase and squash it.

Member

drogus commented Mar 4, 2012

@hmcfletch there is one issue brought up in #5251, the same secret key should not be used for both encryption and authentication.

Member

NZKoz commented Mar 4, 2012

I don't believe that the issues raised in #5251 are relevant here, CBC-MAC vs HMAC are completely different.

If someone wanted to reach out to a cryptographer for confirmation that'd be good, but for now barring specific issues with the HMAC / encrypt and sign approach we're using I think we can keep using it.

thaidn commented Mar 5, 2012

Hi NZKoz,

I also stated clearly in #5251 that I know CookieStore uses HMAC, but using the wrong MAC is just one of the issues that I raised.

Ask any cryptographers, and they would tell you the same issues or more.

Contributor

hmcfletch commented Mar 5, 2012

I was going to do my rebase tonight and resubmit the pull request. I can try to address the issue of separate secrets as well. Briefly looking at Cookie and MessageEncryptor I tentatively propose the following for now:

In MessageEncryptor

  • Add an options key for verifier_secret that falls back to secret if absent
  • Raise warning/deprecation if secret and verify_secret are the same
  • A verifier_secret argument could made required at a later time

In Cookies

  • Add an ENCRYPTION_TOKEN_KEY = "action_dispatch.encryption_secret_token".freeze to Cookies
  • Look for ENCRYPTION_TOKEN_KEY while initializing, but fall back to TOKEN_KEY if absent
  • Raise a warning in EncryptedCookie saying that action_dispatch.encryption_secret_token is needed if secret and encryption_secret are the same

Thoughts?

Member

NZKoz commented Mar 5, 2012

@thaidn the other two issues you mention are padding oracle attacks which I believe are only possible when the encrypted data isn't signed at all. So of the three issues you mentioned, none of them apply here as far as I can tell. I'm loathe to introduce another configuration option 'just because maybe there are issues with other crypto systems'.

@igrigorik could you get some word from your friends on the google security team who suggested the feature, see if we can get a final answer on it?

@hmcfletch: don't make those changes until we get final word on whether they're necessary. If they are truly necessary then the fallback behaviour you're describing shouldn't be implemented as we'd be allowing users to run less-secure systems.

Contributor

hmcfletch commented Mar 5, 2012

cool, i'll hold off until i hear otherwise on this thread.

Member

NZKoz commented Mar 5, 2012

However I certainly agree with you @thaidn that we shouldn't permit people from using the EncryptedCookie without using the HMACs as well, that should be deprecated in MessageEncryptor and not exposed at all in Cookies

thaidn commented Mar 5, 2012

@NZKoz I'm from the google security team. I'm one of the researchers that discovered possible crypto attacks in Ruby on Rails (and many other frameworks), so I guess I'm qualified to comment here.

What I meant in the second issue is that we don't want somebody with a key recovery attack on HMAC to be able to decrypt our data as well.

And for the padding oracle issue: we want to prevent the re-use of config.secret_token in unauthenticated encryptions elsewhere in plugins, applications or Rails itself from undermining the authenticated encryption that we use in CookieStore. If we use the same key everywhere, then developers might think it's okay to re-use the key for other purposes as well, and then all it takes to decrypt our cookies is only one mistake. This is exactly what happened in ASP.NET, and I strongly suggest we should not make the same mistake in Rails.

You might think that all of the issues I raised are too conservative, but this is crypto and experience tells us that we should be conservative as much as possible when it comes to this area.

Member

NZKoz commented Mar 5, 2012

@thaidn, HA! apologies for the miscommunication then :)

So in your ticket you mentioned using key derivation functions like pbkdf2 from the secret token for the encryption key and the hmac secret, that sounds like a great option. I just see two potential issues:

  1. The existing signed-only cookies will have to continue to use the raw value of the secret to ensure backwards compatibility, and that means that the application will, to some extent, continue to be using the raw key in hmacs. Doesn't that leave us in the same situation we're in now?

  2. We'll need to add a new class to replace MessageEncryptor which does all this transparently and deprecate the old one, this seems fine to me. The existing class exposes raw cbc encryption which people may already be using to shoot themselves in the foot.

So to summarize the suggested approach:

  1. New Message...Something class, which is initialized with a secret, and uses pbkdf2 to generate 'key' and 'secret' from that value in a deterministic fashion
  2. Encrypted cookies should use that class internally

By replacing MessageEncryptor with another class entirely, we can avoid any backwards compatibility issues and also ensure that people aren't inadvertently re-using the secret as a key without hmacs.

thaidn commented Mar 5, 2012

@NZKoz

  1. As long as we use a different keys for new encryption/HMAC, then we'll be fine.

  2. Good idea. Please CC me on any patch so that I can help review.

Member

NZKoz commented Mar 5, 2012

@thaidn: So you'd consider removing/deprecating the existing 'raw keys' encryptor as sufficient for us to say we're using different keys?

Specifically an upgraded app will be using:

  • secret - to sign cookies
  • pbkdf2(secret, 'some salt') - to encrypt values
  • pbkdf2(secret, 'some other salt') - to sign those encrypted values

thaidn commented Mar 9, 2012

@NZKoz yes, exactly. have you come up with anything yet so that I can review?

cheers,

thai.

Contributor

hmcfletch commented Mar 9, 2012

My question is where would these other salts come from? That and what would the name of this new class be? I suck at names.

Member

NZKoz commented Mar 9, 2012

@hmcfletch I'll add a class for deriving keys, takes a secret then has a method like derive_key(salt), once I've done that I'll let you know

@thaidn: I'll have something next week some time. I'm assuming that we can safely use something like "encryption key" and "hmac secret" as default values for the pbkdf2 salt and let the user override them if they want to?

thaidn commented Mar 9, 2012

On Fri, Mar 9, 2012 at 2:54 PM, Michael Koziarski <
reply@reply.github.com

wrote:

@hmcfletch I'll add a class for deriving keys, takes a secret then has a
method like derive_key(salt), once I've done that I'll let you know

@thaidn: I'll have something next week some time. I'm assuming that we
can safely use something like "encryption key" and "hmac secret" as default
values for the pbkdf2 salt and let the user override them if they want to?

yes, that would work.


Reply to this email directly or view it on GitHub:
#5034 (comment)

Thai Duong

http://vnhacker.blogspot.com

meder commented Apr 13, 2012

friendly ping

Member

steveklabnik commented Nov 17, 2012

So, now that #6952 has been merged, is it time for this patch? It's way out of date...

Owner

spastorino commented Dec 19, 2012

I've already implemented this closing ...

spastorino closed this Dec 19, 2012

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