Discussion:
Bug#928893: gnome-disk-utility: disk content permamently lost when changing LUKS password
(too old to reply)
Svjatoslav Agejenko
2019-05-12 18:10:01 UTC
Permalink
Package: gnome-disk-utility
Version: 3.30.2-3
Severity: important

Dear Maintainer,

* What led up to the situation?

Install system using normal full disk encryption LUKS+Ext4.
After install open gnome-disk-utility and change
encryption password. It gives some error dialog and
now you are royally screwed. It deleted the only
LUKS keyslot. Cannot add new keyslots because of that.
All data will be lost after reboot.

Here is output of luksdump:

udo cryptsetup luksDump /dev/sda5
LUKS header information
Version: 2
Epoch: 4
Metadata area: 16384 [bytes]
Keyslots area: 16744448 [bytes]
UUID: 3c16ad4c-294c-4547-bf3e-bb8864ba5ea3
Label: (no label)
Subsystem: (no subsystem)
Flags: (no flags)

Data segments:
0: crypt
offset: 16777216 [bytes]
length: (whole device)
cipher: aes-xts-plain64
sector: 512 [bytes]

Keyslots:
Tokens:
Digests:
0: pbkdf2
Hash: sha256
Iterations: 59904
Salt: XX XX XX XX XX ....
Digest: XX XX XX XX XX ...

----------------------------------------

I changed salt and digest. No Keyslots are present!!!

I tried this 2 times in a row with new install,
exactly same result.



-- System Information:
Debian Release: buster/sid
APT prefers testing
APT policy: (500, 'testing')
Architecture: amd64 (x86_64)
Foreign Architectures: i386

Kernel: Linux 5.0.8-xanmod5 (SMP w/2 CPU cores; PREEMPT)
Locale: LANG=ru_RU.UTF-8, LC_CTYPE=ru_RU.UTF-8 (charmap=UTF-8), LANGUAGE=ru_RU.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /usr/bin/dash
Init: systemd (via /run/systemd/system)
LSM: AppArmor: enabled

Versions of packages gnome-disk-utility depends on:
ii dconf-gsettings-backend [gsettings-backend] 0.30.1-2
ii libatk1.0-0 2.30.0-2
ii libc6 2.28-10
ii libcairo2 1.16.0-4
ii libcanberra-gtk3-0 0.30-7
ii libdvdread4 6.0.1-1
ii libgdk-pixbuf2.0-0 2.38.1+dfsg-1
ii libglib2.0-0 2.58.3-1
ii libgtk-3-0 3.24.5-1
ii liblzma5 5.2.4-1
ii libnotify4 0.7.7-4
ii libpango-1.0-0 1.42.4-6
ii libpangocairo-1.0-0 1.42.4-6
ii libpwquality1 1.4.0-3
ii libsecret-1-0 0.18.7-1
ii libsystemd0 241-3
ii libudisks2-0 2.8.1-4
ii udisks2 2.8.1-4

gnome-disk-utility recommends no packages.

gnome-disk-utility suggests no packages.

-- no debconf information
Marco Villegas
2019-07-03 18:00:02 UTC
Permalink
Starting from buster installer rc3, and installing minimal dependencies
to run, I can confirm the problem exists.

While trying to change the passphrase from gnome-disks, i.e. the old
and new passphrases were entered, I see the following error message:
"Error changing passphrase: Invalid argument (udisks-error-quark, 0)",
screenshot attached.

I have also tried to access again the disk from an external gparted
after the attempt, and I could not unlock the partition with neither
the old nor the new passphrase.

Best,
Paul Gevers
2019-07-03 19:10:02 UTC
Permalink
Control: clone -1 -2
Control: reassign -2 release-notes
Post by Marco Villegas
Starting from buster installer rc3, and installing minimal dependencies
to run, I can confirm the problem exists.
While trying to change the passphrase from gnome-disks, i.e. the old
"Error changing passphrase: Invalid argument (udisks-error-quark, 0)",
screenshot attached.
I have also tried to access again the disk from an external gparted
after the attempt, and I could not unlock the partition with neither
the old nor the new passphrase.
Let's document this in the release notes to warn people as this won't
get fixed in buster before the release.

Paul
Paul Gevers
2019-07-04 08:00:02 UTC
Permalink
Control: tags -1 patch pending

Hi,
Post by Marco Villegas
Starting from buster installer rc3, and installing minimal dependencies
to run, I can confirm the problem exists.
While trying to change the passphrase from gnome-disks, i.e. the old
"Error changing passphrase: Invalid argument (udisks-error-quark, 0)",
screenshot attached.
I have also tried to access again the disk from an external gparted
after the attempt, and I could not unlock the partition with neither
the old nor the new passphrase.
I have prepared the attached text. I will push this shortly. Reviews
still welcome of course.

