| Message ID | 1472215666-13263-1-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/26/2016 07:47 AM, Daniel P. Berrange wrote: > The XTS cipher mode needs to be used with a cipher which has > a block size of 16 bytes. If a mis-matching block size is used, > the code will either corrupt memory beyond the IV array, or > not fully encrypt/decrypt the IV. > > This fixes a memory curruption crash when attempting to use s/curruption/corruption/ > cast5-128 with xts, since the former has an 8 byte block size. > > A test case is added to ensure the cipher creation fails with > such an invalid combination. > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > crypto/cipher-gcrypt.c | 6 ++++++ > crypto/cipher-nettle.c | 12 +++++++----- > tests/test-crypto-cipher.c | 44 ++++++++++++++++++++++++++++++++++++-------- > 3 files changed, 49 insertions(+), 13 deletions(-) Are you aiming for a last-minute 2.7 fix, or should this just be 2.8 material and cc qemu-stable? Reviewed-by: Eric Blake <eblake@redhat.com> > +++ b/tests/test-crypto-cipher.c > @@ -370,6 +370,17 @@ static QCryptoCipherTestData test_data[] = { > @@ -449,8 +468,16 @@ static void test_cipher(const void *opaque) > cipher = qcrypto_cipher_new( > data->alg, data->mode, > key, nkey, > - &error_abort); > - g_assert(cipher != NULL); > + &err); > + if (data->plaintext) { > + g_assert(err == NULL); > + g_assert(cipher != NULL); > + } else { > + g_assert(err != NULL); > + error_free(err); Could shorten these two lines as error_free_or_abort(&err), but that's cosmetic.
On Fri, Aug 26, 2016 at 01:21:50PM -0500, Eric Blake wrote: > On 08/26/2016 07:47 AM, Daniel P. Berrange wrote: > > The XTS cipher mode needs to be used with a cipher which has > > a block size of 16 bytes. If a mis-matching block size is used, > > the code will either corrupt memory beyond the IV array, or > > not fully encrypt/decrypt the IV. > > > > This fixes a memory curruption crash when attempting to use > > s/curruption/corruption/ > > > cast5-128 with xts, since the former has an 8 byte block size. > > > > A test case is added to ensure the cipher creation fails with > > such an invalid combination. > > > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > --- > > crypto/cipher-gcrypt.c | 6 ++++++ > > crypto/cipher-nettle.c | 12 +++++++----- > > tests/test-crypto-cipher.c | 44 ++++++++++++++++++++++++++++++++++++-------- > > 3 files changed, 49 insertions(+), 13 deletions(-) > > Are you aiming for a last-minute 2.7 fix, or should this just be 2.8 > material and cc qemu-stable? This isn't critical for 2.7, as this is already invalid usage. IOW anyone who hits the crash, simply shouldn't use this combination. > Reviewed-by: Eric Blake <eblake@redhat.com> > > > > +++ b/tests/test-crypto-cipher.c > > @@ -370,6 +370,17 @@ static QCryptoCipherTestData test_data[] = { > > > @@ -449,8 +468,16 @@ static void test_cipher(const void *opaque) > > cipher = qcrypto_cipher_new( > > data->alg, data->mode, > > key, nkey, > > - &error_abort); > > - g_assert(cipher != NULL); > > + &err); > > + if (data->plaintext) { > > + g_assert(err == NULL); > > + g_assert(cipher != NULL); > > + } else { > > + g_assert(err != NULL); > > + error_free(err); > > Could shorten these two lines as error_free_or_abort(&err), but that's > cosmetic. Will do. Regards, Daniel
diff --git a/crypto/cipher-gcrypt.c b/crypto/cipher-gcrypt.c index ede2f70..3652aa1 100644 --- a/crypto/cipher-gcrypt.c +++ b/crypto/cipher-gcrypt.c @@ -192,6 +192,12 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg, } if (cipher->mode == QCRYPTO_CIPHER_MODE_XTS) { + if (ctx->blocksize != XTS_BLOCK_SIZE) { + error_setg(errp, + "Cipher block size %zu must equal XTS block size %d", + ctx->blocksize, XTS_BLOCK_SIZE); + goto error; + } ctx->iv = g_new0(uint8_t, ctx->blocksize); } diff --git a/crypto/cipher-nettle.c b/crypto/cipher-nettle.c index 70909fb..0267da5 100644 --- a/crypto/cipher-nettle.c +++ b/crypto/cipher-nettle.c @@ -361,6 +361,13 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg, goto error; } + if (mode == QCRYPTO_CIPHER_MODE_XTS && + ctx->blocksize != XTS_BLOCK_SIZE) { + error_setg(errp, "Cipher block size %zu must equal XTS block size %d", + ctx->blocksize, XTS_BLOCK_SIZE); + goto error; + } + ctx->iv = g_new0(uint8_t, ctx->blocksize); cipher->opaque = ctx; @@ -456,11 +463,6 @@ int qcrypto_cipher_decrypt(QCryptoCipher *cipher, break; case QCRYPTO_CIPHER_MODE_XTS: - if (ctx->blocksize != XTS_BLOCK_SIZE) { - error_setg(errp, "Block size must be %d not %zu", - XTS_BLOCK_SIZE, ctx->blocksize); - return -1; - } xts_decrypt(ctx->ctx, ctx->ctx_tweak, ctx->alg_encrypt_wrapper, ctx->alg_decrypt_wrapper, ctx->iv, len, out, in); diff --git a/tests/test-crypto-cipher.c b/tests/test-crypto-cipher.c index 1b5130d..51e9700 100644 --- a/tests/test-crypto-cipher.c +++ b/tests/test-crypto-cipher.c @@ -370,6 +370,17 @@ static QCryptoCipherTestData test_data[] = { "eb4a427d1923ce3ff262735779a418f2" "0a282df920147beabe421ee5319d0568", }, + { + /* Bad config - cast5-128 has 8 byte block size + * which is incompatible with XTS + */ + .path = "/crypto/cipher/cast5-xts-128", + .alg = QCRYPTO_CIPHER_ALG_CAST5_128, + .mode = QCRYPTO_CIPHER_MODE_XTS, + .key = + "27182818284590452353602874713526" + "31415926535897932384626433832795", + } }; @@ -432,15 +443,23 @@ static void test_cipher(const void *opaque) const QCryptoCipherTestData *data = opaque; QCryptoCipher *cipher; - uint8_t *key, *iv, *ciphertext, *plaintext, *outtext; - size_t nkey, niv, nciphertext, nplaintext; - char *outtexthex; + uint8_t *key, *iv = NULL, *ciphertext = NULL, + *plaintext = NULL, *outtext = NULL; + size_t nkey, niv = 0, nciphertext = 0, nplaintext = 0; + char *outtexthex = NULL; size_t ivsize, keysize, blocksize; + Error *err = NULL; nkey = unhex_string(data->key, &key); - niv = unhex_string(data->iv, &iv); - nciphertext = unhex_string(data->ciphertext, &ciphertext); - nplaintext = unhex_string(data->plaintext, &plaintext); + if (data->iv) { + niv = unhex_string(data->iv, &iv); + } + if (data->ciphertext) { + nciphertext = unhex_string(data->ciphertext, &ciphertext); + } + if (data->plaintext) { + nplaintext = unhex_string(data->plaintext, &plaintext); + } g_assert(nciphertext == nplaintext); @@ -449,8 +468,16 @@ static void test_cipher(const void *opaque) cipher = qcrypto_cipher_new( data->alg, data->mode, key, nkey, - &error_abort); - g_assert(cipher != NULL); + &err); + if (data->plaintext) { + g_assert(err == NULL); + g_assert(cipher != NULL); + } else { + g_assert(err != NULL); + error_free(err); + g_assert(cipher == NULL); + goto cleanup; + } keysize = qcrypto_cipher_get_key_len(data->alg); blocksize = qcrypto_cipher_get_block_len(data->alg); @@ -498,6 +525,7 @@ static void test_cipher(const void *opaque) g_assert_cmpstr(outtexthex, ==, data->plaintext); + cleanup: g_free(outtext); g_free(outtexthex); g_free(key);
The XTS cipher mode needs to be used with a cipher which has a block size of 16 bytes. If a mis-matching block size is used, the code will either corrupt memory beyond the IV array, or not fully encrypt/decrypt the IV. This fixes a memory curruption crash when attempting to use cast5-128 with xts, since the former has an 8 byte block size. A test case is added to ensure the cipher creation fails with such an invalid combination. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- crypto/cipher-gcrypt.c | 6 ++++++ crypto/cipher-nettle.c | 12 +++++++----- tests/test-crypto-cipher.c | 44 ++++++++++++++++++++++++++++++++++++-------- 3 files changed, 49 insertions(+), 13 deletions(-)