AEAD encrypted cookies and sessions by mjc-gh · Pull Request #28132 · rails/rails

mjc-gh · 2017-02-23T19:07:56Z

Summary

This PR is the start of migrating from HMAC AES-CBC encrypted cookies to AEAD encrypted cookies. Commit d4ea18a added AES-256-GCM for Authenticated Encryption support. I'm hoping this PR could be the start of migrating cookies and sessions to this form of encryption.

I think it's worth considering to what degree we should be supporting legacy signed and encrypted cookies as well. This PR includes a UpgradeLegacyHmacAesCbcCookieJar class which aims to seamlessly upgrade encrypted cookies. Should we be looking to deprecate older legacy schemes now?

Other Information

This PR comments out some tests that are now broken. Depending on how we move forward with encrypted cookies and to the degree to which legacy cookies are supported, I will update, fix, and add any tests as needed.

rafaelfranca · 2017-02-23T19:29:53Z

Could you explain briefly too why migrating to AEAD is good?

mjc-gh · 2017-02-23T19:46:09Z

Sure, by using AES with GCM, ciphertexts will be shorter and decryption and encryption will be quicker. With GCM, authentication and encryption are processed together instead of separately like we see with the HMAC and AES CBC authenticated encryption setup.

Additionally, the announcement today from Google about SHA1 collisions somewhat drove me to make this PR. While unrelated to security and encryption features in Rails, the announcement motivated me to look into what it would take to update Rails' cookies to use more modern encryption. If we do not go down the AEAD route for encrypted cookies, we should at least be exploring using SHA256 for the digest option for MessageEncryptor.

As an aside, AEAD in general aims to make cryptographic processes simpler and less error prone. I think it may be worthwhile making GCM the default for MessageEncryptor but that is beyond the scope of this PR.

rafaelfranca · 2017-02-23T20:52:43Z

Thanks for the explanation!

eugeneius · 2017-02-23T21:43:37Z

we should at least be exploring using SHA256 for the digest option for MessageEncryptor.

