[Qemu-devel] [PATCH] crypto: ensure XTS is only used with ciphers with 16 byte blocks

[Qemu-devel] [PATCH] crypto: ensure XTS is only used with ciphers with 16 byte blocks

[Daniel P. Berrange]

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(-)

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);
-- 
2.7.4

Re: [Qemu-devel] [PATCH] crypto: ensure XTS is only used with ciphers with 16 byte blocks

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1728 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] crypto: ensure XTS is only used with ciphers with 16 byte blocks

[Daniel P. Berrange]

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
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|