Skip to content
This repository

encrypted cookie jar #5034

Closed
wants to merge 8 commits into from

10 participants

Les Fletcher José Valim Jeremy Kemper Piotr Sarnacki Michael Koziarski Thai Duong Meder Kydyraliev Steve Klabnik Santiago Pastorino Carlos Antonio da Silva
Les Fletcher

related to #3955

hmcfletch added some commits
Les Fletcher hmcfletch signed on PermanentCookieJar taken care of in CookieJar 308c0a7
Les Fletcher 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
Les Fletcher 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
Les Fletcher hmcfletch EncryptedCookieJar tests
mirroring all SignedCookieJar tests
d8fb51f
Les Fletcher hmcfletch Merge branch 'master' of github.com:hmcfletch/rails
Conflicts:
	actionpack/lib/action_dispatch/middleware/cookies.rb
	actionpack/test/dispatch/cookies_test.rb
0454248
José Valim
Owner

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

actionpack/lib/action_dispatch/middleware/cookies.rb
((8 lines not shown))
280 301 def method_missing(method, *arguments, &block)
281 302 @parent_jar.send(method, *arguments, &block)
282 303 end
283 304 end
284 305
285   - class SignedCookieJar < CookieJar #:nodoc:
  306 + # Base class for the signed and enrypted cookie jar.
1

There is a typo: encrypted

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

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

Les Fletcher

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

Les Fletcher

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.

José Valim
Owner

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.

Piotr Sarnacki
Collaborator

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

Michael Koziarski
Owner
NZKoz commented

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.

Thai Duong

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.

Les Fletcher

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?

Michael Koziarski
Owner
NZKoz commented

@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.

Les Fletcher

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

Michael Koziarski
Owner
NZKoz commented

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

Thai Duong

@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.

Michael Koziarski
Owner
NZKoz commented

@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.

Thai Duong

@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.

Michael Koziarski
Owner
NZKoz commented

@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
Thai Duong

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

cheers,

thai.

Les Fletcher

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.

Michael Koziarski
Owner
NZKoz commented

@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?

Thai Duong
Meder Kydyraliev
meder commented

friendly ping

Steve Klabnik
Collaborator

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

Santiago Pastorino
Owner

I've already implemented this closing ...

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

Showing 8 unique commits by 1 author.

Dec 05, 2011
Les Fletcher hmcfletch signed on PermanentCookieJar taken care of in CookieJar 308c0a7
Les Fletcher 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
Feb 14, 2012
Les Fletcher 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
Les Fletcher hmcfletch EncryptedCookieJar tests
mirroring all SignedCookieJar tests
d8fb51f
Les Fletcher hmcfletch Merge branch 'master' of github.com:hmcfletch/rails
Conflicts:
	actionpack/lib/action_dispatch/middleware/cookies.rb
	actionpack/test/dispatch/cookies_test.rb
0454248
Les Fletcher hmcfletch comment typo 27df38b
Les Fletcher hmcfletch update the CHNAGELOG 9cc77c6
Les Fletcher hmcfletch typo 6842124
This page is out of date. Refresh to see the latest.
8 actionpack/CHANGELOG.md
Source Rendered
... ... @@ -1,5 +1,13 @@
1 1 ## Rails 4.0.0 (unreleased) ##
2 2
  3 +* Add an encrypted cookie jar. It can be used like the signed cookies.
  4 + Example:
  5 +
  6 + cookies.encrypted[:buried_treasure] = "X marks the spot"
  7 + cookies.permanent.encrypted[:encrypted_permanent] = "X marks the spot... forever"
  8 +
  9 + *Les Fletcher*
  10 +