Paul
Justin B Rye
2019-07-04 08:30:02 UTC
Permalink
+ <section id="g-d-u-and-luks-password">
+ <!-- stretch to buster -->
+ <title>
+ gnome-disk-utility fails to change LUKS password causing permamently data
^ ^^
+ loss
+ </title>
Typo and grammar error.
+ <para>
+ Don't change the LUKS password of encrypted disks with the GNOME
Users shouldn't
Style: factual advice rather than commands
+ graphical interface for disk management. The <systemitem
+ role="package">gnome-disk-utility</systemitem> package in buster has a
+ very nasty <ulink url="&url-bts;928893">bug (#928893)</ulink> that will
+ delete the old password but fails to correctly set the new password when
Rephrased to avoid saying the bug fails.
+ it is used to change the LUKS password. As a result all data on the disk
Move "when used"... to allow shorter phrasing.
+ will become inaccessible.
+ </para>
+ </section>
+
</section>
</chapter>
--
JBR with qualifications in linguistics, experience as a Debian
sysadmin, and probably no clue about this particular package
Narcis Garcia
2019-07-06 04:50:01 UTC
Permalink
Simple:
A) Package gnome-disk-utility with this feature disabled, until bug is
solved.
B) Not including buggy gnome-disk-utility in repositories

Is really dangerous for people to have this possibility to loss their
data, because of a small piece of bad code or bad release decision.
--
__________
I'm using this express-made address because personal addresses aren't
masked enough at this mail public archive. Public archive administrator
should fix this against automated addresses collectors.
intrigeri
2019-07-20 01:20:01 UTC
Permalink
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
Guilhem Moulin
2019-07-20 09:10:01 UTC
Permalink
Hi there,
Post by intrigeri
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 <<<test
$ ./928893 /tmp/disk.img "test" "test2"
Key slot 0 is full, please select another one.
928893: Error: crypt_keyslot_add_by_volume_key

The third API call fails because key slot 0 is already enabled. But key
slot 1 was already removed.


Re-format the device as LUKSv2, with a single key slot unlocked with
passphrase “test”.

$ cryptsetup luksFormat --pbkdf-force-iterations 4 --pbkdf-memory 32 \
--type luks2 -q /tmp/disk.img <<<test
$ ./928893 /tmp/disk.img "test" "test2"
Failed to initialise default LUKS2 keyslot parameters.
928893: Error: crypt_keyslot_add_by_volume_key

Ka-boom, the only key slot was removed but not re-created. The whole
device will be lost when the user reboot. This *may* also happen with v1
if for whatever reason (hardware failure for instance) the new key slot
can't be added. But it *always* happens with LUKSv2, which is Buster's
default LUKS format version for `luksFormat`.

Investigating further why crypt_keyslot_add_by_volume_key() *always*
fails with LUKSv2, with some help from gdb I believe the calltrace goes
as follows:

lib/setup.c:crypt_keyslot_add_by_volume_key()
calls crypt_keyslot_add_by_key() at line 3414
lib/setup.c:crypt_keyslot_add_by_key()
calls LUKS2_keyslot_params_default() at line 5291
lib/luks2/luks2_keyslot.c:LUKS2_keyslot_params_default()
calls crypt_keyslot_get_encryption() at line 154
lib/setup.c:crypt_keyslot_get_encryption()
calls crypt_get_volume_key_size() at line 4634
lib/setup.c:crypt_get_volume_key_size()
calls LUKS2_get_volume_key_size() at line 4550, which returns -1
and cd->volume_key is NULL so crypt_get_volume_key_size()
returns 0

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. The volume key is known to the caller,
but not to crypt_keyslot_get_encryption(). This is arguably a libcryptsetup
bug, but blindly calling crypt_keyslot_add_by_volume_key() *after*
crypt_keyslot_destroy() is extremely problematic in itself, and
usingcrypt_keyslot_add_by_volume_key() on an header without any
key-slots is of limited interest. As intrigeri wrote, libblockdev 2.22
fixes the problem by replacing these 2 libcryptsetup API calls with the
more robust crypt_keyslot_change_by_passphrase() (available in
libcryptsetup ≥1.6.0).

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.

Cheers,
--
Guilhem.

[0] https://gitlab.com/cryptsetup/LUKS2-docs/blob/master/luks2_doc_wip.pdf
sec. 3.2
Guilhem Moulin
2019-07-20 20:30:01 UTC
Permalink
Post by Guilhem Moulin
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 <<<test
$ cryptsetup luksAddKey --pbkdf-force-iterations 4 --pbkdf-memory 32 \
--new-keyfile-size 32 /tmp/disk.img /dev/urandom <<<test
$ ./928893 /tmp/disk.img "test" "test2"

Works fine. The first (index #0) key slot is updated with the new
passphrase, while the other (random) one is left unchanged. It works
because by the time crypt_keyslot_add_by_volume_key() is called there is
a bound (ie assigned to a segment) keyslot left, and the volume key size
can be obtained from its ‘key_size’ JSON field.

Just filed a bug against cryptsetup / libcryptsetup:
https://gitlab.com/cryptsetup/cryptsetup/issues/466

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.
--
Guilhem.
Guilhem Moulin
2019-07-21 20:10:01 UTC
Permalink
Hi,
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.
Loading...