Control: reassign -1 libblockdev-crypto2
Control: found -1 2.20-7
Control: fixed -1 2.22-1
Control: affects -1 gnome-disk-utility
Control: affects -1 udisks2
Control: tag -1 + patch
Hi,
it turns out this is caused by a bug in libblockdev, which is fixed in
sid already (although it seems like upstream applied the fix for
unrelated reasons and it's not clear whether they realized this bug
was a possibility).
The attached patch fixes it for me:
- current sid (libblockdev 2.22-1): unreproducible
- current sid with Buster's libblockdev 2.20-7: reproduced the bug
- current sid with Buster's libblockdev + the attached patch: unreproducible
I think we should get this into Buster (10.1, if not via stable-updates).
As explained in the changelog entry, this fixes the data loss problem
but it might still leave udisks2's LUKS2 passphrase changing broken in
some cases, although broken in a way that leaves the old passphrase
working, which is better than making the device totally unusable.
Guilhem (Cc'ed) and I are investigating this possible problems and
potential solutions.
Cheers,
--
intrigeri
diff --git a/debian/changelog b/debian/changelog
index c9bfefa..a5a8c2c 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,29 @@
+libblockdev (2.20-7.1) unstable; urgency=medium
+
+ * Cherry-pick upstream fix: use existing cryptsetup API for atomically
+ changing a keyslot passphrase, instead of deleting the old keyslot
+ before adding the new one. This avoids data loss when attempting to
+ change the passphrase of a LUKS2 device via udisks2, e.g. from GNOME
+ Disks (Closes: #928893).
+
+ Deleting a keyslot and then adding one is risky: if anything goes
+ wrong before the new keyslot is successfully added, no usable keyslot
+ is left and the device cannot be unlocked anymore. There's little
+ chances this causes actual problems with LUKS1, but LUKS2 defaults to
+ the memory-hard Argon2 key derivation algorithm, which is implemented
+ in cryptsetup with the assumption that it runs as root with no MEMLOCK
+ ulimit; this assumption is wrong when run by udisks2.service under
+ LimitMEMLOCK=65536, which breaks adding the new keyslot, and makes us
+ hit the problematic situation (user data loss) every time.
+
+ With this change, changing a LUKS2 passphrase via udisks2 will still
+ fail in some cases, until the MEMLOCK ulimit problem is solved in
+ cryptsetup or workaround'ed in udisks2, which Guilhem Moulin and I are
+ working on. But at least, if it fails, it will fail _atomically_ and
+ the original passphrase will still work.
+
+ -- intrigeri <intrig...@debian.org> Sat, 20 Jul 2019 01:01:29 +0000
+
libblockdev (2.20-7) unstable; urgency=medium
* Cherry-pick Use-512bit-keys-in-LUKS-by-default.patch:
diff --git a/debian/patches/series b/debian/patches/series
index 94d3e03..ddf2825 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -1 +1,2 @@
Use-512bit-keys-in-LUKS-by-default.patch
+use-existing-cryptsetup-api-for-changing.patch
diff --git a/debian/patches/use-existing-cryptsetup-api-for-changing.patch b/debian/patches/use-existing-cryptsetup-api-for-changing.patch
new file mode 100644
index 0000000..45b4fd4
--- /dev/null
+++ b/debian/patches/use-existing-cryptsetup-api-for-changing.patch
@@ -0,0 +1,84 @@
+From: Vojtech Trefny <vtre...@redhat.com>
+Date: Tue, 12 Mar 2019 09:28:05 +0100
+X-Dgit-Generated: 2.20-7.1 8590f51cdbabf22c2b9250c108cf11446e6e91c4
+Subject: Use existing cryptsetup API for changing keyslot passphrase
+
+Instead of manually removing the keyslot and adding new a one.
+Our old code also doesn't work in FIPS mode.
+
+---
+
+--- libblockdev-2.20.orig/src/plugins/crypto.c
++++ libblockdev-2.20/src/plugins/crypto.c
+@@ -1359,8 +1359,6 @@ gboolean bd_crypto_luks_remove_key (cons
+ gboolean bd_crypto_luks_change_key_blob (const gchar *device, const guint8 *pass_data, gsize data_len, const guint8 *npass_data, gsize ndata_len, GError **error) {
+ struct crypt_device *cd = NULL;
+ gint ret = 0;
+- gchar *volume_key = NULL;
+- gsize vk_size = 0;
+ guint64 progress_id = 0;
+ gchar *msg = NULL;
+
+@@ -1385,41 +1383,21 @@ gboolean bd_crypto_luks_change_key_blob
+ return FALSE;
+ }
+
+- vk_size = crypt_get_volume_key_size(cd);
+- volume_key = (gchar *) g_malloc (vk_size);
+-
+- ret = crypt_volume_key_get (cd, CRYPT_ANY_SLOT, volume_key, &vk_size, (char*) pass_data, data_len);
+- if (ret < 0) {
+- g_set_error (error, BD_CRYPTO_ERROR, BD_CRYPTO_ERROR_DEVICE,
+- "Failed to load device's volume key: %s", strerror_l(-ret, c_locale));
+- crypt_free (cd);
+- g_free (volume_key);
+- bd_utils_report_finished (progress_id, (*error)->message);
+- return FALSE;
+- }
+-
+- /* ret is the number of the slot with the given pass */
+- ret = crypt_keyslot_destroy (cd, ret);
+- if (ret != 0) {
+- g_set_error (error, BD_CRYPTO_ERROR, BD_CRYPTO_ERROR_REMOVE_KEY,
+- "Failed to remove the old passphrase: %s", strerror_l(-ret, c_locale));
+- crypt_free (cd);
+- g_free (volume_key);
+- bd_utils_report_finished (progress_id, (*error)->message);
+- return FALSE;
+- }
+-
+- ret = crypt_keyslot_add_by_volume_key (cd, ret, volume_key, vk_size, (char*) npass_data, ndata_len);
++ ret = crypt_keyslot_change_by_passphrase (cd, CRYPT_ANY_SLOT, CRYPT_ANY_SLOT,
++ (char*) pass_data, data_len,
++ (char*) npass_data, ndata_len);
+ if (ret < 0) {
+- g_set_error (error, BD_CRYPTO_ERROR, BD_CRYPTO_ERROR_ADD_KEY,
+- "Failed to add the new passphrase: %s", strerror_l(-ret, c_locale));
++ if (ret == -EPERM)
++ g_set_error (error, BD_CRYPTO_ERROR, BD_CRYPTO_ERROR_DEVICE,
++ "Failed to change the passphrase: No keyslot with given passphrase found.");
++ else
++ g_set_error (error, BD_CRYPTO_ERROR, BD_CRYPTO_ERROR_ADD_KEY,
++ "Failed to change the passphrase: %s", strerror_l (-ret, c_locale));
+ crypt_free (cd);
+- g_free (volume_key);
+ bd_utils_report_finished (progress_id, (*error)->message);
+ return FALSE;
+ }
+
+- g_free (volume_key);
+ crypt_free (cd);
+ bd_utils_report_finished (progress_id, "Completed");
+ return TRUE;
+--- libblockdev-2.20.orig/tests/crypto_test.py
++++ libblockdev-2.20/tests/crypto_test.py
+@@ -401,6 +401,9 @@ class CryptoTestChangeKey(CryptoTestCase
+ succ = create_fn(self.loop_dev, PASSWD, None)
+ self.assertTrue(succ)
+
++ with six.assertRaisesRegex(self, GLib.GError, r"No keyslot with given passphrase found."):
++ BlockDev.crypto_luks_change_key(self.loop_dev, "wrong-passphrase", PASSWD2)
++
+ succ = BlockDev.crypto_luks_change_key(self.loop_dev, PASSWD, PASSWD2)
+ self.assertTrue(succ)
+
diff --git a/src/plugins/crypto.c b/src/plugins/crypto.c
index 6b5be9d..205499f 100644
--- a/src/plugins/crypto.c
+++ b/src/plugins/crypto.c
@@ -1359,8 +1359,6 @@ gboolean bd_crypto_luks_remove_key (const gchar *device, const gchar *pass, cons
gboolean bd_crypto_luks_change_key_blob (const gchar *device, const guint8 *pass_data, gsize data_len, const guint8 *npass_data, gsize ndata_len, GError **error) {
struct crypt_device *cd = NULL;
gint ret = 0;
- gchar *volume_key = NULL;
- gsize vk_size = 0;
guint64 progress_id = 0;
gchar *msg = NULL;
@@ -1385,41 +1383,21 @@ gboolean bd_crypto_luks_change_key_blob (const gchar *device, const guint8 *pass
return FALSE;
}
- vk_size = crypt_get_volume_key_size(cd);
- volume_key = (gchar *) g_malloc (vk_size);
-
- ret = crypt_volume_key_get (cd, CRYPT_ANY_SLOT, volume_key, &vk_size, (char*) pass_data, data_len);
- if (ret < 0) {
- g_set_error (error, BD_CRYPTO_ERROR, BD_CRYPTO_ERROR_DEVICE,
- "Failed to load device's volume key: %s", strerror_l(-ret, c_locale));
- crypt_free (cd);
- g_free (volume_key);
- bd_utils_report_finished (progress_id, (*error)->message);
- return FALSE;
- }
-
- /* ret is the number of the slot with the given pass */
- ret = crypt_keyslot_destroy (cd, ret);
- if (ret != 0) {
- g_set_error (error, BD_CRYPTO_ERROR, BD_CRYPTO_ERROR_REMOVE_KEY,
- "Failed to remove the old passphrase: %s", strerror_l(-ret, c_locale));
- crypt_free (cd);
- g_free (volume_key);
- bd_utils_report_finished (progress_id, (*error)->message);
- return FALSE;
- }
-
- ret = crypt_keyslot_add_by_volume_key (cd, ret, volume_key, vk_size, (char*) npass_data, ndata_len);
+ ret = crypt_keyslot_change_by_passphrase (cd, CRYPT_ANY_SLOT, CRYPT_ANY_SLOT,
+ (char*) pass_data, data_len,
+ (char*) npass_data, ndata_len);
if (ret < 0) {
- g_set_error (error, BD_CRYPTO_ERROR, BD_CRYPTO_ERROR_ADD_KEY,
- "Failed to add the new passphrase: %s", strerror_l(-ret, c_locale));
+ if (ret == -EPERM)
+ g_set_error (error, BD_CRYPTO_ERROR, BD_CRYPTO_ERROR_DEVICE,
+ "Failed to change the passphrase: No keyslot with given passphrase found.");
+ else
+ g_set_error (error, BD_CRYPTO_ERROR, BD_CRYPTO_ERROR_ADD_KEY,
+ "Failed to change the passphrase: %s", strerror_l (-ret, c_locale));
crypt_free (cd);
- g_free (volume_key);
bd_utils_report_finished (progress_id, (*error)->message);
return FALSE;
}
- g_free (volume_key);
crypt_free (cd);
bd_utils_report_finished (progress_id, "Completed");
return TRUE;
diff --git a/tests/crypto_test.py b/tests/crypto_test.py
index c048570..747b702 100644
--- a/tests/crypto_test.py
+++ b/tests/crypto_test.py
@@ -401,6 +401,9 @@ class CryptoTestChangeKey(CryptoTestCase):
succ = create_fn(self.loop_dev, PASSWD, None)
self.assertTrue(succ)
+ with six.assertRaisesRegex(self, GLib.GError, r"No keyslot with given passphrase found."):
+ BlockDev.crypto_luks_change_key(self.loop_dev, "wrong-passphrase", PASSWD2)
+
succ = BlockDev.crypto_luks_change_key(self.loop_dev, PASSWD, PASSWD2)
self.assertTrue(succ)