3 11 * Add `date_field` and `date_field_tag` helpers which render an `input[type="date"]` tag *Olek Janiszewski*
4 12
5 13 * Adds `image_url`, `javascript_url`, `stylesheet_url`, `audio_url`, `video_url`, and `font_url`
104 actionpack/lib/action_dispatch/middleware/cookies.rb
@@ -237,6 +237,31 @@ def signed
237 237 @signed ||= SignedCookieJar.new(self, @secret)
238 238 end
239 239
  240 + # Returns a jar that'll automatically encrypt cookies values before writing them and decrypt the values when reading. If the
  241 + # cookie is tampered with by the user (or a 3rd party), an ActiveSupport::MessageVerifier::InvalidSignature exception will
  242 + # be raised. If the cookie is verified, but unable to be decrypted, an ActiveSupport::MessageEncryptor::InvalidMessage
  243 + # exception will be raised.
  244 + #
  245 + # This jar requires that you set a suitable secret for the verification on your app's config.secret_token.
  246 + #
  247 + # Example:
  248 + #
  249 + # cookies.encrypted[:encrypted_cookie] = "you don't know what this says"
  250 + # # => Set-Cookie: LSuus8pkXd...ckqiG6qGlwuhSQn--4Eb16w1z7ouNXQZAxV5Bjw==; path=/; expires=Sun, 27-Mar-2011 03:24:16 GMT
  251 + #
  252 + # This jar allows chaining with other jars as well, so you can set permanent, encrypted cookies. Examples:
  253 + #
  254 + # cookies.permanent.encypted[:encrypted_permanent] = "you don't know what this says, but it will be here for 20 years"
  255 + # # => Set-Cookie: Sok2G6hGs...XFeUpDWQLT8=--UZe+JlZPlMuxHYSq09oV0w==; path=/; expires=Thu, 27 Mar 2031 13:48:43 GMT
  256 + #
  257 + # To read encypted cookies:
  258 + #
  259 + # cookies.encrypted[:encrypted_cookie] # => "you don't know what this says"
  260 + # cookies.encrypted[:encrypted_permanent] # => "you don't know what this says, but it will be here for 20 years"
  261 + def encrypted
  262 + @encrypted ||= EncryptedCookieJar.new(self, @secret)
  263 + end
  264 +
240 265 def write(headers)
241 266 @set_cookies.each { |k, v| ::Rack::Utils.set_cookie_header!(headers, k, v) if write_cookie?(v) }
242 267 @delete_cookies.each { |k, v| ::Rack::Utils.delete_cookie_header!(headers, k, v) }
@@ -273,22 +298,51 @@ def []=(key, options)
273 298 @parent_jar[key] = options
274 299 end
275 300
276   - def signed
277   - @signed ||= SignedCookieJar.new(self, @secret)
278   - end
279   -
280 301 def method_missing(method, *arguments, &block)
281 302 @parent_jar.send(method, *arguments, &block)
282 303 end
283 304 end
284 305
285   - class SignedCookieJar < CookieJar #:nodoc:
  306 + # Base class for the signed and encrypted cookie jar.
  307 + class SecretCookieJar < CookieJar #:nodoc:
286 308 MAX_COOKIE_SIZE = 4096 # Cookies can typically store 4096 bytes.
287 309 SECRET_MIN_LENGTH = 30 # Characters
288 310
289 311 def initialize(parent_jar, secret)
290 312 ensure_secret_secure(secret)
291 313 @parent_jar = parent_jar
  314 + end
  315 +
  316 +
  317 + def method_missing(method, *arguments, &block)
  318 + @parent_jar.send(method, *arguments, &block)
  319 + end
  320 +
  321 + protected
  322 +
  323 + # To prevent users from using something insecure like "Password" we make sure that the
  324 + # secret they've provided is at least 30 characters in length.
  325 + def ensure_secret_secure(secret)
  326 + if secret.blank?
  327 + raise ArgumentError, "A secret is required to generate an " +
  328 + "integrity hash for cookie session data. Use " +
  329 + "config.secret_token = \"some secret phrase of at " +
  330 + "least #{SECRET_MIN_LENGTH} characters\"" +
  331 + "in config/initializers/secret_token.rb"
  332 + end
  333 +
  334 + if secret.length < SECRET_MIN_LENGTH
  335 + raise ArgumentError, "Secret should be something secure, " +
  336 + "like \"#{SecureRandom.hex(16)}\". The value you " +
  337 + "provided, \"#{secret}\", is shorter than the minimum length " +
  338 + "of #{SECRET_MIN_LENGTH} characters"
  339 + end
  340 + end
  341 + end
  342 +
  343 + class SignedCookieJar < SecretCookieJar #:nodoc:
  344 + def initialize(parent_jar, secret)
  345 + super(parent_jar, secret)
