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

Fix default key length on cipher #25192

Merged
merged 1 commit into from
Jun 28, 2016

Conversation

vipulnsward
Copy link
Member

We default to using aes-256-cbc as our verification/signing cipher. It can accept key lengths of 128, 192 or 256-bit, whereas currently we were providing twice the acceptable value.

ruby < 2.4 allowed accepting these values, as extra key bits were ignored. Since ruby/ruby@ce63526 this now has a strict checking on key length.

Default to key length 32 bytes, to match the compatible length for aes-256-cbc

Fixes #25185

@vipulnsward vipulnsward force-pushed the 25185-default-key-length branch 2 times, most recently from 94b9583 to 9fceeb6 Compare May 29, 2016 18:22
@vipulnsward vipulnsward changed the title WIP: Fix default key length Fix default key length on cipher May 29, 2016
@vipulnsward
Copy link
Member Author

r? @jeremy
review @jeremy @tenderlove
cc @rhenium

@@ -19,7 +19,7 @@ def setup
test "Generating a key of the default length" do
derived_key = @generator.generate_key("some_salt")
assert_kind_of String, derived_key
assert_equal OpenSSL::Digest::SHA1.new.block_length, derived_key.length, "Should have generated a key of the default size"
assert_equal OpenSSL::Cipher::Cipher.new('aes-256-cbc').key_len, derived_key.length, "Should have generated a key of the default size"
Copy link

Choose a reason for hiding this comment

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

This should be OpenSSL::Cipher.new(...). OpenSSL::Cipher::Cipher has been deprecated since Ruby 1.9.

@vipulnsward
Copy link
Member Author

Adding this to 5.0 since, 5.0 Apps can't start on ruby 2.4 without this.

@vipulnsward vipulnsward added this to the 5.0.0 milestone May 31, 2016
@@ -15,7 +15,7 @@ def load(value)
end

def setup
@secret = SecureRandom.hex(64)
@secret = SecureRandom.hex(16)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think SecureRandom.random_bytes(32) is more readable :)

Copy link
Member

Choose a reason for hiding this comment

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

Agree with @yui-knk

@sgrif
Copy link
Contributor

sgrif commented Jun 7, 2016

Moving off the 5.0.0 milestone since we don't need to block the release for something that only affects unreleased versions of Ruby.

@tenderlove
Copy link
Member

@vipulnsward can you update with @yui-knk's suggestion and then I'll merge.

@vipulnsward
Copy link
Member Author

r? @tenderlove Updated

@rails-bot rails-bot assigned tenderlove and unassigned jeremy Jun 28, 2016
@@ -51,7 +51,7 @@ def test_signed_round_tripping
def test_alternative_serialization_method
prev = ActiveSupport.use_standard_json_time_format
ActiveSupport.use_standard_json_time_format = true
encryptor = ActiveSupport::MessageEncryptor.new(SecureRandom.hex(64), SecureRandom.hex(64), :serializer => JSONSerializer.new)
encryptor = ActiveSupport::MessageEncryptor.new(SecureRandom.hex(16), SecureRandom.hex(64), :serializer => JSONSerializer.new)
Copy link
Member

Choose a reason for hiding this comment

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

Change this one to random_bytes too please.

@vipulnsward
Copy link
Member Author

Updated all occurrences.

…t can accept key lengths of 128, 192 or 256-bit, whereas currently we were providing twice the acceptable value.

ruby < 2.4 allowed accepting these values, as extra key bits were ignored. Since ruby/ruby@ce63526 this now has a strict checking on key length.

Default to key length 32 bytes, to match the compatible length for  aes-256-cbc

Fixes rails#25185
matthewd added a commit that referenced this pull request Jun 30, 2016
@Fudoshiki
Copy link
Contributor

Any option for support 2.4 ruby by edge rails?

@schneems
Copy link
Member

schneems commented Jul 7, 2016

will need to have rolling keys support that @matthewd was speaking about.

Is this being worked on?

I re-opened the attached issue since the change was reverted.

@alexch
Copy link

alexch commented Jan 13, 2017

I'm getting this error with Ruby 2.4.0p0 and Rails 4.2.7.1. Are there plans to support Ruby 2.4 with Rails 4.2?

@wpp
Copy link

wpp commented Jan 13, 2017

@alexch Yes. See: #27473

