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

Possible bug: order of setting key vs. IV affects encryption with AES GCM #49

Closed
borama opened this issue Mar 27, 2016 · 3 comments
Closed

Comments

@borama
Copy link

borama commented Mar 27, 2016

Hello, I think I may have found a possible bug in the ruby openssl code for encryption.

If initialization vector is set before setting the encryption key when using one of the AES-*-GCM algorithms, the encryption does not take the IV into account at all and two different IVs (with the same key) produce the same encrypted ciphertext. If IV is set after the key, everything behaves perfectly OK. This issue does not affect other algorithms, only the AES-GCM ones.

For more context about how I came to this conclusion, please see this stack overflow question and this pull request in the encryptor gem.

Let me first show a simple test in ruby, to prove this issue:

# test_gcm.rb
require 'openssl'

def hex_enc(bytes)
  bytes.bytes.map { |b| sprintf("%02x", b) }.join(" ")
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=#{hex_enc(result)}"
end

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)

When you run this test file, you'll get these results:

1 AES-256-CBC, iv BEFORE key: iv=aaaabbbbccccdddd, result=e0 80 06 71 ac d1 98 45 08 44 31 37 66 91 20 a1 2d 0d 9a 6d 7f fa 7a dd e5 54 f6 fd 76 9b d1 63
2 AES-256-CBC, iv BEFORE key: iv=eeeeffffgggghhhh, result=4f bb a6 d9 68 1b da fc 35 af 8b ab c8 5d f1 9c 17 aa f8 aa 33 b9 eb 63 28 62 2d 7c d2 ae ac 61
3 AES-256-CBC, iv AFTER  key: iv=aaaabbbbccccdddd, result=e0 80 06 71 ac d1 98 45 08 44 31 37 66 91 20 a1 2d 0d 9a 6d 7f fa 7a dd e5 54 f6 fd 76 9b d1 63
4 AES-256-CBC, iv AFTER  key: iv=eeeeffffgggghhhh, result=4f bb a6 d9 68 1b da fc 35 af 8b ab c8 5d f1 9c 17 aa f8 aa 33 b9 eb 63 28 62 2d 7c d2 ae ac 61
5 id-aes256-GCM, iv BEFORE key: iv=aaaabbbbcccc, result=4e 5f c7 7e 45 a9 c2 80 72 79 84 73 e8 cc f8 c8 8a
6 id-aes256-GCM, iv BEFORE key: iv=eeeeffffgggg, result=4e 5f c7 7e 45 a9 c2 80 72 79 84 73 e8 cc f8 c8 8a
7 id-aes256-GCM, iv AFTER  key: iv=aaaabbbbcccc, result=fb 82 32 9f b4 52 0c a8 a6 4d 08 b4 4b 78 27 e7 c1
8 id-aes256-GCM, iv AFTER  key: iv=eeeeffffgggg, result=de 6f 6e 10 3c 9b f5 e8 75 44 3d c2 b8 e0 a6 73 9d

The critical lines are lines 5 and 6. They show that when the IV is set before the encryption key, the IV is not taken into account. Lines 1-4 show that it this behavior is not present in the CBC encryption mode.

I tried to do further tests and they suggest that this behavior is caused by the pre-initialization of the encryption key in ossl_cipher.c. In the following test, I tried to closely mimic the C calls that ruby-openssl makes when doing a very simple encryption task:

/* aes_gcm.c */
#include <stdio.h>
#include <stdbool.h>
#include <openssl/bio.h>
#include <openssl/evp.h>

# define EVP_CTRL_AEAD_SET_IVLEN 0x9

static const unsigned char gcm_key[] = {
    'T', 'h', 'i', 's', ' ', 'i', 's', ' ', 'a', ' ', 'k', 'e', 'y', ' ',
    't', 'h', 'a', 't', ' ', 'i', 's', ' ', '2', '5', '6', ' ', 'b', 'i', 't', 's', '!', '!'
};

static const unsigned char gcm_iv[] = {
    'a', 'a', 'a', 'a', 'b', 'b', 'b', 'b', 'c', 'c', 'c', 'c'
};

static const unsigned char gcm_pt[] = {
   's', 'o', 'm', 'e', 't', 'h', 'i', 'n', 'g', ' ', 'p', 'r', 'i', 'v', 'a', 't', 'e'
};

static const unsigned char gcm_aad[] = { };

