* [Qemu-devel] [PATCH v2 00/13] RFC crypto/luks: preparation for encryption key managment
@ 2019-08-26 13:50 Maxim Levitsky
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 01/13] introduce g_autowipe Maxim Levitsky
` (12 more replies)
0 siblings, 13 replies; 30+ messages in thread
From: Maxim Levitsky @ 2019-08-26 13:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Daniel P. Berrangé,
qemu-block, Markus Armbruster, Max Reitz, Stefan Hajnoczi,
Maxim Levitsky
Hi!
This patch series is the refactoring/preparation part of the
former patch series I had sent which adds support for luks
key management.
I tried my best to address all the comments that were given
during the review, and I would like to use that opportunity
to thanks again for the review I was given.
Best regards,
Maxim Levitsky
Maxim Levitsky (13):
introduce g_autowipe
block-crypto: misc refactoring
qcrypto-luks: rename some fields in QCryptoBlockLUKSHeader
qcrypto-luks: don't overwrite cipher_mode in header
qcrypto-luks: simplify masterkey and masterkey length
qcrypto-block: pass keyslot index rather that pointer to the keyslot
qcrypto-luks: use the parsed encryption settings in QCryptoBlockLUKS
qcrypto-luks: extract store and load header
qcrypto-block: extract check and parse header
qcrypto-luks: refactoring: extract store key function
qcrypto-luks: refactoring: simplify the math used for keyslot
locations
qcrypto-luks: use g_autowipe
qcrypto-luks: implement more rigorous header checking
block/crypto.c | 12 +-
crypto/block-luks.c | 1046 +++++++++++++++++++++++++------------------
include/autowipe.h | 52 +++
3 files changed, 666 insertions(+), 444 deletions(-)
create mode 100644 include/autowipe.h
--
2.17.2
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v2 01/13] introduce g_autowipe
2019-08-26 13:50 [Qemu-devel] [PATCH v2 00/13] RFC crypto/luks: preparation for encryption key managment Maxim Levitsky
@ 2019-08-26 13:50 ` Maxim Levitsky
2019-08-27 10:46 ` Tony Nguyen
2019-08-27 10:52 ` Daniel P. Berrangé
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 02/13] block-crypto: misc refactoring Maxim Levitsky
` (11 subsequent siblings)
12 siblings, 2 replies; 30+ messages in thread
From: Maxim Levitsky @ 2019-08-26 13:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Daniel P. Berrangé,
qemu-block, Markus Armbruster, Max Reitz, Stefan Hajnoczi,
Maxim Levitsky
Marking a pointer with g_autowipe, will
not only free it at the scope exit, but also
erase the data it points to just prior to freeing it.
This is first attempt to implement this feature,
as suggested by Daniel and Nir.
The things that need to be verified prior to merging this is
1. Can we just always use memset_s (defined in C++)
or some alternative.
2. is it portable enought for us to use malloc_usable_size
to get the size of malloced pointer in the autofree callback?
This function is aviable in glibc (but no wrapper in glib).
Thanks for Daniel for the g_autowipe and to Nir for the
information about the fact that plain memset is usually
optimized away.
Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Suggested-by: Nir Soffer <nsoffer@redhat.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
include/autowipe.h | 52 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
create mode 100644 include/autowipe.h
diff --git a/include/autowipe.h b/include/autowipe.h
new file mode 100644
index 0000000000..1ed4eaf3ba
--- /dev/null
+++ b/include/autowipe.h
@@ -0,0 +1,52 @@
+/*
+ * g_autowipe implementation for crypto secret wiping
+ *
+ * Copyright (c) 2019 Red Hat, Inc.
+ * Copyright (c) 2019 Maxim Levitsky
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+
+#include <stddef.h>
+#include <malloc.h>
+#include <glib.h>
+
+
+/*
+ * based on
+ * https://www.cryptologie.net/article/419/zeroing-memory-compiler-optimizations-and-memset_s/
+ */
+
+static inline void memerase(void *pointer, size_t size)
+{
+#ifdef __STDC_LIB_EXT1__
+ memset_s(pointer, size, 0, size);
+#else
+ /*volatile used to force compiler to not optimize the code away*/
+ volatile unsigned char *p = pointer;
+ while (size--) {
+ *p++ = 0;
+ }
+#endif
+}
+
+static void g_autoptr_cleanup_generic_wipe_gfree(void *p)
+{
+ void **pp = (void **)p;
+ size_t size = malloc_usable_size(*pp);
+ memerase(*pp, size);
+ g_free(*pp);
+}
+
+#define g_autowipe _GLIB_CLEANUP(g_autoptr_cleanup_generic_wipe_gfree)
--
2.17.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v2 02/13] block-crypto: misc refactoring
2019-08-26 13:50 [Qemu-devel] [PATCH v2 00/13] RFC crypto/luks: preparation for encryption key managment Maxim Levitsky
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 01/13] introduce g_autowipe Maxim Levitsky
@ 2019-08-26 13:50 ` Maxim Levitsky
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 03/13] qcrypto-luks: rename some fields in QCryptoBlockLUKSHeader Maxim Levitsky
` (10 subsequent siblings)
12 siblings, 0 replies; 30+ messages in thread
From: Maxim Levitsky @ 2019-08-26 13:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Daniel P. Berrangé,
qemu-block, Markus Armbruster, Max Reitz, Stefan Hajnoczi,
Maxim Levitsky
* rename the write_func to create_write_func,
and init_func to create_init_func
this is preparation for other write_func that will
be used to update the encryption keys.
No functional changes
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
block/crypto.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/block/crypto.c b/block/crypto.c
index 7eb698774e..6e822c6e50 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -78,7 +78,7 @@ struct BlockCryptoCreateData {
};
-static ssize_t block_crypto_write_func(QCryptoBlock *block,
+static ssize_t block_crypto_create_write_func(QCryptoBlock *block,
size_t offset,
const uint8_t *buf,
size_t buflen,
@@ -96,8 +96,7 @@ static ssize_t block_crypto_write_func(QCryptoBlock *block,
return ret;
}
-
-static ssize_t block_crypto_init_func(QCryptoBlock *block,
+static ssize_t block_crypto_create_init_func(QCryptoBlock *block,
size_t headerlen,
void *opaque,
Error **errp)
@@ -109,7 +108,8 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block,
return -EFBIG;
}
- /* User provided size should reflect amount of space made
+ /*
+ * User provided size should reflect amount of space made
* available to the guest, so we must take account of that
* which will be used by the crypto header
*/
@@ -279,8 +279,8 @@ static int block_crypto_co_create_generic(BlockDriverState *bs,
};
crypto = qcrypto_block_create(opts, NULL,
- block_crypto_init_func,
- block_crypto_write_func,
+ block_crypto_create_init_func,
+ block_crypto_create_write_func,
&data,
errp);
--
2.17.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v2 03/13] qcrypto-luks: rename some fields in QCryptoBlockLUKSHeader
2019-08-26 13:50 [Qemu-devel] [PATCH v2 00/13] RFC crypto/luks: preparation for encryption key managment Maxim Levitsky
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 01/13] introduce g_autowipe Maxim Levitsky
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 02/13] block-crypto: misc refactoring Maxim Levitsky
@ 2019-08-26 13:50 ` Maxim Levitsky
2019-09-06 12:27 ` Daniel P. Berrangé
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 04/13] qcrypto-luks: don't overwrite cipher_mode in header Maxim Levitsky
` (9 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Maxim Levitsky @ 2019-08-26 13:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Daniel P. Berrangé,
qemu-block, Markus Armbruster, Max Reitz, Stefan Hajnoczi,
Maxim Levitsky
* key_bytes -> master_key_len
* payload_offset = payload_offset_sector (to emphasise that this isn't byte offset)
* key_offset -> key_offset_sector - same as above for luks slots
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
crypto/block-luks.c | 91 +++++++++++++++++++++++----------------------
1 file changed, 47 insertions(+), 44 deletions(-)
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 743949adbf..f12fa2d270 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -143,7 +143,7 @@ struct QCryptoBlockLUKSKeySlot {
/* salt for PBKDF2 */
uint8_t salt[QCRYPTO_BLOCK_LUKS_SALT_LEN];
/* start sector of key material */
- uint32_t key_offset;
+ uint32_t key_offset_sector;
/* number of anti-forensic stripes */
uint32_t stripes;
};
@@ -172,10 +172,10 @@ struct QCryptoBlockLUKSHeader {
char hash_spec[QCRYPTO_BLOCK_LUKS_HASH_SPEC_LEN];
/* start offset of the volume data (in 512 byte sectors) */
- uint32_t payload_offset;
+ uint32_t payload_offset_sector;
/* Number of key bytes */
- uint32_t key_bytes;
+ uint32_t master_key_len;
/* master key checksum after PBKDF2 */
uint8_t master_key_digest[QCRYPTO_BLOCK_LUKS_DIGEST_LEN];
@@ -466,7 +466,7 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
* then encrypted.
*/
rv = readfunc(block,
- slot->key_offset * QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
+ slot->key_offset_sector * QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
splitkey, splitkeylen,
opaque,
errp);
@@ -584,8 +584,8 @@ qcrypto_block_luks_find_key(QCryptoBlock *block,
size_t i;
int rv;
- *masterkey = g_new0(uint8_t, luks->header.key_bytes);
- *masterkeylen = luks->header.key_bytes;
+ *masterkey = g_new0(uint8_t, luks->header.master_key_len);
+ *masterkeylen = luks->header.master_key_len;
for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
rv = qcrypto_block_luks_load_key(block,
@@ -677,14 +677,14 @@ qcrypto_block_luks_open(QCryptoBlock *block,
/* 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);
- be32_to_cpus(&luks->header.key_bytes);
+ 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);
+ be32_to_cpus(&luks->header.key_slots[i].key_offset_sector);
be32_to_cpus(&luks->header.key_slots[i].stripes);
}
@@ -743,10 +743,11 @@ qcrypto_block_luks_open(QCryptoBlock *block,
goto fail;
}
- cipheralg = qcrypto_block_luks_cipher_name_lookup(luks->header.cipher_name,
- ciphermode,
- luks->header.key_bytes,
- &local_err);
+ cipheralg =
+ qcrypto_block_luks_cipher_name_lookup(luks->header.cipher_name,
+ ciphermode,
+ luks->header.master_key_len,
+ &local_err);
if (local_err) {
ret = -ENOTSUP;
error_propagate(errp, local_err);
@@ -838,7 +839,7 @@ qcrypto_block_luks_open(QCryptoBlock *block,
}
block->sector_size = QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
- block->payload_offset = luks->header.payload_offset *
+ block->payload_offset = luks->header.payload_offset_sector *
block->sector_size;
luks->cipher_alg = cipheralg;
@@ -993,9 +994,11 @@ qcrypto_block_luks_create(QCryptoBlock *block,
strcpy(luks->header.cipher_mode, cipher_mode_spec);
strcpy(luks->header.hash_spec, hash_alg);
- luks->header.key_bytes = qcrypto_cipher_get_key_len(luks_opts.cipher_alg);
+ luks->header.master_key_len =
+ qcrypto_cipher_get_key_len(luks_opts.cipher_alg);
+
if (luks_opts.cipher_mode == QCRYPTO_CIPHER_MODE_XTS) {
- luks->header.key_bytes *= 2;
+ luks->header.master_key_len *= 2;
}
/* Generate the salt used for hashing the master key
@@ -1008,9 +1011,9 @@ qcrypto_block_luks_create(QCryptoBlock *block,
}
/* Generate random master key */
- masterkey = g_new0(uint8_t, luks->header.key_bytes);
+ masterkey = g_new0(uint8_t, luks->header.master_key_len);
if (qcrypto_random_bytes(masterkey,
- luks->header.key_bytes, errp) < 0) {
+ luks->header.master_key_len, errp) < 0) {
goto error;
}
@@ -1018,7 +1021,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
/* Setup the block device payload encryption objects */
if (qcrypto_block_init_cipher(block, luks_opts.cipher_alg,
luks_opts.cipher_mode, masterkey,
- luks->header.key_bytes, 1, errp) < 0) {
+ luks->header.master_key_len, 1, errp) < 0) {
goto error;
}
@@ -1028,7 +1031,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
block->ivgen = qcrypto_ivgen_new(luks_opts.ivgen_alg,
ivcipheralg,
luks_opts.ivgen_hash_alg,
- masterkey, luks->header.key_bytes,
+ masterkey, luks->header.master_key_len,
errp);
if (!block->ivgen) {
@@ -1040,7 +1043,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
* key, in order to have 1 second of compute time used
*/
iters = qcrypto_pbkdf2_count_iters(luks_opts.hash_alg,
- masterkey, luks->header.key_bytes,
+ masterkey, luks->header.master_key_len,
luks->header.master_key_salt,
QCRYPTO_BLOCK_LUKS_SALT_LEN,
QCRYPTO_BLOCK_LUKS_DIGEST_LEN,
@@ -1080,7 +1083,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
* valid master key
*/
if (qcrypto_pbkdf2(luks_opts.hash_alg,
- masterkey, luks->header.key_bytes,
+ masterkey, luks->header.master_key_len,
luks->header.master_key_salt,
QCRYPTO_BLOCK_LUKS_SALT_LEN,
luks->header.master_key_iterations,
@@ -1093,7 +1096,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
/* Although LUKS has multiple key slots, we're just going
* to use the first key slot */
- splitkeylen = luks->header.key_bytes * QCRYPTO_BLOCK_LUKS_STRIPES;
+ splitkeylen = luks->header.master_key_len * QCRYPTO_BLOCK_LUKS_STRIPES;
for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
luks->header.key_slots[i].active = i == 0 ?
QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED :
@@ -1103,7 +1106,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
/* This calculation doesn't match that shown in the spec,
* but instead follows the cryptsetup implementation.
*/
- luks->header.key_slots[i].key_offset =
+ luks->header.key_slots[i].key_offset_sector =
(QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
QCRYPTO_BLOCK_LUKS_SECTOR_SIZE) +
(ROUND_UP(DIV_ROUND_UP(splitkeylen, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE),
@@ -1124,7 +1127,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
(uint8_t *)password, strlen(password),
luks->header.key_slots[0].salt,
QCRYPTO_BLOCK_LUKS_SALT_LEN,
- luks->header.key_bytes,
+ luks->header.master_key_len,
&local_err);
if (local_err) {
error_propagate(errp, local_err);
@@ -1155,13 +1158,13 @@ qcrypto_block_luks_create(QCryptoBlock *block,
/* Generate a key that we'll use to encrypt the master
* key, from the user's password
*/
- slotkey = g_new0(uint8_t, luks->header.key_bytes);
+ slotkey = g_new0(uint8_t, luks->header.master_key_len);
if (qcrypto_pbkdf2(luks_opts.hash_alg,
(uint8_t *)password, strlen(password),
luks->header.key_slots[0].salt,
QCRYPTO_BLOCK_LUKS_SALT_LEN,
luks->header.key_slots[0].iterations,
- slotkey, luks->header.key_bytes,
+ slotkey, luks->header.master_key_len,
errp) < 0) {
goto error;
}
@@ -1172,7 +1175,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
*/
cipher = qcrypto_cipher_new(luks_opts.cipher_alg,
luks_opts.cipher_mode,
- slotkey, luks->header.key_bytes,
+ slotkey, luks->header.master_key_len,
errp);
if (!cipher) {
goto error;
@@ -1181,7 +1184,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
ivgen = qcrypto_ivgen_new(luks_opts.ivgen_alg,
ivcipheralg,
luks_opts.ivgen_hash_alg,
- slotkey, luks->header.key_bytes,
+ slotkey, luks->header.master_key_len,
errp);
if (!ivgen) {
goto error;
@@ -1193,7 +1196,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
splitkey = g_new0(uint8_t, splitkeylen);
if (qcrypto_afsplit_encode(luks_opts.hash_alg,
- luks->header.key_bytes,
+ luks->header.master_key_len,
luks->header.key_slots[0].stripes,
masterkey,
splitkey,
@@ -1217,7 +1220,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
* slot headers, rounded up to the nearest sector, combined with
* the size of each master key material region, also rounded up
* to the nearest sector */
- luks->header.payload_offset =
+ luks->header.payload_offset_sector =
(QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
QCRYPTO_BLOCK_LUKS_SECTOR_SIZE) +
(ROUND_UP(DIV_ROUND_UP(splitkeylen, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE),
@@ -1226,7 +1229,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
block->sector_size = QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
- block->payload_offset = luks->header.payload_offset *
+ block->payload_offset = luks->header.payload_offset_sector *
block->sector_size;
/* Reserve header space to match payload offset */
@@ -1239,14 +1242,14 @@ qcrypto_block_luks_create(QCryptoBlock *block,
/* Everything on disk uses Big Endian, so flip header fields
* before writing them */
cpu_to_be16s(&luks->header.version);
- cpu_to_be32s(&luks->header.payload_offset);
- cpu_to_be32s(&luks->header.key_bytes);
+ cpu_to_be32s(&luks->header.payload_offset_sector);
+ cpu_to_be32s(&luks->header.master_key_len);
cpu_to_be32s(&luks->header.master_key_iterations);
for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
cpu_to_be32s(&luks->header.key_slots[i].active);
cpu_to_be32s(&luks->header.key_slots[i].iterations);
- cpu_to_be32s(&luks->header.key_slots[i].key_offset);
+ cpu_to_be32s(&luks->header.key_slots[i].key_offset_sector);
cpu_to_be32s(&luks->header.key_slots[i].stripes);
}
@@ -1263,14 +1266,14 @@ qcrypto_block_luks_create(QCryptoBlock *block,
/* Byte swap the header back to native, in case we need
* to read it again later */
be16_to_cpus(&luks->header.version);
- be32_to_cpus(&luks->header.payload_offset);
- be32_to_cpus(&luks->header.key_bytes);
+ 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);
+ be32_to_cpus(&luks->header.key_slots[i].key_offset_sector);
be32_to_cpus(&luks->header.key_slots[i].stripes);
}
@@ -1282,7 +1285,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
/* Write out the master key material, starting at the
* sector immediately following the partition header. */
if (writefunc(block,
- luks->header.key_slots[0].key_offset *
+ luks->header.key_slots[0].key_offset_sector *
QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
splitkey, splitkeylen,
opaque,
@@ -1296,17 +1299,17 @@ qcrypto_block_luks_create(QCryptoBlock *block,
luks->ivgen_hash_alg = luks_opts.ivgen_hash_alg;
luks->hash_alg = luks_opts.hash_alg;
- memset(masterkey, 0, luks->header.key_bytes);
- memset(slotkey, 0, luks->header.key_bytes);
+ memset(masterkey, 0, luks->header.master_key_len);
+ memset(slotkey, 0, luks->header.master_key_len);
return 0;
error:
if (masterkey) {
- memset(masterkey, 0, luks->header.key_bytes);
+ memset(masterkey, 0, luks->header.master_key_len);
}
if (slotkey) {
- memset(slotkey, 0, luks->header.key_bytes);
+ memset(slotkey, 0, luks->header.master_key_len);
}
qcrypto_block_free_cipher(block);
@@ -1346,7 +1349,7 @@ static int qcrypto_block_luks_get_info(QCryptoBlock *block,
slots->value = slot = g_new0(QCryptoBlockInfoLUKSSlot, 1);
slot->active = luks->header.key_slots[i].active ==
QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED;
- slot->key_offset = luks->header.key_slots[i].key_offset
+ slot->key_offset = luks->header.key_slots[i].key_offset_sector
* QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
if (slot->active) {
slot->has_iters = true;
--
2.17.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v2 04/13] qcrypto-luks: don't overwrite cipher_mode in header
2019-08-26 13:50 [Qemu-devel] [PATCH v2 00/13] RFC crypto/luks: preparation for encryption key managment Maxim Levitsky
` (2 preceding siblings ...)
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 03/13] qcrypto-luks: rename some fields in QCryptoBlockLUKSHeader Maxim Levitsky
@ 2019-08-26 13:50 ` Maxim Levitsky
2019-09-06 12:29 ` Daniel P. Berrangé
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 05/13] qcrypto-luks: simplify masterkey and masterkey length Maxim Levitsky
` (8 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Maxim Levitsky @ 2019-08-26 13:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Daniel P. Berrangé,
qemu-block, Markus Armbruster, Max Reitz, Stefan Hajnoczi,
Maxim Levitsky
This way we can store the header we loaded, which
will be used in key management code
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
crypto/block-luks.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index f12fa2d270..e9ae3f6baa 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -645,6 +645,7 @@ qcrypto_block_luks_open(QCryptoBlock *block,
QCryptoHashAlgorithm hash;
QCryptoHashAlgorithm ivhash;
g_autofree char *password = NULL;
+ g_autofree char *cipher_mode = NULL;
if (!(flags & QCRYPTO_BLOCK_OPEN_NO_IO)) {
if (!options->u.luks.key_secret) {
@@ -701,6 +702,8 @@ qcrypto_block_luks_open(QCryptoBlock *block,
goto fail;
}
+ cipher_mode = g_strdup(luks->header.cipher_mode);
+
/*
* The cipher_mode header contains a string that we have
* to further parse, of the format
@@ -709,11 +712,11 @@ qcrypto_block_luks_open(QCryptoBlock *block,
*
* eg cbc-essiv:sha256, cbc-plain64
*/
- ivgen_name = strchr(luks->header.cipher_mode, '-');
+ ivgen_name = strchr(cipher_mode, '-');
if (!ivgen_name) {
ret = -EINVAL;
error_setg(errp, "Unexpected cipher mode string format %s",
- luks->header.cipher_mode);
+ cipher_mode);
goto fail;
}
*ivgen_name = '\0';
@@ -735,7 +738,7 @@ qcrypto_block_luks_open(QCryptoBlock *block,
}
}
- ciphermode = qcrypto_block_luks_cipher_mode_lookup(luks->header.cipher_mode,
+ ciphermode = qcrypto_block_luks_cipher_mode_lookup(cipher_mode,
&local_err);
if (local_err) {
ret = -ENOTSUP;
--
2.17.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v2 05/13] qcrypto-luks: simplify masterkey and masterkey length
2019-08-26 13:50 [Qemu-devel] [PATCH v2 00/13] RFC crypto/luks: preparation for encryption key managment Maxim Levitsky
` (3 preceding siblings ...)
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 04/13] qcrypto-luks: don't overwrite cipher_mode in header Maxim Levitsky
@ 2019-08-26 13:50 ` Maxim Levitsky
2019-09-06 12:30 ` Daniel P. Berrangé
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 06/13] qcrypto-block: pass keyslot index rather that pointer to the keyslot Maxim Levitsky
` (7 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Maxim Levitsky @ 2019-08-26 13:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Daniel P. Berrangé,
qemu-block, Markus Armbruster, Max Reitz, Stefan Hajnoczi,
Maxim Levitsky
Let the caller allocate masterkey
Always use master key len from the header
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
crypto/block-luks.c | 44 +++++++++++++++++++++-----------------------
1 file changed, 21 insertions(+), 23 deletions(-)
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index e9ae3f6baa..331377293d 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -419,7 +419,6 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
QCryptoCipherAlgorithm ivcipheralg,
QCryptoHashAlgorithm ivhash,
uint8_t *masterkey,
- size_t masterkeylen,
QCryptoBlockReadFunc readfunc,
void *opaque,
Error **errp)
@@ -438,9 +437,9 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
return 0;
}
- splitkeylen = masterkeylen * slot->stripes;
+ splitkeylen = luks->header.master_key_len * slot->stripes;
splitkey = g_new0(uint8_t, splitkeylen);
- possiblekey = g_new0(uint8_t, masterkeylen);
+ possiblekey = g_new0(uint8_t, luks->header.master_key_len);
/*
* The user password is used to generate a (possible)
@@ -453,7 +452,7 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
(const uint8_t *)password, strlen(password),
slot->salt, QCRYPTO_BLOCK_LUKS_SALT_LEN,
slot->iterations,
- possiblekey, masterkeylen,
+ possiblekey, luks->header.master_key_len,
errp) < 0) {
return -1;
}
@@ -478,7 +477,7 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
/* Setup the cipher/ivgen that we'll use to try to decrypt
* the split master key material */
cipher = qcrypto_cipher_new(cipheralg, ciphermode,
- possiblekey, masterkeylen,
+ possiblekey, luks->header.master_key_len,
errp);
if (!cipher) {
return -1;
@@ -489,7 +488,7 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
ivgen = qcrypto_ivgen_new(ivalg,
ivcipheralg,
ivhash,
- possiblekey, masterkeylen,
+ possiblekey, luks->header.master_key_len,
errp);
if (!ivgen) {
return -1;
@@ -519,7 +518,7 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
* it back together to get the actual master key.
*/
if (qcrypto_afsplit_decode(hash,
- masterkeylen,
+ luks->header.master_key_len,
slot->stripes,
splitkey,
masterkey,
@@ -537,11 +536,13 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
* header
*/
if (qcrypto_pbkdf2(hash,
- masterkey, masterkeylen,
+ masterkey,
+ luks->header.master_key_len,
luks->header.master_key_salt,
QCRYPTO_BLOCK_LUKS_SALT_LEN,
luks->header.master_key_iterations,
- keydigest, G_N_ELEMENTS(keydigest),
+ keydigest,
+ G_N_ELEMENTS(keydigest),
errp) < 0) {
return -1;
}
@@ -574,8 +575,7 @@ qcrypto_block_luks_find_key(QCryptoBlock *block,
QCryptoIVGenAlgorithm ivalg,
QCryptoCipherAlgorithm ivcipheralg,
QCryptoHashAlgorithm ivhash,
- uint8_t **masterkey,
- size_t *masterkeylen,
+ uint8_t *masterkey,
QCryptoBlockReadFunc readfunc,
void *opaque,
Error **errp)
@@ -584,9 +584,6 @@ qcrypto_block_luks_find_key(QCryptoBlock *block,
size_t i;
int rv;
- *masterkey = g_new0(uint8_t, luks->header.master_key_len);
- *masterkeylen = luks->header.master_key_len;
-
for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
rv = qcrypto_block_luks_load_key(block,
&luks->header.key_slots[i],
@@ -597,8 +594,7 @@ qcrypto_block_luks_find_key(QCryptoBlock *block,
ivalg,
ivcipheralg,
ivhash,
- *masterkey,
- *masterkeylen,
+ masterkey,
readfunc,
opaque,
errp);
@@ -613,9 +609,6 @@ qcrypto_block_luks_find_key(QCryptoBlock *block,
error_setg(errp, "Invalid password, cannot unlock any keyslot");
error:
- g_free(*masterkey);
- *masterkey = NULL;
- *masterkeylen = 0;
return -1;
}
@@ -636,7 +629,6 @@ qcrypto_block_luks_open(QCryptoBlock *block,
size_t i;
ssize_t rv;
g_autofree uint8_t *masterkey = NULL;
- size_t masterkeylen;
char *ivgen_name, *ivhash_name;
QCryptoCipherMode ciphermode;
QCryptoCipherAlgorithm cipheralg;
@@ -802,6 +794,9 @@ qcrypto_block_luks_open(QCryptoBlock *block,
/* Try to find which key slot our password is valid for
* and unlock the master key from that slot.
*/
+
+ masterkey = g_new0(uint8_t, luks->header.master_key_len);
+
if (qcrypto_block_luks_find_key(block,
password,
cipheralg, ciphermode,
@@ -809,7 +804,7 @@ qcrypto_block_luks_open(QCryptoBlock *block,
ivalg,
ivcipheralg,
ivhash,
- &masterkey, &masterkeylen,
+ masterkey,
readfunc, opaque,
errp) < 0) {
ret = -EACCES;
@@ -825,7 +820,8 @@ qcrypto_block_luks_open(QCryptoBlock *block,
block->ivgen = qcrypto_ivgen_new(ivalg,
ivcipheralg,
ivhash,
- masterkey, masterkeylen,
+ masterkey,
+ luks->header.master_key_len,
errp);
if (!block->ivgen) {
ret = -ENOTSUP;
@@ -833,7 +829,9 @@ qcrypto_block_luks_open(QCryptoBlock *block,
}
ret = qcrypto_block_init_cipher(block, cipheralg, ciphermode,
- masterkey, masterkeylen, n_threads,
+ masterkey,
+ luks->header.master_key_len,
+ n_threads,
errp);
if (ret < 0) {
ret = -ENOTSUP;
--
2.17.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v2 06/13] qcrypto-block: pass keyslot index rather that pointer to the keyslot
2019-08-26 13:50 [Qemu-devel] [PATCH v2 00/13] RFC crypto/luks: preparation for encryption key managment Maxim Levitsky
` (4 preceding siblings ...)
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 05/13] qcrypto-luks: simplify masterkey and masterkey length Maxim Levitsky
@ 2019-08-26 13:50 ` Maxim Levitsky
2019-09-06 12:32 ` Daniel P. Berrangé
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 07/13] qcrypto-luks: use the parsed encryption settings in QCryptoBlockLUKS Maxim Levitsky
` (6 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Maxim Levitsky @ 2019-08-26 13:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Daniel P. Berrangé,
qemu-block, Markus Armbruster, Max Reitz, Stefan Hajnoczi,
Maxim Levitsky
Another minor refactoring
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
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 331377293d..0d81f2ac61 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -410,7 +410,7 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher,
*/
static int
qcrypto_block_luks_load_key(QCryptoBlock *block,
- QCryptoBlockLUKSKeySlot *slot,
+ unsigned int slot_idx,
const char *password,
QCryptoCipherAlgorithm cipheralg,
QCryptoCipherMode ciphermode,
@@ -424,6 +424,7 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
Error **errp)
{
QCryptoBlockLUKS *luks = block->opaque;
+ const QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[slot_idx];
g_autofree uint8_t *splitkey = NULL;
size_t splitkeylen;
g_autofree uint8_t *possiblekey = NULL;
@@ -580,13 +581,12 @@ qcrypto_block_luks_find_key(QCryptoBlock *block,
void *opaque,
Error **errp)
{
- QCryptoBlockLUKS *luks = block->opaque;
size_t i;
int rv;
for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
rv = qcrypto_block_luks_load_key(block,
- &luks->header.key_slots[i],
+ i,
password,
cipheralg,
ciphermode,
--
2.17.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v2 07/13] qcrypto-luks: use the parsed encryption settings in QCryptoBlockLUKS
2019-08-26 13:50 [Qemu-devel] [PATCH v2 00/13] RFC crypto/luks: preparation for encryption key managment Maxim Levitsky
` (5 preceding siblings ...)
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 06/13] qcrypto-block: pass keyslot index rather that pointer to the keyslot Maxim Levitsky
@ 2019-08-26 13:50 ` Maxim Levitsky
2019-09-06 12:35 ` Daniel P. Berrangé
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 08/13] qcrypto-luks: extract store and load header Maxim Levitsky
` (5 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Maxim Levitsky @ 2019-08-26 13:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Daniel P. Berrangé,
qemu-block, Markus Armbruster, Max Reitz, Stefan Hajnoczi,
Maxim Levitsky
Prior to that patch, the parsed encryptio settings
were alrady stored into the QCryptoBlockLUKS but not
used anywhere but in qcrypto_block_luks_get_info
Using them simplifies the code
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
crypto/block-luks.c | 169 +++++++++++++++++++++-----------------------
1 file changed, 79 insertions(+), 90 deletions(-)
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 0d81f2ac61..cad65ae0aa 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -199,13 +199,25 @@ QEMU_BUILD_BUG_ON(sizeof(struct QCryptoBlockLUKSHeader) != 592);
struct QCryptoBlockLUKS {
QCryptoBlockLUKSHeader header;
- /* Cache parsed versions of what's in header fields,
- * as we can't rely on QCryptoBlock.cipher being
- * non-NULL */
+ /* Main encryption algorithm used for encryption*/
QCryptoCipherAlgorithm cipher_alg;
+
+ /* Mode of encryption for the selected encryption algorithm */
QCryptoCipherMode cipher_mode;
+
+ /* Initialization vector generation algorithm */
QCryptoIVGenAlgorithm ivgen_alg;
+
+ /* Hash algorithm used for IV generation*/
QCryptoHashAlgorithm ivgen_hash_alg;
+
+ /*
+ * Encryption algorithm used for IV generation.
+ * Usually the same as main encryption algorithm
+ */
+ QCryptoCipherAlgorithm ivgen_cipher_alg;
+
+ /* Hash algorithm used in pbkdf2 function */
QCryptoHashAlgorithm hash_alg;
};
@@ -412,12 +424,6 @@ static int
qcrypto_block_luks_load_key(QCryptoBlock *block,
unsigned int slot_idx,
const char *password,
- QCryptoCipherAlgorithm cipheralg,
- QCryptoCipherMode ciphermode,
- QCryptoHashAlgorithm hash,
- QCryptoIVGenAlgorithm ivalg,
- QCryptoCipherAlgorithm ivcipheralg,
- QCryptoHashAlgorithm ivhash,
uint8_t *masterkey,
QCryptoBlockReadFunc readfunc,
void *opaque,
@@ -449,7 +455,7 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
* the key is correct and validate the results of
* decryption later.
*/
- if (qcrypto_pbkdf2(hash,
+ if (qcrypto_pbkdf2(luks->hash_alg,
(const uint8_t *)password, strlen(password),
slot->salt, QCRYPTO_BLOCK_LUKS_SALT_LEN,
slot->iterations,
@@ -477,19 +483,23 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
/* Setup the cipher/ivgen that we'll use to try to decrypt
* the split master key material */
- cipher = qcrypto_cipher_new(cipheralg, ciphermode,
- possiblekey, luks->header.master_key_len,
+ cipher = qcrypto_cipher_new(luks->cipher_alg,
+ luks->cipher_mode,
+ possiblekey,
+ luks->header.master_key_len,
errp);
if (!cipher) {
return -1;
}
- niv = qcrypto_cipher_get_iv_len(cipheralg,
- ciphermode);
- ivgen = qcrypto_ivgen_new(ivalg,
- ivcipheralg,
- ivhash,
- possiblekey, luks->header.master_key_len,
+ niv = qcrypto_cipher_get_iv_len(luks->cipher_alg,
+ luks->cipher_mode);
+
+ ivgen = qcrypto_ivgen_new(luks->ivgen_alg,
+ luks->ivgen_cipher_alg,
+ luks->ivgen_hash_alg,
+ possiblekey,
+ luks->header.master_key_len,
errp);
if (!ivgen) {
return -1;
@@ -518,7 +528,7 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
* Now we've decrypted the split master key, join
* it back together to get the actual master key.
*/
- if (qcrypto_afsplit_decode(hash,
+ if (qcrypto_afsplit_decode(luks->hash_alg,
luks->header.master_key_len,
slot->stripes,
splitkey,
@@ -536,7 +546,7 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
* then comparing that to the hash stored in the key slot
* header
*/
- if (qcrypto_pbkdf2(hash,
+ if (qcrypto_pbkdf2(luks->hash_alg,
masterkey,
luks->header.master_key_len,
luks->header.master_key_salt,
@@ -570,12 +580,6 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
static int
qcrypto_block_luks_find_key(QCryptoBlock *block,
const char *password,
- QCryptoCipherAlgorithm cipheralg,
- QCryptoCipherMode ciphermode,
- QCryptoHashAlgorithm hash,
- QCryptoIVGenAlgorithm ivalg,
- QCryptoCipherAlgorithm ivcipheralg,
- QCryptoHashAlgorithm ivhash,
uint8_t *masterkey,
QCryptoBlockReadFunc readfunc,
void *opaque,
@@ -588,12 +592,6 @@ qcrypto_block_luks_find_key(QCryptoBlock *block,
rv = qcrypto_block_luks_load_key(block,
i,
password,
- cipheralg,
- ciphermode,
- hash,
- ivalg,
- ivcipheralg,
- ivhash,
masterkey,
readfunc,
opaque,
@@ -607,7 +605,6 @@ qcrypto_block_luks_find_key(QCryptoBlock *block,
}
error_setg(errp, "Invalid password, cannot unlock any keyslot");
-
error:
return -1;
}
@@ -623,19 +620,13 @@ qcrypto_block_luks_open(QCryptoBlock *block,
size_t n_threads,
Error **errp)
{
- QCryptoBlockLUKS *luks;
+ QCryptoBlockLUKS *luks = NULL;
Error *local_err = NULL;
int ret = 0;
size_t i;
ssize_t rv;
g_autofree uint8_t *masterkey = NULL;
char *ivgen_name, *ivhash_name;
- QCryptoCipherMode ciphermode;
- QCryptoCipherAlgorithm cipheralg;
- QCryptoIVGenAlgorithm ivalg;
- QCryptoCipherAlgorithm ivcipheralg;
- QCryptoHashAlgorithm hash;
- QCryptoHashAlgorithm ivhash;
g_autofree char *password = NULL;
g_autofree char *cipher_mode = NULL;
@@ -716,13 +707,13 @@ qcrypto_block_luks_open(QCryptoBlock *block,
ivhash_name = strchr(ivgen_name, ':');
if (!ivhash_name) {
- ivhash = 0;
+ luks->ivgen_hash_alg = 0;
} else {
*ivhash_name = '\0';
ivhash_name++;
- ivhash = qcrypto_block_luks_hash_name_lookup(ivhash_name,
- &local_err);
+ luks->ivgen_hash_alg = qcrypto_block_luks_hash_name_lookup(ivhash_name,
+ &local_err);
if (local_err) {
ret = -ENOTSUP;
error_propagate(errp, local_err);
@@ -730,17 +721,17 @@ qcrypto_block_luks_open(QCryptoBlock *block,
}
}
- ciphermode = qcrypto_block_luks_cipher_mode_lookup(cipher_mode,
- &local_err);
+ luks->cipher_mode = qcrypto_block_luks_cipher_mode_lookup(cipher_mode,
+ &local_err);
if (local_err) {
ret = -ENOTSUP;
error_propagate(errp, local_err);
goto fail;
}
- cipheralg =
+ luks->cipher_alg =
qcrypto_block_luks_cipher_name_lookup(luks->header.cipher_name,
- ciphermode,
+ luks->cipher_mode,
luks->header.master_key_len,
&local_err);
if (local_err) {
@@ -749,31 +740,33 @@ qcrypto_block_luks_open(QCryptoBlock *block,
goto fail;
}
- hash = qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec,
- &local_err);
+ luks->hash_alg =
+ qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec,
+ &local_err);
if (local_err) {
ret = -ENOTSUP;
error_propagate(errp, local_err);
goto fail;
}
- ivalg = qcrypto_block_luks_ivgen_name_lookup(ivgen_name,
- &local_err);
+ luks->ivgen_alg = qcrypto_block_luks_ivgen_name_lookup(ivgen_name,
+ &local_err);
if (local_err) {
ret = -ENOTSUP;
error_propagate(errp, local_err);
goto fail;
}
- if (ivalg == QCRYPTO_IVGEN_ALG_ESSIV) {
+ if (luks->ivgen_alg == QCRYPTO_IVGEN_ALG_ESSIV) {
if (!ivhash_name) {
ret = -EINVAL;
error_setg(errp, "Missing IV generator hash specification");
goto fail;
}
- ivcipheralg = qcrypto_block_luks_essiv_cipher(cipheralg,
- ivhash,
- &local_err);
+ luks->ivgen_cipher_alg =
+ qcrypto_block_luks_essiv_cipher(luks->cipher_alg,
+ luks->ivgen_hash_alg,
+ &local_err);
if (local_err) {
ret = -ENOTSUP;
error_propagate(errp, local_err);
@@ -787,7 +780,7 @@ qcrypto_block_luks_open(QCryptoBlock *block,
* ignore hash names with these ivgens rather than report
* an error about the invalid usage
*/
- ivcipheralg = cipheralg;
+ luks->ivgen_cipher_alg = luks->cipher_alg;
}
if (!(flags & QCRYPTO_BLOCK_OPEN_NO_IO)) {
@@ -799,11 +792,6 @@ qcrypto_block_luks_open(QCryptoBlock *block,
if (qcrypto_block_luks_find_key(block,
password,
- cipheralg, ciphermode,
- hash,
- ivalg,
- ivcipheralg,
- ivhash,
masterkey,
readfunc, opaque,
errp) < 0) {
@@ -814,12 +802,13 @@ qcrypto_block_luks_open(QCryptoBlock *block,
/* We have a valid master key now, so can setup the
* block device payload decryption objects
*/
- block->kdfhash = hash;
- block->niv = qcrypto_cipher_get_iv_len(cipheralg,
- ciphermode);
- block->ivgen = qcrypto_ivgen_new(ivalg,
- ivcipheralg,
- ivhash,
+ block->kdfhash = luks->hash_alg;
+ block->niv = qcrypto_cipher_get_iv_len(luks->cipher_alg,
+ luks->cipher_mode);
+
+ block->ivgen = qcrypto_ivgen_new(luks->ivgen_alg,
+ luks->ivgen_cipher_alg,
+ luks->ivgen_hash_alg,
masterkey,
luks->header.master_key_len,
errp);
@@ -828,7 +817,9 @@ qcrypto_block_luks_open(QCryptoBlock *block,
goto fail;
}
- ret = qcrypto_block_init_cipher(block, cipheralg, ciphermode,
+ ret = qcrypto_block_init_cipher(block,
+ luks->cipher_alg,
+ luks->cipher_mode,
masterkey,
luks->header.master_key_len,
n_threads,
@@ -843,11 +834,6 @@ qcrypto_block_luks_open(QCryptoBlock *block,
block->payload_offset = luks->header.payload_offset_sector *
block->sector_size;
- luks->cipher_alg = cipheralg;
- luks->cipher_mode = ciphermode;
- luks->ivgen_alg = ivalg;
- luks->ivgen_hash_alg = ivhash;
- luks->hash_alg = hash;
return 0;
@@ -893,7 +879,6 @@ qcrypto_block_luks_create(QCryptoBlock *block,
const char *ivgen_hash_alg = NULL;
const char *hash_alg;
g_autofree char *cipher_mode_spec = NULL;
- QCryptoCipherAlgorithm ivcipheralg = 0;
uint64_t iters;
memcpy(&luks_opts, &options->u.luks, sizeof(luks_opts));
@@ -918,6 +903,17 @@ qcrypto_block_luks_create(QCryptoBlock *block,
luks_opts.has_ivgen_hash_alg = true;
}
}
+
+ luks = g_new0(QCryptoBlockLUKS, 1);
+ block->opaque = luks;
+
+ luks->cipher_alg = luks_opts.cipher_alg;
+ luks->cipher_mode = luks_opts.cipher_mode;
+ luks->ivgen_alg = luks_opts.ivgen_alg;
+ luks->ivgen_hash_alg = luks_opts.ivgen_hash_alg;
+ luks->hash_alg = luks_opts.hash_alg;
+
+
/* Note we're allowing ivgen_hash_alg to be set even for
* non-essiv iv generators that don't need a hash. It will
* be silently ignored, for compatibility with dm-crypt */
@@ -925,15 +921,13 @@ qcrypto_block_luks_create(QCryptoBlock *block,
if (!options->u.luks.key_secret) {
error_setg(errp, "Parameter '%skey-secret' is required for cipher",
optprefix ? optprefix : "");
- return -1;
+ goto error;
}
password = qcrypto_secret_lookup_as_utf8(luks_opts.key_secret, errp);
if (!password) {
- return -1;
+ goto error;
}
- luks = g_new0(QCryptoBlockLUKS, 1);
- block->opaque = luks;
memcpy(luks->header.magic, qcrypto_block_luks_magic,
QCRYPTO_BLOCK_LUKS_MAGIC_LEN);
@@ -980,15 +974,16 @@ qcrypto_block_luks_create(QCryptoBlock *block,
}
if (luks_opts.ivgen_alg == QCRYPTO_IVGEN_ALG_ESSIV) {
- ivcipheralg = qcrypto_block_luks_essiv_cipher(luks_opts.cipher_alg,
- luks_opts.ivgen_hash_alg,
- &local_err);
+ luks->ivgen_cipher_alg =
+ qcrypto_block_luks_essiv_cipher(luks_opts.cipher_alg,
+ luks_opts.ivgen_hash_alg,
+ &local_err);
if (local_err) {
error_propagate(errp, local_err);
goto error;
}
} else {
- ivcipheralg = luks_opts.cipher_alg;
+ luks->ivgen_cipher_alg = luks_opts.cipher_alg;
}
strcpy(luks->header.cipher_name, cipher_alg);
@@ -1030,7 +1025,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
block->niv = qcrypto_cipher_get_iv_len(luks_opts.cipher_alg,
luks_opts.cipher_mode);
block->ivgen = qcrypto_ivgen_new(luks_opts.ivgen_alg,
- ivcipheralg,
+ luks->ivgen_cipher_alg,
luks_opts.ivgen_hash_alg,
masterkey, luks->header.master_key_len,
errp);
@@ -1183,7 +1178,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
}
ivgen = qcrypto_ivgen_new(luks_opts.ivgen_alg,
- ivcipheralg,
+ luks->ivgen_cipher_alg,
luks_opts.ivgen_hash_alg,
slotkey, luks->header.master_key_len,
errp);
@@ -1294,12 +1289,6 @@ qcrypto_block_luks_create(QCryptoBlock *block,
goto error;
}
- luks->cipher_alg = luks_opts.cipher_alg;
- luks->cipher_mode = luks_opts.cipher_mode;
- luks->ivgen_alg = luks_opts.ivgen_alg;
- luks->ivgen_hash_alg = luks_opts.ivgen_hash_alg;
- luks->hash_alg = luks_opts.hash_alg;
-
memset(masterkey, 0, luks->header.master_key_len);
memset(slotkey, 0, luks->header.master_key_len);
--
2.17.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v2 08/13] qcrypto-luks: extract store and load header
2019-08-26 13:50 [Qemu-devel] [PATCH v2 00/13] RFC crypto/luks: preparation for encryption key managment Maxim Levitsky
` (6 preceding siblings ...)
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 07/13] qcrypto-luks: use the parsed encryption settings in QCryptoBlockLUKS Maxim Levitsky
@ 2019-08-26 13:50 ` Maxim Levitsky
2019-09-06 13:06 ` Daniel P. Berrangé
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 09/13] qcrypto-block: extract check and parse header Maxim Levitsky
` (4 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Maxim Levitsky @ 2019-08-26 13:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Daniel P. Berrangé,
qemu-block, Markus Armbruster, Max Reitz, Stefan Hajnoczi,
Maxim Levitsky
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
crypto/block-luks.c | 166 +++++++++++++++++++++++++++-----------------
1 file changed, 102 insertions(+), 64 deletions(-)
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index cad65ae0aa..b4dc6fc899 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -409,6 +409,105 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher,
}
}
+/*
+ * Stores the main LUKS header, taking care of endianess
+ */
+static int
+qcrypto_block_luks_store_header(QCryptoBlock *block,
+ QCryptoBlockWriteFunc writefunc,
+ void *opaque,
+ Error **errp)
+{
+ const QCryptoBlockLUKS *luks = block->opaque;
+ Error *local_err = NULL;
+ size_t i;
+ QCryptoBlockLUKSHeader *hdr_copy;
+
+ /* 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);
+ }
+
+ /* Write out the partition header and key slot headers */
+ writefunc(block, 0, (const uint8_t *)hdr_copy, sizeof(*hdr_copy),
+ opaque, &local_err);
+
+ g_free(hdr_copy);
+
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return -1;
+ }
+ return 0;
+}
+
+/*
+ * Loads the main LUKS header,and byteswaps it to native endianess
+ * And run basic sanity checks on it
+ */
+static int
+qcrypto_block_luks_load_header(QCryptoBlock *block,
+ QCryptoBlockReadFunc readfunc,
+ void *opaque,
+ Error **errp)
+{
+ ssize_t rv;
+ size_t i;
+ int ret = 0;
+ QCryptoBlockLUKS *luks = block->opaque;
+
+ /*
+ * Read the entire LUKS header, minus the key material from
+ * the underlying device
+ */
+
+ rv = readfunc(block, 0,
+ (uint8_t *)&luks->header,
+ sizeof(luks->header),
+ opaque,
+ errp);
+ if (rv < 0) {
+ ret = rv;
+ goto fail;
+ }
+
+ /*
+ * 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);
+ }
+
+
+ return 0;
+fail:
+ return ret;
+}
+
/*
* Given a key slot, and user password, this will attempt to unlock
* the master encryption key from the key slot.
@@ -623,8 +722,6 @@ qcrypto_block_luks_open(QCryptoBlock *block,
QCryptoBlockLUKS *luks = NULL;
Error *local_err = NULL;
int ret = 0;
- size_t i;
- ssize_t rv;
g_autofree uint8_t *masterkey = NULL;
char *ivgen_name, *ivhash_name;
g_autofree char *password = NULL;
@@ -646,31 +743,11 @@ qcrypto_block_luks_open(QCryptoBlock *block,
luks = g_new0(QCryptoBlockLUKS, 1);
block->opaque = luks;
- /* Read the entire LUKS header, minus the key material from
- * the underlying device */
- rv = readfunc(block, 0,
- (uint8_t *)&luks->header,
- sizeof(luks->header),
- opaque,
- errp);
- if (rv < 0) {
- ret = rv;
+ ret = qcrypto_block_luks_load_header(block, readfunc, opaque, errp);
+ if (ret) {
goto fail;
}
- /* 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);
- }
if (memcmp(luks->header.magic, qcrypto_block_luks_magic,
QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) {
@@ -1235,46 +1312,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
goto error;
}
- /* Everything on disk uses Big Endian, so flip header fields
- * before writing them */
- cpu_to_be16s(&luks->header.version);
- cpu_to_be32s(&luks->header.payload_offset_sector);
- cpu_to_be32s(&luks->header.master_key_len);
- cpu_to_be32s(&luks->header.master_key_iterations);
-
- for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
- cpu_to_be32s(&luks->header.key_slots[i].active);
- cpu_to_be32s(&luks->header.key_slots[i].iterations);
- cpu_to_be32s(&luks->header.key_slots[i].key_offset_sector);
- cpu_to_be32s(&luks->header.key_slots[i].stripes);
- }
-
-
- /* Write out the partition header and key slot headers */
- writefunc(block, 0,
- (const uint8_t *)&luks->header,
- sizeof(luks->header),
- opaque,
- &local_err);
-
- /* Delay checking local_err until we've byte-swapped */
-
- /* Byte swap the header back to native, in case we need
- * to read it again later */
- 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);
- }
-
- if (local_err) {
- error_propagate(errp, local_err);
+ if (qcrypto_block_luks_store_header(block, writefunc, opaque, errp)) {
goto error;
}
--
2.17.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v2 09/13] qcrypto-block: extract check and parse header
2019-08-26 13:50 [Qemu-devel] [PATCH v2 00/13] RFC crypto/luks: preparation for encryption key managment Maxim Levitsky
` (7 preceding siblings ...)
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 08/13] qcrypto-luks: extract store and load header Maxim Levitsky
@ 2019-08-26 13:50 ` Maxim Levitsky
2019-09-06 13:11 ` Daniel P. Berrangé
2019-08-26 13:51 ` [Qemu-devel] [PATCH v2 10/13] qcrypto-luks: refactoring: extract store key function Maxim Levitsky
` (3 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Maxim Levitsky @ 2019-08-26 13:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Daniel P. Berrangé,
qemu-block, Markus Armbruster, Max Reitz, Stefan Hajnoczi,
Maxim Levitsky
This is just to make qcrypto_block_luks_open more
reasonable in size.
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
crypto/block-luks.c | 254 +++++++++++++++++++++++++-------------------
1 file changed, 146 insertions(+), 108 deletions(-)
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index b4dc6fc899..cc9a52c9af 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -508,6 +508,148 @@ fail:
return ret;
}
+/*
+ * Does basic sanity checks on the LUKS header
+ */
+static int
+qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp)
+{
+ int ret;
+
+ if (memcmp(luks->header.magic, qcrypto_block_luks_magic,
+ QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) {
+ error_setg(errp, "Volume is not in LUKS format");
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ if (luks->header.version != QCRYPTO_BLOCK_LUKS_VERSION) {
+ error_setg(errp, "LUKS version %" PRIu32 " is not supported",
+ luks->header.version);
+ ret = -ENOTSUP;
+ goto fail;
+ }
+
+ return 0;
+fail:
+ return ret;
+}
+
+/*
+ * Parses the crypto parameters that are stored in the LUKS header
+ */
+
+static int
+qcrypto_block_luks_parse_header(QCryptoBlockLUKS *luks, Error **errp)
+{
+ g_autofree char *cipher_mode = g_strdup(luks->header.cipher_mode);
+ char *ivgen_name, *ivhash_name;
+ int ret = -1;
+ Error *local_err = NULL;
+
+ /*
+ * The cipher_mode header contains a string that we have
+ * to further parse, of the format
+ *
+ * <cipher-mode>-<iv-generator>[:<iv-hash>]
+ *
+ * eg cbc-essiv:sha256, cbc-plain64
+ */
+ ivgen_name = strchr(cipher_mode, '-');
+ if (!ivgen_name) {
+ ret = -EINVAL;
+ error_setg(errp, "Unexpected cipher mode string format %s",
+ luks->header.cipher_mode);
+ goto out;
+ }
+ *ivgen_name = '\0';
+ ivgen_name++;
+
+ ivhash_name = strchr(ivgen_name, ':');
+ if (!ivhash_name) {
+ luks->ivgen_hash_alg = 0;
+ } else {
+ *ivhash_name = '\0';
+ ivhash_name++;
+
+ luks->ivgen_hash_alg = qcrypto_block_luks_hash_name_lookup(ivhash_name,
+ &local_err);
+ if (local_err) {
+ ret = -ENOTSUP;
+ error_propagate(errp, local_err);
+ goto out;
+ }
+ }
+
+ luks->cipher_mode = qcrypto_block_luks_cipher_mode_lookup(cipher_mode,
+ &local_err);
+ if (local_err) {
+ ret = -ENOTSUP;
+ error_propagate(errp, local_err);
+ goto out;
+ }
+
+ luks->cipher_alg =
+ qcrypto_block_luks_cipher_name_lookup(luks->header.cipher_name,
+ luks->cipher_mode,
+ luks->header.master_key_len,
+ &local_err);
+ if (local_err) {
+ ret = -ENOTSUP;
+ error_propagate(errp, local_err);
+ goto out;
+ }
+
+ luks->hash_alg =
+ qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec,
+ &local_err);
+ if (local_err) {
+ ret = -ENOTSUP;
+ error_propagate(errp, local_err);
+ goto out;
+ }
+
+ luks->ivgen_alg = qcrypto_block_luks_ivgen_name_lookup(ivgen_name,
+ &local_err);
+ if (local_err) {
+ ret = -ENOTSUP;
+ error_propagate(errp, local_err);
+ goto out;
+ }
+
+ if (luks->ivgen_alg == QCRYPTO_IVGEN_ALG_ESSIV) {
+ if (!ivhash_name) {
+ ret = -EINVAL;
+ error_setg(errp, "Missing IV generator hash specification");
+ goto out;
+ }
+ luks->ivgen_cipher_alg =
+ qcrypto_block_luks_essiv_cipher(luks->cipher_alg,
+ luks->ivgen_hash_alg,
+ &local_err);
+ if (local_err) {
+ ret = -ENOTSUP;
+ error_propagate(errp, local_err);
+ goto out;
+ }
+ } else {
+
+ /*
+ * Note we parsed the ivhash_name earlier in the cipher_mode
+ * spec string even with plain/plain64 ivgens, but we
+ * will ignore it, since it is irrelevant for these ivgens.
+ * This is for compat with dm-crypt which will silently
+ * ignore hash names with these ivgens rather than report
+ * an error about the invalid usage
+ */
+ luks->ivgen_cipher_alg = luks->cipher_alg;
+ }
+ ret = 0;
+out:
+ return ret;
+
+}
+
/*
* Given a key slot, and user password, this will attempt to unlock
* the master encryption key from the key slot.
@@ -720,12 +862,9 @@ qcrypto_block_luks_open(QCryptoBlock *block,
Error **errp)
{
QCryptoBlockLUKS *luks = NULL;
- Error *local_err = NULL;
int ret = 0;
g_autofree uint8_t *masterkey = NULL;
- char *ivgen_name, *ivhash_name;
g_autofree char *password = NULL;
- g_autofree char *cipher_mode = NULL;
if (!(flags & QCRYPTO_BLOCK_OPEN_NO_IO)) {
if (!options->u.luks.key_secret) {
@@ -748,117 +887,16 @@ qcrypto_block_luks_open(QCryptoBlock *block,
goto fail;
}
-
- if (memcmp(luks->header.magic, qcrypto_block_luks_magic,
- QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) {
- error_setg(errp, "Volume is not in LUKS format");
- ret = -EINVAL;
- goto fail;
- }
- if (luks->header.version != QCRYPTO_BLOCK_LUKS_VERSION) {
- error_setg(errp, "LUKS version %" PRIu32 " is not supported",
- luks->header.version);
- ret = -ENOTSUP;
- goto fail;
- }
-
- cipher_mode = g_strdup(luks->header.cipher_mode);
-
- /*
- * The cipher_mode header contains a string that we have
- * to further parse, of the format
- *
- * <cipher-mode>-<iv-generator>[:<iv-hash>]
- *
- * eg cbc-essiv:sha256, cbc-plain64
- */
- ivgen_name = strchr(cipher_mode, '-');
- if (!ivgen_name) {
- ret = -EINVAL;
- error_setg(errp, "Unexpected cipher mode string format %s",
- cipher_mode);
- goto fail;
- }
- *ivgen_name = '\0';
- ivgen_name++;
-
- ivhash_name = strchr(ivgen_name, ':');
- if (!ivhash_name) {
- luks->ivgen_hash_alg = 0;
- } else {
- *ivhash_name = '\0';
- ivhash_name++;
-
- luks->ivgen_hash_alg = qcrypto_block_luks_hash_name_lookup(ivhash_name,
- &local_err);
- if (local_err) {
- ret = -ENOTSUP;
- error_propagate(errp, local_err);
- goto fail;
- }
- }
-
- luks->cipher_mode = qcrypto_block_luks_cipher_mode_lookup(cipher_mode,
- &local_err);
- if (local_err) {
- ret = -ENOTSUP;
- error_propagate(errp, local_err);
- goto fail;
- }
-
- luks->cipher_alg =
- qcrypto_block_luks_cipher_name_lookup(luks->header.cipher_name,
- luks->cipher_mode,
- luks->header.master_key_len,
- &local_err);
- if (local_err) {
- ret = -ENOTSUP;
- error_propagate(errp, local_err);
- goto fail;
- }
-
- luks->hash_alg =
- qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec,
- &local_err);
- if (local_err) {
- ret = -ENOTSUP;
- error_propagate(errp, local_err);
+ ret = qcrypto_block_luks_check_header(luks, errp);
+ if (ret) {
goto fail;
}
- luks->ivgen_alg = qcrypto_block_luks_ivgen_name_lookup(ivgen_name,
- &local_err);
- if (local_err) {
- ret = -ENOTSUP;
- error_propagate(errp, local_err);
+ ret = qcrypto_block_luks_parse_header(luks, errp);
+ if (ret) {
goto fail;
}
- if (luks->ivgen_alg == QCRYPTO_IVGEN_ALG_ESSIV) {
- if (!ivhash_name) {
- ret = -EINVAL;
- error_setg(errp, "Missing IV generator hash specification");
- goto fail;
- }
- luks->ivgen_cipher_alg =
- qcrypto_block_luks_essiv_cipher(luks->cipher_alg,
- luks->ivgen_hash_alg,
- &local_err);
- if (local_err) {
- ret = -ENOTSUP;
- error_propagate(errp, local_err);
- goto fail;
- }
- } else {
- /* Note we parsed the ivhash_name earlier in the cipher_mode
- * spec string even with plain/plain64 ivgens, but we
- * will ignore it, since it is irrelevant for these ivgens.
- * This is for compat with dm-crypt which will silently
- * ignore hash names with these ivgens rather than report
- * an error about the invalid usage
- */
- luks->ivgen_cipher_alg = luks->cipher_alg;
- }
if (!(flags & QCRYPTO_BLOCK_OPEN_NO_IO)) {
/* Try to find which key slot our password is valid for
--
2.17.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v2 10/13] qcrypto-luks: refactoring: extract store key function
2019-08-26 13:50 [Qemu-devel] [PATCH v2 00/13] RFC crypto/luks: preparation for encryption key managment Maxim Levitsky
` (8 preceding siblings ...)
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 09/13] qcrypto-block: extract check and parse header Maxim Levitsky
@ 2019-08-26 13:51 ` Maxim Levitsky
2019-09-06 13:14 ` Daniel P. Berrangé
2019-08-26 13:51 ` [Qemu-devel] [PATCH v2 11/13] qcrypto-luks: refactoring: simplify the math used for keyslot locations Maxim Levitsky
` (2 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Maxim Levitsky @ 2019-08-26 13:51 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Daniel P. Berrangé,
qemu-block, Markus Armbruster, Max Reitz, Stefan Hajnoczi,
Maxim Levitsky
This function will be used later to store
new keys to the luks metadata
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
crypto/block-luks.c | 310 ++++++++++++++++++++++++++------------------
1 file changed, 184 insertions(+), 126 deletions(-)
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index cc9a52c9af..d713125925 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -650,6 +650,176 @@ out:
}
+/*
+ * Given a key slot, user password, and the master key,
+ * will store the encrypted master key there, and update the
+ * in-memory header. User must then write the in-memory header
+ *
+ * Returns:
+ * 0 if the keyslot was written successfully
+ * with the provided password
+ * -1 if a fatal error occurred while storing the key
+ */
+static int
+qcrypto_block_luks_store_key(QCryptoBlock *block,
+ unsigned int slot_idx,
+ const char *password,
+ uint8_t *masterkey,
+ uint64_t iter_time,
+ QCryptoBlockWriteFunc writefunc,
+ void *opaque,
+ Error **errp)
+{
+ QCryptoBlockLUKS *luks = block->opaque;
+ QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[slot_idx];
+ g_autofree uint8_t *splitkey = NULL;
+ size_t splitkeylen;
+ g_autofree uint8_t *slotkey = NULL;
+ g_autoptr(QCryptoCipher) cipher = NULL;
+ g_autoptr(QCryptoIVGen) ivgen = NULL;
+ Error *local_err = NULL;
+ uint64_t iters;
+ int ret = -1;
+
+ if (qcrypto_random_bytes(slot->salt,
+ QCRYPTO_BLOCK_LUKS_SALT_LEN,
+ errp) < 0) {
+ goto cleanup;
+ }
+
+ splitkeylen = luks->header.master_key_len * slot->stripes;
+
+ /*
+ * Determine how many iterations are required to
+ * hash the user password while consuming 1 second of compute
+ * time
+ */
+ iters = qcrypto_pbkdf2_count_iters(luks->hash_alg,
+ (uint8_t *)password, strlen(password),
+ slot->salt,
+ QCRYPTO_BLOCK_LUKS_SALT_LEN,
+ luks->header.master_key_len,
+ &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ goto cleanup;
+ }
+
+ if (iters > (ULLONG_MAX / iter_time)) {
+ error_setg_errno(errp, ERANGE,
+ "PBKDF iterations %llu too large to scale",
+ (unsigned long long)iters);
+ goto cleanup;
+ }
+
+ /* iter_time was in millis, but count_iters reported for secs */
+ iters = iters * iter_time / 1000;
+
+ if (iters > UINT32_MAX) {
+ error_setg_errno(errp, ERANGE,
+ "PBKDF iterations %llu larger than %u",
+ (unsigned long long)iters, UINT32_MAX);
+ goto cleanup;
+ }
+
+ slot->iterations =
+ MAX(iters, QCRYPTO_BLOCK_LUKS_MIN_SLOT_KEY_ITERS);
+
+
+ /*
+ * Generate a key that we'll use to encrypt the master
+ * key, from the user's password
+ */
+ slotkey = g_new0(uint8_t, luks->header.master_key_len);
+ if (qcrypto_pbkdf2(luks->hash_alg,
+ (uint8_t *)password, strlen(password),
+ slot->salt,
+ QCRYPTO_BLOCK_LUKS_SALT_LEN,
+ slot->iterations,
+ slotkey, luks->header.master_key_len,
+ errp) < 0) {
+ goto cleanup;
+ }
+
+
+ /*
+ * Setup the encryption objects needed to encrypt the
+ * master key material
+ */
+ cipher = qcrypto_cipher_new(luks->cipher_alg,
+ luks->cipher_mode,
+ slotkey, luks->header.master_key_len,
+ errp);
+ if (!cipher) {
+ goto cleanup;
+ }
+
+ ivgen = qcrypto_ivgen_new(luks->ivgen_alg,
+ luks->ivgen_cipher_alg,
+ luks->ivgen_hash_alg,
+ slotkey, luks->header.master_key_len,
+ errp);
+ if (!ivgen) {
+ goto cleanup;
+ }
+
+ /*
+ * Before storing the master key, we need to vastly
+ * increase its size, as protection against forensic
+ * disk data recovery
+ */
+ splitkey = g_new0(uint8_t, splitkeylen);
+
+ if (qcrypto_afsplit_encode(luks->hash_alg,
+ luks->header.master_key_len,
+ slot->stripes,
+ masterkey,
+ splitkey,
+ errp) < 0) {
+ goto cleanup;
+ }
+
+ /*
+ * Now we encrypt the split master key with the key generated
+ * from the user's password, before storing it
+ */
+ if (qcrypto_block_cipher_encrypt_helper(cipher, block->niv, ivgen,
+ QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
+ 0,
+ splitkey,
+ splitkeylen,
+ errp) < 0) {
+ goto cleanup;
+ }
+
+ /* Write out the slot's master key material. */
+ if (writefunc(block,
+ slot->key_offset_sector *
+ QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
+ splitkey, splitkeylen,
+ opaque,
+ errp) != splitkeylen) {
+ goto cleanup;
+ }
+
+ slot->active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED;
+
+ if (qcrypto_block_luks_store_header(block, writefunc, opaque, errp)) {
+ goto cleanup;
+ }
+
+ ret = 0;
+
+cleanup:
+ if (slotkey) {
+ memset(slotkey, 0, luks->header.master_key_len);
+ }
+ if (splitkey) {
+ memset(splitkey, 0, splitkeylen);
+ }
+ return ret;
+}
+
/*
* Given a key slot, and user password, this will attempt to unlock
* the master encryption key from the key slot.
@@ -981,13 +1151,9 @@ qcrypto_block_luks_create(QCryptoBlock *block,
QCryptoBlockCreateOptionsLUKS luks_opts;
Error *local_err = NULL;
g_autofree uint8_t *masterkey = NULL;
- g_autofree uint8_t *slotkey = NULL;
- g_autofree uint8_t *splitkey = NULL;
size_t splitkeylen = 0;
size_t i;
- g_autoptr(QCryptoCipher) cipher = NULL;
- g_autoptr(QCryptoIVGen) ivgen = NULL;
- g_autofree char *password = NULL;
+ g_autofree char *password;
const char *cipher_alg;
const char *cipher_mode;
const char *ivgen_alg;
@@ -1209,9 +1375,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
* to use the first key slot */
splitkeylen = luks->header.master_key_len * QCRYPTO_BLOCK_LUKS_STRIPES;
for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
- luks->header.key_slots[i].active = i == 0 ?
- QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED :
- QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
+ luks->header.key_slots[i].active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
luks->header.key_slots[i].stripes = QCRYPTO_BLOCK_LUKS_STRIPES;
/* This calculation doesn't match that shown in the spec,
@@ -1225,107 +1389,6 @@ qcrypto_block_luks_create(QCryptoBlock *block,
QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) * i);
}
- if (qcrypto_random_bytes(luks->header.key_slots[0].salt,
- QCRYPTO_BLOCK_LUKS_SALT_LEN,
- errp) < 0) {
- goto error;
- }
-
- /* Again we determine how many iterations are required to
- * hash the user password while consuming 1 second of compute
- * time */
- iters = qcrypto_pbkdf2_count_iters(luks_opts.hash_alg,
- (uint8_t *)password, strlen(password),
- luks->header.key_slots[0].salt,
- QCRYPTO_BLOCK_LUKS_SALT_LEN,
- luks->header.master_key_len,
- &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- goto error;
- }
-
- if (iters > (ULLONG_MAX / luks_opts.iter_time)) {
- error_setg_errno(errp, ERANGE,
- "PBKDF iterations %llu too large to scale",
- (unsigned long long)iters);
- goto error;
- }
-
- /* iter_time was in millis, but count_iters reported for secs */
- iters = iters * luks_opts.iter_time / 1000;
-
- if (iters > UINT32_MAX) {
- error_setg_errno(errp, ERANGE,
- "PBKDF iterations %llu larger than %u",
- (unsigned long long)iters, UINT32_MAX);
- goto error;
- }
-
- luks->header.key_slots[0].iterations =
- MAX(iters, QCRYPTO_BLOCK_LUKS_MIN_SLOT_KEY_ITERS);
-
-
- /* Generate a key that we'll use to encrypt the master
- * key, from the user's password
- */
- slotkey = g_new0(uint8_t, luks->header.master_key_len);
- if (qcrypto_pbkdf2(luks_opts.hash_alg,
- (uint8_t *)password, strlen(password),
- luks->header.key_slots[0].salt,
- QCRYPTO_BLOCK_LUKS_SALT_LEN,
- luks->header.key_slots[0].iterations,
- slotkey, luks->header.master_key_len,
- errp) < 0) {
- goto error;
- }
-
-
- /* Setup the encryption objects needed to encrypt the
- * master key material
- */
- cipher = qcrypto_cipher_new(luks_opts.cipher_alg,
- luks_opts.cipher_mode,
- slotkey, luks->header.master_key_len,
- errp);
- if (!cipher) {
- goto error;
- }
-
- ivgen = qcrypto_ivgen_new(luks_opts.ivgen_alg,
- luks->ivgen_cipher_alg,
- luks_opts.ivgen_hash_alg,
- slotkey, luks->header.master_key_len,
- errp);
- if (!ivgen) {
- goto error;
- }
-
- /* Before storing the master key, we need to vastly
- * increase its size, as protection against forensic
- * disk data recovery */
- splitkey = g_new0(uint8_t, splitkeylen);
-
- if (qcrypto_afsplit_encode(luks_opts.hash_alg,
- luks->header.master_key_len,
- luks->header.key_slots[0].stripes,
- masterkey,
- splitkey,
- errp) < 0) {
- goto error;
- }
-
- /* Now we encrypt the split master key with the key generated
- * from the user's password, before storing it */
- if (qcrypto_block_cipher_encrypt_helper(cipher, block->niv, ivgen,
- QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
- 0,
- splitkey,
- splitkeylen,
- errp) < 0) {
- goto error;
- }
-
/* The total size of the LUKS headers is the partition header + key
* slot headers, rounded up to the nearest sector, combined with
@@ -1350,33 +1413,28 @@ qcrypto_block_luks_create(QCryptoBlock *block,
goto error;
}
- if (qcrypto_block_luks_store_header(block, writefunc, opaque, errp)) {
- goto error;
- }
- /* Write out the master key material, starting at the
- * sector immediately following the partition header. */
- if (writefunc(block,
- luks->header.key_slots[0].key_offset_sector *
- QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
- splitkey, splitkeylen,
- opaque,
- errp) != splitkeylen) {
+ /* populate the slot 0 with the password encrypted master key*/
+ /* This will also store the header */
+ if (qcrypto_block_luks_store_key(block,
+ 0,
+ password,
+ masterkey,
+ luks_opts.iter_time,
+ writefunc,
+ opaque,
+ errp)) {
goto error;
- }
+ }
- memset(masterkey, 0, luks->header.master_key_len);
- memset(slotkey, 0, luks->header.master_key_len);
+ memset(masterkey, 0, luks->header.master_key_len);
return 0;
error:
if (masterkey) {
memset(masterkey, 0, luks->header.master_key_len);
}
- if (slotkey) {
- memset(slotkey, 0, luks->header.master_key_len);
- }
qcrypto_block_free_cipher(block);
qcrypto_ivgen_free(block->ivgen);
--
2.17.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v2 11/13] qcrypto-luks: refactoring: simplify the math used for keyslot locations
2019-08-26 13:50 [Qemu-devel] [PATCH v2 00/13] RFC crypto/luks: preparation for encryption key managment Maxim Levitsky
` (9 preceding siblings ...)
2019-08-26 13:51 ` [Qemu-devel] [PATCH v2 10/13] qcrypto-luks: refactoring: extract store key function Maxim Levitsky
@ 2019-08-26 13:51 ` Maxim Levitsky
2019-09-06 13:17 ` Daniel P. Berrangé
2019-08-26 13:51 ` [Qemu-devel] [PATCH v2 12/13] qcrypto-luks: use g_autowipe Maxim Levitsky
2019-08-26 13:51 ` [Qemu-devel] [PATCH v2 13/13] qcrypto-luks: implement more rigorous header checking Maxim Levitsky
12 siblings, 1 reply; 30+ messages in thread
From: Maxim Levitsky @ 2019-08-26 13:51 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Daniel P. Berrangé,
qemu-block, Markus Armbruster, Max Reitz, Stefan Hajnoczi,
Maxim Levitsky
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
crypto/block-luks.c | 64 +++++++++++++++++++++++++++++----------------
1 file changed, 41 insertions(+), 23 deletions(-)
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index d713125925..6a43d97ce5 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -409,6 +409,32 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher,
}
}
+/*
+ * Returns number of sectors needed to store the key material
+ * given number of anti forensic stripes
+ */
+static int
+qcrypto_block_luks_splitkeylen_sectors(const QCryptoBlockLUKS *luks,
+ unsigned int stripes)
+{
+ /*
+ * This calculation doesn't match that shown in the spec,
+ * but instead follows the cryptsetup implementation.
+ */
+
+ size_t header_sectors = QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
+ QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
+
+ size_t splitkeylen = luks->header.master_key_len * stripes;
+
+ /* First align the key material size to block size*/
+ size_t splitkeylen_sectors =
+ DIV_ROUND_UP(splitkeylen, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE);
+
+ /* Then also align the key material size to the size of the header */
+ return ROUND_UP(splitkeylen_sectors, header_sectors);
+}
+
/*
* Stores the main LUKS header, taking care of endianess
*/
@@ -1151,7 +1177,8 @@ qcrypto_block_luks_create(QCryptoBlock *block,
QCryptoBlockCreateOptionsLUKS luks_opts;
Error *local_err = NULL;
g_autofree uint8_t *masterkey = NULL;
- size_t splitkeylen = 0;
+ size_t header_sectors;
+ size_t split_key_sectors;
size_t i;
g_autofree char *password;
const char *cipher_alg;
@@ -1370,37 +1397,28 @@ qcrypto_block_luks_create(QCryptoBlock *block,
goto error;
}
+ /* start with the sector that follows the header*/
+ header_sectors = QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
+ QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
+
+ split_key_sectors =
+ qcrypto_block_luks_splitkeylen_sectors(luks,
+ QCRYPTO_BLOCK_LUKS_STRIPES);
- /* Although LUKS has multiple key slots, we're just going
- * to use the first key slot */
- splitkeylen = luks->header.master_key_len * QCRYPTO_BLOCK_LUKS_STRIPES;
for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
- luks->header.key_slots[i].active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
- luks->header.key_slots[i].stripes = QCRYPTO_BLOCK_LUKS_STRIPES;
+ QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[i];
+ slot->active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
- /* This calculation doesn't match that shown in the spec,
- * but instead follows the cryptsetup implementation.
- */
- luks->header.key_slots[i].key_offset_sector =
- (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
- QCRYPTO_BLOCK_LUKS_SECTOR_SIZE) +
- (ROUND_UP(DIV_ROUND_UP(splitkeylen, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE),
- (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
- QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) * i);
+ slot->key_offset_sector = header_sectors + i * split_key_sectors;
+ slot->stripes = QCRYPTO_BLOCK_LUKS_STRIPES;
}
-
/* The total size of the LUKS headers is the partition header + key
* slot headers, rounded up to the nearest sector, combined with
* the size of each master key material region, also rounded up
* to the nearest sector */
- luks->header.payload_offset_sector =
- (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
- QCRYPTO_BLOCK_LUKS_SECTOR_SIZE) +
- (ROUND_UP(DIV_ROUND_UP(splitkeylen, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE),
- (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
- QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) *
- QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
+ luks->header.payload_offset_sector = header_sectors +
+ QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS * split_key_sectors;
block->sector_size = QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
block->payload_offset = luks->header.payload_offset_sector *
--
2.17.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v2 12/13] qcrypto-luks: use g_autowipe
2019-08-26 13:50 [Qemu-devel] [PATCH v2 00/13] RFC crypto/luks: preparation for encryption key managment Maxim Levitsky
` (10 preceding siblings ...)
2019-08-26 13:51 ` [Qemu-devel] [PATCH v2 11/13] qcrypto-luks: refactoring: simplify the math used for keyslot locations Maxim Levitsky
@ 2019-08-26 13:51 ` Maxim Levitsky
2019-08-26 13:51 ` [Qemu-devel] [PATCH v2 13/13] qcrypto-luks: implement more rigorous header checking Maxim Levitsky
12 siblings, 0 replies; 30+ messages in thread
From: Maxim Levitsky @ 2019-08-26 13:51 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Daniel P. Berrangé,
qemu-block, Markus Armbruster, Max Reitz, Stefan Hajnoczi,
Maxim Levitsky
This patch makes the luks crypto driver use the g_autowipe to erase the master keys,
and the passwords from the memory.
Note that this is not a complete solution, since these keys are also present in the
chipers, and in the secrets.
Some of them still can be erased, at least at driver instance close.
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
crypto/block-luks.c | 61 ++++++++++++++++-----------------------------
1 file changed, 22 insertions(+), 39 deletions(-)
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 6a43d97ce5..db0fb764b4 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -32,6 +32,7 @@
#include "qemu/uuid.h"
#include "qemu/coroutine.h"
+#include "autowipe.h"
/*
* Reference for the LUKS format implemented here is
@@ -698,19 +699,18 @@ qcrypto_block_luks_store_key(QCryptoBlock *block,
{
QCryptoBlockLUKS *luks = block->opaque;
QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[slot_idx];
- g_autofree uint8_t *splitkey = NULL;
+ g_autowipe uint8_t *splitkey = NULL;
size_t splitkeylen;
- g_autofree uint8_t *slotkey = NULL;
+ g_autowipe uint8_t *slotkey = NULL;
g_autoptr(QCryptoCipher) cipher = NULL;
g_autoptr(QCryptoIVGen) ivgen = NULL;
Error *local_err = NULL;
uint64_t iters;
- int ret = -1;
if (qcrypto_random_bytes(slot->salt,
QCRYPTO_BLOCK_LUKS_SALT_LEN,
errp) < 0) {
- goto cleanup;
+ return -1;
}
splitkeylen = luks->header.master_key_len * slot->stripes;
@@ -728,14 +728,14 @@ qcrypto_block_luks_store_key(QCryptoBlock *block,
&local_err);
if (local_err) {
error_propagate(errp, local_err);
- goto cleanup;
+ return -1;
}
if (iters > (ULLONG_MAX / iter_time)) {
error_setg_errno(errp, ERANGE,
"PBKDF iterations %llu too large to scale",
(unsigned long long)iters);
- goto cleanup;
+ return -1;
}
/* iter_time was in millis, but count_iters reported for secs */
@@ -745,7 +745,7 @@ qcrypto_block_luks_store_key(QCryptoBlock *block,
error_setg_errno(errp, ERANGE,
"PBKDF iterations %llu larger than %u",
(unsigned long long)iters, UINT32_MAX);
- goto cleanup;
+ return -1;
}
slot->iterations =
@@ -764,7 +764,7 @@ qcrypto_block_luks_store_key(QCryptoBlock *block,
slot->iterations,
slotkey, luks->header.master_key_len,
errp) < 0) {
- goto cleanup;
+ return -1;
}
@@ -777,7 +777,7 @@ qcrypto_block_luks_store_key(QCryptoBlock *block,
slotkey, luks->header.master_key_len,
errp);
if (!cipher) {
- goto cleanup;
+ return -1;
}
ivgen = qcrypto_ivgen_new(luks->ivgen_alg,
@@ -786,7 +786,7 @@ qcrypto_block_luks_store_key(QCryptoBlock *block,
slotkey, luks->header.master_key_len,
errp);
if (!ivgen) {
- goto cleanup;
+ return -1;
}
/*
@@ -802,7 +802,7 @@ qcrypto_block_luks_store_key(QCryptoBlock *block,
masterkey,
splitkey,
errp) < 0) {
- goto cleanup;
+ return -1;
}
/*
@@ -815,7 +815,7 @@ qcrypto_block_luks_store_key(QCryptoBlock *block,
splitkey,
splitkeylen,
errp) < 0) {
- goto cleanup;
+ return -1;
}
/* Write out the slot's master key material. */
@@ -825,25 +825,16 @@ qcrypto_block_luks_store_key(QCryptoBlock *block,
splitkey, splitkeylen,
opaque,
errp) != splitkeylen) {
- goto cleanup;
+ return -1;
}
slot->active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED;
if (qcrypto_block_luks_store_header(block, writefunc, opaque, errp)) {
- goto cleanup;
+ return -1;
}
- ret = 0;
-
-cleanup:
- if (slotkey) {
- memset(slotkey, 0, luks->header.master_key_len);
- }
- if (splitkey) {
- memset(splitkey, 0, splitkeylen);
- }
- return ret;
+ return 0;
}
/*
@@ -868,9 +859,9 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
{
QCryptoBlockLUKS *luks = block->opaque;
const QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[slot_idx];
- g_autofree uint8_t *splitkey = NULL;
+ g_autowipe uint8_t *splitkey = NULL;
size_t splitkeylen;
- g_autofree uint8_t *possiblekey = NULL;
+ g_autowipe uint8_t *possiblekey = NULL;
ssize_t rv;
g_autoptr(QCryptoCipher) cipher = NULL;
uint8_t keydigest[QCRYPTO_BLOCK_LUKS_DIGEST_LEN];
@@ -1059,8 +1050,8 @@ qcrypto_block_luks_open(QCryptoBlock *block,
{
QCryptoBlockLUKS *luks = NULL;
int ret = 0;
- g_autofree uint8_t *masterkey = NULL;
- g_autofree char *password = NULL;
+ g_autowipe uint8_t *masterkey = NULL;
+ g_autowipe char *password = NULL;
if (!(flags & QCRYPTO_BLOCK_OPEN_NO_IO)) {
if (!options->u.luks.key_secret) {
@@ -1151,6 +1142,7 @@ qcrypto_block_luks_open(QCryptoBlock *block,
fail:
qcrypto_block_free_cipher(block);
qcrypto_ivgen_free(block->ivgen);
+
g_free(luks);
return ret;
}
@@ -1176,11 +1168,11 @@ qcrypto_block_luks_create(QCryptoBlock *block,
QCryptoBlockLUKS *luks;
QCryptoBlockCreateOptionsLUKS luks_opts;
Error *local_err = NULL;
- g_autofree uint8_t *masterkey = NULL;
+ g_autowipe uint8_t *masterkey = NULL;
size_t header_sectors;
size_t split_key_sectors;
size_t i;
- g_autofree char *password;
+ g_autowipe char *password;
const char *cipher_alg;
const char *cipher_mode;
const char *ivgen_alg;
@@ -1445,23 +1437,14 @@ qcrypto_block_luks_create(QCryptoBlock *block,
goto error;
}
-
- memset(masterkey, 0, luks->header.master_key_len);
return 0;
-
error:
- if (masterkey) {
- memset(masterkey, 0, luks->header.master_key_len);
- }
-
qcrypto_block_free_cipher(block);
qcrypto_ivgen_free(block->ivgen);
-
g_free(luks);
return -1;
}
-
static int qcrypto_block_luks_get_info(QCryptoBlock *block,
QCryptoBlockInfo *info,
Error **errp)
--
2.17.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v2 13/13] qcrypto-luks: implement more rigorous header checking
2019-08-26 13:50 [Qemu-devel] [PATCH v2 00/13] RFC crypto/luks: preparation for encryption key managment Maxim Levitsky
` (11 preceding siblings ...)
2019-08-26 13:51 ` [Qemu-devel] [PATCH v2 12/13] qcrypto-luks: use g_autowipe Maxim Levitsky
@ 2019-08-26 13:51 ` Maxim Levitsky
2019-09-06 13:34 ` Daniel P. Berrangé
12 siblings, 1 reply; 30+ messages in thread
From: Maxim Levitsky @ 2019-08-26 13:51 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Daniel P. Berrangé,
qemu-block, Markus Armbruster, Max Reitz, Stefan Hajnoczi,
Maxim Levitsky
Check that keyslots don't overlap with the data,
and check that keyslots don't overlap with each other.
(this is done using naive O(n^2) nested loops,
but since there are just 8 keyslots, this doesn't really matter.
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
crypto/block-luks.c | 46 +++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 44 insertions(+), 2 deletions(-)
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index db0fb764b4..fdf4c41f8a 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -541,12 +541,12 @@ fail:
static int
qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp)
{
- int ret;
+ int ret = -EINVAL;
+ size_t i, j;
if (memcmp(luks->header.magic, qcrypto_block_luks_magic,
QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) {
error_setg(errp, "Volume is not in LUKS format");
- ret = -EINVAL;
goto fail;
}
@@ -557,6 +557,48 @@ qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp)
goto fail;
}
+ /* Check all keyslots for corruption */
+ for (i = 0 ; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; i++) {
+
+ const QCryptoBlockLUKSKeySlot *slot1 = &luks->header.key_slots[i];
+ unsigned int start1 = slot1->key_offset_sector;
+ unsigned int len1 =
+ qcrypto_block_luks_splitkeylen_sectors(luks, slot1->stripes);
+
+ if (slot1->stripes == 0) {
+ error_setg(errp, "Keyslot %zu is corrupted (stripes == 0)", i);
+ goto fail;
+ }
+
+ if (slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED &&
+ slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED) {
+ error_setg(errp,
+ "Keyslot %zu state (active/disable) is corrupted", i);
+ goto fail;
+ }
+
+ if (start1 + len1 > luks->header.payload_offset_sector) {
+ error_setg(errp,
+ "Keyslot %zu is overlapping with the encrypted payload",
+ i);
+ goto fail;
+ }
+
+ for (j = i + 1 ; j < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; j++) {
+ const QCryptoBlockLUKSKeySlot *slot2 = &luks->header.key_slots[j];
+ unsigned int start2 = slot2->key_offset_sector;
+ unsigned int len2 =
+ qcrypto_block_luks_splitkeylen_sectors(luks, slot2->stripes);
+
+ if (start1 + len1 > start2 && start2 + len2 > start1) {
+ error_setg(errp,
+ "Keyslots %zu and %zu are overlapping in the header",
+ i, j);
+ goto fail;
+ }
+ }
+
+ }
return 0;
fail:
return ret;
--
2.17.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v2 01/13] introduce g_autowipe
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 01/13] introduce g_autowipe Maxim Levitsky
@ 2019-08-27 10:46 ` Tony Nguyen
2019-08-27 10:52 ` Daniel P. Berrangé
1 sibling, 0 replies; 30+ messages in thread
From: Tony Nguyen @ 2019-08-27 10:46 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, Fam Zheng, Daniel P. Berrangé,
qemu-block, qemu-devel, Markus Armbruster, Stefan Hajnoczi,
Max Reitz
Hi Maxim,
On Mon, Aug 26, 2019 at 04:50:51PM +0300, Maxim Levitsky wrote:
> 2. is it portable enought for us to use malloc_usable_size
> to get the size of malloced pointer in the autofree callback?
> This function is aviable in glibc (but no wrapper in glib).
We will also need to consider host portability: malloc_usable_size
for Linux, malloc_size for MacOS, _msize for Windows, etc.
[...]
> +#include <stddef.h>
> +#include <malloc.h>
Again host portability, <malloc.h> is not present on MacOS.
Regards,
Tony
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v2 01/13] introduce g_autowipe
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 01/13] introduce g_autowipe Maxim Levitsky
2019-08-27 10:46 ` Tony Nguyen
@ 2019-08-27 10:52 ` Daniel P. Berrangé
2019-08-27 11:24 ` Maxim Levitsky
1 sibling, 1 reply; 30+ messages in thread
From: Daniel P. Berrangé @ 2019-08-27 10:52 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, Fam Zheng, qemu-block, Markus Armbruster, qemu-devel,
Max Reitz, Stefan Hajnoczi
On Mon, Aug 26, 2019 at 04:50:51PM +0300, Maxim Levitsky wrote:
> Marking a pointer with g_autowipe, will
> not only free it at the scope exit, but also
> erase the data it points to just prior to freeing it.
>
> This is first attempt to implement this feature,
> as suggested by Daniel and Nir.
>
> The things that need to be verified prior to merging this is
>
> 1. Can we just always use memset_s (defined in C++)
> or some alternative.
>
> 2. is it portable enought for us to use malloc_usable_size
> to get the size of malloced pointer in the autofree callback?
> This function is aviable in glibc (but no wrapper in glib).
Urgh, no, we can't rely on that.
I completely forgot that we would need to know the size during
the deallocate function. The portable way to deal with this
will be to change all our code that handles passwords to use
GString instead, since that is a struct carrying around the
allocated size.
As mentioned in v1, I'm fine if you just let this series use
memset as this is a pre-existing problem & we can fix it
in separate yseries.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v2 01/13] introduce g_autowipe
2019-08-27 10:52 ` Daniel P. Berrangé
@ 2019-08-27 11:24 ` Maxim Levitsky
0 siblings, 0 replies; 30+ messages in thread
From: Maxim Levitsky @ 2019-08-27 11:24 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Kevin Wolf, Fam Zheng, qemu-block, qemu-devel, Markus Armbruster,
Max Reitz, Stefan Hajnoczi
On Tue, 2019-08-27 at 11:52 +0100, Daniel P. Berrangé wrote:
> On Mon, Aug 26, 2019 at 04:50:51PM +0300, Maxim Levitsky wrote:
> > Marking a pointer with g_autowipe, will
> > not only free it at the scope exit, but also
> > erase the data it points to just prior to freeing it.
> >
> > This is first attempt to implement this feature,
> > as suggested by Daniel and Nir.
> >
> > The things that need to be verified prior to merging this is
> >
> > 1. Can we just always use memset_s (defined in C++)
> > or some alternative.
> >
> > 2. is it portable enought for us to use malloc_usable_size
> > to get the size of malloced pointer in the autofree callback?
> > This function is aviable in glibc (but no wrapper in glib).
>
> Urgh, no, we can't rely on that.
>
> I completely forgot that we would need to know the size during
> the deallocate function. The portable way to deal with this
> will be to change all our code that handles passwords to use
> GString instead, since that is a struct carrying around the
> allocated size.
>
> As mentioned in v1, I'm fine if you just let this series use
> memset as this is a pre-existing problem & we can fix it
> in separate yseries.
All right, I *was* afraid of that, but I guess it was worth a try anyway.
So I think I'll keep that patch that adds few missing memsets,
just to consistency/documentation purposes since anyway
in addtion to these there are lot of other places that keys are kept,
like the ciphers itself, secrets (which aren't even freed usually
as long as VM is running)
The purpose was that I just that memsetting caught my eye and
I wanted to make it at least consistent.
Thanks for the review,
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v2 03/13] qcrypto-luks: rename some fields in QCryptoBlockLUKSHeader
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 03/13] qcrypto-luks: rename some fields in QCryptoBlockLUKSHeader Maxim Levitsky
@ 2019-09-06 12:27 ` Daniel P. Berrangé
0 siblings, 0 replies; 30+ messages in thread
From: Daniel P. Berrangé @ 2019-09-06 12:27 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, Fam Zheng, qemu-block, Markus Armbruster, qemu-devel,
Max Reitz, Stefan Hajnoczi
On Mon, Aug 26, 2019 at 04:50:53PM +0300, Maxim Levitsky wrote:
> * key_bytes -> master_key_len
> * payload_offset = payload_offset_sector (to emphasise that this isn't byte offset)
> * key_offset -> key_offset_sector - same as above for luks slots
>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
> crypto/block-luks.c | 91 +++++++++++++++++++++++----------------------
> 1 file changed, 47 insertions(+), 44 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v2 04/13] qcrypto-luks: don't overwrite cipher_mode in header
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 04/13] qcrypto-luks: don't overwrite cipher_mode in header Maxim Levitsky
@ 2019-09-06 12:29 ` Daniel P. Berrangé
0 siblings, 0 replies; 30+ messages in thread
From: Daniel P. Berrangé @ 2019-09-06 12:29 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, Fam Zheng, qemu-block, Markus Armbruster, qemu-devel,
Max Reitz, Stefan Hajnoczi
On Mon, Aug 26, 2019 at 04:50:54PM +0300, Maxim Levitsky wrote:
> This way we can store the header we loaded, which
> will be used in key management code
>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
> crypto/block-luks.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index f12fa2d270..e9ae3f6baa 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -645,6 +645,7 @@ qcrypto_block_luks_open(QCryptoBlock *block,
> QCryptoHashAlgorithm hash;
> QCryptoHashAlgorithm ivhash;
> g_autofree char *password = NULL;
> + g_autofree char *cipher_mode = NULL;
>
> if (!(flags & QCRYPTO_BLOCK_OPEN_NO_IO)) {
> if (!options->u.luks.key_secret) {
> @@ -701,6 +702,8 @@ qcrypto_block_luks_open(QCryptoBlock *block,
> goto fail;
> }
>
> + cipher_mode = g_strdup(luks->header.cipher_mode);
> +
> /*
> * The cipher_mode header contains a string that we have
> * to further parse, of the format
> @@ -709,11 +712,11 @@ qcrypto_block_luks_open(QCryptoBlock *block,
> *
> * eg cbc-essiv:sha256, cbc-plain64
> */
> - ivgen_name = strchr(luks->header.cipher_mode, '-');
> + ivgen_name = strchr(cipher_mode, '-');
> if (!ivgen_name) {
> ret = -EINVAL;
> error_setg(errp, "Unexpected cipher mode string format %s",
> - luks->header.cipher_mode);
> + cipher_mode);
nitpick - align with the (
> goto fail;
> }
> *ivgen_name = '\0';
> @@ -735,7 +738,7 @@ qcrypto_block_luks_open(QCryptoBlock *block,
> }
> }
>
> - ciphermode = qcrypto_block_luks_cipher_mode_lookup(luks->header.cipher_mode,
> + ciphermode = qcrypto_block_luks_cipher_mode_lookup(cipher_mode,
> &local_err);
> if (local_err) {
> ret = -ENOTSUP;
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v2 05/13] qcrypto-luks: simplify masterkey and masterkey length
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 05/13] qcrypto-luks: simplify masterkey and masterkey length Maxim Levitsky
@ 2019-09-06 12:30 ` Daniel P. Berrangé
0 siblings, 0 replies; 30+ messages in thread
From: Daniel P. Berrangé @ 2019-09-06 12:30 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, Fam Zheng, qemu-block, Markus Armbruster, qemu-devel,
Max Reitz, Stefan Hajnoczi
On Mon, Aug 26, 2019 at 04:50:55PM +0300, Maxim Levitsky wrote:
> Let the caller allocate masterkey
> Always use master key len from the header
>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
> crypto/block-luks.c | 44 +++++++++++++++++++++-----------------------
> 1 file changed, 21 insertions(+), 23 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v2 06/13] qcrypto-block: pass keyslot index rather that pointer to the keyslot
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 06/13] qcrypto-block: pass keyslot index rather that pointer to the keyslot Maxim Levitsky
@ 2019-09-06 12:32 ` Daniel P. Berrangé
0 siblings, 0 replies; 30+ messages in thread
From: Daniel P. Berrangé @ 2019-09-06 12:32 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, Fam Zheng, qemu-block, Markus Armbruster, qemu-devel,
Max Reitz, Stefan Hajnoczi
On Mon, Aug 26, 2019 at 04:50:56PM +0300, Maxim Levitsky wrote:
> Another minor refactoring
>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
> 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 331377293d..0d81f2ac61 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -410,7 +410,7 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher,
> */
> static int
> qcrypto_block_luks_load_key(QCryptoBlock *block,
> - QCryptoBlockLUKSKeySlot *slot,
> + unsigned int slot_idx,
FWIW, I tend to prefer 'size_t' for things which are array
indexes especially....
> const char *password,
> QCryptoCipherAlgorithm cipheralg,
> QCryptoCipherMode ciphermode,
> @@ -424,6 +424,7 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
> Error **errp)
> {
> QCryptoBlockLUKS *luks = block->opaque;
> + const QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[slot_idx];
> g_autofree uint8_t *splitkey = NULL;
> size_t splitkeylen;
> g_autofree uint8_t *possiblekey = NULL;
> @@ -580,13 +581,12 @@ qcrypto_block_luks_find_key(QCryptoBlock *block,
> void *opaque,
> Error **errp)
> {
> - QCryptoBlockLUKS *luks = block->opaque;
> size_t i;
> int rv;
>
> for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> rv = qcrypto_block_luks_load_key(block,
> - &luks->header.key_slots[i],
> + i,
..given that 'i' is size_t here.
> password,
> cipheralg,
> ciphermode,
With that type changed
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v2 07/13] qcrypto-luks: use the parsed encryption settings in QCryptoBlockLUKS
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 07/13] qcrypto-luks: use the parsed encryption settings in QCryptoBlockLUKS Maxim Levitsky
@ 2019-09-06 12:35 ` Daniel P. Berrangé
0 siblings, 0 replies; 30+ messages in thread
From: Daniel P. Berrangé @ 2019-09-06 12:35 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, Fam Zheng, qemu-block, Markus Armbruster, qemu-devel,
Max Reitz, Stefan Hajnoczi
On Mon, Aug 26, 2019 at 04:50:57PM +0300, Maxim Levitsky wrote:
> Prior to that patch, the parsed encryptio settings
^^encryption
> were alrady stored into the QCryptoBlockLUKS but not
^^already
> used anywhere but in qcrypto_block_luks_get_info
>
> Using them simplifies the code
>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
> crypto/block-luks.c | 169 +++++++++++++++++++++-----------------------
> 1 file changed, 79 insertions(+), 90 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v2 08/13] qcrypto-luks: extract store and load header
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 08/13] qcrypto-luks: extract store and load header Maxim Levitsky
@ 2019-09-06 13:06 ` Daniel P. Berrangé
0 siblings, 0 replies; 30+ messages in thread
From: Daniel P. Berrangé @ 2019-09-06 13:06 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, Fam Zheng, qemu-block, Markus Armbruster, qemu-devel,
Max Reitz, Stefan Hajnoczi
On Mon, Aug 26, 2019 at 04:50:58PM +0300, Maxim Levitsky wrote:
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
> crypto/block-luks.c | 166 +++++++++++++++++++++++++++-----------------
> 1 file changed, 102 insertions(+), 64 deletions(-)
>
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index cad65ae0aa..b4dc6fc899 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -409,6 +409,105 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher,
> }
> }
>
> +/*
> + * Stores the main LUKS header, taking care of endianess
> + */
> +static int
> +qcrypto_block_luks_store_header(QCryptoBlock *block,
> + QCryptoBlockWriteFunc writefunc,
> + void *opaque,
> + Error **errp)
> +{
> + const QCryptoBlockLUKS *luks = block->opaque;
> + Error *local_err = NULL;
> + size_t i;
> + QCryptoBlockLUKSHeader *hdr_copy;
Initialize to NULL and mark with g_autofree
> +
> + /* 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);
> + }
> +
> + /* Write out the partition header and key slot headers */
> + writefunc(block, 0, (const uint8_t *)hdr_copy, sizeof(*hdr_copy),
> + opaque, &local_err);
> +
> + g_free(hdr_copy);
And then this can be removed
> +
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return -1;
> + }
> + return 0;
> +}
> +
> +/*
> + * Loads the main LUKS header,and byteswaps it to native endianess
> + * And run basic sanity checks on it
> + */
> +static int
> +qcrypto_block_luks_load_header(QCryptoBlock *block,
> + QCryptoBlockReadFunc readfunc,
> + void *opaque,
> + Error **errp)
> +{
> + ssize_t rv;
> + size_t i;
> + int ret = 0;
> + QCryptoBlockLUKS *luks = block->opaque;
> +
> + /*
> + * Read the entire LUKS header, minus the key material from
> + * the underlying device
> + */
> +
> + rv = readfunc(block, 0,
> + (uint8_t *)&luks->header,
> + sizeof(luks->header),
> + opaque,
> + errp);
> + if (rv < 0) {
> + ret = rv;
> + goto fail;
Nothing happens at the fail: label, so you can just 'return rv'
straightaway IMHO
> + }
> +
> + /*
> + * 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);
> + }
> +
> +
> + return 0;
> +fail:
> + return ret;
> +}
> - /* Read the entire LUKS header, minus the key material from
> - * the underlying device */
> - rv = readfunc(block, 0,
> - (uint8_t *)&luks->header,
> - sizeof(luks->header),
> - opaque,
> - errp);
> - if (rv < 0) {
> - ret = rv;
> + ret = qcrypto_block_luks_load_header(block, readfunc, opaque, errp);
> + if (ret) {
if (ret < 0)
> goto fail;
> }
>
> - /* 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);
> - }
>
> if (memcmp(luks->header.magic, qcrypto_block_luks_magic,
> QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) {
> @@ -1235,46 +1312,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> goto error;
> }
> - if (local_err) {
> - error_propagate(errp, local_err);
> + if (qcrypto_block_luks_store_header(block, writefunc, opaque, errp)) {
The comparison should be "< 0"
> goto error;
> }
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v2 09/13] qcrypto-block: extract check and parse header
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 09/13] qcrypto-block: extract check and parse header Maxim Levitsky
@ 2019-09-06 13:11 ` Daniel P. Berrangé
2019-09-12 7:24 ` Maxim Levitsky
0 siblings, 1 reply; 30+ messages in thread
From: Daniel P. Berrangé @ 2019-09-06 13:11 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, Fam Zheng, qemu-block, Markus Armbruster, qemu-devel,
Max Reitz, Stefan Hajnoczi
On Mon, Aug 26, 2019 at 04:50:59PM +0300, Maxim Levitsky wrote:
> This is just to make qcrypto_block_luks_open more
> reasonable in size.
>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
> crypto/block-luks.c | 254 +++++++++++++++++++++++++-------------------
> 1 file changed, 146 insertions(+), 108 deletions(-)
>
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index b4dc6fc899..cc9a52c9af 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -508,6 +508,148 @@ fail:
> return ret;
> }
>
> +/*
> + * Does basic sanity checks on the LUKS header
> + */
> +static int
> +qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp)
> +{
> + int ret;
> +
> + if (memcmp(luks->header.magic, qcrypto_block_luks_magic,
> + QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) {
> + error_setg(errp, "Volume is not in LUKS format");
> + ret = -EINVAL;
> + goto fail;
> + }
Just 'return -1' here immediately - don't return an errno, as we're
using Error objects for reporting.
> +
> + if (luks->header.version != QCRYPTO_BLOCK_LUKS_VERSION) {
> + error_setg(errp, "LUKS version %" PRIu32 " is not supported",
> + luks->header.version);
> + ret = -ENOTSUP;
> + goto fail;
> + }
> +
> + return 0;
> +fail:
> + return ret;
> +}
> +
> +/*
> + * Parses the crypto parameters that are stored in the LUKS header
> + */
> +
> +static int
> +qcrypto_block_luks_parse_header(QCryptoBlockLUKS *luks, Error **errp)
> +{
> + g_autofree char *cipher_mode = g_strdup(luks->header.cipher_mode);
> + char *ivgen_name, *ivhash_name;
> + int ret = -1;
> + Error *local_err = NULL;
> +
> + /*
> + * The cipher_mode header contains a string that we have
> + * to further parse, of the format
> + *
> + * <cipher-mode>-<iv-generator>[:<iv-hash>]
> + *
> + * eg cbc-essiv:sha256, cbc-plain64
> + */
> + ivgen_name = strchr(cipher_mode, '-');
> + if (!ivgen_name) {
> + ret = -EINVAL;
Again, don't use errnos - just return -1 in this method.
> + error_setg(errp, "Unexpected cipher mode string format %s",
> + luks->header.cipher_mode);
> + goto out;
> + }
> + *ivgen_name = '\0';
> + ivgen_name++;
> +
> + ivhash_name = strchr(ivgen_name, ':');
> + if (!ivhash_name) {
> + luks->ivgen_hash_alg = 0;
> + } else {
> + *ivhash_name = '\0';
> + ivhash_name++;
> +
> + luks->ivgen_hash_alg = qcrypto_block_luks_hash_name_lookup(ivhash_name,
> + &local_err);
> + if (local_err) {
> + ret = -ENOTSUP;
> + error_propagate(errp, local_err);
> + goto out;
> + }
> + }
> +
> + luks->cipher_mode = qcrypto_block_luks_cipher_mode_lookup(cipher_mode,
> + &local_err);
> + if (local_err) {
> + ret = -ENOTSUP;
> + error_propagate(errp, local_err);
> + goto out;
> + }
> +
> + luks->cipher_alg =
> + qcrypto_block_luks_cipher_name_lookup(luks->header.cipher_name,
> + luks->cipher_mode,
> + luks->header.master_key_len,
> + &local_err);
> + if (local_err) {
> + ret = -ENOTSUP;
> + error_propagate(errp, local_err);
> + goto out;
> + }
> +
> + luks->hash_alg =
> + qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec,
> + &local_err);
> + if (local_err) {
> + ret = -ENOTSUP;
> + error_propagate(errp, local_err);
> + goto out;
> + }
> +
> + luks->ivgen_alg = qcrypto_block_luks_ivgen_name_lookup(ivgen_name,
> + &local_err);
> + if (local_err) {
> + ret = -ENOTSUP;
> + error_propagate(errp, local_err);
> + goto out;
> + }
> +
> + if (luks->ivgen_alg == QCRYPTO_IVGEN_ALG_ESSIV) {
> + if (!ivhash_name) {
> + ret = -EINVAL;
> + error_setg(errp, "Missing IV generator hash specification");
> + goto out;
> + }
> + luks->ivgen_cipher_alg =
> + qcrypto_block_luks_essiv_cipher(luks->cipher_alg,
> + luks->ivgen_hash_alg,
> + &local_err);
> + if (local_err) {
> + ret = -ENOTSUP;
> + error_propagate(errp, local_err);
> + goto out;
> + }
> + } else {
> +
> + /*
> + * Note we parsed the ivhash_name earlier in the cipher_mode
> + * spec string even with plain/plain64 ivgens, but we
> + * will ignore it, since it is irrelevant for these ivgens.
> + * This is for compat with dm-crypt which will silently
> + * ignore hash names with these ivgens rather than report
> + * an error about the invalid usage
> + */
> + luks->ivgen_cipher_alg = luks->cipher_alg;
> + }
> + ret = 0;
> +out:
> + return ret;
> +
> +}
> +
> /*
> * Given a key slot, and user password, this will attempt to unlock
> * the master encryption key from the key slot.
> @@ -720,12 +862,9 @@ qcrypto_block_luks_open(QCryptoBlock *block,
> Error **errp)
> {
> QCryptoBlockLUKS *luks = NULL;
> - Error *local_err = NULL;
> int ret = 0;
> g_autofree uint8_t *masterkey = NULL;
> - char *ivgen_name, *ivhash_name;
> g_autofree char *password = NULL;
> - g_autofree char *cipher_mode = NULL;
>
> if (!(flags & QCRYPTO_BLOCK_OPEN_NO_IO)) {
> if (!options->u.luks.key_secret) {
> @@ -748,117 +887,16 @@ qcrypto_block_luks_open(QCryptoBlock *block,
> goto fail;
> }
>
> -
> - if (memcmp(luks->header.magic, qcrypto_block_luks_magic,
> - QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) {
> - error_setg(errp, "Volume is not in LUKS format");
> - ret = -EINVAL;
I can't remember why I set ret to an errno in my original code.
it is entirely pointless, as the caller of this method merely
checks "ret < 0" and doesn't do anything else with the return
value. IOW, we should just purge the errnors from this existing
code entirely.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v2 10/13] qcrypto-luks: refactoring: extract store key function
2019-08-26 13:51 ` [Qemu-devel] [PATCH v2 10/13] qcrypto-luks: refactoring: extract store key function Maxim Levitsky
@ 2019-09-06 13:14 ` Daniel P. Berrangé
0 siblings, 0 replies; 30+ messages in thread
From: Daniel P. Berrangé @ 2019-09-06 13:14 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, Fam Zheng, qemu-block, Markus Armbruster, qemu-devel,
Max Reitz, Stefan Hajnoczi
On Mon, Aug 26, 2019 at 04:51:00PM +0300, Maxim Levitsky wrote:
> This function will be used later to store
> new keys to the luks metadata
>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
> crypto/block-luks.c | 310 ++++++++++++++++++++++++++------------------
> 1 file changed, 184 insertions(+), 126 deletions(-)
>
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index cc9a52c9af..d713125925 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -650,6 +650,176 @@ out:
>
> }
>
> +/*
> + * Given a key slot, user password, and the master key,
> + * will store the encrypted master key there, and update the
> + * in-memory header. User must then write the in-memory header
> + *
> + * Returns:
> + * 0 if the keyslot was written successfully
> + * with the provided password
> + * -1 if a fatal error occurred while storing the key
> + */
> +static int
> +qcrypto_block_luks_store_key(QCryptoBlock *block,
> + unsigned int slot_idx,
> + const char *password,
> + uint8_t *masterkey,
> + uint64_t iter_time,
> + QCryptoBlockWriteFunc writefunc,
> + void *opaque,
> + Error **errp)
> +{
> + QCryptoBlockLUKS *luks = block->opaque;
> + QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[slot_idx];
> + g_autofree uint8_t *splitkey = NULL;
> + size_t splitkeylen;
> + g_autofree uint8_t *slotkey = NULL;
> + g_autoptr(QCryptoCipher) cipher = NULL;
> + g_autoptr(QCryptoIVGen) ivgen = NULL;
> + Error *local_err = NULL;
> + uint64_t iters;
> + int ret = -1;
> +
> + if (qcrypto_random_bytes(slot->salt,
> + QCRYPTO_BLOCK_LUKS_SALT_LEN,
> + errp) < 0) {
> + goto cleanup;
> + }
> +
> + splitkeylen = luks->header.master_key_len * slot->stripes;
> +
> + /*
> + * Determine how many iterations are required to
> + * hash the user password while consuming 1 second of compute
> + * time
> + */
> + iters = qcrypto_pbkdf2_count_iters(luks->hash_alg,
> + (uint8_t *)password, strlen(password),
> + slot->salt,
> + QCRYPTO_BLOCK_LUKS_SALT_LEN,
> + luks->header.master_key_len,
> + &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + goto cleanup;
> + }
> +
> + if (iters > (ULLONG_MAX / iter_time)) {
> + error_setg_errno(errp, ERANGE,
> + "PBKDF iterations %llu too large to scale",
> + (unsigned long long)iters);
> + goto cleanup;
> + }
> +
> + /* iter_time was in millis, but count_iters reported for secs */
> + iters = iters * iter_time / 1000;
> +
> + if (iters > UINT32_MAX) {
> + error_setg_errno(errp, ERANGE,
> + "PBKDF iterations %llu larger than %u",
> + (unsigned long long)iters, UINT32_MAX);
> + goto cleanup;
> + }
> +
> + slot->iterations =
> + MAX(iters, QCRYPTO_BLOCK_LUKS_MIN_SLOT_KEY_ITERS);
> +
> +
> + /*
> + * Generate a key that we'll use to encrypt the master
> + * key, from the user's password
> + */
> + slotkey = g_new0(uint8_t, luks->header.master_key_len);
> + if (qcrypto_pbkdf2(luks->hash_alg,
> + (uint8_t *)password, strlen(password),
> + slot->salt,
> + QCRYPTO_BLOCK_LUKS_SALT_LEN,
> + slot->iterations,
> + slotkey, luks->header.master_key_len,
> + errp) < 0) {
> + goto cleanup;
> + }
> +
> +
> + /*
> + * Setup the encryption objects needed to encrypt the
> + * master key material
> + */
> + cipher = qcrypto_cipher_new(luks->cipher_alg,
> + luks->cipher_mode,
> + slotkey, luks->header.master_key_len,
> + errp);
> + if (!cipher) {
> + goto cleanup;
> + }
> +
> + ivgen = qcrypto_ivgen_new(luks->ivgen_alg,
> + luks->ivgen_cipher_alg,
> + luks->ivgen_hash_alg,
> + slotkey, luks->header.master_key_len,
> + errp);
> + if (!ivgen) {
> + goto cleanup;
> + }
> +
> + /*
> + * Before storing the master key, we need to vastly
> + * increase its size, as protection against forensic
> + * disk data recovery
> + */
> + splitkey = g_new0(uint8_t, splitkeylen);
> +
> + if (qcrypto_afsplit_encode(luks->hash_alg,
> + luks->header.master_key_len,
> + slot->stripes,
> + masterkey,
> + splitkey,
> + errp) < 0) {
> + goto cleanup;
> + }
> +
> + /*
> + * Now we encrypt the split master key with the key generated
> + * from the user's password, before storing it
> + */
> + if (qcrypto_block_cipher_encrypt_helper(cipher, block->niv, ivgen,
> + QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
> + 0,
> + splitkey,
> + splitkeylen,
> + errp) < 0) {
> + goto cleanup;
> + }
> +
> + /* Write out the slot's master key material. */
> + if (writefunc(block,
> + slot->key_offset_sector *
> + QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
> + splitkey, splitkeylen,
> + opaque,
> + errp) != splitkeylen) {
> + goto cleanup;
> + }
> +
> + slot->active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED;
> +
> + if (qcrypto_block_luks_store_header(block, writefunc, opaque, errp)) {
Check < 0
> + goto cleanup;
> + }
> +
> + ret = 0;
> +
> +cleanup:
> + if (slotkey) {
> + memset(slotkey, 0, luks->header.master_key_len);
> + }
> + if (splitkey) {
> + memset(splitkey, 0, splitkeylen);
> + }
> + return ret;
> +}
> +
> + /* populate the slot 0 with the password encrypted master key*/
> + /* This will also store the header */
> + if (qcrypto_block_luks_store_key(block,
> + 0,
> + password,
> + masterkey,
> + luks_opts.iter_time,
> + writefunc,
> + opaque,
> + errp)) {
> goto error;
Check < 0
> - }
> + }
Indent off by one
>
> - memset(masterkey, 0, luks->header.master_key_len);
> - memset(slotkey, 0, luks->header.master_key_len);
>
> + memset(masterkey, 0, luks->header.master_key_len);
> return 0;
The blank line was moved by accident.
>
> error:
> if (masterkey) {
> memset(masterkey, 0, luks->header.master_key_len);
> }
> - if (slotkey) {
> - memset(slotkey, 0, luks->header.master_key_len);
> - }
>
>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v2 11/13] qcrypto-luks: refactoring: simplify the math used for keyslot locations
2019-08-26 13:51 ` [Qemu-devel] [PATCH v2 11/13] qcrypto-luks: refactoring: simplify the math used for keyslot locations Maxim Levitsky
@ 2019-09-06 13:17 ` Daniel P. Berrangé
2019-09-12 7:40 ` Maxim Levitsky
0 siblings, 1 reply; 30+ messages in thread
From: Daniel P. Berrangé @ 2019-09-06 13:17 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, Fam Zheng, qemu-block, Markus Armbruster, qemu-devel,
Max Reitz, Stefan Hajnoczi
On Mon, Aug 26, 2019 at 04:51:01PM +0300, Maxim Levitsky wrote:
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
> crypto/block-luks.c | 64 +++++++++++++++++++++++++++++----------------
> 1 file changed, 41 insertions(+), 23 deletions(-)
>
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index d713125925..6a43d97ce5 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -409,6 +409,32 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher,
> }
> }
>
> +/*
> + * Returns number of sectors needed to store the key material
> + * given number of anti forensic stripes
> + */
> +static int
> +qcrypto_block_luks_splitkeylen_sectors(const QCryptoBlockLUKS *luks,
> + unsigned int stripes)
> +{
> + /*
> + * This calculation doesn't match that shown in the spec,
> + * but instead follows the cryptsetup implementation.
> + */
> +
> + size_t header_sectors = QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> + QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
The caller already calculated that so just pass it in
> +
> + size_t splitkeylen = luks->header.master_key_len * stripes;
> +
> + /* First align the key material size to block size*/
> + size_t splitkeylen_sectors =
> + DIV_ROUND_UP(splitkeylen, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE);
> +
> + /* Then also align the key material size to the size of the header */
> + return ROUND_UP(splitkeylen_sectors, header_sectors);
> +}
> +
> /*
> * Stores the main LUKS header, taking care of endianess
> */
> @@ -1151,7 +1177,8 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> QCryptoBlockCreateOptionsLUKS luks_opts;
> Error *local_err = NULL;
> g_autofree uint8_t *masterkey = NULL;
> - size_t splitkeylen = 0;
> + size_t header_sectors;
> + size_t split_key_sectors;
> size_t i;
> g_autofree char *password;
> const char *cipher_alg;
> @@ -1370,37 +1397,28 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> goto error;
> }
>
> + /* start with the sector that follows the header*/
> + header_sectors = QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> + QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
> +
> + split_key_sectors =
> + qcrypto_block_luks_splitkeylen_sectors(luks,
> + QCRYPTO_BLOCK_LUKS_STRIPES);
>
> - /* Although LUKS has multiple key slots, we're just going
> - * to use the first key slot */
> - splitkeylen = luks->header.master_key_len * QCRYPTO_BLOCK_LUKS_STRIPES;
> for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> - luks->header.key_slots[i].active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
> - luks->header.key_slots[i].stripes = QCRYPTO_BLOCK_LUKS_STRIPES;
> + QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[i];
> + slot->active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
>
> - /* This calculation doesn't match that shown in the spec,
> - * but instead follows the cryptsetup implementation.
> - */
> - luks->header.key_slots[i].key_offset_sector =
> - (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> - QCRYPTO_BLOCK_LUKS_SECTOR_SIZE) +
> - (ROUND_UP(DIV_ROUND_UP(splitkeylen, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE),
> - (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> - QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) * i);
> + slot->key_offset_sector = header_sectors + i * split_key_sectors;
> + slot->stripes = QCRYPTO_BLOCK_LUKS_STRIPES;
> }
>
> -
> /* The total size of the LUKS headers is the partition header + key
> * slot headers, rounded up to the nearest sector, combined with
> * the size of each master key material region, also rounded up
> * to the nearest sector */
> - luks->header.payload_offset_sector =
> - (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> - QCRYPTO_BLOCK_LUKS_SECTOR_SIZE) +
> - (ROUND_UP(DIV_ROUND_UP(splitkeylen, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE),
> - (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> - QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) *
> - QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
> + luks->header.payload_offset_sector = header_sectors +
> + QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS * split_key_sectors;
>
> block->sector_size = QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
> block->payload_offset = luks->header.payload_offset_sector *
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v2 13/13] qcrypto-luks: implement more rigorous header checking
2019-08-26 13:51 ` [Qemu-devel] [PATCH v2 13/13] qcrypto-luks: implement more rigorous header checking Maxim Levitsky
@ 2019-09-06 13:34 ` Daniel P. Berrangé
2019-09-12 8:11 ` Maxim Levitsky
0 siblings, 1 reply; 30+ messages in thread
From: Daniel P. Berrangé @ 2019-09-06 13:34 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Kevin Wolf, Fam Zheng, qemu-block, Markus Armbruster, qemu-devel,
Max Reitz, Stefan Hajnoczi
On Mon, Aug 26, 2019 at 04:51:03PM +0300, Maxim Levitsky wrote:
> Check that keyslots don't overlap with the data,
> and check that keyslots don't overlap with each other.
> (this is done using naive O(n^2) nested loops,
> but since there are just 8 keyslots, this doesn't really matter.
>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
> crypto/block-luks.c | 46 +++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index db0fb764b4..fdf4c41f8a 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -541,12 +541,12 @@ fail:
> static int
> qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp)
> {
> - int ret;
> + int ret = -EINVAL;
As before, no need to use errnos, just return -1 immediately.
> + size_t i, j;
>
> if (memcmp(luks->header.magic, qcrypto_block_luks_magic,
> QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) {
> error_setg(errp, "Volume is not in LUKS format");
> - ret = -EINVAL;
> goto fail;
> }
>
> @@ -557,6 +557,48 @@ qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp)
> goto fail;
> }
>
> + /* Check all keyslots for corruption */
> + for (i = 0 ; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; i++) {
> +
> + const QCryptoBlockLUKSKeySlot *slot1 = &luks->header.key_slots[i];
> + unsigned int start1 = slot1->key_offset_sector;
> + unsigned int len1 =
> + qcrypto_block_luks_splitkeylen_sectors(luks, slot1->stripes);
> +
> + if (slot1->stripes == 0) {
> + error_setg(errp, "Keyslot %zu is corrupted (stripes == 0)", i);
> + goto fail;
> + }
How about checking stripes != QCRYPTO_BLOCK_LUKS_STRIPES because
AFAIR, you're required to use 4k stripes in luks v1.
Also how about checking iters >= MIN_SLOT_KEY_ITERS
> +
> + if (slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED &&
> + slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED) {
Align the two lines with (
> + error_setg(errp,
> + "Keyslot %zu state (active/disable) is corrupted", i);
> + goto fail;
> + }
> +
> + if (start1 + len1 > luks->header.payload_offset_sector) {
> + error_setg(errp,
> + "Keyslot %zu is overlapping with the encrypted payload",
> + i);
> + goto fail;
> + }
> +
> + for (j = i + 1 ; j < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; j++) {
> + const QCryptoBlockLUKSKeySlot *slot2 = &luks->header.key_slots[j];
> + unsigned int start2 = slot2->key_offset_sector;
> + unsigned int len2 =
> + qcrypto_block_luks_splitkeylen_sectors(luks, slot2->stripes);
> +
> + if (start1 + len1 > start2 && start2 + len2 > start1) {
> + error_setg(errp,
> + "Keyslots %zu and %zu are overlapping in the header",
> + i, j);
> + goto fail;
> + }
> + }
> +
> + }
> return 0;
> fail:
> return ret;
> --
> 2.17.2
>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v2 09/13] qcrypto-block: extract check and parse header
2019-09-06 13:11 ` Daniel P. Berrangé
@ 2019-09-12 7:24 ` Maxim Levitsky
0 siblings, 0 replies; 30+ messages in thread
From: Maxim Levitsky @ 2019-09-12 7:24 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Kevin Wolf, Fam Zheng, qemu-block, qemu-devel, Markus Armbruster,
Max Reitz, Stefan Hajnoczi
On Fri, 2019-09-06 at 14:11 +0100, Daniel P. Berrangé wrote:
> On Mon, Aug 26, 2019 at 04:50:59PM +0300, Maxim Levitsky wrote:
> > This is just to make qcrypto_block_luks_open more
> > reasonable in size.
> >
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> > crypto/block-luks.c | 254 +++++++++++++++++++++++++-------------------
> > 1 file changed, 146 insertions(+), 108 deletions(-)
> >
> > diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> > index b4dc6fc899..cc9a52c9af 100644
> > --- a/crypto/block-luks.c
> > +++ b/crypto/block-luks.c
> > @@ -508,6 +508,148 @@ fail:
> > return ret;
> > }
> >
> > +/*
> > + * Does basic sanity checks on the LUKS header
> > + */
> > +static int
> > +qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp)
> > +{
> > + int ret;
> > +
> > + if (memcmp(luks->header.magic, qcrypto_block_luks_magic,
> > + QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) {
> > + error_setg(errp, "Volume is not in LUKS format");
> > + ret = -EINVAL;
> > + goto fail;
> > + }
>
> Just 'return -1' here immediately - don't return an errno, as we're
> using Error objects for reporting.
>
> > +
> > + if (luks->header.version != QCRYPTO_BLOCK_LUKS_VERSION) {
> > + error_setg(errp, "LUKS version %" PRIu32 " is not supported",
> > + luks->header.version);
> > + ret = -ENOTSUP;
> > + goto fail;
> > + }
> > +
> > + return 0;
> > +fail:
> > + return ret;
> > +}
> > +
> > +/*
> > + * Parses the crypto parameters that are stored in the LUKS header
> > + */
> > +
> > +static int
> > +qcrypto_block_luks_parse_header(QCryptoBlockLUKS *luks, Error **errp)
> > +{
> > + g_autofree char *cipher_mode = g_strdup(luks->header.cipher_mode);
> > + char *ivgen_name, *ivhash_name;
> > + int ret = -1;
> > + Error *local_err = NULL;
> > +
> > + /*
> > + * The cipher_mode header contains a string that we have
> > + * to further parse, of the format
> > + *
> > + * <cipher-mode>-<iv-generator>[:<iv-hash>]
> > + *
> > + * eg cbc-essiv:sha256, cbc-plain64
> > + */
> > + ivgen_name = strchr(cipher_mode, '-');
> > + if (!ivgen_name) {
> > + ret = -EINVAL;
>
> Again, don't use errnos - just return -1 in this method.
>
> > + error_setg(errp, "Unexpected cipher mode string format %s",
> > + luks->header.cipher_mode);
> > + goto out;
> > + }
> > + *ivgen_name = '\0';
> > + ivgen_name++;
> > +
> > + ivhash_name = strchr(ivgen_name, ':');
> > + if (!ivhash_name) {
> > + luks->ivgen_hash_alg = 0;
> > + } else {
> > + *ivhash_name = '\0';
> > + ivhash_name++;
> > +
> > + luks->ivgen_hash_alg = qcrypto_block_luks_hash_name_lookup(ivhash_name,
> > + &local_err);
> > + if (local_err) {
> > + ret = -ENOTSUP;
> > + error_propagate(errp, local_err);
> > + goto out;
> > + }
> > + }
> > +
> > + luks->cipher_mode = qcrypto_block_luks_cipher_mode_lookup(cipher_mode,
> > + &local_err);
> > + if (local_err) {
> > + ret = -ENOTSUP;
> > + error_propagate(errp, local_err);
> > + goto out;
> > + }
> > +
> > + luks->cipher_alg =
> > + qcrypto_block_luks_cipher_name_lookup(luks->header.cipher_name,
> > + luks->cipher_mode,
> > + luks->header.master_key_len,
> > + &local_err);
> > + if (local_err) {
> > + ret = -ENOTSUP;
> > + error_propagate(errp, local_err);
> > + goto out;
> > + }
> > +
> > + luks->hash_alg =
> > + qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec,
> > + &local_err);
> > + if (local_err) {
> > + ret = -ENOTSUP;
> > + error_propagate(errp, local_err);
> > + goto out;
> > + }
> > +
> > + luks->ivgen_alg = qcrypto_block_luks_ivgen_name_lookup(ivgen_name,
> > + &local_err);
> > + if (local_err) {
> > + ret = -ENOTSUP;
> > + error_propagate(errp, local_err);
> > + goto out;
> > + }
> > +
> > + if (luks->ivgen_alg == QCRYPTO_IVGEN_ALG_ESSIV) {
> > + if (!ivhash_name) {
> > + ret = -EINVAL;
> > + error_setg(errp, "Missing IV generator hash specification");
> > + goto out;
> > + }
> > + luks->ivgen_cipher_alg =
> > + qcrypto_block_luks_essiv_cipher(luks->cipher_alg,
> > + luks->ivgen_hash_alg,
> > + &local_err);
> > + if (local_err) {
> > + ret = -ENOTSUP;
> > + error_propagate(errp, local_err);
> > + goto out;
> > + }
> > + } else {
> > +
> > + /*
> > + * Note we parsed the ivhash_name earlier in the cipher_mode
> > + * spec string even with plain/plain64 ivgens, but we
> > + * will ignore it, since it is irrelevant for these ivgens.
> > + * This is for compat with dm-crypt which will silently
> > + * ignore hash names with these ivgens rather than report
> > + * an error about the invalid usage
> > + */
> > + luks->ivgen_cipher_alg = luks->cipher_alg;
> > + }
> > + ret = 0;
> > +out:
> > + return ret;
> > +
> > +}
> > +
> > /*
> > * Given a key slot, and user password, this will attempt to unlock
> > * the master encryption key from the key slot.
> > @@ -720,12 +862,9 @@ qcrypto_block_luks_open(QCryptoBlock *block,
> > Error **errp)
> > {
> > QCryptoBlockLUKS *luks = NULL;
> > - Error *local_err = NULL;
> > int ret = 0;
> > g_autofree uint8_t *masterkey = NULL;
> > - char *ivgen_name, *ivhash_name;
> > g_autofree char *password = NULL;
> > - g_autofree char *cipher_mode = NULL;
> >
> > if (!(flags & QCRYPTO_BLOCK_OPEN_NO_IO)) {
> > if (!options->u.luks.key_secret) {
> > @@ -748,117 +887,16 @@ qcrypto_block_luks_open(QCryptoBlock *block,
> > goto fail;
> > }
> >
> > -
> > - if (memcmp(luks->header.magic, qcrypto_block_luks_magic,
> > - QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) {
> > - error_setg(errp, "Volume is not in LUKS format");
> > - ret = -EINVAL;
>
> I can't remember why I set ret to an errno in my original code.
> it is entirely pointless, as the caller of this method merely
> checks "ret < 0" and doesn't do anything else with the return
> value. IOW, we should just purge the errnors from this existing
> code entirely.
Nothing against that, I'll do that in a separate patch now.
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v2 11/13] qcrypto-luks: refactoring: simplify the math used for keyslot locations
2019-09-06 13:17 ` Daniel P. Berrangé
@ 2019-09-12 7:40 ` Maxim Levitsky
0 siblings, 0 replies; 30+ messages in thread
From: Maxim Levitsky @ 2019-09-12 7:40 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Kevin Wolf, Fam Zheng, qemu-block, qemu-devel, Markus Armbruster,
Max Reitz, Stefan Hajnoczi
On Fri, 2019-09-06 at 14:17 +0100, Daniel P. Berrangé wrote:
> On Mon, Aug 26, 2019 at 04:51:01PM +0300, Maxim Levitsky wrote:
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> > crypto/block-luks.c | 64 +++++++++++++++++++++++++++++----------------
> > 1 file changed, 41 insertions(+), 23 deletions(-)
> >
> > diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> > index d713125925..6a43d97ce5 100644
> > --- a/crypto/block-luks.c
> > +++ b/crypto/block-luks.c
> > @@ -409,6 +409,32 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher,
> > }
> > }
> >
> > +/*
> > + * Returns number of sectors needed to store the key material
> > + * given number of anti forensic stripes
> > + */
> > +static int
> > +qcrypto_block_luks_splitkeylen_sectors(const QCryptoBlockLUKS *luks,
> > + unsigned int stripes)
> > +{
> > + /*
> > + * This calculation doesn't match that shown in the spec,
> > + * but instead follows the cryptsetup implementation.
> > + */
> > +
> > + size_t header_sectors = QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> > + QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
>
> The caller already calculated that so just pass it in
All right, no problem.
>
> > +
> > + size_t splitkeylen = luks->header.master_key_len * stripes;
> > +
> > + /* First align the key material size to block size*/
> > + size_t splitkeylen_sectors =
> > + DIV_ROUND_UP(splitkeylen, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE);
> > +
> > + /* Then also align the key material size to the size of the header */
> > + return ROUND_UP(splitkeylen_sectors, header_sectors);
> > +}
> > +
> > /*
> > * Stores the main LUKS header, taking care of endianess
> > */
> > @@ -1151,7 +1177,8 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> > QCryptoBlockCreateOptionsLUKS luks_opts;
> > Error *local_err = NULL;
> > g_autofree uint8_t *masterkey = NULL;
> > - size_t splitkeylen = 0;
> > + size_t header_sectors;
> > + size_t split_key_sectors;
> > size_t i;
> > g_autofree char *password;
> > const char *cipher_alg;
> > @@ -1370,37 +1397,28 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> > goto error;
> > }
> >
> > + /* start with the sector that follows the header*/
> > + header_sectors = QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> > + QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
> > +
> > + split_key_sectors =
> > + qcrypto_block_luks_splitkeylen_sectors(luks,
> > + QCRYPTO_BLOCK_LUKS_STRIPES);
> >
> > - /* Although LUKS has multiple key slots, we're just going
> > - * to use the first key slot */
> > - splitkeylen = luks->header.master_key_len * QCRYPTO_BLOCK_LUKS_STRIPES;
> > for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> > - luks->header.key_slots[i].active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
> > - luks->header.key_slots[i].stripes = QCRYPTO_BLOCK_LUKS_STRIPES;
> > + QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[i];
> > + slot->active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
> >
> > - /* This calculation doesn't match that shown in the spec,
> > - * but instead follows the cryptsetup implementation.
> > - */
> > - luks->header.key_slots[i].key_offset_sector =
> > - (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> > - QCRYPTO_BLOCK_LUKS_SECTOR_SIZE) +
> > - (ROUND_UP(DIV_ROUND_UP(splitkeylen, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE),
> > - (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> > - QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) * i);
> > + slot->key_offset_sector = header_sectors + i * split_key_sectors;
> > + slot->stripes = QCRYPTO_BLOCK_LUKS_STRIPES;
> > }
> >
> > -
> > /* The total size of the LUKS headers is the partition header + key
> > * slot headers, rounded up to the nearest sector, combined with
> > * the size of each master key material region, also rounded up
> > * to the nearest sector */
> > - luks->header.payload_offset_sector =
> > - (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> > - QCRYPTO_BLOCK_LUKS_SECTOR_SIZE) +
> > - (ROUND_UP(DIV_ROUND_UP(splitkeylen, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE),
> > - (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> > - QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) *
> > - QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
> > + luks->header.payload_offset_sector = header_sectors +
> > + QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS * split_key_sectors;
> >
> > block->sector_size = QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
> > block->payload_offset = luks->header.payload_offset_sector *
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
> Regards,
> Daniel
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v2 13/13] qcrypto-luks: implement more rigorous header checking
2019-09-06 13:34 ` Daniel P. Berrangé
@ 2019-09-12 8:11 ` Maxim Levitsky
0 siblings, 0 replies; 30+ messages in thread
From: Maxim Levitsky @ 2019-09-12 8:11 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Kevin Wolf, Fam Zheng, qemu-block, qemu-devel, Markus Armbruster,
Max Reitz, Stefan Hajnoczi
On Fri, 2019-09-06 at 14:34 +0100, Daniel P. Berrangé wrote:
> On Mon, Aug 26, 2019 at 04:51:03PM +0300, Maxim Levitsky wrote:
> > Check that keyslots don't overlap with the data,
> > and check that keyslots don't overlap with each other.
> > (this is done using naive O(n^2) nested loops,
> > but since there are just 8 keyslots, this doesn't really matter.
> >
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> > crypto/block-luks.c | 46 +++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 44 insertions(+), 2 deletions(-)
> >
> > diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> > index db0fb764b4..fdf4c41f8a 100644
> > --- a/crypto/block-luks.c
> > +++ b/crypto/block-luks.c
> > @@ -541,12 +541,12 @@ fail:
> > static int
> > qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp)
> > {
> > - int ret;
> > + int ret = -EINVAL;
>
> As before, no need to use errnos, just return -1 immediately.
>
> > + size_t i, j;
> >
> > if (memcmp(luks->header.magic, qcrypto_block_luks_magic,
> > QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) {
> > error_setg(errp, "Volume is not in LUKS format");
> > - ret = -EINVAL;
> > goto fail;
> > }
> >
> > @@ -557,6 +557,48 @@ qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp)
> > goto fail;
> > }
> >
> > + /* Check all keyslots for corruption */
> > + for (i = 0 ; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; i++) {
> > +
> > + const QCryptoBlockLUKSKeySlot *slot1 = &luks->header.key_slots[i];
> > + unsigned int start1 = slot1->key_offset_sector;
> > + unsigned int len1 =
> > + qcrypto_block_luks_splitkeylen_sectors(luks, slot1->stripes);
> > +
> > + if (slot1->stripes == 0) {
> > + error_setg(errp, "Keyslot %zu is corrupted (stripes == 0)", i);
> > + goto fail;
> > + }
>
> How about checking stripes != QCRYPTO_BLOCK_LUKS_STRIPES because
> AFAIR, you're required to use 4k stripes in luks v1.
I see that spec does allow for user defined number of stripes.
>
> Also how about checking iters >= MIN_SLOT_KEY_ITERS
Also this is only a suggested minimum
>
> > +
> > + if (slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED &&
> > + slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED) {
>
> Align the two lines with (
Done
>
> > + error_setg(errp,
> > + "Keyslot %zu state (active/disable) is corrupted", i);
> > + goto fail;
> > + }
> > +
> > + if (start1 + len1 > luks->header.payload_offset_sector) {
> > + error_setg(errp,
> > + "Keyslot %zu is overlapping with the encrypted payload",
> > + i);
> > + goto fail;
> > + }
> > +
> > + for (j = i + 1 ; j < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; j++) {
> > + const QCryptoBlockLUKSKeySlot *slot2 = &luks->header.key_slots[j];
> > + unsigned int start2 = slot2->key_offset_sector;
> > + unsigned int len2 =
> > + qcrypto_block_luks_splitkeylen_sectors(luks, slot2->stripes);
> > +
> > + if (start1 + len1 > start2 && start2 + len2 > start1) {
> > + error_setg(errp,
> > + "Keyslots %zu and %zu are overlapping in the header",
> > + i, j);
> > + goto fail;
> > + }
> > + }
> > +
> > + }
> > return 0;
> > fail:
> > return ret;
> > --
> > 2.17.2
> >
>
> Regards,
> Daniel
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2019-09-12 8:12 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-26 13:50 [Qemu-devel] [PATCH v2 00/13] RFC crypto/luks: preparation for encryption key managment Maxim Levitsky
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 01/13] introduce g_autowipe Maxim Levitsky
2019-08-27 10:46 ` Tony Nguyen
2019-08-27 10:52 ` Daniel P. Berrangé
2019-08-27 11:24 ` Maxim Levitsky
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 02/13] block-crypto: misc refactoring Maxim Levitsky
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 03/13] qcrypto-luks: rename some fields in QCryptoBlockLUKSHeader Maxim Levitsky
2019-09-06 12:27 ` Daniel P. Berrangé
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 04/13] qcrypto-luks: don't overwrite cipher_mode in header Maxim Levitsky
2019-09-06 12:29 ` Daniel P. Berrangé
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 05/13] qcrypto-luks: simplify masterkey and masterkey length Maxim Levitsky
2019-09-06 12:30 ` Daniel P. Berrangé
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 06/13] qcrypto-block: pass keyslot index rather that pointer to the keyslot Maxim Levitsky
2019-09-06 12:32 ` Daniel P. Berrangé
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 07/13] qcrypto-luks: use the parsed encryption settings in QCryptoBlockLUKS Maxim Levitsky
2019-09-06 12:35 ` Daniel P. Berrangé
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 08/13] qcrypto-luks: extract store and load header Maxim Levitsky
2019-09-06 13:06 ` Daniel P. Berrangé
2019-08-26 13:50 ` [Qemu-devel] [PATCH v2 09/13] qcrypto-block: extract check and parse header Maxim Levitsky
2019-09-06 13:11 ` Daniel P. Berrangé
2019-09-12 7:24 ` Maxim Levitsky
2019-08-26 13:51 ` [Qemu-devel] [PATCH v2 10/13] qcrypto-luks: refactoring: extract store key function Maxim Levitsky
2019-09-06 13:14 ` Daniel P. Berrangé
2019-08-26 13:51 ` [Qemu-devel] [PATCH v2 11/13] qcrypto-luks: refactoring: simplify the math used for keyslot locations Maxim Levitsky
2019-09-06 13:17 ` Daniel P. Berrangé
2019-09-12 7:40 ` Maxim Levitsky
2019-08-26 13:51 ` [Qemu-devel] [PATCH v2 12/13] qcrypto-luks: use g_autowipe Maxim Levitsky
2019-08-26 13:51 ` [Qemu-devel] [PATCH v2 13/13] qcrypto-luks: implement more rigorous header checking Maxim Levitsky
2019-09-06 13:34 ` Daniel P. Berrangé
2019-09-12 8:11 ` Maxim Levitsky
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).