New Secrets feature uses poor cryptography #28135

Closed
stouset opened this Issue Feb 23, 2017 · 9 comments

Comments

Projects
None yet
7 participants
@stouset
Contributor

stouset commented Feb 23, 2017

    def new_cipher
      OpenSSL::Cipher.new("aes-256-cbc")
    end

    def cipher(mode, data)
      cipher = new_cipher.public_send(mode)
      cipher.key = key
      cipher.update(data) << cipher.final
    end

Unauthenticated CBC mode with a constant IV. This is not up to the minimum standards of how a modern cryptosystem should be designed. Please use an authenticated mode (GCM, or if unavailable, derive an authentication key and HMAC the ciphertext) and generate an unpredictable, random IV each and every time an encryption is performed.

@cliochris

This comment has been minimized.

Show comment
Hide comment
@cliochris

cliochris Feb 23, 2017

Contributor

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

Contributor

cliochris commented Feb 23, 2017

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

@rafaelfranca rafaelfranca added this to the 5.1.0 milestone Feb 23, 2017

@stouset

This comment has been minimized.

Show comment
Hide comment
@stouset

stouset Feb 23, 2017

Contributor

I have a fix written, running tests now.

Contributor

stouset commented Feb 23, 2017

I have a fix written, running tests now.

@cliochris

This comment has been minimized.

Show comment
Hide comment
@cliochris

cliochris Feb 23, 2017

Contributor

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

Contributor

cliochris commented Feb 23, 2017

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

@cliochris

This comment has been minimized.

Show comment
Hide comment
@cliochris

cliochris Feb 23, 2017

Contributor

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.

Contributor

cliochris commented Feb 23, 2017

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.

stouset added a commit to stouset/rails that referenced this issue Feb 23, 2017

@ptoomey3

This comment has been minimized.

Show comment
Hide comment
@ptoomey3

ptoomey3 Feb 23, 2017

Contributor

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"
Contributor

ptoomey3 commented Feb 23, 2017

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"
@stouset

This comment has been minimized.

Show comment
Hide comment
@stouset

stouset Feb 23, 2017

Contributor

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

Contributor

stouset commented Feb 23, 2017

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

@ptoomey3

This comment has been minimized.

Show comment
Hide comment
@ptoomey3

ptoomey3 Feb 23, 2017

Contributor

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

Contributor

ptoomey3 commented Feb 23, 2017

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

stouset added a commit to stouset/rails that referenced this issue Feb 23, 2017

stouset added a commit to stouset/rails that referenced this issue Feb 23, 2017

stouset added a commit to stouset/rails that referenced this issue Feb 24, 2017

stouset added a commit to stouset/rails that referenced this issue Feb 24, 2017

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 24, 2017

Member

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

Member

dhh commented Feb 24, 2017

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

@Najaf

This comment has been minimized.

Show comment
Hide comment
@Najaf

Najaf Feb 27, 2017

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.

Najaf commented Feb 27, 2017

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.

stouset added a commit to stouset/rails that referenced this issue Mar 1, 2017

stouset added a commit to stouset/rails that referenced this issue Mar 1, 2017

stouset added a commit to stouset/rails that referenced this issue Mar 2, 2017

@kaspth kaspth closed this in #28139 Mar 2, 2017

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