iwz pushed a commit to iwz/rails that referenced this pull request Feb 15, 2017
We default to using aes-256-cbc as our verification/signing cipher.
It can accept key lengths of 128, 192 or 256-bit, whereas currently we
were providing twice the acceptable value.

ruby < 2.4 allowed accepting these values, as extra key bits were
ignored. Since ruby/ruby@ce63526 this now has a strict checking on key
length.

Default to key length 32 bytes, to match the compatible length for
aes-256-cbc

Backport to Rails 4.2.8 of fix for rails#25185

Credit to @vipulnsward

See: rails#25192
iwz added a commit to iwz/rails that referenced this pull request Feb 15, 2017
We default to using aes-256-cbc as our verification/signing cipher.
It can accept key lengths of 128, 192 or 256-bit, whereas currently we
were providing twice the acceptable value.

ruby < 2.4 allowed accepting these values, as extra key bits were
ignored. Since ruby/ruby@ce63526 this now has a strict checking on key
length.

Default to key length 32 bytes, to match the compatible length for
aes-256-cbc

Backport to Rails 4.2.8 of fix for rails#25185

Credit to @vipulnsward, @matthewd

See: rails#25192
See: rails#25602
@benoittgt
Copy link
Contributor

benoittgt commented Mar 13, 2017

On my personal projet I use a key size of 128 chars. This change break my code. I'm using rails 4.2.8 and ruby 2.4.0.

def encryptor(id)
  ActiveSupport::MessageEncryptor.new(key(id)) # key(id).length = 128
end

def key(id)
  ActiveSupport::KeyGenerator.new(
    Digest::SHA512.base64digest(
      id.to_s + (0...64).map { ('a'..'z').to_a[rand(26)] }.join
    )
  ).generate_key((0...64).map { ('a'..'z').to_a[rand(26)] }.join, 128)
end

I get

ArgumentError:
       key must be 32 bytes

Does it means I can't use 128 key size?

@matthewd
Copy link
Member

@benoittgt this PR changed the default value of an argument you're explicitly supplying; it is not involved in the change you're seeing.

The change you care about is ruby/ruby@ce63526, which is not in Rails.

@benoittgt
Copy link
Contributor

Thanks @matthewd. Sorry for the noise.

sonots added a commit to triglav-dataflow/triglav that referenced this pull request Apr 15, 2017
@vanbumi
Copy link

vanbumi commented Apr 23, 2017

still no idea how the steps to fix this ? help pelase

@MartinNowak
Copy link

MartinNowak commented Aug 16, 2017

This needs some more work IMO.
One option would be to keep implicitly slicing the cipher secret (but not the signature secret), at best with some deprecation warning.
In any case the documentation should to be updated http://api.rubyonrails.org/classes/ActiveSupport/MessageEncryptor.html (secret must be at least as long as the cipher key size.).

As a workaround for people updating their code, provide a sliced secret for en/decryption but the full length for signing/verifying.

ActiveSupport::MessageEncryptor.new(mykey[0, 32], mykey)

@MartinNowak
Copy link

Forgot to mention people @vipulnsward, @matthewd, @vanbumi

gylaz pushed a commit to houndci/hound that referenced this pull request Aug 31, 2017
This reverts commit b77a3e3.

Because Ruby 2.4.0+ does not allow keys longer than `32` bytes, as per
rails/rails#25192
gylaz pushed a commit to houndci/hound that referenced this pull request Aug 31, 2017
This reverts commit b77a3e3.

Because Ruby 2.4.0+ does not allow keys longer than `32` bytes, as per
rails/rails#25192

On staging and production we are currently using longer than 32 strings,
and we can't just shorten them (grab the first 32 chars) since we have
tokens stored in the DB encrypted with those longer strings.
gylaz pushed a commit to houndci/hound that referenced this pull request Aug 31, 2017
This reverts commit b77a3e3.

Because Ruby 2.4.0+ does not allow keys longer than `32` bytes, as per
rails/rails#25192

On staging and production we are currently using longer than 32 strings,
and we can't just shorten them (grab the first 32 chars) since we have
tokens stored in the DB encrypted with those longer strings.
seanpdoyle added a commit to thoughtbot/ember-cli-rails that referenced this pull request Mar 30, 2018
seanpdoyle added a commit to thoughtbot/ember-cli-rails that referenced this pull request Mar 30, 2018
whalesalad added a commit to whalesalad/rails that referenced this pull request Jan 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.