I filed this and a related issue as #208496 on the bounty program page at HackerOne, though I believe it's not publicly visible. The link is https://hackerone.com/bugs?report_id=208496

I have a fix written, running tests now.

Stephen, check your email. There's another security concern with the original addition, also easily fixed.

Stephen and I raised this issue at almost the same time, but in different venues. I chose to use the bounty program page at HackerOne because this is what http://rubyonrails.org/security/ recommends.

Here is my report:

Pull request https://github.com/rails/rails/pull/28038/files which is part of Rails 5.1.0.beta1, uses AES-256 in CBC mode. According to Wikipedia, "Most modes require a unique binary sequence, often called an initialization vector (IV), for each encryption operation. The IV has to be non-repeating and, for some modes, random as well. The initialization vector is used to ensure distinct ciphertexts are produced even when the same plaintext is encrypted multiple times independently with the same key" -- https://en.wikipedia.org/wiki/Block_cipher_mode_of_operation

Rails chooses not to set an iv. As a result, secrets.yml.enc will always have identical content, given the same plaintext input and the same key. And it's actually worse than that.

Is this relevant? Well, again quoting Wikipedia, "An initialization vector has different security requirements than a key, so the IV usually does not need to be secret. However, in most cases, it is important that an initialization vector is never reused under the same key. For CBC and CFB, reusing an IV leaks some information about the first block of plaintext, and about any common prefix shared by the two messages. [...] In CBC mode, the IV must, in addition, be unpredictable at encryption time; in particular, the (previously) common practice of re-using the last ciphertext block of a message as the IV for the next message is insecure (for example, this method was used by SSL 2.0). If an attacker knows the IV (or the previous block of ciphertext) before he specifies the next plaintext, he can check his guess about plaintext of some block that was encrypted with the same key before (this is known as the TLS CBC IV attack)."

I have verified that the current implementation does not use an iv and as a result, secrets.yml.enc produces identical output, given identical input and the same key. It is UNCLEAR to me if this is catastrophic; the key itself is randomly generated and it would be unusual, but most certainly not difficult, to pick your own key. Still, given the current implementation, I believe I can tell if you have changed a key early in the file (which will result in pretty much every block of secrets.yml.enc changing) or only changed a key late in the file (which will result in the early blocks being left unchanged, and only the late blocks changing). If you set a random_iv, I would not be able to make this determination, nor should I be able to.

Additionally, I am concerned that you are calling SecureRandom.hex(cipher.key_len)[0, cipher.key_len] in generate_key. This generates a hex string of length 64, and then truncates it. What you actually want is SecureRandom.random_bytes(cipher.key_len), or you want to not truncate it, store it as hex, and convert it to bytes before feeding it in to your cipher. What you have is 128 bits of entropy when you are looking for 256 bits.

I then pointed out that stouset has addressed both of these issues and I believe this PR, if acceptable, resolves the security concerns. Note that he chose to go with aes-128-gcm which uses "only" 128 bits of entropy, but I believe that's sufficient here.

I glanced at stouset@b020b04 and wonder if that fixes the key truncation issue. I didn't see any changes to the key generation bit, so I assume that is still putting 32 hex chars in config/secrets.yml.key. Unfortunately, Ruby has an annoying issue of expecting the key to be a binary string and will just silently ignore all bytes longer than the expected key size. So, a 32 hex char, fed into a cipher expecting 16 bytes will just truncate the remaining 16 hex chars. For example:

16.upto(48) do |key_length|
  cipher = OpenSSL::Cipher.new("aes-128-gcm")
  cipher.encrypt
  cipher.key = "a" * key_length
  cipher.iv = "b" * 16
  ct = cipher.update("A") + cipher.final
  tag = cipher.auth_tag
  puts ct.inspect
  puts tag.inspect
end

"\xF4"
"82\xA5z{&\xCBH\xC14\x9A\xA0\xB3\xFF\xA3L"
"\xF4"
"82\xA5z{&\xCBH\xC14\x9A\xA0\xB3\xFF\xA3L"
"\xF4"
"82\xA5z{&\xCBH\xC14\x9A\xA0\xB3\xFF\xA3L"
"\xF4"
"82\xA5z{&\xCBH\xC14\x9A\xA0\xB3\xFF\xA3L"
"\xF4"
"82\xA5z{&\xCBH\xC14\x9A\xA0\xB3\xFF\xA3L"
"\xF4"
"82\xA5z{&\xCBH\xC14\x9A\xA0\xB3\xFF\xA3L"
"\xF4"
"82\xA5z{&\xCBH\xC14\x9A\xA0\xB3\xFF\xA3L"
"\xF4"
.....
"82\xA5z{&\xCBH\xC14\x9A\xA0\xB3\xFF\xA3L"
"\xF4"

@ptoomey3 I've already committed f7d9b1f which resolves the key generation issue.

@stouset - Nice! I've seen the hex string thing byte people so many times. Good call to just stick with bytes.

Awesomely quick response @stouset and @cliochris 😍. Thank you so much.

Hi @stouset great work on this!

This is a total drive-by comment, but might it be worth using ActiveSupport::MessageEncryptor for this? That way there's only one place to look for bad crypto in Rails 😄 .

MessageEncryptor handles iv generation in a similar way you do here (generating it and storing it alongside the ciphertext). You can also pass it a block cipher mode of operation like aes-gcm (it uses aes-cbc-256, authenticated with MessageVerifier by default).

MessageEncryptor/Verifier are currently used for the default cookie-based session store so are put to use in more or less 99% of Rails applications in production. While I'm not a crypto expert, I'm fairly confident there aren't any easily exploitable vulnerabilities in that construction.