set IV after the encryption key to actually use IV in AES-*-GCM algorithms

[borama]

Hello,

a stack overflow user noticed a weird behavior when using attr_encrypted (see the SO question and discussion). The problem is that when encrypting with one of the aes-*-gcm algorithms while using initialization vector (IV), the encrypted result is the same for any IV, i.e. the init. vector is not taken into account at all. This seems to be an important security problem, especially considering that the aes-256-gcm is the default encryption algorithm in this gem.

Further investigation showed that the problem is only with the -gcm algorithms, other algorithms seem to be OK, which is btw also proved by the test_should_crypt_with_the_#{algorithm}_algorithm_with_iv test. Unfortunately this test does not verify the -gcm algorithms.

The problem can be fixed simply by setting the IV after setting the encryption key (current code sets IV before setting the key).

It is unclear to me where exactly this problem originates from, but the fact that it affects only the -gcm algorithms and not others suggests that it is a bug somewhere in the OpenSSL library code for AES-*-GCM algorithms. I could not find anything related to the order of setting IVs and keys in any documentation.

Anyway, this pull request moves setting the IV after setting the key and adds an accompanying test. This test fails in the current master branch but will run OK after merging.

It is very unfortunate that the default encryption algorithm is aes-256-gcm and thus the encryption is less secure for everybody who is not using random salt. A sad thing to note is also that this fix will probably make all these setups undecipherable as suddenly the IV would be taken into account after upgrading this gem.

Testing the issue:

Tested under ruby 2.3.0 compiled against OpenSSL version "1.0.1f 6 Jan 2014".

def base64_enc(bytes)
  [bytes].pack("m")
end

def test_aes_encr(n, cipher, data, key, iv, iv_before_key = true)
  cipher = OpenSSL::Cipher.new(cipher)
  cipher.encrypt

  if iv_before_key
    cipher.iv = iv
    cipher.key = key
  else
    cipher.key = key
    cipher.iv = iv
  end

  if cipher.name.downcase.end_with?("gcm")
    cipher.auth_data = ""
  end

  result = cipher.update(data)
  result << cipher.final

  puts "#{n} #{cipher.name}, iv #{iv_before_key ? "BEFORE" : "AFTER "} key: " +
           "iv=#{iv}, result=#{base64_enc(result)}"
end

def test_encryption
  data = "something private"
  key = "This is a key that is 256 bits!!"

  # control tests using AES-256-CBC
  test_aes_encr(1, "aes-256-cbc", data, key, "aaaabbbbccccdddd", true)
  test_aes_encr(2, "aes-256-cbc", data, key, "eeeeffffgggghhhh", true)
  test_aes_encr(3, "aes-256-cbc", data, key, "aaaabbbbccccdddd", false)
  test_aes_encr(4, "aes-256-cbc", data, key, "eeeeffffgggghhhh", false)

  # failing tests using AES-256-GCM
  test_aes_encr(5, "aes-256-gcm", data, key, "aaaabbbbcccc", true)
  test_aes_encr(6, "aes-256-gcm", data, key, "eeeeffffgggg", true)
  test_aes_encr(7, "aes-256-gcm", data, key, "aaaabbbbcccc", false)
  test_aes_encr(8, "aes-256-gcm", data, key, "eeeeffffgggg", false)
end

Running the test_encryption gives:

>> test_encryption
1 AES-256-CBC, iv BEFORE key: iv=aaaabbbbccccdddd, result=4IAGcazRmEUIRDE3ZpEgoS0Nmm1/+nrd5VT2/Xab0WM=
2 AES-256-CBC, iv BEFORE key: iv=eeeeffffgggghhhh, result=T7um2Wgb2vw1r4uryF3xnBeq+KozuetjKGItfNKurGE=
3 AES-256-CBC, iv AFTER  key: iv=aaaabbbbccccdddd, result=4IAGcazRmEUIRDE3ZpEgoS0Nmm1/+nrd5VT2/Xab0WM=
4 AES-256-CBC, iv AFTER  key: iv=eeeeffffgggghhhh, result=T7um2Wgb2vw1r4uryF3xnBeq+KozuetjKGItfNKurGE=

5 id-aes256-GCM, iv BEFORE key: iv=aaaabbbbcccc, result=Tl/HfkWpwoByeYRz6Mz4yIo=
6 id-aes256-GCM, iv BEFORE key: iv=eeeeffffgggg, result=Tl/HfkWpwoByeYRz6Mz4yIo=
7 id-aes256-GCM, iv AFTER  key: iv=aaaabbbbcccc, result=+4Iyn7RSDKimTQi0S3gn58E=
8 id-aes256-GCM, iv AFTER  key: iv=eeeeffffgggg, result=3m9uEDyb9eh1RD3CuOCmc50=

Test results 1-4 show that for CBC it is irrelevant if IV is set before or after the encryption key and that IV is always taken into account (the results differ for different IVs).

