Bug#928893: gnome-disk-utility: disk content permamently lost when changing LUKS password
On Sun, 21 Jul 2019 at 22:40:38 +0200, Michael Biebl wrote: > I already uploaded 2.20-7+deb10u1 with this changelog, so it's not > really possible anymore to undo this other then making a 2.20-7+deb10u2 > upload, which seems like overkill to me. > I don't think the changelog is that misleading that we need another > upload fixing it. Ack, agreed -- Guilhem. signature.asc Description: PGP signature
Bug#928893: gnome-disk-utility: disk content permamently lost when changing LUKS password
Hi Am 21.07.19 um 21:58 schrieb Guilhem Moulin: > Now that libblockdev uses crypt_keyslot_change_by_passphrase() there is > AFAICT nothing more to be done on the libblockdev or udisks2 side with > respect to that bug. But maybe the Changelog entry for libblockdev > 2.20-7+deb10u1 should be changed to remove the references to MEMLOCK. > As I wrote in https://gitlab.com/cryptsetup/cryptsetup/issues/466 I > believe the problem with LUKSv2 is elsewhere (crypt_get_volume_key_size() > fails because there is no bound keyslot object to retrieve the key size > from). Maybe changing it to > > * Use existing cryptsetup API for changing keyslot passphrase. > Cherry-pick upstream fix to 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. > 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 as of 2.1.0 libcrypsetup > fails to add a new keyslot to a LUKS2 header without any > pre-existing keyslot. > (Closes: #928893) > > Or maybe remoing the last sentence alltogether, ending with “[…] cannot > be unlocked anymore.” I already uploaded 2.20-7+deb10u1 with this changelog, so it's not really possible anymore to undo this other then making a 2.20-7+deb10u2 upload, which seems like overkill to me. I don't think the changelog is that misleading that we need another upload fixing it. Regards, Michael -- Why is it that all of the instruments seeking intelligent life in the universe are pointed away from Earth? signature.asc Description: OpenPGP digital signature
Bug#928893: gnome-disk-utility: disk content permamently lost when changing LUKS password
Hi, On Sun, 21 Jul 2019 at 13:36:06 +0200, Michael Biebl wrote: > Agreed. I've just uploaded a libblockdev with that cherry-pick to buster > and this change was acked by the SRMs, so should be in 10.1. Awesome! :-) > Regarding the LUKS2/udisks2/LimitMEMLOCK issue, would you prefer to > track this as a udisks2 issue or cryptsetup issue? Is there already a > bug report for this or should we clone/reassign this one? I filed https://gitlab.com/cryptsetup/cryptsetup/issues/465 “Argon2i/d benchmark doesn't honor `getrlimit(RLIMIT_MEMLOCK,)`”, but on second thought I don' think udisks2 is affected. As I wrote in Message #59, | Apologies for incorrectly pointing to getrlimits(2) earlier: I'm | personally not familiar with udisks/libblockdev myself and hadn't | realized `getrlimit(RLIMIT_MEMLOCK,)` was bypassed since the Argon2d/i | benchmark process is privileged. Now that libblockdev uses crypt_keyslot_change_by_passphrase() there is AFAICT nothing more to be done on the libblockdev or udisks2 side with respect to that bug. But maybe the Changelog entry for libblockdev 2.20-7+deb10u1 should be changed to remove the references to MEMLOCK. As I wrote in https://gitlab.com/cryptsetup/cryptsetup/issues/466 I believe the problem with LUKSv2 is elsewhere (crypt_get_volume_key_size() fails because there is no bound keyslot object to retrieve the key size from). Maybe changing it to * Use existing cryptsetup API for changing keyslot passphrase. Cherry-pick upstream fix to 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. 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 as of 2.1.0 libcrypsetup fails to add a new keyslot to a LUKS2 header without any pre-existing keyslot. (Closes: #928893) Or maybe remoing the last sentence alltogether, ending with “[…] cannot be unlocked anymore.” Cheers, -- Guilhem. signature.asc Description: PGP signature
Bug#928893: gnome-disk-utility: disk content permamently lost when changing LUKS password
Hi Guilhem, hi intrigeri, first of all, thanks for debugging this issue and this excellent bug report! On Sat, 20 Jul 2019 17:24:21 -0300 Guilhem Moulin wrote: > However as far as libblockdev is concerned, FWIW I fully support > intrigeri's cherry-pick of upstream's 34ed7be. Adding a key slot > *after* having removed it can have very nasty consequences (entire > device lost), and that not just for LUKSv2. Agreed. I've just uploaded a libblockdev with that cherry-pick to buster and this change was acked by the SRMs, so should be in 10.1. Regarding the LUKS2/udisks2/LimitMEMLOCK issue, would you prefer to track this as a udisks2 issue or cryptsetup issue? Is there already a bug report for this or should we clone/reassign this one? Regards, Michael -- Why is it that all of the instruments seeking intelligent life in the universe are pointed away from Earth? signature.asc Description: OpenPGP digital signature
Bug#928893: gnome-disk-utility: disk content permamently lost when changing LUKS password
On Sat, 20 Jul 2019 at 06:01:35 -0300, Guilhem Moulin wrote: > LUKS2_get_volume_key_size() fails because the key size is specified in > the ‘keyslots’ object of LUKSv2's JSON header [0], and that object is > the empty array at that point. Forgot to add another data point which supports my claim: with a LUKSv2 device with two active key slots (#0 unlocked by passphrase “test” and #1 unlocked by a random passphrase), LUKS2_get_volume_key_size() succeeds and so does crypt_keyslot_add_by_volume_key(). $ cryptsetup luksFormat --pbkdf-force-iterations 4 --pbkdf-memory 32 \ --type luks2 -q /tmp/disk.img <
Bug#928893: gnome-disk-utility: disk content permamently lost when changing LUKS password
Hi there, On Fri, 19 Jul 2019 at 22:14:49 -0300, intrigeri wrote: > 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). AFAICT there were two bugs in src/plugins/crypto.c:bd_crypto_luks_change_key_blob() https://sources.debian.org/src/libblockdev/2.20-7/src/plugins/crypto.c/#L1359 The API calls to libcryptsetup roughly goes as follows: keyslot = crypt_volume_key_get(cd, …, volume_key, old_passphrase); crypt_keyslot_destroy(cd, keyslot); crypt_keyslot_add_by_volume_key(cd, 0, volume_key, new_passphrase); The first call uses the old passphrase to unlock a keyslot and set the volume key. (In LUKS2 the volume key of open devices isn't accessible to userspace, but of course it's no problem here since the passphrase is used to unlock a keyslot, which yields the volume key.) The second call removes the keyslot used to get the volume key in the first call. The third call adds a new key slot with the new passphrase. There are IMHO two issues with these calls (regardless of the LUKS format version): 1. It only adds the new slot *after* deleting the old one, so there is moment where the LUKS header might have no active key slot left. Worse, if crypt_keyslot_add_by_volume_key() fails for whichever reason, then the header is left in a broken state; if the user doesn't notice and closes the mapped device (or simply reboots) then the entire content of the device is lost. 2. The second argument of crypt_keyslot_add_by_volume_key() is always 0, while the user might want to change another key slot. I'm unfortunately not familiar with libblockdev, but the attached program, to be linked against libcryptsetup, shows these problems AFAICT. Format a device as LUKSv1 (although the same happens with v2), with a random passphrase for key slot 0, and passphrase “test” for key slot 1. (The extra PBKDF argument are there just so the test doesn't take too long.) $ dd if=/dev/zero of=/tmp/disk.img bs=1M count=64 $ head -c 32 /dev/urandom >/tmp/keyslot0 $ cryptsetup luksFormat --pbkdf-force-iterations 1000 --type luks1 \ -q --key-file=/tmp/keyslot0 /tmp/disk.img $ cryptsetup luksAddKey --pbkdf-force-iterations 1000 \ --key-file=/tmp/keyslot0 -q /tmp/disk.img <
Bug#928893: gnome-disk-utility: disk content permamently lost when changing LUKS password
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 Sat, 20 Jul 2019 01:01:29 +
+
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 000..45b4fd4
--- /dev/null
+++ b/debian/patches/use-existing-cryptsetup-api-for-changing.patch
@@ -0,0 +1,84 @@
+From: Vojtech Trefny
+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, _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,
+-
Processed: Re: Bug#928893: gnome-disk-utility: disk content permamently lost when changing LUKS password
Processing control commands: > reassign -1 libblockdev-crypto2 Bug #928893 [gnome-disk-utility] gnome-disk-utility: disk content permanently lost when changing LUKS password Bug reassigned from package 'gnome-disk-utility' to 'libblockdev-crypto2'. No longer marked as found in versions gnome-disk-utility/3.30.2-3. Ignoring request to alter fixed versions of bug #928893 to the same values previously set > found -1 2.20-7 Bug #928893 [libblockdev-crypto2] gnome-disk-utility: disk content permanently lost when changing LUKS password Marked as found in versions libblockdev/2.20-7. > fixed -1 2.22-1 Bug #928893 [libblockdev-crypto2] gnome-disk-utility: disk content permanently lost when changing LUKS password Marked as fixed in versions libblockdev/2.22-1. > affects -1 gnome-disk-utility Bug #928893 [libblockdev-crypto2] gnome-disk-utility: disk content permanently lost when changing LUKS password Added indication that 928893 affects gnome-disk-utility > affects -1 udisks2 Bug #928893 [libblockdev-crypto2] gnome-disk-utility: disk content permanently lost when changing LUKS password Added indication that 928893 affects udisks2 > tag -1 + patch Bug #928893 [libblockdev-crypto2] gnome-disk-utility: disk content permanently lost when changing LUKS password Added tag(s) patch. -- 928893: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=928893 Debian Bug Tracking System Contact ow...@bugs.debian.org with problems
