From a5d2f44d0d3e7523670e103a8c37faed29ff2b76 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Wed, 24 Aug 2016 16:28:15 +0100 Subject: [PATCH 1/3] crypto: ensure XTS is only used with ciphers with 16 byte blocks 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 Signed-off-by: Daniel P. Berrange --- 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 ede2f70df8..3652aa1e1b 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 70909fb7fe..0267da5ba6 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 1b5130d5f6..b89dfa2b65 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); From d9269b274a9a08c8cbae14deb514d4e30b4095cf Mon Sep 17 00:00:00 2001 From: Gonglei Date: Mon, 5 Sep 2016 20:36:19 +0800 Subject: [PATCH 2/3] crypto: fix building complaint 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 Signed-off-by: Daniel P. Berrange --- 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 1e564d9492..16e099b489 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 520d34d77e..50eb54f6bb 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 356b49cd5a..a8faa92bc0 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 From 90d6f60d0727df084b62674bf2310ac74467a5a4 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Mon, 5 Sep 2016 18:02:05 +0100 Subject: [PATCH 3/3] crypto: report enum strings instead of values in errors 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 Signed-off-by: Daniel P. Berrange --- 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 be823eebeb..64c8420425 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 88963f65c8..9d258428b0 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 3652aa1e1b..da3f4c74db 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 0267da5ba6..879d831694 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;