The results 5-8 on the other hand prove that the encrypted results are the same for two different IVs if IV set before the key, whereas the results are different when IV set after key.

If you need anything else from me regarding this issue, I'll be happy to help.

[trak3r]

Just got bit by this as well; it's breaking attr_encrypted gem attr-encrypted/attr_encrypted#203

[saghaulor]

@borama thank you for opening this issue. The intention of using aes-*-gcm was to improve security. It looks like this bug prevented that effort.

It looks like the only indication that the IV should be set after the key is in the example here: http://ruby-doc.org/stdlib-2.3.0/libdoc/openssl/rdoc/OpenSSL/Cipher.html#class-OpenSSL::Cipher-label-Authenticated+Encryption+and+Associated+Data+-28AEAD-29

However, it's not obvious that this ordering is required for the algorithm to work correctly.

I believe you are correct in your assessment regarding the breaking changes it will introduce. Do you have any recommendations on how to move forward with this?

I'm not sure if this requires another major version change, but since it will indeed break existing code using aes-*-gcm I'm leaning to the fact that yes it should.

There is a path forward for those that have been bitten by this bug, but it's not pretty. They can upgrade, and implement code to decrypt the old value and then re-encrypt using the new code that utilizes the IV. It's not pretty but it's a path forward.

Any suggestions that you have would be very much appreciated.

[brad-werth]

I would tend to agree with you about the major version change - a careless update could be catastrophic.

Perhaps the data manipulations required by the update could be automated in a rake task, or something, to help people do it correctly...

[borama]

Hello guys, I came to about the same conclusions as you.

I was also thinking about a rake task that would help users to decrypt (without IVs) and reencrypt (with IVs) their data. There is a risk I see in the fact that there is no way to automatically recognize if an encrypted value was calculated using or not using IV (unless you know the plain text data), so I'd be worried that someone ran this task more than once, for example. Perhaps the task should be interactive, showing a sample decrypted data and forcing the user to confirm that the decrypted data is ok before actually running the process?

Another problem I see is that it would be advisable for attr_encrypted users to upgrade the gem and their data offline because otherwise the currently encrypted data could be immediately decrypted wrong (with IVs) after deployment of the upgraded gem. If somebody using the web "corrected" the decrypted value, it would be saved with the IV and this would lead to data corruption later when the above-mentioned rake task would be ran.

I think I'd also agree with a major update, with big warnings everywhere about the consequences of the upgrade...

[trak3r]

I would tend to agree with you about the major version change

Agreed. But you want to make super sure this bug is really squashed; you don't want to have several major version releases in a single week.

with big warnings everywhere about the consequences of the upgrade

Seconded.

There is a path forward for those that have been bitten by this bug, but it's not pretty.

Suggestion for interim point version: leave the -gcm algorithms broken for backwards compatibility but (a) add a deprecation warning when they are invoked then (b) implement fixed versions suffixed like -gcm-fixed - yeah I know that's yucky but in order to migrate you need both version working so you can decrypt from the bad algorithm and encrypt with the good one.

[saghaulor]

I like the idea of a rake task, but I think there's too many variables to consider. Perhaps at the very least an example of how to do it is in order.

For users of attr_encrypted, the path forward is a bit easier, as you can pass in the encryption library to use. An affected app can copy the broken implementation and use that for decryption, and use the new version of encryptor for encrypting.

I agree with @borama's suggestion that this data migration should happen offline to prevent the different implementations from being used otherwise the migration becomes significantly more complex.

@trak3r I think the test that @borama provided is fairly conclusive, but point taken. Please feel free to review the tests and offer any suggestions.

[saghaulor]

I think this test proves that this is only a problem for the GCM algorithm. https://github.com/attr-encrypted/encryptor/blob/master/test/compatibility_test.rb#L44

[saghaulor]

I've pushed a new version of Encryptor. https://rubygems.org/gems/encryptor/versions/3.0.0 The bug that @borama pointed out is fixed. I've also added an option to allow the IV to be set in the way that it was in Encryptor v2.0.0, so that decryption of data encrypted using an AES-*-GCM algorithm is possible. This should facilitate migrating data that was incorrectly encrypted.

[borama]

@saghaulor thanks for the speedy update, great work!

[borama]

FYI, I made some further tests and came to believe this is either bug or an undocumented feature in the ruby-openssl code. I filed an issue there so you can track it if you like.

[borama]

FYI, it was recently confirmed that this had actually been an issue in the underlying ruby-openssl library and the bug has been fixed today. So, in the future, it is possible that attr_encrypted gem version 2.x will actually work correctly when used with the new openssl-2.0.0 gem version which is now in beta.

[saghaulor]

@borama I saw that, thanks for the update. The problem is, if people use v2 and upgrade to the fix, they're going to have records encrypted with varying implementations. It will be a mess to sort out.

[borama]

@saghaulor thanks, you are correct, this did not occur to me and I can see now that the fix may actually complicate things even more :(

[saghaulor]

Hopefully there won't be too much fallout. 🙏