From bf98afc75efedf10965a3f2d98aa43c234cf69ed Mon Sep 17 00:00:00 2001 From: Jungmin Park Date: Wed, 24 Aug 2022 21:37:06 +0900 Subject: [PATCH 01/20] crypto/luks: Support creating LUKS image on Darwin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the user creates a LUKS-encrypted qcow2 image using the qemu-img program, the passphrase is hashed using PBKDF2 with a dynamic number of iterations. The number of iterations is determined by measuring thread cpu time usage, such that it takes approximately 2 seconds to compute the hash. Because Darwin doesn't implement getrusage(RUSAGE_THREAD), we get an error message: > qemu-img: test.qcow2: Unable to calculate thread CPU usage on this platform for this command: > qemu-img create --object secret,id=key,data=1234 -f qcow2 -o 'encrypt.format=luks,encrypt.key-secret=key' test.qcow2 100M This patch implements qcrypto_pbkdf2_get_thread_cpu() for Darwin so that the above command works. Signed-off-by: Jungmin Park Signed-off-by: Daniel P. Berrangé --- crypto/pbkdf.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/crypto/pbkdf.c b/crypto/pbkdf.c index 3775ddc6c5..8d198c152c 100644 --- a/crypto/pbkdf.c +++ b/crypto/pbkdf.c @@ -24,6 +24,11 @@ #ifndef _WIN32 #include #endif +#ifdef CONFIG_DARWIN +#include +#include +#include +#endif static int qcrypto_pbkdf2_get_thread_cpu(unsigned long long *val_ms, @@ -45,6 +50,24 @@ static int qcrypto_pbkdf2_get_thread_cpu(unsigned long long *val_ms, /* QuadPart is units of 100ns and we want ms as unit */ *val_ms = thread_time.QuadPart / 10000ll; return 0; +#elif defined(CONFIG_DARWIN) + mach_port_t thread; + kern_return_t kr; + mach_msg_type_number_t count; + thread_basic_info_data_t info; + + thread = mach_thread_self(); + count = THREAD_BASIC_INFO_COUNT; + kr = thread_info(thread, THREAD_BASIC_INFO, (thread_info_t)&info, &count); + mach_port_deallocate(mach_task_self(), thread); + if (kr != KERN_SUCCESS || (info.flags & TH_FLAGS_IDLE) != 0) { + error_setg_errno(errp, errno, "Unable to get thread CPU usage"); + return -1; + } + + *val_ms = ((info.user_time.seconds * 1000ll) + + (info.user_time.microseconds / 1000)); + return 0; #elif defined(RUSAGE_THREAD) struct rusage ru; if (getrusage(RUSAGE_THREAD, &ru) < 0) { From 926a895c2cfcd621373eb86288dd83d08cb53f15 Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Tue, 27 Sep 2022 19:05:42 +0800 Subject: [PATCH 02/20] util/qemu-sockets: Use g_get_tmp_dir() to get the directory for temporary files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the existing logic to get the directory for temporary files with g_get_tmp_dir(), which works for win32 too. Signed-off-by: Bin Meng Reviewed-by: Marc-André Lureau Signed-off-by: Daniel P. Berrangé --- util/qemu-sockets.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index 83f4bd6fd2..0c41ca9e42 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -919,9 +919,8 @@ static int unix_listen_saddr(UnixSocketAddress *saddr, if (saddr->path[0] || abstract) { path = saddr->path; } else { - const char *tmpdir = getenv("TMPDIR"); - tmpdir = tmpdir ? tmpdir : "/tmp"; - path = pathbuf = g_strdup_printf("%s/qemu-socket-XXXXXX", tmpdir); + path = pathbuf = g_strdup_printf("%s/qemu-socket-XXXXXX", + g_get_tmp_dir()); } pathlen = strlen(path); From 985be62d4481e11830485a25a47a932e5829d223 Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Thu, 6 Oct 2022 23:19:23 +0800 Subject: [PATCH 03/20] io/channel-watch: Drop a superfluous '#ifdef WIN32' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the win32 version qio_channel_create_socket_watch() body there is no need to do a '#ifdef WIN32'. Signed-off-by: Bin Meng Reviewed-by: Marc-André Lureau Signed-off-by: Daniel P. Berrangé --- io/channel-watch.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/io/channel-watch.c b/io/channel-watch.c index 0289b3647c..89f3c8a88a 100644 --- a/io/channel-watch.c +++ b/io/channel-watch.c @@ -285,11 +285,9 @@ GSource *qio_channel_create_socket_watch(QIOChannel *ioc, GSource *source; QIOChannelSocketSource *ssource; -#ifdef WIN32 WSAEventSelect(socket, ioc->event, FD_READ | FD_ACCEPT | FD_CLOSE | FD_CONNECT | FD_WRITE | FD_OOB); -#endif source = g_source_new(&qio_channel_socket_source_funcs, sizeof(QIOChannelSocketSource)); From 6c822a031b9e87fea8303373d6501f6d6a3c1e31 Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Thu, 6 Oct 2022 23:19:24 +0800 Subject: [PATCH 04/20] io/channel-watch: Drop the unnecessary cast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is no need to do a type cast on ssource->socket as it is already declared as a SOCKET. Suggested-by: Marc-André Lureau Signed-off-by: Bin Meng Reviewed-by: Marc-André Lureau Signed-off-by: Daniel P. Berrangé --- io/channel-watch.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/io/channel-watch.c b/io/channel-watch.c index 89f3c8a88a..43d38494f7 100644 --- a/io/channel-watch.c +++ b/io/channel-watch.c @@ -130,13 +130,13 @@ qio_channel_socket_source_check(GSource *source) FD_ZERO(&wfds); FD_ZERO(&xfds); if (ssource->condition & G_IO_IN) { - FD_SET((SOCKET)ssource->socket, &rfds); + FD_SET(ssource->socket, &rfds); } if (ssource->condition & G_IO_OUT) { - FD_SET((SOCKET)ssource->socket, &wfds); + FD_SET(ssource->socket, &wfds); } if (ssource->condition & G_IO_PRI) { - FD_SET((SOCKET)ssource->socket, &xfds); + FD_SET(ssource->socket, &xfds); } ssource->revents = 0; if (select(0, &rfds, &wfds, &xfds, &tv0) == 0) { From 23f77f05f2faa8ff3028fef388322f7b4f09533e Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Thu, 6 Oct 2022 23:19:25 +0800 Subject: [PATCH 05/20] io/channel-watch: Fix socket watch on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Random failure was observed when running qtests on Windows due to "Broken pipe" detected by qmp_fd_receive(). What happened is that the qtest executable sends testing data over a socket to the QEMU under test but no response is received. The errno of the recv() call from the qtest executable indicates ETIMEOUT, due to the qmp chardev's tcp_chr_read() is never called to receive testing data hence no response is sent to the other side. tcp_chr_read() is registered as the callback of the socket watch GSource. The reason of the callback not being called by glib, is that the source check fails to indicate the source is ready. There are two socket watch sources created to monitor the same socket event object from the char-socket backend in update_ioc_handlers(). During the source check phase, qio_channel_socket_source_check() calls WSAEnumNetworkEvents() to discover occurrences of network events for the indicated socket, clear internal network event records, and reset the event object. Testing shows that if we don't reset the event object by not passing the event handle to WSAEnumNetworkEvents() the symptom goes away and qtest runs very stably. It seems we don't need to call WSAEnumNetworkEvents() at all, as we don't parse the result of WSANETWORKEVENTS returned from this API. We use select() to poll the socket status. Fix this instability by dropping the WSAEnumNetworkEvents() call. Some side notes: During the testing, I removed the following codes in update_ioc_handlers(): remove_hup_source(s); s->hup_source = qio_channel_create_watch(s->ioc, G_IO_HUP); g_source_set_callback(s->hup_source, (GSourceFunc)tcp_chr_hup, chr, NULL); g_source_attach(s->hup_source, chr->gcontext); and such change also makes the symptom go away. And if I moved the above codes to the beginning, before the call to io_add_watch_poll(), the symptom also goes away. It seems two sources watching on the same socket event object is the key that leads to the instability. The order of adding a source watch seems to also play a role but I can't explain why. Hopefully a Windows and glib expert could explain this behavior. Signed-off-by: Bin Meng Signed-off-by: Daniel P. Berrangé --- io/channel-watch.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/io/channel-watch.c b/io/channel-watch.c index 43d38494f7..ad7c568a84 100644 --- a/io/channel-watch.c +++ b/io/channel-watch.c @@ -115,17 +115,13 @@ static gboolean qio_channel_socket_source_check(GSource *source) { static struct timeval tv0; - QIOChannelSocketSource *ssource = (QIOChannelSocketSource *)source; - WSANETWORKEVENTS ev; fd_set rfds, wfds, xfds; if (!ssource->condition) { return 0; } - WSAEnumNetworkEvents(ssource->socket, ssource->ioc->event, &ev); - FD_ZERO(&rfds); FD_ZERO(&wfds); FD_ZERO(&xfds); From 73422d9524376526ab5950b3be3098901cd605d6 Mon Sep 17 00:00:00 2001 From: Michal Privoznik Date: Wed, 26 Oct 2022 09:30:24 +0200 Subject: [PATCH 06/20] seccomp: Get actual errno value from failed seccomp functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Upon failure, a libseccomp API returns actual errno value very rarely. Fortunately, after its commit 34bf78ab (contained in 2.5.0 release), the SCMP_FLTATR_API_SYSRAWRC attribute can be set which makes subsequent APIs return true errno on failure. This is especially critical when seccomp_load() fails, because generic -ECANCELED says nothing. Signed-off-by: Michal Privoznik Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Daniel P. Berrangé --- meson.build | 9 +++++++++ softmmu/qemu-seccomp.c | 13 +++++++++++++ 2 files changed, 22 insertions(+) diff --git a/meson.build b/meson.build index b686dfef75..5f114c89d9 100644 --- a/meson.build +++ b/meson.build @@ -636,10 +636,16 @@ if vmnet.found() and not cc.has_header_symbol('vmnet/vmnet.h', endif seccomp = not_found +seccomp_has_sysrawrc = false if not get_option('seccomp').auto() or have_system or have_tools seccomp = dependency('libseccomp', version: '>=2.3.0', required: get_option('seccomp'), method: 'pkg-config', kwargs: static_kwargs) + if seccomp.found() + seccomp_has_sysrawrc = cc.has_header_symbol('seccomp.h', + 'SCMP_FLTATR_API_SYSRAWRC', + dependencies: seccomp) + endif endif libcap_ng = not_found @@ -1849,6 +1855,9 @@ config_host_data.set('CONFIG_RDMA', rdma.found()) config_host_data.set('CONFIG_SDL', sdl.found()) config_host_data.set('CONFIG_SDL_IMAGE', sdl_image.found()) config_host_data.set('CONFIG_SECCOMP', seccomp.found()) +if seccomp.found() + config_host_data.set('CONFIG_SECCOMP_SYSRAWRC', seccomp_has_sysrawrc) +endif config_host_data.set('CONFIG_SNAPPY', snappy.found()) config_host_data.set('CONFIG_TPM', have_tpm) config_host_data.set('CONFIG_USB_LIBUSB', libusb.found()) diff --git a/softmmu/qemu-seccomp.c b/softmmu/qemu-seccomp.c index deaf8a4ef5..d66a2a1226 100644 --- a/softmmu/qemu-seccomp.c +++ b/softmmu/qemu-seccomp.c @@ -312,6 +312,19 @@ static int seccomp_start(uint32_t seccomp_opts, Error **errp) goto seccomp_return; } +#if defined(CONFIG_SECCOMP_SYSRAWRC) + /* + * This must be the first seccomp_attr_set() call to have full + * error propagation from subsequent seccomp APIs. + */ + rc = seccomp_attr_set(ctx, SCMP_FLTATR_API_SYSRAWRC, 1); + if (rc != 0) { + error_setg_errno(errp, -rc, + "failed to set seccomp rawrc attribute"); + goto seccomp_return; + } +#endif + rc = seccomp_attr_set(ctx, SCMP_FLTATR_CTL_TSYNC, 1); if (rc != 0) { error_setg_errno(errp, -rc, From dd84a906e061550daaedea6ce88762f1839253ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Tue, 26 Apr 2022 14:06:49 +0100 Subject: [PATCH 07/20] scripts: check if .git exists before checking submodule status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently we check status of each submodule, before actually checking if we're in a git repo. These status commands will all fail, but we are hiding their output so we don't see it currently. Reviewed-by: Richard Henderson Reviewed-by: Philippe Mathieu-Daudé --- scripts/git-submodule.sh | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/scripts/git-submodule.sh b/scripts/git-submodule.sh index e225d3a963..7be41f5948 100755 --- a/scripts/git-submodule.sh +++ b/scripts/git-submodule.sh @@ -51,6 +51,12 @@ validate_error() { exit 1 } +if test -n "$maybe_modules" && ! test -e ".git" +then + echo "$0: unexpectedly called with submodules but no git checkout exists" + exit 1 +fi + modules="" for m in $maybe_modules do @@ -63,12 +69,6 @@ do fi done -if test -n "$maybe_modules" && ! test -e ".git" -then - echo "$0: unexpectedly called with submodules but no git checkout exists" - exit 1 -fi - case "$command" in status|validate) if test -z "$maybe_modules" From 3983bf1b41cefcf553a2c6316f767367d6977b51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Wed, 28 Sep 2022 15:56:42 +0100 Subject: [PATCH 08/20] crypto: check for and report errors setting PSK credentials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If setting credentials fails, the handshake will later fail to complete with an obscure error message which is hard to diagnose. Reviewed-by: Bin Meng Tested-by: Bin Meng Signed-off-by: Daniel P. Berrangé --- crypto/tlscredspsk.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/crypto/tlscredspsk.c b/crypto/tlscredspsk.c index a4f9891274..546cad1c5a 100644 --- a/crypto/tlscredspsk.c +++ b/crypto/tlscredspsk.c @@ -109,7 +109,12 @@ qcrypto_tls_creds_psk_load(QCryptoTLSCredsPSK *creds, goto cleanup; } - gnutls_psk_set_server_credentials_file(creds->data.server, pskfile); + ret = gnutls_psk_set_server_credentials_file(creds->data.server, pskfile); + if (ret < 0) { + error_setg(errp, "Cannot set PSK server credentials: %s", + gnutls_strerror(ret)); + goto cleanup; + } gnutls_psk_set_server_dh_params(creds->data.server, creds->parent_obj.dh_params); } else { @@ -135,8 +140,13 @@ qcrypto_tls_creds_psk_load(QCryptoTLSCredsPSK *creds, goto cleanup; } - gnutls_psk_set_client_credentials(creds->data.client, - username, &key, GNUTLS_PSK_KEY_HEX); + ret = gnutls_psk_set_client_credentials(creds->data.client, + username, &key, GNUTLS_PSK_KEY_HEX); + if (ret < 0) { + error_setg(errp, "Cannot set PSK client credentials: %s", + gnutls_strerror(ret)); + goto cleanup; + } } rv = 0; From f1018ea0a30f577d1e3515d0a6362e362a0cb86f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Wed, 28 Sep 2022 15:57:24 +0100 Subject: [PATCH 09/20] tests: avoid DOS line endings in PSK file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Using FILE * APIs for writing the PSK file results in translation from UNIX to DOS line endings on Windows. When the crypto PSK code later loads the credentials the stray \r will result in failure to load the PSK credentials into GNUTLS. Rather than switching the FILE* APIs to open in binary format, just switch to the more concise g_file_set_contents API. Reviewed-by: Bin Meng Tested-by: Bin Meng Signed-off-by: Daniel P. Berrangé --- tests/unit/crypto-tls-psk-helpers.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/tests/unit/crypto-tls-psk-helpers.c b/tests/unit/crypto-tls-psk-helpers.c index 511e08cc9c..c6cc740772 100644 --- a/tests/unit/crypto-tls-psk-helpers.c +++ b/tests/unit/crypto-tls-psk-helpers.c @@ -27,15 +27,14 @@ static void test_tls_psk_init_common(const char *pskfile, const char *user, const char *key) { - FILE *fp; + g_autoptr(GError) gerr = NULL; + g_autofree char *line = g_strdup_printf("%s:%s\n", user, key); - fp = fopen(pskfile, "w"); - if (fp == NULL) { - g_critical("Failed to create pskfile %s: %s", pskfile, strerror(errno)); + g_file_set_contents(pskfile, line, strlen(line), &gerr); + if (gerr != NULL) { + g_critical("Failed to create pskfile %s: %s", pskfile, gerr->message); abort(); } - fprintf(fp, "%s:%s\n", user, key); - fclose(fp); } void test_tls_psk_init(const char *pskfile) From c1d8634c207defb547a57515729233e47f65718f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Tue, 10 May 2022 14:17:43 +0100 Subject: [PATCH 10/20] crypto: sanity check that LUKS header strings are NUL-terminated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The LUKS spec requires that header strings are NUL-terminated, and our code relies on that. Protect against maliciously crafted headers by adding validation. Reviewed-by: Richard W.M. Jones Signed-off-by: Daniel P. Berrangé --- crypto/block-luks.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/crypto/block-luks.c b/crypto/block-luks.c index f62be6836b..27d1b34c1d 100644 --- a/crypto/block-luks.c +++ b/crypto/block-luks.c @@ -554,6 +554,24 @@ qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp) return -1; } + if (!memchr(luks->header.cipher_name, '\0', + sizeof(luks->header.cipher_name))) { + error_setg(errp, "LUKS header cipher name is not NUL terminated"); + return -1; + } + + if (!memchr(luks->header.cipher_mode, '\0', + sizeof(luks->header.cipher_mode))) { + error_setg(errp, "LUKS header cipher mode is not NUL terminated"); + return -1; + } + + if (!memchr(luks->header.hash_spec, '\0', + sizeof(luks->header.hash_spec))) { + error_setg(errp, "LUKS header hash spec is not NUL terminated"); + return -1; + } + /* Check all keyslots for corruption */ for (i = 0 ; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; i++) { From f1195961f36b19ce9008dabf11ee8362803bcd92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Tue, 10 May 2022 14:27:33 +0100 Subject: [PATCH 11/20] crypto: enforce that LUKS stripes is always a fixed value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Although the LUKS stripes are encoded in the keyslot header and so potentially configurable, in pratice the cryptsetup impl mandates this has the fixed value 4000. To avoid incompatibility apply the same enforcement in QEMU too. This also caps the memory usage for key material when QEMU tries to open a LUKS volume. Reviewed-by: Richard W.M. Jones Signed-off-by: Daniel P. Berrangé --- crypto/block-luks.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crypto/block-luks.c b/crypto/block-luks.c index 27d1b34c1d..81744e2a8e 100644 --- a/crypto/block-luks.c +++ b/crypto/block-luks.c @@ -582,8 +582,9 @@ qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp) header_sectors, slot1->stripes); - if (slot1->stripes == 0) { - error_setg(errp, "Keyslot %zu is corrupted (stripes == 0)", i); + if (slot1->stripes != QCRYPTO_BLOCK_LUKS_STRIPES) { + error_setg(errp, "Keyslot %zu is corrupted (stripes %d != %d)", + i, slot1->stripes, QCRYPTO_BLOCK_LUKS_STRIPES); return -1; } From 93569c373027c5c46e518e01c0c3e2d07fbb6890 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Tue, 10 May 2022 14:35:57 +0100 Subject: [PATCH 12/20] crypto: enforce that key material doesn't overlap with LUKS header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We already check that key material doesn't overlap between key slots, and that it doesn't overlap with the payload. We didn't check for overlap with the LUKS header. Reviewed-by: Richard W.M. Jones Signed-off-by: Daniel P. Berrangé --- crypto/block-luks.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/crypto/block-luks.c b/crypto/block-luks.c index 81744e2a8e..6ef9a89ffa 100644 --- a/crypto/block-luks.c +++ b/crypto/block-luks.c @@ -595,6 +595,14 @@ qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp) return -1; } + if (start1 < DIV_ROUND_UP(sizeof(QCryptoBlockLUKSHeader), + QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) { + error_setg(errp, + "Keyslot %zu is overlapping with the LUKS header", + i); + return -1; + } + if (start1 + len1 > luks->header.payload_offset_sector) { error_setg(errp, "Keyslot %zu is overlapping with the encrypted payload", From d233fbc327d3f1f03bc30e0486b9ade3aa23f9ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Mon, 5 Sep 2022 13:50:03 +0100 Subject: [PATCH 13/20] crypto: validate that LUKS payload doesn't overlap with header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We already validate that LUKS keyslots don't overlap with the header, or with each other. This closes the remaining hole in validation of LUKS file regions. Reviewed-by: Richard W.M. Jones Signed-off-by: Daniel P. Berrangé --- crypto/block-luks.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crypto/block-luks.c b/crypto/block-luks.c index 6ef9a89ffa..f22bc63e54 100644 --- a/crypto/block-luks.c +++ b/crypto/block-luks.c @@ -572,6 +572,13 @@ qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp) return -1; } + if (luks->header.payload_offset_sector < + DIV_ROUND_UP(QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET, + QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) { + error_setg(errp, "LUKS payload is overlapping with the header"); + return -1; + } + /* Check all keyslots for corruption */ for (i = 0 ; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; i++) { From c5f6962801b868b02fbaf01861f64783470d3d2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Mon, 5 Sep 2022 13:57:01 +0100 Subject: [PATCH 14/20] crypto: strengthen the check for key slots overlapping with LUKS header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The LUKS header data on disk is a fixed size, however, there's expected to be a gap between the end of the header and the first key slot to get alignment with the 2nd sector on 4k drives. This wasn't originally part of the LUKS spec, but was always part of the reference implementation, so it is worth validating this. Reviewed-by: Richard W.M. Jones Signed-off-by: Daniel P. Berrangé --- crypto/block-luks.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crypto/block-luks.c b/crypto/block-luks.c index f22bc63e54..e6ee8506b2 100644 --- a/crypto/block-luks.c +++ b/crypto/block-luks.c @@ -602,7 +602,7 @@ qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp) return -1; } - if (start1 < DIV_ROUND_UP(sizeof(QCryptoBlockLUKSHeader), + if (start1 < DIV_ROUND_UP(QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) { error_setg(errp, "Keyslot %zu is overlapping with the LUKS header", From b57151ac0366d3fb14318a55b0fc943134f7f80b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Mon, 5 Sep 2022 13:52:29 +0100 Subject: [PATCH 15/20] crypto: check that LUKS PBKDF2 iterations count is non-zero MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both the master key and key slot passphrases are run through the PBKDF2 algorithm. The iterations count is expected to be generally very large (many 10's or 100's of 1000s). It is hard to define a low level cutoff, but we can certainly say that iterations count should be non-zero. A zero count likely indicates an initialization mistake so reject it. Reviewed-by: Richard W.M. Jones Signed-off-by: Daniel P. Berrangé --- crypto/block-luks.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/crypto/block-luks.c b/crypto/block-luks.c index e6ee8506b2..254490c256 100644 --- a/crypto/block-luks.c +++ b/crypto/block-luks.c @@ -579,6 +579,11 @@ qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp) return -1; } + if (luks->header.master_key_iterations == 0) { + error_setg(errp, "LUKS key iteration count is zero"); + return -1; + } + /* Check all keyslots for corruption */ for (i = 0 ; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; i++) { @@ -602,6 +607,12 @@ qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp) return -1; } + if (slot1->active == QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED && + slot1->iterations == 0) { + error_setg(errp, "Keyslot %zu iteration count is zero", i); + return -1; + } + if (start1 < DIV_ROUND_UP(QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) { error_setg(errp, From 36445acebdd100237551b47b4fd77f0c5403a10f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Tue, 10 May 2022 15:19:58 +0100 Subject: [PATCH 16/20] crypto: split LUKS header definitions off into file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will allow unit testing code to use the structs. Reviewed-by: Richard W.M. Jones Signed-off-by: Daniel P. Berrangé --- crypto/block-luks-priv.h | 137 +++++++++++++++++++++++++++++++++++++++ crypto/block-luks.c | 94 +-------------------------- 2 files changed, 138 insertions(+), 93 deletions(-) create mode 100644 crypto/block-luks-priv.h diff --git a/crypto/block-luks-priv.h b/crypto/block-luks-priv.h new file mode 100644 index 0000000000..1516571dcb --- /dev/null +++ b/crypto/block-luks-priv.h @@ -0,0 +1,137 @@ +/* + * QEMU Crypto block device encryption LUKS format + * + * Copyright (c) 2015-2016 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, see . + * + */ + +#include "qemu/osdep.h" +#include "qapi/error.h" +#include "qemu/bswap.h" + +#include "block-luks.h" + +#include "crypto/hash.h" +#include "crypto/afsplit.h" +#include "crypto/pbkdf.h" +#include "crypto/secret.h" +#include "crypto/random.h" +#include "qemu/uuid.h" + +#include "qemu/coroutine.h" +#include "qemu/bitmap.h" + +/* + * Reference for the LUKS format implemented here is + * + * docs/on-disk-format.pdf + * + * in 'cryptsetup' package source code + * + * This file implements the 1.2.1 specification, dated + * Oct 16, 2011. + */ + +typedef struct QCryptoBlockLUKSHeader QCryptoBlockLUKSHeader; +typedef struct QCryptoBlockLUKSKeySlot QCryptoBlockLUKSKeySlot; + + +/* The following constants are all defined by the LUKS spec */ +#define QCRYPTO_BLOCK_LUKS_VERSION 1 + +#define QCRYPTO_BLOCK_LUKS_MAGIC_LEN 6 +#define QCRYPTO_BLOCK_LUKS_CIPHER_NAME_LEN 32 +#define QCRYPTO_BLOCK_LUKS_CIPHER_MODE_LEN 32 +#define QCRYPTO_BLOCK_LUKS_HASH_SPEC_LEN 32 +#define QCRYPTO_BLOCK_LUKS_DIGEST_LEN 20 +#define QCRYPTO_BLOCK_LUKS_SALT_LEN 32 +#define QCRYPTO_BLOCK_LUKS_UUID_LEN 40 +#define QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS 8 +#define QCRYPTO_BLOCK_LUKS_STRIPES 4000 +#define QCRYPTO_BLOCK_LUKS_MIN_SLOT_KEY_ITERS 1000 +#define QCRYPTO_BLOCK_LUKS_MIN_MASTER_KEY_ITERS 1000 +#define QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET 4096 + +#define QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED 0x0000DEAD +#define QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED 0x00AC71F3 + +#define QCRYPTO_BLOCK_LUKS_SECTOR_SIZE 512LL + +#define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS 2000 +#define QCRYPTO_BLOCK_LUKS_ERASE_ITERATIONS 40 + +static const char qcrypto_block_luks_magic[QCRYPTO_BLOCK_LUKS_MAGIC_LEN] = { + 'L', 'U', 'K', 'S', 0xBA, 0xBE +}; + +/* + * This struct is written to disk in big-endian format, + * but operated upon in native-endian format. + */ +struct QCryptoBlockLUKSKeySlot { + /* state of keyslot, enabled/disable */ + uint32_t active; + /* iterations for PBKDF2 */ + uint32_t iterations; + /* salt for PBKDF2 */ + uint8_t salt[QCRYPTO_BLOCK_LUKS_SALT_LEN]; + /* start sector of key material */ + uint32_t key_offset_sector; + /* number of anti-forensic stripes */ + uint32_t stripes; +}; + +/* + * This struct is written to disk in big-endian format, + * but operated upon in native-endian format. + */ +struct QCryptoBlockLUKSHeader { + /* 'L', 'U', 'K', 'S', '0xBA', '0xBE' */ + char magic[QCRYPTO_BLOCK_LUKS_MAGIC_LEN]; + + /* LUKS version, currently 1 */ + uint16_t version; + + /* cipher name specification (aes, etc) */ + char cipher_name[QCRYPTO_BLOCK_LUKS_CIPHER_NAME_LEN]; + + /* cipher mode specification (cbc-plain, xts-essiv:sha256, etc) */ + char cipher_mode[QCRYPTO_BLOCK_LUKS_CIPHER_MODE_LEN]; + + /* hash specification (sha256, etc) */ + char hash_spec[QCRYPTO_BLOCK_LUKS_HASH_SPEC_LEN]; + + /* start offset of the volume data (in 512 byte sectors) */ + uint32_t payload_offset_sector; + + /* Number of key bytes */ + uint32_t master_key_len; + + /* master key checksum after PBKDF2 */ + uint8_t master_key_digest[QCRYPTO_BLOCK_LUKS_DIGEST_LEN]; + + /* salt for master key PBKDF2 */ + uint8_t master_key_salt[QCRYPTO_BLOCK_LUKS_SALT_LEN]; + + /* iterations for master key PBKDF2 */ + uint32_t master_key_iterations; + + /* UUID of the partition in standard ASCII representation */ + uint8_t uuid[QCRYPTO_BLOCK_LUKS_UUID_LEN]; + + /* key slots */ + QCryptoBlockLUKSKeySlot key_slots[QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS]; +}; diff --git a/crypto/block-luks.c b/crypto/block-luks.c index 254490c256..375cce44cd 100644 --- a/crypto/block-luks.c +++ b/crypto/block-luks.c @@ -23,6 +23,7 @@ #include "qemu/bswap.h" #include "block-luks.h" +#include "block-luks-priv.h" #include "crypto/hash.h" #include "crypto/afsplit.h" @@ -46,37 +47,6 @@ */ typedef struct QCryptoBlockLUKS QCryptoBlockLUKS; -typedef struct QCryptoBlockLUKSHeader QCryptoBlockLUKSHeader; -typedef struct QCryptoBlockLUKSKeySlot QCryptoBlockLUKSKeySlot; - - -/* The following constants are all defined by the LUKS spec */ -#define QCRYPTO_BLOCK_LUKS_VERSION 1 - -#define QCRYPTO_BLOCK_LUKS_MAGIC_LEN 6 -#define QCRYPTO_BLOCK_LUKS_CIPHER_NAME_LEN 32 -#define QCRYPTO_BLOCK_LUKS_CIPHER_MODE_LEN 32 -#define QCRYPTO_BLOCK_LUKS_HASH_SPEC_LEN 32 -#define QCRYPTO_BLOCK_LUKS_DIGEST_LEN 20 -#define QCRYPTO_BLOCK_LUKS_SALT_LEN 32 -#define QCRYPTO_BLOCK_LUKS_UUID_LEN 40 -#define QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS 8 -#define QCRYPTO_BLOCK_LUKS_STRIPES 4000 -#define QCRYPTO_BLOCK_LUKS_MIN_SLOT_KEY_ITERS 1000 -#define QCRYPTO_BLOCK_LUKS_MIN_MASTER_KEY_ITERS 1000 -#define QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET 4096 - -#define QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED 0x0000DEAD -#define QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED 0x00AC71F3 - -#define QCRYPTO_BLOCK_LUKS_SECTOR_SIZE 512LL - -#define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS 2000 -#define QCRYPTO_BLOCK_LUKS_ERASE_ITERATIONS 40 - -static const char qcrypto_block_luks_magic[QCRYPTO_BLOCK_LUKS_MAGIC_LEN] = { - 'L', 'U', 'K', 'S', 0xBA, 0xBE -}; typedef struct QCryptoBlockLUKSNameMap QCryptoBlockLUKSNameMap; struct QCryptoBlockLUKSNameMap { @@ -134,69 +104,7 @@ qcrypto_block_luks_cipher_name_map[] = { { "twofish", qcrypto_block_luks_cipher_size_map_twofish }, }; - -/* - * This struct is written to disk in big-endian format, - * but operated upon in native-endian format. - */ -struct QCryptoBlockLUKSKeySlot { - /* state of keyslot, enabled/disable */ - uint32_t active; - /* iterations for PBKDF2 */ - uint32_t iterations; - /* salt for PBKDF2 */ - uint8_t salt[QCRYPTO_BLOCK_LUKS_SALT_LEN]; - /* start sector of key material */ - uint32_t key_offset_sector; - /* number of anti-forensic stripes */ - uint32_t stripes; -}; - QEMU_BUILD_BUG_ON(sizeof(struct QCryptoBlockLUKSKeySlot) != 48); - - -/* - * This struct is written to disk in big-endian format, - * but operated upon in native-endian format. - */ -struct QCryptoBlockLUKSHeader { - /* 'L', 'U', 'K', 'S', '0xBA', '0xBE' */ - char magic[QCRYPTO_BLOCK_LUKS_MAGIC_LEN]; - - /* LUKS version, currently 1 */ - uint16_t version; - - /* cipher name specification (aes, etc) */ - char cipher_name[QCRYPTO_BLOCK_LUKS_CIPHER_NAME_LEN]; - - /* cipher mode specification (cbc-plain, xts-essiv:sha256, etc) */ - char cipher_mode[QCRYPTO_BLOCK_LUKS_CIPHER_MODE_LEN]; - - /* hash specification (sha256, etc) */ - char hash_spec[QCRYPTO_BLOCK_LUKS_HASH_SPEC_LEN]; - - /* start offset of the volume data (in 512 byte sectors) */ - uint32_t payload_offset_sector; - - /* Number of key bytes */ - uint32_t master_key_len; - - /* master key checksum after PBKDF2 */ - uint8_t master_key_digest[QCRYPTO_BLOCK_LUKS_DIGEST_LEN]; - - /* salt for master key PBKDF2 */ - uint8_t master_key_salt[QCRYPTO_BLOCK_LUKS_SALT_LEN]; - - /* iterations for master key PBKDF2 */ - uint32_t master_key_iterations; - - /* UUID of the partition in standard ASCII representation */ - uint8_t uuid[QCRYPTO_BLOCK_LUKS_UUID_LEN]; - - /* key slots */ - QCryptoBlockLUKSKeySlot key_slots[QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS]; -}; - QEMU_BUILD_BUG_ON(sizeof(struct QCryptoBlockLUKSHeader) != 592); From 98c72dfb714385b03da03abb45f931a14af6138e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Tue, 10 May 2022 15:40:55 +0100 Subject: [PATCH 17/20] crypto: split off helpers for converting LUKS header endianess MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The unit test suite is shortly going to want to convert header endianness separately from the main I/O functions. Reviewed-by: Richard W.M. Jones Signed-off-by: Daniel P. Berrangé --- crypto/block-luks-priv.h | 6 +++ crypto/block-luks.c | 79 ++++++++++++++++++++++++---------------- 2 files changed, 53 insertions(+), 32 deletions(-) diff --git a/crypto/block-luks-priv.h b/crypto/block-luks-priv.h index 1516571dcb..90a20d432b 100644 --- a/crypto/block-luks-priv.h +++ b/crypto/block-luks-priv.h @@ -135,3 +135,9 @@ struct QCryptoBlockLUKSHeader { /* key slots */ QCryptoBlockLUKSKeySlot key_slots[QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS]; }; + + +void +qcrypto_block_luks_to_disk_endian(QCryptoBlockLUKSHeader *hdr); +void +qcrypto_block_luks_from_disk_endian(QCryptoBlockLUKSHeader *hdr); diff --git a/crypto/block-luks.c b/crypto/block-luks.c index 375cce44cd..bb89c10225 100644 --- a/crypto/block-luks.c +++ b/crypto/block-luks.c @@ -348,6 +348,51 @@ qcrypto_block_luks_splitkeylen_sectors(const QCryptoBlockLUKS *luks, return ROUND_UP(splitkeylen_sectors, header_sectors); } + +void +qcrypto_block_luks_to_disk_endian(QCryptoBlockLUKSHeader *hdr) +{ + size_t i; + + /* + * Everything on disk uses Big Endian (tm), so flip header fields + * before writing them + */ + cpu_to_be16s(&hdr->version); + cpu_to_be32s(&hdr->payload_offset_sector); + cpu_to_be32s(&hdr->master_key_len); + cpu_to_be32s(&hdr->master_key_iterations); + + for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { + cpu_to_be32s(&hdr->key_slots[i].active); + cpu_to_be32s(&hdr->key_slots[i].iterations); + cpu_to_be32s(&hdr->key_slots[i].key_offset_sector); + cpu_to_be32s(&hdr->key_slots[i].stripes); + } +} + +void +qcrypto_block_luks_from_disk_endian(QCryptoBlockLUKSHeader *hdr) +{ + size_t i; + + /* + * The header is always stored in big-endian format, so + * convert everything to native + */ + be16_to_cpus(&hdr->version); + be32_to_cpus(&hdr->payload_offset_sector); + be32_to_cpus(&hdr->master_key_len); + be32_to_cpus(&hdr->master_key_iterations); + + for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { + be32_to_cpus(&hdr->key_slots[i].active); + be32_to_cpus(&hdr->key_slots[i].iterations); + be32_to_cpus(&hdr->key_slots[i].key_offset_sector); + be32_to_cpus(&hdr->key_slots[i].stripes); + } +} + /* * Stores the main LUKS header, taking care of endianess */ @@ -359,28 +404,13 @@ qcrypto_block_luks_store_header(QCryptoBlock *block, { const QCryptoBlockLUKS *luks = block->opaque; Error *local_err = NULL; - size_t i; g_autofree QCryptoBlockLUKSHeader *hdr_copy = NULL; /* Create a copy of the header */ hdr_copy = g_new0(QCryptoBlockLUKSHeader, 1); memcpy(hdr_copy, &luks->header, sizeof(QCryptoBlockLUKSHeader)); - /* - * Everything on disk uses Big Endian (tm), so flip header fields - * before writing them - */ - cpu_to_be16s(&hdr_copy->version); - cpu_to_be32s(&hdr_copy->payload_offset_sector); - cpu_to_be32s(&hdr_copy->master_key_len); - cpu_to_be32s(&hdr_copy->master_key_iterations); - - for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { - cpu_to_be32s(&hdr_copy->key_slots[i].active); - cpu_to_be32s(&hdr_copy->key_slots[i].iterations); - cpu_to_be32s(&hdr_copy->key_slots[i].key_offset_sector); - cpu_to_be32s(&hdr_copy->key_slots[i].stripes); - } + qcrypto_block_luks_to_disk_endian(hdr_copy); /* Write out the partition header and key slot headers */ writefunc(block, 0, (const uint8_t *)hdr_copy, sizeof(*hdr_copy), @@ -404,7 +434,6 @@ qcrypto_block_luks_load_header(QCryptoBlock *block, Error **errp) { int rv; - size_t i; QCryptoBlockLUKS *luks = block->opaque; /* @@ -420,21 +449,7 @@ qcrypto_block_luks_load_header(QCryptoBlock *block, return rv; } - /* - * The header is always stored in big-endian format, so - * convert everything to native - */ - be16_to_cpus(&luks->header.version); - be32_to_cpus(&luks->header.payload_offset_sector); - be32_to_cpus(&luks->header.master_key_len); - be32_to_cpus(&luks->header.master_key_iterations); - - for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { - be32_to_cpus(&luks->header.key_slots[i].active); - be32_to_cpus(&luks->header.key_slots[i].iterations); - be32_to_cpus(&luks->header.key_slots[i].key_offset_sector); - be32_to_cpus(&luks->header.key_slots[i].stripes); - } + qcrypto_block_luks_from_disk_endian(&luks->header); return 0; } From 6c1989321eb95d5d2e29d3537484836bf0f21744 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Mon, 5 Sep 2022 12:08:21 +0100 Subject: [PATCH 18/20] crypto: quote algorithm names in error messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If given a malformed LUKS header, it is possible that the algorithm names end up being an empty string. This leads to confusing error messages unless quoting is used to highlight where the empty string is subsituted in the error message. Reviewed-by: Richard W.M. Jones Signed-off-by: Daniel P. Berrangé --- crypto/block-luks.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crypto/block-luks.c b/crypto/block-luks.c index bb89c10225..df2b4105d6 100644 --- a/crypto/block-luks.c +++ b/crypto/block-luks.c @@ -162,7 +162,7 @@ static int qcrypto_block_luks_cipher_name_lookup(const char *name, } } - error_setg(errp, "Algorithm %s with key size %d bytes not supported", + error_setg(errp, "Algorithm '%s' with key size %d bytes not supported", name, key_bytes); return 0; } @@ -198,7 +198,7 @@ static int qcrypto_block_luks_name_lookup(const char *name, int ret = qapi_enum_parse(map, name, -1, NULL); if (ret < 0) { - error_setg(errp, "%s %s not supported", type, name); + error_setg(errp, "%s '%s' not supported", type, name); return 0; } return ret; @@ -592,7 +592,7 @@ qcrypto_block_luks_parse_header(QCryptoBlockLUKS *luks, Error **errp) */ ivgen_name = strchr(cipher_mode, '-'); if (!ivgen_name) { - error_setg(errp, "Unexpected cipher mode string format %s", + error_setg(errp, "Unexpected cipher mode string format '%s'", luks->header.cipher_mode); return -1; } From 741c314a337ff4c79b8a86fbd5891054b0191d44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Mon, 5 Sep 2022 14:37:05 +0100 Subject: [PATCH 19/20] crypto: ensure LUKS tests run with GNUTLS crypto provider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GNUTLS is supported as a crypto provider since commit cc4c7c738297958b3d1d16269f57d71d22f5a9ff Author: Daniel P. Berrangé Date: Wed Jun 30 17:20:02 2021 +0100 crypto: introduce build system for gnutls crypto backend So enable the LUKS tests in this config. Reviewed-by: Richard W.M. Jones Signed-off-by: Daniel P. Berrangé --- tests/unit/test-crypto-block.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/unit/test-crypto-block.c b/tests/unit/test-crypto-block.c index 3417b67be5..3d50eb4b6e 100644 --- a/tests/unit/test-crypto-block.c +++ b/tests/unit/test-crypto-block.c @@ -30,7 +30,8 @@ #endif #if (defined(_WIN32) || defined RUSAGE_THREAD) && \ - (defined(CONFIG_NETTLE) || defined(CONFIG_GCRYPT)) + (defined(CONFIG_NETTLE) || defined(CONFIG_GCRYPT) || \ + defined(CONFIG_GNUTLS_CRYPTO)) #define TEST_LUKS #else #undef TEST_LUKS From da0ab2c4c4d22dece12acd9ddaed901a10a5edee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Mon, 5 Sep 2022 14:57:17 +0100 Subject: [PATCH 20/20] crypto: add test cases for many malformed LUKS header scenarios MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validate that we diagnose each malformed LUKS header scenario with a distinct error report. Reviewed-by: Richard W.M. Jones Signed-off-by: Daniel P. Berrangé --- tests/unit/test-crypto-block.c | 299 +++++++++++++++++++++++++++++++++ 1 file changed, 299 insertions(+) diff --git a/tests/unit/test-crypto-block.c b/tests/unit/test-crypto-block.c index 3d50eb4b6e..b629e240a9 100644 --- a/tests/unit/test-crypto-block.c +++ b/tests/unit/test-crypto-block.c @@ -22,6 +22,7 @@ #include "qapi/error.h" #include "crypto/init.h" #include "crypto/block.h" +#include "crypto/block-luks-priv.h" #include "qemu/buffer.h" #include "qemu/module.h" #include "crypto/secret.h" @@ -345,6 +346,230 @@ static void test_block(gconstpointer opaque) } +#ifdef TEST_LUKS +typedef const char *(*LuksHeaderDoBadStuff)(QCryptoBlockLUKSHeader *hdr); + +static void +test_luks_bad_header(gconstpointer data) +{ + LuksHeaderDoBadStuff badstuff = data; + QCryptoBlock *blk; + Buffer buf; + Object *sec = test_block_secret(); + QCryptoBlockLUKSHeader hdr; + Error *err = NULL; + const char *msg; + + memset(&buf, 0, sizeof(buf)); + buffer_init(&buf, "header"); + + /* Correctly create the volume initially */ + blk = qcrypto_block_create(&luks_create_opts_default, NULL, + test_block_init_func, + test_block_write_func, + &buf, + &error_abort); + g_assert(blk); + + qcrypto_block_free(blk); + + /* Mangle it in some unpleasant way */ + g_assert(buf.offset >= sizeof(hdr)); + memcpy(&hdr, buf.buffer, sizeof(hdr)); + qcrypto_block_luks_to_disk_endian(&hdr); + + msg = badstuff(&hdr); + + qcrypto_block_luks_from_disk_endian(&hdr); + memcpy(buf.buffer, &hdr, sizeof(hdr)); + + /* Check that we fail to open it again */ + blk = qcrypto_block_open(&luks_open_opts, NULL, + test_block_read_func, + &buf, + 0, + 1, + &err); + g_assert(!blk); + g_assert(err); + + g_assert_cmpstr(error_get_pretty(err), ==, msg); + error_free(err); + + object_unparent(sec); + + buffer_free(&buf); +} + +static const char *luks_bad_null_term_cipher_name(QCryptoBlockLUKSHeader *hdr) +{ + /* Replace NUL termination with spaces */ + char *offset = hdr->cipher_name + strlen(hdr->cipher_name); + memset(offset, ' ', sizeof(hdr->cipher_name) - (offset - hdr->cipher_name)); + + return "LUKS header cipher name is not NUL terminated"; +} + +static const char *luks_bad_null_term_cipher_mode(QCryptoBlockLUKSHeader *hdr) +{ + /* Replace NUL termination with spaces */ + char *offset = hdr->cipher_mode + strlen(hdr->cipher_mode); + memset(offset, ' ', sizeof(hdr->cipher_mode) - (offset - hdr->cipher_mode)); + + return "LUKS header cipher mode is not NUL terminated"; +} + +static const char *luks_bad_null_term_hash_spec(QCryptoBlockLUKSHeader *hdr) +{ + /* Replace NUL termination with spaces */ + char *offset = hdr->hash_spec + strlen(hdr->hash_spec); + memset(offset, ' ', sizeof(hdr->hash_spec) - (offset - hdr->hash_spec)); + + return "LUKS header hash spec is not NUL terminated"; +} + +static const char *luks_bad_cipher_name_empty(QCryptoBlockLUKSHeader *hdr) +{ + memcpy(hdr->cipher_name, "", 1); + + return "Algorithm '' with key size 32 bytes not supported"; +} + +static const char *luks_bad_cipher_name_unknown(QCryptoBlockLUKSHeader *hdr) +{ + memcpy(hdr->cipher_name, "aess", 5); + + return "Algorithm 'aess' with key size 32 bytes not supported"; +} + +static const char *luks_bad_cipher_xts_size(QCryptoBlockLUKSHeader *hdr) +{ + hdr->master_key_len = 33; + + return "XTS cipher key length should be a multiple of 2"; +} + +static const char *luks_bad_cipher_cbc_size(QCryptoBlockLUKSHeader *hdr) +{ + hdr->master_key_len = 33; + memcpy(hdr->cipher_mode, "cbc-essiv", 10); + + return "Algorithm 'aes' with key size 33 bytes not supported"; +} + +static const char *luks_bad_cipher_mode_empty(QCryptoBlockLUKSHeader *hdr) +{ + memcpy(hdr->cipher_mode, "", 1); + + return "Unexpected cipher mode string format ''"; +} + +static const char *luks_bad_cipher_mode_unknown(QCryptoBlockLUKSHeader *hdr) +{ + memcpy(hdr->cipher_mode, "xfs", 4); + + return "Unexpected cipher mode string format 'xfs'"; +} + +static const char *luks_bad_ivgen_separator(QCryptoBlockLUKSHeader *hdr) +{ + memcpy(hdr->cipher_mode, "xts:plain64", 12); + + return "Unexpected cipher mode string format 'xts:plain64'"; +} + +static const char *luks_bad_ivgen_name_empty(QCryptoBlockLUKSHeader *hdr) +{ + memcpy(hdr->cipher_mode, "xts-", 5); + + return "IV generator '' not supported"; +} + +static const char *luks_bad_ivgen_name_unknown(QCryptoBlockLUKSHeader *hdr) +{ + memcpy(hdr->cipher_mode, "xts-plain65", 12); + + return "IV generator 'plain65' not supported"; +} + +static const char *luks_bad_ivgen_hash_empty(QCryptoBlockLUKSHeader *hdr) +{ + memcpy(hdr->cipher_mode, "xts-plain65:", 13); + + return "Hash algorithm '' not supported"; +} + +static const char *luks_bad_ivgen_hash_unknown(QCryptoBlockLUKSHeader *hdr) +{ + memcpy(hdr->cipher_mode, "xts-plain65:sha257", 19); + + return "Hash algorithm 'sha257' not supported"; +} + +static const char *luks_bad_hash_spec_empty(QCryptoBlockLUKSHeader *hdr) +{ + memcpy(hdr->hash_spec, "", 1); + + return "Hash algorithm '' not supported"; +} + +static const char *luks_bad_hash_spec_unknown(QCryptoBlockLUKSHeader *hdr) +{ + memcpy(hdr->hash_spec, "sha2566", 8); + + return "Hash algorithm 'sha2566' not supported"; +} + +static const char *luks_bad_stripes(QCryptoBlockLUKSHeader *hdr) +{ + hdr->key_slots[0].stripes = 3999; + + return "Keyslot 0 is corrupted (stripes 3999 != 4000)"; +} + +static const char *luks_bad_key_overlap_header(QCryptoBlockLUKSHeader *hdr) +{ + hdr->key_slots[0].key_offset_sector = 2; + + return "Keyslot 0 is overlapping with the LUKS header"; +} + +static const char *luks_bad_key_overlap_key(QCryptoBlockLUKSHeader *hdr) +{ + hdr->key_slots[0].key_offset_sector = hdr->key_slots[1].key_offset_sector; + + return "Keyslots 0 and 1 are overlapping in the header"; +} + +static const char *luks_bad_key_overlap_payload(QCryptoBlockLUKSHeader *hdr) +{ + hdr->key_slots[0].key_offset_sector = hdr->payload_offset_sector + 42; + + return "Keyslot 0 is overlapping with the encrypted payload"; +} + +static const char *luks_bad_payload_overlap_header(QCryptoBlockLUKSHeader *hdr) +{ + hdr->payload_offset_sector = 2; + + return "LUKS payload is overlapping with the header"; +} + +static const char *luks_bad_key_iterations(QCryptoBlockLUKSHeader *hdr) +{ + hdr->key_slots[0].iterations = 0; + + return "Keyslot 0 iteration count is zero"; +} + +static const char *luks_bad_iterations(QCryptoBlockLUKSHeader *hdr) +{ + hdr->master_key_iterations = 0; + + return "LUKS key iteration count is zero"; +} +#endif + int main(int argc, char **argv) { gsize i; @@ -365,5 +590,79 @@ int main(int argc, char **argv) } } +#ifdef TEST_LUKS + if (g_test_slow()) { + g_test_add_data_func("/crypto/block/luks/bad/cipher-name-nul-term", + luks_bad_null_term_cipher_name, + test_luks_bad_header); + g_test_add_data_func("/crypto/block/luks/bad/cipher-mode-nul-term", + luks_bad_null_term_cipher_mode, + test_luks_bad_header); + g_test_add_data_func("/crypto/block/luks/bad/hash-spec-nul-term", + luks_bad_null_term_hash_spec, + test_luks_bad_header); + g_test_add_data_func("/crypto/block/luks/bad/cipher-name-empty", + luks_bad_cipher_name_empty, + test_luks_bad_header); + g_test_add_data_func("/crypto/block/luks/bad/cipher-name-unknown", + luks_bad_cipher_name_unknown, + test_luks_bad_header); + g_test_add_data_func("/crypto/block/luks/bad/cipher-xts-size", + luks_bad_cipher_xts_size, + test_luks_bad_header); + g_test_add_data_func("/crypto/block/luks/bad/cipher-cbc-size", + luks_bad_cipher_cbc_size, + test_luks_bad_header); + g_test_add_data_func("/crypto/block/luks/bad/cipher-mode-empty", + luks_bad_cipher_mode_empty, + test_luks_bad_header); + g_test_add_data_func("/crypto/block/luks/bad/cipher-mode-unknown", + luks_bad_cipher_mode_unknown, + test_luks_bad_header); + g_test_add_data_func("/crypto/block/luks/bad/ivgen-separator", + luks_bad_ivgen_separator, + test_luks_bad_header); + g_test_add_data_func("/crypto/block/luks/bad/ivgen-name-empty", + luks_bad_ivgen_name_empty, + test_luks_bad_header); + g_test_add_data_func("/crypto/block/luks/bad/ivgen-name-unknown", + luks_bad_ivgen_name_unknown, + test_luks_bad_header); + g_test_add_data_func("/crypto/block/luks/bad/ivgen-hash-empty", + luks_bad_ivgen_hash_empty, + test_luks_bad_header); + g_test_add_data_func("/crypto/block/luks/bad/ivgen-hash-unknown", + luks_bad_ivgen_hash_unknown, + test_luks_bad_header); + g_test_add_data_func("/crypto/block/luks/bad/hash-spec-empty", + luks_bad_hash_spec_empty, + test_luks_bad_header); + g_test_add_data_func("/crypto/block/luks/bad/hash-spec-unknown", + luks_bad_hash_spec_unknown, + test_luks_bad_header); + g_test_add_data_func("/crypto/block/luks/bad/stripes", + luks_bad_stripes, + test_luks_bad_header); + g_test_add_data_func("/crypto/block/luks/bad/key-overlap-header", + luks_bad_key_overlap_header, + test_luks_bad_header); + g_test_add_data_func("/crypto/block/luks/bad/key-overlap-key", + luks_bad_key_overlap_key, + test_luks_bad_header); + g_test_add_data_func("/crypto/block/luks/bad/key-overlap-payload", + luks_bad_key_overlap_payload, + test_luks_bad_header); + g_test_add_data_func("/crypto/block/luks/bad/payload-overlap-header", + luks_bad_payload_overlap_header, + test_luks_bad_header); + g_test_add_data_func("/crypto/block/luks/bad/iterations", + luks_bad_iterations, + test_luks_bad_header); + g_test_add_data_func("/crypto/block/luks/bad/key-iterations", + luks_bad_key_iterations, + test_luks_bad_header); + } +#endif + return g_test_run(); }