Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Follow up of #25602 #25758

Merged
merged 2 commits into from Aug 31, 2016
Merged

Follow up of #25602 #25758

merged 2 commits into from Aug 31, 2016

Conversation

@vipulnsward
Copy link
Member

@vipulnsward vipulnsward commented Jul 9, 2016

Probably a WIP?

Since keys are truncated, ruby 2.4 doesn't accept keys greater than their lenghts.
keys of same value but different lenght and greater than key size of cipher, produce the same results
as reproduced at https://gist.github.com/rhenium/b81355fe816dcfae459cc5eadfc4f6f9
Since our default cipher is 'aes-256-cbc', key length for which is 32 bytes, limit the length of key being passed to Encryptor to 32 bytes.
This continues to support backwards compat with any existing signed data, already encrupted and signed with 32+ byte keys.
Also fixes the passing of this value in multiple tests.

@matthewd
Copy link
Member

@matthewd matthewd commented Jul 9, 2016

IMO secret[0, 32] reads slightly better than secret[0..31] (and technically saves a range allocation, though that obviously doesn't really matter).

Maybe we could:

DEFAULT_CIPHER = 'aes-256-cbc'

def self.key_length(cipher = DEFAULT_CIPHER)
  OpenSSL::Cipher.new(cipher).key_length
end

(and change initialize to use DEFAULT_CIPHER)

That way, we can leave the truncation up to the caller, but still encapsulate the knowledge of the target key length (and the default cipher). I don't anticipate us changing those, for pretty much the same reasons this is needing careful handling now: we'd break existing uses. But it still seems neater to avoid spreading locally-arbitrary integers around the place.

@matthewd
matthewd reviewed Jul 9, 2016
View changes
activesupport/test/message_encryptor_test.rb Outdated
# Message generated with 64 bit key
message = "eHdGeExnZEwvMSt3U3dKaFl1WFo0TjVvYzA0eGpjbm5WSkt5MXlsNzhpZ0ZnbWhBWFlQZTRwaXE1bVJCS2oxMDZhYVp2dVN3V0lNZUlWQ3c2eVhQbnhnVjFmeVVubmhRKzF3WnZyWHVNMDg9LS1HSisyakJVSFlPb05ISzRMaXRzcFdBPT0=--831a1d54a3cda8a0658dc668a03dedcbce13b5ca"
assert_equal 'data', encryptor.decrypt_and_verify(message)[:some]
end

This comment has been minimized.

@matthewd

matthewd Jul 9, 2016
Member

Do we have a similar "known data" test up at the cookie jar / middleware level? I guess not, because it should've been failing between #25192 and #25602. It's probably worth adding one of those too... this level is important because we have an API contract with callers that we need to preserve, but arguably the most important of all is that we retain the ability to read an actual cookie value a previous version could have handed out.

@vipulnsward vipulnsward force-pushed the vipulnsward:fix-key-len-issue branch 2 times, most recently Jul 9, 2016
@vipulnsward
Copy link
Member Author

@vipulnsward vipulnsward commented Jul 9, 2016

More changes, ready for another review.
r? @matthewd

@matthewd
Copy link
Member

@matthewd matthewd commented Jul 10, 2016

@vipulnsward sorry, I missed the most important piece of information 😳... I meant we could put that constant / class method onto MessageEncryptor.

So then the cookie store (and the tests) can do:

secret = key_generator.generate_key(request.encrypted_cookie_salt || '')[0, ActiveSupport::MessageEncryptor.key_length]
sign_secret = ...
@encryptor = ActiveSupport::MessageEncryptor.new(secret, sign_secret, digest: digest, serializer: ActiveSupport::MessageEncryptor::NullSerializer)

Slightly verbose, but no mention of AES or 32.

(Still only a suggestion... just clarifying my previous meaning.)

@vipulnsward vipulnsward force-pushed the vipulnsward:fix-key-len-issue branch Jul 10, 2016
@vipulnsward
Copy link
Member Author

@vipulnsward vipulnsward commented Jul 10, 2016

Alright, looks like we are all set now. Maybe needs a changelog?

@vipulnsward vipulnsward force-pushed the vipulnsward:fix-key-len-issue branch Jul 10, 2016
@Fudoshiki
Copy link
Contributor

@Fudoshiki Fudoshiki commented Aug 2, 2016

Hi, any progress?

Since keys are truncated, ruby 2.4 doesn't accept keys greater than their lenghts.
keys of same value but different lenght and greater than key size of cipher, produce the same results
as reproduced at https://gist.github.com/rhenium/b81355fe816dcfae459cc5eadfc4f6f9
Since our default cipher is 'aes-256-cbc', key length for which is 32 bytes, limit the length of key being passed to Encryptor to 32 bytes.
This continues to support backwards compat with any existing signed data, already encrupted and signed with 32+ byte keys.
Also fixes the passing of this value in multiple tests.
@vipulnsward vipulnsward force-pushed the vipulnsward:fix-key-len-issue branch Aug 31, 2016
…mine key length
@vipulnsward vipulnsward force-pushed the vipulnsward:fix-key-len-issue branch to 79c8478 Aug 31, 2016
@vipulnsward
Copy link
Member Author

@vipulnsward vipulnsward commented Aug 31, 2016

Rebased and updated.

@matthewd let me know what you think about the new changes.

@@ -372,7 +372,7 @@ def []=(name, options)

handle_options(options)

if @cookies[name.to_s] != value or options[:expires]
if @cookies[name.to_s] != value || options[:expires]

This comment has been minimized.

@vipulnsward

vipulnsward Aug 31, 2016
Author Member

This change is coming from rubocop. I ran rubocop to fix the single quote usage from my change below, causing this.

@matthewd matthewd merged commit df63c0d into rails:master Aug 31, 2016
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
codeclimate Code Climate found 1 fixed issue.
Details
matthewd added a commit that referenced this pull request Sep 1, 2016
@matthewd
Copy link
Member

@matthewd matthewd commented Sep 1, 2016

Backported in 15a972e

@@ -603,7 +603,7 @@ def test_encrypted_cookie_using_hybrid_serializer_can_migrate_marshal_dumped_val
secret = key_generator.generate_key(encrypted_cookie_salt)
sign_secret = key_generator.generate_key(encrypted_signed_cookie_salt)

marshal_value = ActiveSupport::MessageEncryptor.new(secret, sign_secret, serializer: Marshal).encrypt_and_sign("bar")
marshal_value = ActiveSupport::MessageEncryptor.new(secret[0, ActiveSupport::MessageEncryptor.key_len], sign_secret, serializer: Marshal).encrypt_and_sign("bar")

This comment has been minimized.

@chiragsinghal

chiragsinghal Sep 12, 2016
Contributor

@vipulnsward ActiveSupport::MessageEncryptor.key_len is repeated on multiple tests, can we define a method say max_key_length or something similar? Or is it an overkill to DRY this up in tests?

maclover7 added a commit to maclover7/rails that referenced this pull request Dec 20, 2016
Very similar to PR rails#25758, see more in depth reasoning there.
matthewd added a commit to matthewd/rails that referenced this pull request Dec 26, 2016
Very similar to PR rails#25758, see more in depth reasoning there.
matthewd added a commit to matthewd/rails that referenced this pull request Dec 27, 2016
Very similar to PR rails#25758, see more in depth reasoning there.
jmromer added a commit to bikeindex/bike_index that referenced this pull request Dec 6, 2019
Rails 5.0.0: produces a "key must be 32 bytes" error
Upgrading to Rails 5.0.1 addresses this issue.

Fixed in rails/rails#25758
jmromer added a commit to bikeindex/bike_index that referenced this pull request Dec 10, 2019
Rails 5.0.0: produces a "key must be 32 bytes" error
Upgrading to Rails 5.0.1 addresses this issue.

Fixed in rails/rails#25758
jmromer added a commit to bikeindex/bike_index that referenced this pull request Dec 11, 2019
Rails 5.0.0: produces a "key must be 32 bytes" error
Upgrading to Rails 5.0.1 addresses this issue.

Fixed in rails/rails#25758
jmromer added a commit to bikeindex/bike_index that referenced this pull request Dec 12, 2019
Rails 5.0.0: produces a "key must be 32 bytes" error
Upgrading to Rails 5.0.1 addresses this issue.

Fixed in rails/rails#25758
jmromer added a commit to bikeindex/bike_index that referenced this pull request Dec 14, 2019
Rails 5.0.0: produces a "key must be 32 bytes" error
Upgrading to Rails 5.0.1 addresses this issue.

Fixed in rails/rails#25758
jmromer added a commit to bikeindex/bike_index that referenced this pull request Dec 14, 2019
Rails 5.0.0: produces a "key must be 32 bytes" error
Upgrading to Rails 5.0.1 addresses this issue.

Fixed in rails/rails#25758
jmromer added a commit to bikeindex/bike_index that referenced this pull request Dec 16, 2019
Rails 5.0.0: produces a "key must be 32 bytes" error
Upgrading to Rails 5.0.1 addresses this issue.

Fixed in rails/rails#25758
jmromer added a commit to bikeindex/bike_index that referenced this pull request Dec 17, 2019
Rails 5.0.0: produces a "key must be 32 bytes" error
Upgrading to Rails 5.0.1 addresses this issue.

Fixed in rails/rails#25758
jmromer added a commit to bikeindex/bike_index that referenced this pull request Dec 19, 2019
Rails 5.0.0: produces a "key must be 32 bytes" error
Upgrading to Rails 5.0.1 addresses this issue.

Fixed in rails/rails#25758
jmromer added a commit to bikeindex/bike_index that referenced this pull request Dec 20, 2019
Rails 5.0.0: produces a "key must be 32 bytes" error
Upgrading to Rails 5.0.1 addresses this issue.

Fixed in rails/rails#25758
jmromer added a commit to bikeindex/bike_index that referenced this pull request Dec 24, 2019
Rails 5.0.0: produces a "key must be 32 bytes" error
Upgrading to Rails 5.0.1 addresses this issue.

Fixed in rails/rails#25758
jmromer added a commit to bikeindex/bike_index that referenced this pull request Jan 6, 2020
Rails 5.0.0: produces a "key must be 32 bytes" error
Upgrading to Rails 5.0.1 addresses this issue.

Fixed in rails/rails#25758
jmromer added a commit to bikeindex/bike_index that referenced this pull request Jan 8, 2020
Rails 5.0.0: produces a "key must be 32 bytes" error
Upgrading to Rails 5.0.1 addresses this issue.

Fixed in rails/rails#25758
jmromer added a commit to bikeindex/bike_index that referenced this pull request Jan 9, 2020
Rails 5.0.0: produces a "key must be 32 bytes" error
Upgrading to Rails 5.0.1 addresses this issue.

Fixed in rails/rails#25758
jmromer added a commit to bikeindex/bike_index that referenced this pull request Jan 12, 2020
Rails 5.0.0: produces a "key must be 32 bytes" error
Upgrading to Rails 5.0.1 addresses this issue.

Fixed in rails/rails#25758
jmromer added a commit to bikeindex/bike_index that referenced this pull request Jan 12, 2020
Rails 5.0.0: produces a "key must be 32 bytes" error
Upgrading to Rails 5.0.1 addresses this issue.

Fixed in rails/rails#25758
jmromer added a commit to bikeindex/bike_index that referenced this pull request Jan 13, 2020
Rails 5.0.0: produces a "key must be 32 bytes" error
Upgrading to Rails 5.0.1 addresses this issue.

Fixed in rails/rails#25758
jmromer added a commit to bikeindex/bike_index that referenced this pull request Jan 13, 2020
Rails 5.0.0: produces a "key must be 32 bytes" error
Upgrading to Rails 5.0.1 addresses this issue.

Fixed in rails/rails#25758
jmromer added a commit to bikeindex/bike_index that referenced this pull request Jan 15, 2020
Rails 5.0.0: produces a "key must be 32 bytes" error
Upgrading to Rails 5.0.1 addresses this issue.

Fixed in rails/rails#25758
jmromer added a commit to bikeindex/bike_index that referenced this pull request Jan 15, 2020
Rails 5.0.0: produces a "key must be 32 bytes" error
Upgrading to Rails 5.0.1 addresses this issue.

Fixed in rails/rails#25758
jmromer added a commit to bikeindex/bike_index that referenced this pull request Jan 17, 2020
Rails 5.0.0: produces a "key must be 32 bytes" error
Upgrading to Rails 5.0.1 addresses this issue.

Fixed in rails/rails#25758
@vipulnsward vipulnsward deleted the vipulnsward:fix-key-len-issue branch Jan 23, 2020
jmromer added a commit to bikeindex/bike_index that referenced this pull request Jan 27, 2020
Rails 5.0.0: produces a "key must be 32 bytes" error
Upgrading to Rails 5.0.1 addresses this issue.

Fixed in rails/rails#25758
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.