292 346 @verifier = ActiveSupport::MessageVerifier.new(secret)
293 347 end
294 348
@@ -311,30 +365,34 @@ def []=(key, options)
311 365 raise CookieOverflow if options[:value].size > MAX_COOKIE_SIZE
312 366 @parent_jar[key] = options
313 367 end
  368 + end
314 369
315   - def method_missing(method, *arguments, &block)
316   - @parent_jar.send(method, *arguments, &block)
  370 + class EncryptedCookieJar < SecretCookieJar #:nodoc:
  371 + def initialize(parent_jar, secret)
  372 + super(parent_jar, secret)
  373 + @encrypter = ActiveSupport::MessageEncryptor.new(secret)
317 374 end
318 375
319   - protected
320   -
321   - # To prevent users from using something insecure like "Password" we make sure that the
322   - # secret they've provided is at least 30 characters in length.
323   - def ensure_secret_secure(secret)
324   - if secret.blank?
325   - raise ArgumentError, "A secret is required to generate an " +
326   - "integrity hash for cookie session data. Use " +
327   - "config.secret_token = \"some secret phrase of at " +
328   - "least #{SECRET_MIN_LENGTH} characters\"" +
329   - "in config/initializers/secret_token.rb"
  376 + def [](name)
  377 + if encrypted_message = @parent_jar[name]
  378 + @encrypter.decrypt_and_verify(encrypted_message)
330 379 end
  380 + rescue ActiveSupport::MessageVerifier::InvalidSignature
  381 + nil
  382 + rescue ActiveSupport::MessageEncryptor::InvalidMessage
  383 + nil
  384 + end
331 385
332   - if secret.length < SECRET_MIN_LENGTH
333   - raise ArgumentError, "Secret should be something secure, " +
334   - "like \"#{SecureRandom.hex(16)}\". The value you " +
335   - "provided, \"#{secret}\", is shorter than the minimum length " +
336   - "of #{SECRET_MIN_LENGTH} characters"
  386 + def []=(key, options)
  387 + if options.is_a?(Hash)
  388 + options.symbolize_keys!
  389 + options[:value] = @encrypter.encrypt_and_sign(options[:value])
  390 + else
  391 + options = { :value => @encrypter.encrypt_and_sign(options) }
337 392 end
  393 +
  394 + raise CookieOverflow if options[:value].size > MAX_COOKIE_SIZE
  395 + @parent_jar[key] = options
338 396 end
339 397 end
340 398
69 actionpack/test/dispatch/cookies_test.rb
@@ -63,6 +63,11 @@ def set_signed_cookie
63 63 head :ok
64 64 end
65 65
  66 + def set_encrypted_cookie
  67 + cookies.encrypted[:treasure_map] = "X marks the spot"
  68 + head :ok
  69 + end
  70 +
66 71 def raise_data_overflow
67 72 cookies.signed[:foo] = 'bye!' * 1024
68 73 head :ok
@@ -74,11 +79,22 @@ def tampered_cookies
74 79 head :ok
75 80 end
76 81
  82 + def tampered_encrypted_cookies
  83 + cookies[:tampered_encrypted] = "BAhJIlBzZGZhc2YxeUd2NmFJZTluZUVpdFZVck1BZDV0KytnS1JJVXBsU0RXUmpBTDdIRms9LS02ZGczb04vRGpsMyswN0xnWjJmeExRPT0GOgZFVA%3D%3D--637b89be586e940f9c3e25ef51f1e6d0dc9f6251"
  84 + cookies.encrypted[:tampered_encrypted]
  85 + head :ok
  86 + end
  87 +
77 88 def set_permanent_signed_cookie
78 89 cookies.permanent.signed[:remember_me] = 100
79 90 head :ok
80 91 end
81 92
  93 + def set_permanent_encrypted_cookie
  94 + cookies.permanent.encrypted[:buried_tresure] = "Gold!!!"
  95 + head :ok
  96 + end
  97 +
82 98 def delete_and_set_cookie
83 99 cookies.delete :user_name
84 100 cookies[:user_name] = { :value => "david", :expires => Time.utc(2005, 10, 10,5) }
@@ -272,17 +288,33 @@ def test_signed_cookie
272 288 assert_equal 45, @controller.send(:cookies).signed[:user_id]
273 289 end
274 290
  291 + def test_encrypted_cookie
  292 + get :set_encrypted_cookie
  293 + assert_equal "X marks the spot", @controller.send(:cookies).encrypted[:treasure_map]
  294 + end
  295 +
