[Qemu-devel] [PULL v1 0/3] Merge qcrypto 2016/09/12

[Qemu-devel] [PULL v1 0/3] Merge qcrypto 2016/09/12

[Daniel P. Berrange]

The following changes since commit c2a57aae9a1c3dd7de77daf5478df10379aeeebf:

  Merge remote-tracking branch 'remotes/famz/tags/docker-pull-request' into staging (2016-09-09 12:49:41 +0100)

are available in the git repository at:

  git://github.com/berrange/qemu tags/pull-qcrypto-2016-09-12-1

for you to fetch changes up to 90d6f60d0727df084b62674bf2310ac74467a5a4:

  crypto: report enum strings instead of values in errors (2016-09-12 12:00:52 +0100)

----------------------------------------------------------------
Merge qcrypto 2016/09/12 v1

----------------------------------------------------------------
Daniel P. Berrange (2):
      crypto: ensure XTS is only used with ciphers with 16 byte blocks
      crypto: report enum strings instead of values in errors

Gonglei (1):
      crypto: fix building complaint

 crypto/block.c                  |  6 ++++--
 crypto/cipher-builtin.c         |  9 ++++++---
 crypto/cipher-gcrypt.c          | 12 ++++++++++--
 crypto/cipher-nettle.c          | 26 +++++++++++++++-----------
 crypto/init.c                   |  3 +--
 crypto/tlscredsx509.c           |  6 +++---
 tests/crypto-tls-x509-helpers.h |  1 -
 tests/test-crypto-cipher.c      | 43 +++++++++++++++++++++++++++++++++++--------
 8 files changed, 74 insertions(+), 32 deletions(-)

-- 
2.7.4

[Qemu-devel] [PULL v1 1/3] 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 corruption 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.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 crypto/cipher-gcrypt.c     |  6 ++++++
 crypto/cipher-nettle.c     | 12 +++++++-----
 tests/test-crypto-cipher.c | 43 +++++++++++++++++++++++++++++++++++--------
 3 files changed, 48 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..b89dfa2 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,15 @@ 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 {
+        error_free_or_abort(&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 +524,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

[Qemu-devel] [PULL v1 2/3] crypto: fix building complaint

[Daniel P. Berrange]

From: Gonglei <arei.gonglei@huawei.com>

gnutls commit 846753877d renamed LIBGNUTLS_VERSION_NUMBER to GNUTLS_VERSION_NUMBER.
If using gnutls before that verion, we'll get the below warning:
crypto/tlscredsx509.c:618:5: warning: "GNUTLS_VERSION_NUMBER" is not defined

Because gnutls 3.x still defines LIBGNUTLS_VERSION_NUMBER for back compat, Let's
use LIBGNUTLS_VERSION_NUMBER instead of GNUTLS_VERSION_NUMBER to fix building
complaint.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 crypto/init.c                   | 3 +--
 crypto/tlscredsx509.c           | 6 +++---
 tests/crypto-tls-x509-helpers.h | 1 -
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/crypto/init.c b/crypto/init.c
index 1e564d9..16e099b 100644
--- a/crypto/init.c
+++ b/crypto/init.c
@@ -59,8 +59,7 @@
 
 #if (defined(CONFIG_GCRYPT) &&                  \
      (!defined(CONFIG_GNUTLS) ||                \
-      !defined(GNUTLS_VERSION_NUMBER) ||       \
-      (GNUTLS_VERSION_NUMBER < 0x020c00)) &&    \
+     (LIBGNUTLS_VERSION_NUMBER < 0x020c00)) &&    \
      (!defined(GCRYPT_VERSION_NUMBER) ||        \
       (GCRYPT_VERSION_NUMBER < 0x010600)))
 #define QCRYPTO_INIT_GCRYPT_THREADS
diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index 520d34d..50eb54f 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -615,7 +615,7 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
     }
 
     if (cert != NULL && key != NULL) {
-#if GNUTLS_VERSION_NUMBER >= 0x030111
+#if LIBGNUTLS_VERSION_NUMBER >= 0x030111
         char *password = NULL;
         if (creds->passwordid) {
             password = qcrypto_secret_lookup_as_utf8(creds->passwordid,
@@ -630,7 +630,7 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
                                                     password,
                                                     0);
         g_free(password);
-#else /* GNUTLS_VERSION_NUMBER < 0x030111 */
+#else /* LIBGNUTLS_VERSION_NUMBER < 0x030111 */
         if (creds->passwordid) {
             error_setg(errp, "PKCS8 decryption requires GNUTLS >= 3.1.11");
             goto cleanup;
@@ -638,7 +638,7 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
         ret = gnutls_certificate_set_x509_key_file(creds->data,
                                                    cert, key,
                                                    GNUTLS_X509_FMT_PEM);
-#endif /* GNUTLS_VERSION_NUMBER < 0x030111 */
+#endif
         if (ret < 0) {
             error_setg(errp, "Cannot load certificate '%s' & key '%s': %s",
                        cert, key, gnutls_strerror(ret));
diff --git a/tests/crypto-tls-x509-helpers.h b/tests/crypto-tls-x509-helpers.h
index 356b49c..a8faa92 100644
--- a/tests/crypto-tls-x509-helpers.h
+++ b/tests/crypto-tls-x509-helpers.h
@@ -26,7 +26,6 @@
 
 #if !(defined WIN32) && \
     defined(CONFIG_TASN1) && \
-    defined(LIBGNUTLS_VERSION_NUMBER) && \
     (LIBGNUTLS_VERSION_NUMBER >= 0x020600)
 # define QCRYPTO_HAVE_TLS_TEST_SUPPORT
 #endif
-- 
2.7.4

[Qemu-devel] [PULL v1 3/3] crypto: report enum strings instead of values in errors

[Daniel P. Berrange]

Several error messages print out the raw enum value, which
is less than helpful to users, as these values are not
documented, nor stable across QEMU releases. Switch to use
the enum string instead.

The nettle impl also had two typos where it mistakenly
said "algorithm" instead of "mode", and actually reported
the algorithm value too.

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 crypto/block.c          |  6 ++++--
 crypto/cipher-builtin.c |  9 ++++++---
 crypto/cipher-gcrypt.c  |  6 ++++--
 crypto/cipher-nettle.c  | 14 ++++++++------
 4 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/crypto/block.c b/crypto/block.c
index be823ee..64c8420 100644
--- a/crypto/block.c
+++ b/crypto/block.c
@@ -59,7 +59,8 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options,
 
     if (options->format >= G_N_ELEMENTS(qcrypto_block_drivers) ||
         !qcrypto_block_drivers[options->format]) {
-        error_setg(errp, "Unsupported block driver %d", options->format);
+        error_setg(errp, "Unsupported block driver %s",
+                   QCryptoBlockFormat_lookup[options->format]);
         g_free(block);
         return NULL;
     }
@@ -88,7 +89,8 @@ QCryptoBlock *qcrypto_block_create(QCryptoBlockCreateOptions *options,
 
     if (options->format >= G_N_ELEMENTS(qcrypto_block_drivers) ||
         !qcrypto_block_drivers[options->format]) {
-        error_setg(errp, "Unsupported block driver %d", options->format);
+        error_setg(errp, "Unsupported block driver %s",
+                   QCryptoBlockFormat_lookup[options->format]);
         g_free(block);
         return NULL;
     }
diff --git a/crypto/cipher-builtin.c b/crypto/cipher-builtin.c
index 88963f6..9d25842 100644
--- a/crypto/cipher-builtin.c
+++ b/crypto/cipher-builtin.c
@@ -244,7 +244,8 @@ static int qcrypto_cipher_init_aes(QCryptoCipher *cipher,
     if (cipher->mode != QCRYPTO_CIPHER_MODE_CBC &&
         cipher->mode != QCRYPTO_CIPHER_MODE_ECB &&
         cipher->mode != QCRYPTO_CIPHER_MODE_XTS) {
-        error_setg(errp, "Unsupported cipher mode %d", cipher->mode);
+        error_setg(errp, "Unsupported cipher mode %s",
+                   QCryptoCipherMode_lookup[cipher->mode]);
         return -1;
     }
 
@@ -376,7 +377,8 @@ static int qcrypto_cipher_init_des_rfb(QCryptoCipher *cipher,
     QCryptoCipherBuiltin *ctxt;
 
     if (cipher->mode != QCRYPTO_CIPHER_MODE_ECB) {
-        error_setg(errp, "Unsupported cipher mode %d", cipher->mode);
+        error_setg(errp, "Unsupported cipher mode %s",
+                   QCryptoCipherMode_lookup[cipher->mode]);
         return -1;
     }
 
@@ -442,7 +444,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
         break;
     default:
         error_setg(errp,
-                   "Unsupported cipher algorithm %d", cipher->alg);
+                   "Unsupported cipher algorithm %s",
+                   QCryptoCipherAlgorithm_lookup[cipher->alg]);
         goto error;
     }
 
diff --git a/crypto/cipher-gcrypt.c b/crypto/cipher-gcrypt.c
index 3652aa1..da3f4c7 100644
--- a/crypto/cipher-gcrypt.c
+++ b/crypto/cipher-gcrypt.c
@@ -70,7 +70,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
         gcrymode = GCRY_CIPHER_MODE_CBC;
         break;
     default:
-        error_setg(errp, "Unsupported cipher mode %d", mode);
+        error_setg(errp, "Unsupported cipher mode %s",
+                   QCryptoCipherMode_lookup[mode]);
         return NULL;
     }
 
@@ -120,7 +121,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
         break;
 
     default:
-        error_setg(errp, "Unsupported cipher algorithm %d", alg);
+        error_setg(errp, "Unsupported cipher algorithm %s",
+                   QCryptoCipherAlgorithm_lookup[alg]);
         return NULL;
     }
 
diff --git a/crypto/cipher-nettle.c b/crypto/cipher-nettle.c
index 0267da5..879d831 100644
--- a/crypto/cipher-nettle.c
+++ b/crypto/cipher-nettle.c
@@ -227,7 +227,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
     case QCRYPTO_CIPHER_MODE_XTS:
         break;
     default:
-        error_setg(errp, "Unsupported cipher mode %d", mode);
+        error_setg(errp, "Unsupported cipher mode %s",
+                   QCryptoCipherMode_lookup[mode]);
         return NULL;
     }
 
@@ -357,7 +358,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
         break;
 
     default:
-        error_setg(errp, "Unsupported cipher algorithm %d", alg);
+        error_setg(errp, "Unsupported cipher algorithm %s",
+                   QCryptoCipherAlgorithm_lookup[alg]);
         goto error;
     }
 
@@ -429,8 +431,8 @@ int qcrypto_cipher_encrypt(QCryptoCipher *cipher,
         break;
 
     default:
-        error_setg(errp, "Unsupported cipher algorithm %d",
-                   cipher->alg);
+        error_setg(errp, "Unsupported cipher mode %s",
+                   QCryptoCipherMode_lookup[cipher->mode]);
         return -1;
     }
     return 0;
@@ -469,8 +471,8 @@ int qcrypto_cipher_decrypt(QCryptoCipher *cipher,
         break;
 
     default:
-        error_setg(errp, "Unsupported cipher algorithm %d",
-                   cipher->alg);
+        error_setg(errp, "Unsupported cipher mode %s",
+                   QCryptoCipherMode_lookup[cipher->mode]);
         return -1;
     }
     return 0;
-- 
2.7.4

Re: [Qemu-devel] [PULL v1 0/3] Merge qcrypto 2016/09/12

[Peter Maydell]

On 12 September 2016 at 12:07, Daniel P. Berrange <berrange@redhat.com> wrote:
> The following changes since commit c2a57aae9a1c3dd7de77daf5478df10379aeeebf:
>
>   Merge remote-tracking branch 'remotes/famz/tags/docker-pull-request' into staging (2016-09-09 12:49:41 +0100)
>
> are available in the git repository at:
>
>   git://github.com/berrange/qemu tags/pull-qcrypto-2016-09-12-1
>
> for you to fetch changes up to 90d6f60d0727df084b62674bf2310ac74467a5a4:
>
>   crypto: report enum strings instead of values in errors (2016-09-12 12:00:52 +0100)
>
> ----------------------------------------------------------------
> Merge qcrypto 2016/09/12 v1
>

Applied, thanks.

-- PMM