void aes_gcm_encrypt(bool iv_before_key, bool initialize_key) {
    int outlen;
    unsigned char outbuf[1024];
    EVP_CIPHER_CTX *ctx;
    const EVP_CIPHER *cipher;
    unsigned char null_key[EVP_MAX_KEY_LENGTH];


    /* initialize context */
    ctx = EVP_CIPHER_CTX_new();
    EVP_CIPHER_CTX_init(ctx);


    /* configure cipher */
    cipher = EVP_aes_256_gcm();
    if (initialize_key) {
      printf("init key, ");
      memset(null_key, 0, EVP_MAX_KEY_LENGTH);
      EVP_CipherInit_ex(ctx, cipher, NULL, null_key, NULL, -1);
    } else {
      printf("no init key, ");
      EVP_CipherInit_ex(ctx, cipher, NULL, NULL, NULL, -1);
    }

    /* encrypt */
    EVP_CipherInit_ex(ctx, NULL, NULL, NULL, NULL, 1);

    /* set key and IV */
    if (iv_before_key) {
      printf("iv before key:\n");
      EVP_CipherInit_ex(ctx, NULL, NULL, NULL, gcm_iv, -1);
      EVP_CipherInit_ex(ctx, NULL, NULL, gcm_key, NULL, -1);
    } else {
      printf("iv after  key:\n");
      EVP_CipherInit_ex(ctx, NULL, NULL, gcm_key, NULL, -1);
      EVP_CipherInit_ex(ctx, NULL, NULL, NULL, gcm_iv, -1);
    }

    /* set the authenticated data */
    EVP_CipherUpdate(ctx, NULL, &outlen, gcm_aad, sizeof(gcm_aad));

    /* encrypt! */
    EVP_CipherUpdate(ctx, outbuf, &outlen, gcm_pt, sizeof(gcm_pt));

    /* print result */
    BIO_dump_fp(stdout, outbuf, outlen);

    EVP_EncryptFinal_ex(ctx, outbuf, &outlen);
    EVP_CIPHER_CTX_free(ctx);
}

int main(int argc, char **argv)  {
    aes_gcm_encrypt(true, true);
    aes_gcm_encrypt(true, false);
    aes_gcm_encrypt(false, true);
    aes_gcm_encrypt(false, false);
 }

The test tries to encrypt the same data as in the ruby test above, with IV set before / after the key and with or without the pre-initialization of the key. Compiling and running the test reveals the following:

init key, iv before key:
0000 - 4e 5f c7 7e 45 a9 c2 80-72 79 84 73 e8 cc f8 c8   N_.~E...ry.s....
0010 - 8a                                                .
no init key, iv before key:
0000 - fb 82 32 9f b4 52 0c a8-a6 4d 08 b4 4b 78 27 e7   ..2..R...M..Kx'.
0010 - c1                                                .
init key, iv after  key:
0000 - fb 82 32 9f b4 52 0c a8-a6 4d 08 b4 4b 78 27 e7   ..2..R...M..Kx'.
0010 - c1                                                .
no init key, iv after  key:
0000 - fb 82 32 9f b4 52 0c a8-a6 4d 08 b4 4b 78 27 e7   ..2..R...M..Kx'.
0010 - c1         

The test shows that when the key pre-initialization (i.e. setting the key to all zeroes when configuring the cipher) is skipped, the IV, even if set before the encryption key, is correctly taken into account. On the other hand, when the pre-initialization takes place, the IV must be set after the key for the data to be encrypted correctly. I have not experienced any seg-faults when not preinitializing the key (a warning about this is present in the comment above the preinitialization code). I compiled and tested the code above against master branch of openssl as well as openssl-1.0.1f with the same results.

Overall, this behavior seems to me like a bug. Nowhere in the ruby-openssl documentation I have found any mention about the order of setting IV vs key being relevant for the encryption process. I believe this should perhaps be more explicitly documented, because accidental setting IVs before keys with GCM algorithms would lead to a severe weakening of the whole encryption, without the user being warned in any way.

What do you think? Let me know if you need further info and thanks!

@saghaulor
Copy link

👍

@rhenium
Copy link
Member

rhenium commented Sep 20, 2016

Thank you for detailed analysis.

From aes*_gcm_init_key() in crypto/evp/e_aes.c that is called through EVP_CipherInit_ex():

1432     if (key) {
1433         do {
<snip>
1472             CRYPTO_gcm128_init(&gctx->gcm, &gctx->ks,
1473                                (block128_f) AES_encrypt);
<snip>
1479         } while (0);
1480
1481         /*
1482          * If we have an iv can set it directly, otherwise use saved IV.
1483          */
1484         if (iv == NULL && gctx->iv_set)
1485             iv = gctx->iv;
1486         if (iv) {
1487             CRYPTO_gcm128_setiv(&gctx->gcm, iv, gctx->ivlen);
1488             gctx->iv_set = 1;
1489         }
1490         gctx->key_set = 1;
1491     } else {
1492         /* If key set use IV, otherwise copy */
1493         if (gctx->key_set)
1494             CRYPTO_gcm128_setiv(&gctx->gcm, iv, gctx->ivlen);
1495         else
1496             memcpy(gctx->iv, iv, gctx->ivlen);
1497         gctx->iv_set = 1;
1498         gctx->iv_gen = 0;
1499     }