275 296 def test_accessing_nonexistant_signed_cookie_should_not_raise_an_invalid_signature
276 297 get :set_signed_cookie
277 298 assert_nil @controller.send(:cookies).signed[:non_existant_attribute]
278 299 end
279 300
  301 + def test_accessing_nonexistant_encrypted_cookie_should_not_raise_an_invalid_signature
  302 + get :set_encrypted_cookie
  303 + assert_nil @controller.send(:cookies).encrypted[:non_existant_attribute]
  304 + end
  305 +
280 306 def test_permanent_signed_cookie
281 307 get :set_permanent_signed_cookie
282 308 assert_match(%r(#{20.years.from_now.utc.year}), @response.headers["Set-Cookie"])
283 309 assert_equal 100, @controller.send(:cookies).signed[:remember_me]
284 310 end
285 311
  312 + def test_permanent_encrypted_cookie
  313 + get :set_permanent_encrypted_cookie
  314 + assert_match(%r(#{20.years.from_now.utc.year}), @response.headers["Set-Cookie"])
  315 + assert_equal "Gold!!!", @controller.send(:cookies).encrypted[:buried_tresure]
  316 + end
  317 +
286 318 def test_delete_and_set_cookie
287 319 get :delete_and_set_cookie
288 320 assert_cookie_header "user_name=david; path=/; expires=Mon, 10-Oct-2005 05:00:00 GMT"
@@ -302,6 +334,13 @@ def test_tampered_cookies
302 334 end
303 335 end
304 336
  337 + def test_tampered_encrypted_cookies
  338 + assert_nothing_raised do
  339 + get :tampered_encrypted_cookies
  340 + assert_response :success
  341 + end
  342 + end
  343 +
305 344 def test_raises_argument_error_if_missing_secret
306 345 assert_raise(ArgumentError, nil.inspect) {
307 346 @request.env["action_dispatch.secret_token"] = nil
@@ -312,6 +351,16 @@ def test_raises_argument_error_if_missing_secret
312 351 @request.env["action_dispatch.secret_token"] = ""
313 352 get :set_signed_cookie
314 353 }
  354 +
  355 + assert_raise(ArgumentError, nil.inspect) {
  356 + @request.env["action_dispatch.secret_token"] = nil
  357 + get :set_encrypted_cookie
  358 + }
  359 +
  360 + assert_raise(ArgumentError, ''.inspect) {
  361 + @request.env["action_dispatch.secret_token"] = ""
  362 + get :set_encrypted_cookie
  363 + }
315 364 end
316 365
317 366 def test_raises_argument_error_if_secret_is_probably_insecure
@@ -329,6 +378,21 @@ def test_raises_argument_error_if_secret_is_probably_insecure
329 378 @request.env["action_dispatch.secret_token"] = "12345678901234567890123456789"
330 379 get :set_signed_cookie
331 380 }
  381 +
  382 + assert_raise(ArgumentError, "password".inspect) {
  383 + @request.env["action_dispatch.secret_token"] = "password"
  384 + get :set_encrypted_cookie
  385 + }
  386 +
  387 + assert_raise(ArgumentError, "secret".inspect) {
  388 + @request.env["action_dispatch.secret_token"] = "secret"
  389 + get :set_encrypted_cookie
  390 + }
  391 +
  392 + assert_raise(ArgumentError, "12345678901234567890123456789".inspect) {
  393 + @request.env["action_dispatch.secret_token"] = "12345678901234567890123456789"
  394 + get :set_encrypted_cookie
  395 + }
332 396 end
333 397
334 398 def test_cookie_with_all_domain_option
@@ -463,8 +527,6 @@ def test_cookies_hash_is_indifferent_access
463 527 assert_equal "dhh", cookies['user_name']
464 528 end
465 529
466   -
467   -
468 530 def test_setting_request_cookies_is_indifferent_access
469 531 cookies.clear
470 532 cookies[:user_name] = "andrew"
@@ -575,4 +637,5 @@ def assert_not_cookie_header(expected)
575 637 assert_not_equal expected.split("\n"), header
576 638 end
577 639 end
578   -end
  640 +
  641 +end

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.