There's an open pull request to do just that (#25204), but it's blocked on the ability to transparently upgrade existing cookies to the new format (#18772) which I suspect we'll need for this change too.

mjc-gh · 2017-02-23T22:15:00Z

@eugeneius there is a small cookie jar class in this PR to upgrade HMAC CBC encrypted cookies to GCM. This could be altered to handle the upgrade from SHA1 to SHA256 though.

I think in the long run introducing GCM cookies is better though. The smaller ciphertexts is a worthwhile improvement.

eugeneius · 2017-02-23T22:59:03Z

Oh, nice! You even mentioned it in the description 🤦‍♂️

mjc-gh · 2017-02-23T23:18:42Z

Put together some small benchmarks comparing the two modes. The script also outputs the length of two ciphertexts, one for each mode.

The repo is here: https://github.com/mikeycgto/message_encryptor-benchmark

bdewater · 2017-02-24T04:35:46Z

Additionally, the announcement today from Google about SHA1 collisions somewhat drove me to make this PR.

Important to note here this finding of a SHA1 collision has nothing to do with the security of HMAC-SHA1, which MessageVerifier uses. So people shouldn't panic if they cannot upgrade straight away.

That said, not only is an AEAD mode faster for encryption/decryption (AES-GCM especially with processor instructions) but it also produces smaller cookies (shown in #25874) so 👍👍👍 if we can transparently upgrade.

mjc-gh · 2017-02-24T17:09:01Z

Agreed @bdewater.

HMACs in general are still secure even when the underlying hash function have known collisions issues. Google's announcement motivated me to look into what it would take to update Rails' cookies to use more modern encryption and security. This is actually something I've been thinking about for a while, today's news just lit the spark for me!

mjc-gh · 2017-02-24T17:59:11Z

Notes on Legacy Support

Thought more about this and want to clarify somethings as far as legacy support goes. Currently there are two legacy cookie jar classes UpgradeLegacySignedCookieJar and UpgradeLegacyEncryptedCookieJar. Both of these classes include VerifyAndUpgradeLegacySignedMessage which upgrades cookies that are just HMAC'd using the old secret_token approach which was deprecated in Rails v4.

This PR adds a UpgradeLegacyHmacAesCbcCookieJar class for upgrading encrypted cookies to the new GCM encryption scheme. This jar is used when action_dispatch.encrypted_cookie_salt and action_dispatch.action_dispatch.encrypted_signed_cookie_salt are set along with the new action_dispatch.authenticated_encrypted_cookie_salt configuration value. These options should enable developers to finely control the migration path. Applications that need migration can continue to set the encrypted_cookie_salt and encrypted_signed_cookie_salt values. New applications can just have the authenticated_encrypted_cookie_salt value set and no migration will be attempted when decryption fails.

I think it's worth considering at this time if secret_token legacy support should be removed. Doing so will help simplify the cookie jar middleware code base significantly. The tests can also be cleaned up and far less cases will need to be covered.

If support is kept, I fully intend to test all legacy configurations. That is, I want to make sure "encrypted" cookies that are just signed with a secret_token are upgraded to GCM encryption as expected. I believe this already the case but this needs to be more thoroughly tested.

Thus, my main question is, should secret_token legacy support be removed now with these changes?

Possible Major API Changes

I'd also go as far as to suggest some major API changes to the cookie middleware. Specifically, we could deprecate the encrypted and signed methods for a single secured method. The two main reasons for this suggestion are:

From a developer usage security prospective, it is best to limit options. Developers may be unsure if authenticity is sufficient or if confidential is needed for their use cases.
Signed cookies are very large since the message is just base64 encoded. With the introduction of GCM cookies we now have smaller resulting ciphertexts. A single secured cookie method which just uses GCM encrypted cookies could be sufficient for when only authenticity is needed.

This suggestion is obviously a lot more significant than what the PR has initially set out to do. I figured it is worth suggesting and discussing though!

bdewater · 2017-02-28T02:19:42Z

Thus, my main question is, should secret_token legacy support be removed now with these changes?

AFAIK Rails usually introduces deprecation warnings in minor versions before removing stuff. With 5.1 already in beta, removal seems more suited when master targets 5.2. Just my $0.02.

kaspth · 2017-05-15T19:42:13Z

guides/source/security.md

Also needs to keep the 80 char limit.

kaspth · 2017-05-15T19:44:16Z

...ib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_5_2.rb.tt

We need to copy over the intro from the _5_1.rb file and nestle the config to be under config.action_dispatch.

kaspth · 2017-05-15T19:47:46Z

railties/lib/rails/application/configuration.rb

To nestle the config under action_dispatch we'll need to move the false assign to this file:

rails/actionpack/lib/action_dispatch/railtie.rb

Line 18 in 7cc526b

config.action_dispatch.encrypted_signed_cookie_salt = "signed encrypted cookie"

Your override in initializer further below in that file then needs to take the new namespace into account.

kaspth · 2017-05-15T19:48:08Z

actionpack/CHANGELOG.md

Remember to rewrite this too 😉

kaspth · 2017-05-15T19:49:08Z

actionpack/CHANGELOG.md

Don't forget to add your name too 😄

*Michael Coyne*

mjc-gh · 2017-05-15T19:51:00Z

@kaspth these changes sound good. Will get to them either later tonight or tomorrow morning sometime. Thanks for reviewing it again!

mjc-gh · 2017-05-16T08:21:35Z

Made some more changes and moved the use_authenticated_cookie_encryption configuration value to under the action_dispatch namespace. I added a small test to app_generator_test.rb as well to make sure the framework defaults file is generated.

One small question I have is if this config value is set to true in the new framework defaults initializer, will this initializer block here see the flag as true and thus set the salt as need? Maybe setting this salt should be done in an after_initialize block? Just wanted to ask about this and double check the configuration logic is sound.

Thanks again for all the reviews! Can't wait to see this feature merged in -- smaller 🍪 for all!!

mjc-gh · 2017-05-16T22:12:31Z

Thought a bit more about this and I realized that because the two old salt values are still set by default, upgrade_legacy_hmac_aes_cbc_cookies? will always be true (assuming users have opted in to AEAD cookies in the first place).

Perhaps we should add a second configuration flag, such as config.action_dispatch.upgrade_hmac_cbc_cookie_encryption that will, for now, default to true. We can then change upgrade_legacy_hmac_aes_cbc_cookies? to just examine this configuration value as well as the use_authenticated_cookie_encryption value.

Ultimately, I think having direct configuration flags for both AEAD cookies as well as supporting the upgrade behavior is easier for developers to deal with than indirectly controlling this behavior via the salt config values. This way, new applications or applications that no longer need the upgrade behavior can easily opt-out of it. Thoughts?

mjc-gh · 2017-05-19T21:22:38Z

Just rebased and my test_new_5_2_application_has_defaults in railties/test/generators/app_generator_test.rb is now failing. It seems this file isn't expected to be generated for a new 5.2 application (there is a test above it which indicate just that). However, since this feature is opt-in, we need this file to be generated so users can enable the new behavior if they'd like to. Any idea how we should solve this @kaspth?

mjc-gh · 2017-05-21T13:08:36Z

I decided to add the upgrade_legacy_hmac_aes_cbc_cookies configuration flag as mention above so users can explicitly now control both opting-in to AES GCM cookies and whether or not upgrade behavior should be used. This should be easier to manage than fiddling with salt configuration values.

Not sure how to handle the failing test with regards to new_framework_defaults_5_1.rb.tt file. This file should be generated for new 5.2 applications so users can opt-in to AES GCM cookies. Perhaps these two configuration values should be moved to independent initializer for new applications? Any thoughts on this?

kaspth · 2017-05-21T15:27:07Z

However, since this feature is opt-in

It shouldn't be opt-in on a rails new 5.2 app. There it should be enabled by default, which then also solves the test failure. We need to add:

if respond_to?(:action_dispatch)
  action_dispatch.use_authenticated_cookie_encryption = true
end

right after here:

rails/railties/lib/rails/application/configuration.rb

Lines 87 to 89 in 75fa8dd

    
           if respond_to?(:active_record) 
        
             active_record.cache_versioning = true 
        
           end

I don't understand the need for upgrade_hmac_cbc_cookie_encryption — that's what use_authenticated_cookie_encryption governs.

kaspth

Some last defaults wrangling, but we're getting pretty close!

kaspth · 2017-05-21T15:30:57Z

guides/source/configuring.md

This seems off?

Will double check this.

kaspth · 2017-05-21T15:31:05Z

guides/source/configuring.md

Are we sure that's the default?

Will double check this.

kaspth · 2017-05-21T15:32:50Z

...ib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_5_2.rb.tt

I think this gets solved by Rails removing support for upgrading the cookies from CBC, so I'd rather not make this a config.

See my latest comment for more discussion on this flag. I'm thinking to we just leave the upgrade behavior as is and have future version use "pure" GCM cookies...

kaspth · 2017-05-21T15:35:54Z

...ib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_5_2.rb.tt

I was recently schooled on the new_framework_defaults_* file by @matthewd and how I've been doing them wrong.

We should follow the now added example above: use the future default, but start it commented out.

Understood. This file was a bit of mystery to me as well. Did this discussion occur in the PR from dhh for the new fragment caching feature? I'm definitely interested in fully understanding its uses!

The intent is captured here: #28469 😊

kaspth · 2017-05-21T15:38:00Z

railties/test/generators/app_generator_test.rb

Newer Rails app load_defaults from the version they were created with:

rails/railties/lib/rails/generators/rails/app/templates/config/application.rb

Lines 25 to 26 in e8b4a92

# Initialize configuration defaults for originally generated Rails version.

config.load_defaults <%= Rails::VERSION::STRING.to_f %>

So a brand new 5.2 app shouldn't have this file, it's only for 5.1 upgrading apps.

kaspth · 2017-05-21T15:40:05Z

actionpack/CHANGELOG.md

Let's mention the AES GCM benefits here (faster, shorter — copy from the security guide if you like) and that new_framework_defaults_5.2.rb shows how to upgrade.

mjc-gh · 2017-05-21T15:48:28Z

It shouldn't be opt-in on a rails new 5.2 app. There it should be enabled by default, which then also solves the test failure. We need to add:
if respond_to?(:action_dispatch)
  action_dispatch.use_authenticated_cookie_encryption = true
end
right after here:

rails/railties/lib/rails/application/configuration.rb

Lines 87 to 89 in 75fa8dd

if respond_to?(:active_record)

active_record.cache_versioning = true

end

I can make this change and remove the test I added.

I don't understand the need for upgrade_hmac_cbc_cookie_encryption — that's what use_authenticated_cookie_encryption governs.

Originally I had the upgrade_legacy_hmac_aes_cbc_cookies? method in the cookies middleware check if the salt values for CBC HMAC encryption were defined. If they were, the UpgradeLegacyHmacAesCbcCookieJar middleware is used and cookies are seamlessly upgraded. If they were not defined the EncryptedCookieJar middleware is used and no upgrading is attempted when decryption fails.

Due to these salt values still being defined the UpgradeLegacyHmacAesCbcCookieJar middleware will always be used even if upgrade behavior is not needed or desired. I thought adding an explicit flag would be easier to manage this behavior than clearing the salt values (thus having upgrade_legacy_hmac_aes_cbc_cookies? return false).

Perhaps this can be solved by removing the upgrade middleware altogether in future versions. I know cleaning up the cookies middleware in general is one of the overall goals for the GSoC project so perhaps we should just leave this be for now and have it get released as is in Rails 5.2?

kaspth · 2017-05-21T16:09:14Z

Perhaps this can be solved by removing the upgrade middleware altogether in future versions. I know cleaning up the cookies middleware in general is one of the overall goals for the GSoC project so perhaps we should just leave this be for now and have it get released as is in Rails 5.2?

My thoughts exactly 👍 (though the GSoC project might not necessarily clean up old cookies versions, but we'll see).

I think it's fine to check for the presence of the 3 salt values because use_authenticated_cookie_encryption guards the new salt being assigned the upgrade won't happen.

kaspth · 2017-05-21T16:14:02Z

actionpack/lib/action_dispatch/middleware/cookies.rb

So this would be:

request.secret_key_base.present? && request.authenticated_encrypted_cookie_salt.present? && request.encrypted_signed_cookie_salt.present? && request.encrypted_cookie_salt.present?

And we can kill the use_authenticated_cookie_encryption? and upgrade_legacy_hmac_aes_cbc_cookies? methods then.

authenticated_encrypted_cookie_salt was placed up further, since it's less likely to be assigned on upgrading apps and we'll avoid the presence check on the present in older releases salt values.

Related: I'm not sure why we check the presence of the secret_key_base here. Rails apps won't boot without it these days.

Checking secret_key_base seemed a bit unnecessary and I do think it's likely safe to drop at this point.

I'll also rework my commits soon and drop the upgrade_legacy_hmac_aes_cbc_cookies flag and revert the upgrade_legacy_hmac_aes_cbc_cookies? method to just check if the salt values are present?. I'll also update the above mention changes to the various guides. At that point, I think we'll be close to ready for this PR. Thanks again!!

This commit changes encrypted cookies from AES in CBC HMAC mode to Authenticated Encryption using AES-GCM. It also provides a cookie jar to transparently upgrade encrypted cookies to this new scheme. Some other notable changes include: - There is a new application configuration value: +use_authenticated_cookie_encryption+. When enabled, AEAD encrypted cookies will be used. - +cookies.signed+ does not raise a +TypeError+ now if the name of an encrypted cookie is used. Encrypted cookies using the same key as signed cookies would be verified and serialization would then fail due the message still be encrypted.

kaspth · 2017-05-28T15:02:34Z

@mikeycgto great work! 👏

mjc-gh · 2017-05-28T19:28:40Z

@kaspth thanks so much for all the help and the feedback! Excited to see this get merged and see Rails get more modern encryption!!

kaspth · 2017-05-29T07:31:21Z

@mikeycgto no problem! Thanks for bringing Rails up to speed 💪

This reverts commit 9a4700f.

rafaelfranca added the actionpack label Feb 23, 2017

rafaelfranca added the security label Feb 23, 2017

rafaelfranca requested review from jeremy, matthewd and tenderlove February 23, 2017 20:52

maclover7 added the needs feedback label Feb 23, 2017

mjc-gh force-pushed the aead-encrypted-cookies branch 2 times, most recently from 54c40b5 to 3b1e5f4 Compare February 27, 2017 15:42

mjc-gh changed the title ~~Start of AEAD encrypted cookies and sessions~~ AEAD encrypted cookies and sessions Feb 27, 2017

mjc-gh force-pushed the aead-encrypted-cookies branch from 3b1e5f4 to be8b9e8 Compare February 27, 2017 17:15

mjc-gh force-pushed the aead-encrypted-cookies branch 9 times, most recently from c5eaeaf to 8170de3 Compare March 5, 2017 19:16

kaspth reviewed May 15, 2017

View reviewed changes

guides/source/security.md Outdated

Copy link

Contributor

kaspth May 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also needs to keep the 80 char limit.

kaspth reviewed May 15, 2017

View reviewed changes

actionpack/CHANGELOG.md Outdated

Copy link

Contributor

kaspth May 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember to rewrite this too 😉

kaspth reviewed May 15, 2017

View reviewed changes

actionpack/CHANGELOG.md Outdated

Copy link

Contributor

kaspth May 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to add your name too 😄

*Michael Coyne*

mjc-gh reacted with heart emoji

mjc-gh force-pushed the aead-encrypted-cookies branch from f5fc1ba to 362d263 Compare May 16, 2017 08:12

mjc-gh force-pushed the aead-encrypted-cookies branch from 362d263 to d6bfe8d Compare May 16, 2017 22:00

mjc-gh force-pushed the aead-encrypted-cookies branch from d6bfe8d to 3faa392 Compare May 19, 2017 16:58

mjc-gh force-pushed the aead-encrypted-cookies branch 3 times, most recently from 0c37842 to 834db45 Compare May 21, 2017 13:05

kaspth reviewed May 21, 2017

View reviewed changes

mjc-gh force-pushed the aead-encrypted-cookies branch from 834db45 to 5a3ba63 Compare May 22, 2017 08:51

kaspth merged commit b88200f into rails:master May 28, 2017

zzak mentioned this pull request May 28, 2017

Encrypt cookies with AES-GCM sinatra/sinatra#1300

Closed

mjc-gh referenced this pull request in rack/rack Jun 4, 2017

Revert "join the remaining instead of reversing"

734a00c

This reverts commit 9a4700f.

cconstantin mentioned this pull request Feb 6, 2019

Issues with Rails 5.2.2 cconstantin/plug_rails_cookie_session_store#14

Open

	# Initialize configuration defaults for originally generated Rails version.
	config.load_defaults <%= Rails::VERSION::STRING.to_f %>

Conversation

mjc-gh commented Feb 23, 2017

Summary

Other Information

Uh oh!

rafaelfranca commented Feb 23, 2017

Uh oh!

mjc-gh commented Feb 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rafaelfranca commented Feb 23, 2017

Uh oh!

eugeneius commented Feb 23, 2017

Uh oh!

mjc-gh commented Feb 23, 2017

Uh oh!

eugeneius commented Feb 23, 2017

Uh oh!

mjc-gh commented Feb 23, 2017

Uh oh!

bdewater commented Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjc-gh commented Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjc-gh commented Feb 24, 2017

Notes on Legacy Support

Possible Major API Changes

Uh oh!

bdewater commented Feb 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjc-gh commented May 15, 2017

Uh oh!

mjc-gh commented May 16, 2017

Uh oh!

mjc-gh commented May 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjc-gh commented May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjc-gh commented May 21, 2017

Uh oh!

kaspth commented May 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaspth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjc-gh commented Feb 23, 2017 •

edited

Loading

bdewater commented Feb 24, 2017 •

edited

Loading

mjc-gh commented Feb 24, 2017 •

edited

Loading

mjc-gh commented May 16, 2017 •

edited

Loading

mjc-gh commented May 19, 2017 •

edited

Loading

kaspth commented May 21, 2017 •

edited

Loading

kaspth commented May 21, 2017 •

edited

Loading