So Cipher#iv= does not preserve the IV in gctx->iv because gctx->key_set is already set by the pre-initialization in Cipher#initialize, and the subsequent call of Cipher#key= resets the IV to uninitialized (zeroed by OPENSSL_zalloc() in EVP_CipherInit_ex()) gctx->iv.

The pre-initialization in Cipher#initialize was added as the fix for https://bugs.ruby-lang.org/issues/2768. I still get the SEGV with OpenSSL::Cipher.new("aes-128-ecb").update("x"*16) if I remove it.

The possible workaround is to remove the initialization and make Cipher#update check that a key is set by the user.

rhenium added a commit to rhenium/ruby-openssl that referenced this issue Sep 20, 2016
Don't set an initial key in Cipher#initialize, and instead, make
Cipher#update raise an exception if an encryption key not provided by
user.

We currently set the all-zero key as a workaround for [Bug #2768].
Calling EVP_CipherUpdate() with no key set can cause the process to
crash for some certain ciphers. This has worked well for most others
which don't have the issue, but not for aes-*-gcm ciphers. aes-*-gcm
ciphers don't seem to expect the key to be set more than once - setting
a key, an IV, a key in this order causes the IV to be discarded.

An user can still call Cipher#key= multiple times, and this won't be
rescued.

Reference: https://bugs.ruby-lang.org/issues/2768
Reference: https://bugs.ruby-lang.org/issues/8221
Reference: ruby#49
rhenium added a commit that referenced this issue Sep 28, 2016
Remove the encryption key initialization from Cipher#initialize. This
is effectively a revert of r32723 ("Avoid possible SEGV from AES
encryption/decryption", 2011-07-28).

r32723, which added the key initialization, was a workaround for
Ruby Bug #2768. For some certain ciphers, calling EVP_CipherUpdate()
before setting an encryption key caused segfault. It was not a problem
until OpenSSL implemented GCM mode - the encryption key could be
overridden by repeated calls of EVP_CipherInit_ex(). But, it is not the
case for AES-GCM ciphers. Setting a key, an IV, a key, in this order
causes the IV to be reset to an all-zero IV.

The problem of Bug #2768 persists on the current versions of OpenSSL.
So, make Cipher#update raise an exception if a key is not yet set by the
user. Since encrypting or decrypting without key does not make any
sense, this should not break existing applications.

Users can still call Cipher#key= and Cipher#iv= multiple times with
their own responsibility.

Reference: https://bugs.ruby-lang.org/issues/2768
Reference: https://bugs.ruby-lang.org/issues/8221
Reference: #49
@rhenium
Copy link
Member

rhenium commented Sep 28, 2016

Fixed by 8108e0a. Thanks again.

@rhenium rhenium closed this as completed Sep 28, 2016
ssgelm pushed a commit to ssgelm/ruby-debian that referenced this issue Oct 9, 2017
Remove the encryption key initialization from Cipher#initialize. This
is effectively a revert of r32723 ("Avoid possible SEGV from AES
encryption/decryption", 2011-07-28).

r32723, which added the key initialization, was a workaround for
Ruby Bug #2768. For some certain ciphers, calling EVP_CipherUpdate()
before setting an encryption key caused segfault. It was not a problem
until OpenSSL implemented GCM mode - the encryption key could be
overridden by repeated calls of EVP_CipherInit_ex(). But, it is not the
case for AES-GCM ciphers. Setting a key, an IV, a key, in this order
causes the IV to be reset to an all-zero IV.

The problem of Bug #2768 persists on the current versions of OpenSSL.
So, make Cipher#update raise an exception if a key is not yet set by the
user. Since encrypting or decrypting without key does not make any
sense, this should not break existing applications.

Users can still call Cipher#key= and Cipher#iv= multiple times with
their own responsibility.

Reference: https://bugs.ruby-lang.org/issues/2768
Reference: https://bugs.ruby-lang.org/issues/8221
Reference: ruby/openssl#49

Closes: #842432 [CVE-2016-7798]

Backportted for Debian by Christian Hofstaedtler <zeha@debian.org>

Signed-off-by: Christian Hofstaedtler <zeha@debian.org>
Signed-off-by: Antonio Terceiro <terceiro@debian.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants