[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#934956: marked as done (buster-pu: package cryptsetup/2:2.1.0-5+deb10u1)

Your message dated Sat, 07 Sep 2019 14:34:49 +0100
with message-id <[🔎] f49e2985d8466065c49c03185c24465a32228fb5.camel@adam-barratt.org.uk>
and subject line Closing bugs for fixes including in 10.1 point release
has caused the Debian Bug report #934956,
regarding buster-pu: package cryptsetup/2:2.1.0-5+deb10u1
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
Bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact owner@bugs.debian.org
immediately.)


-- 
934956: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=934956
Debian Bug Tracking System
Contact owner@bugs.debian.org with problems
--- Begin Message --- 
Package: release.debian.org
Severity: normal
Tags: buster
User: release.debian.org@packages.debian.org
Usertags: pu

Dear release team,

Buster's cryptsetup (2:2.1.0-5) doesn't cope well with LUKS2 headers
without any bound keyslot: adding a new key slot to such a header fails,
both via the crypt_keyslot_add_by_volume_key() API call and with
`luksAddKey --master-key`.

This alone doesn't warrant a s-p-u, but some applications seem to use
the following sequences of API calls to change a passphrase:

    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);

This is a *very* bad idea in the first place, and they should use
crypt_keyslot_change_by_passphrase() instead: if there was only one
active keyslot and for some reason the crypt_keyslot_add_by_volume_key()
call fails (for instance due to an hardware failure, or because the
PBKDF benchmark triggers the OOM killer) then the entire volume is lost…
However the libcryptsetup bug makes it much, much worse as the call
*always* fails on LUKS2 headers without any bound keyslot.  This is what
happened in #928893 (gnome-disk-utility: disk content permanently lost
when changing LUKS password).

Using codesearch.d.n I found that (as far as sid is concerned) beside
src:cryptsetup, only src:libblockdev and src:cryptmount are calling
crypt_keyslot_destroy().  AFAICT src:cryptmount is making a sane use of
the call [0]; libblockdev is affected in Buster but per #932588 will be
fixed to use crypt_keyslot_change_by_passphrase() in the upcoming point
release.  Still there might be third party programs we don't know about,
and considering the serious risk of data loss I think it makes sense to
fix the libcryptsetup bug in the next point release, too.  Changelog
since 2:2.1.0-5 is as follows, and debdiff attached.

cryptsetup (2:2.1.0-5+deb10u1) buster; urgency=high

  * Backport upstream commits c03e3fe8, 725720df and fe4e1de5 to fix support
    for LUKS2 headers without any bound keyslot.  Adding a new key slot using
    the volume key was failing, both via the crypt_keyslot_add_by_volume_key()
    API call and with `luksAddKey --master-key`.  The former in particular
    might yield data loss if, in order to change a passphrase, an application
    destroys the keyslot before adding a new one (using the volume key), cf.
    #928893.  Note that doing so is *unsafe*: applications should instead use
    crypt_keyslot_change_by_passphrase() from libcryptsetup >=1.6.0.
    Trying to open LUKS2 volume by supplying the volume key on the command
    line was also failing if there were no bound keyslot on the header.
    (Closes: #934715)

 -- Guilhem Moulin <guilhem@debian.org>  Fri, 16 Aug 2019 19:18:10 +0200

The 3 cherry-picked patches are all backported from 2.2.0 [1,2], and the
version in sid is not affected.  (The one in Stretch is not affected
either as it doesn't have LUKS2 support.)  The diff also includes unit
tests, but note that the tests in question need root access hence are
ignored by the build daemons.  I did ensure that the whole test-suite
still passes on amd64, though.

In case you're unhappy with this changeset, then I propose to only
include the first 2 patches.  IMHO what should really be fixed in Buster
is the libcryptsetup part.  For the CLI part (third patch) the risk of
data loss is lower as the volume key is stored in a file.

Thanks for considering its inclusion in Buster!  CC'ing KiBi for the d-i ack.
Cheers,
-- 
Guilhem.

[0] https://codesearch.debian.net/show?file=cryptmount_5.3.1-1%2Farmour-luks.c&line=247
[1] https://gitlab.com/cryptsetup/cryptsetup/issues/466
[2] https://gitlab.com/cryptsetup/cryptsetup/issues/470

diffstat for cryptsetup-2.1.0 cryptsetup-2.1.0

 changelog                                                          |   16 +
 gbp.conf                                                           |    1 
 patches/Fix-getting-default-LUKS2-keyslot-encryption-paramet.patch |   56 ++++++
 patches/Fix-volume-key-file-if-no-LUKS2-keyslots-are-present.patch |   86 ++++++++++
 patches/Mention-limitation-of-crypt_get_volume_key_size.patch      |   20 ++
 patches/series                                                     |    3 
 6 files changed, 182 insertions(+)

diff -Nru cryptsetup-2.1.0/debian/changelog cryptsetup-2.1.0/debian/changelog
--- cryptsetup-2.1.0/debian/changelog	2019-06-10 14:51:15.000000000 +0200
+++ cryptsetup-2.1.0/debian/changelog	2019-08-16 19:18:10.000000000 +0200
@@ -1,3 +1,19 @@
+cryptsetup (2:2.1.0-5+deb10u1) buster; urgency=high
+
+  * Backport upstream commits c03e3fe8, 725720df and fe4e1de5 to fix support
+    for LUKS2 headers without any bound keyslot.  Adding a new key slot using
+    the volume key was failing, both via the crypt_keyslot_add_by_volume_key()
+    API call and with `luksAddKey --master-key`.  The former in particular
+    might yield data loss if, in order to change a passphrase, an application
+    destroys the keyslot before adding a new one (using the volume key), cf.
+    #928893.  Note that doing so is *unsafe*: applications should instead use
+    crypt_keyslot_change_by_passphrase() from libcryptsetup >=1.6.0.
+    Trying to open LUKS2 volume by supplying the volume key on the command
+    line was also failing if there were no bound keyslot on the header.
+    (Closes: #934715)
+
+ -- Guilhem Moulin <guilhem@debian.org>  Fri, 16 Aug 2019 19:18:10 +0200
+
 cryptsetup (2:2.1.0-5) unstable; urgency=medium
 
   [ Jonas Meurer ]
diff -Nru cryptsetup-2.1.0/debian/gbp.conf cryptsetup-2.1.0/debian/gbp.conf
--- cryptsetup-2.1.0/debian/gbp.conf	2019-06-10 14:51:15.000000000 +0200
+++ cryptsetup-2.1.0/debian/gbp.conf	2019-08-16 19:18:10.000000000 +0200
@@ -4,3 +4,4 @@
 [buildpackage]
 upstream-tag    = v%(version)s
 upstream-branch = upstream-2.0.x
+debian-branch   = debian-buster
diff -Nru cryptsetup-2.1.0/debian/patches/Fix-getting-default-LUKS2-keyslot-encryption-paramet.patch cryptsetup-2.1.0/debian/patches/Fix-getting-default-LUKS2-keyslot-encryption-paramet.patch
--- cryptsetup-2.1.0/debian/patches/Fix-getting-default-LUKS2-keyslot-encryption-paramet.patch	1970-01-01 01:00:00.000000000 +0100
+++ cryptsetup-2.1.0/debian/patches/Fix-getting-default-LUKS2-keyslot-encryption-paramet.patch	2019-08-16 19:18:10.000000000 +0200
@@ -0,0 +1,56 @@
+From c03e3fe88a9761f34b22d2b4d4654353783e2d4f Mon Sep 17 00:00:00 2001
+From: Ondrej Kozina <okozina@redhat.com>
+Date: Tue, 26 Feb 2019 11:49:58 +0100
+Subject: Fix getting default LUKS2 keyslot encryption parameters.
+
+When information about original keyslot size is missing (no active
+keyslot assigned to default segment) we have to fallback to
+default luks2 encryption parameters even though we know default
+segment cipher and mode.
+
+Fixes: #442.
+---
+ lib/setup.c        |    3 ++-
+ tests/api-test-2.c |   19 +++++++++++++++++++
+ 2 files changed, 21 insertions(+), 1 deletion(-)
+
+--- a/lib/setup.c
++++ b/lib/setup.c
+@@ -4632,7 +4632,8 @@ const char *crypt_keyslot_get_encryption
+ 	cipher =  LUKS2_get_cipher(&cd->u.luks2.hdr, CRYPT_DEFAULT_SEGMENT);
+ 	if (!LUKS2_keyslot_cipher_incompatible(cd, cipher)) {
+ 		*key_size = crypt_get_volume_key_size(cd);
+-		return cipher;
++		if (*key_size)
++			return cipher;
+ 	}
+ 
+ 	/* Fallback to default LUKS2 keyslot encryption */
+--- a/tests/api-test-2.c
++++ b/tests/api-test-2.c
+@@ -914,6 +914,25 @@ static void AddDeviceLuks2(void)
+ 	FAIL_(crypt_activate_by_volume_key(cd, CDEVICE_1, key3, key_size, 0), "VK doesn't match any digest assigned to segment 0");
+ 	crypt_free(cd);
+ 
++	/*
++	 * Check regression in getting keyslot encryption parameters when
++	 * volume key size is unknown (no active keyslots).
++	 */
++	if (!_fips_mode) {
++		OK_(crypt_init(&cd, DMDIR L_DEVICE_1S));
++		crypt_set_iteration_time(cd, 1);
++		OK_(crypt_format(cd, CRYPT_LUKS2, cipher, cipher_mode, NULL, key, key_size, NULL));
++		EQ_(crypt_keyslot_add_by_volume_key(cd, 0, NULL, key_size, PASSPHRASE, strlen(PASSPHRASE)), 0);
++		/* drop context copy of volume key */
++		crypt_free(cd);
++		OK_(crypt_init(&cd, DMDIR L_DEVICE_1S));
++		OK_(crypt_load(cd, CRYPT_LUKS, NULL));
++		EQ_(crypt_volume_key_get(cd, CRYPT_ANY_SLOT, key, &key_size, PASSPHRASE, strlen(PASSPHRASE)), 0);
++		OK_(crypt_keyslot_destroy(cd, 0));
++		EQ_(crypt_keyslot_add_by_volume_key(cd, 0, key, key_size, PASSPHRASE, strlen(PASSPHRASE)), 0);
++		crypt_free(cd);
++	}
++
+ 	_cleanup_dmdevices();
+ }
+ 
diff -Nru cryptsetup-2.1.0/debian/patches/Fix-volume-key-file-if-no-LUKS2-keyslots-are-present.patch cryptsetup-2.1.0/debian/patches/Fix-volume-key-file-if-no-LUKS2-keyslots-are-present.patch
--- cryptsetup-2.1.0/debian/patches/Fix-volume-key-file-if-no-LUKS2-keyslots-are-present.patch	1970-01-01 01:00:00.000000000 +0100
+++ cryptsetup-2.1.0/debian/patches/Fix-volume-key-file-if-no-LUKS2-keyslots-are-present.patch	2019-08-16 19:18:10.000000000 +0200
@@ -0,0 +1,86 @@
+From 725720dfc31ff26c4a60089a478fe5e882925ef3 Mon Sep 17 00:00:00 2001
+From: Milan Broz <gmazyland@gmail.com>
+Date: Wed, 14 Aug 2019 12:31:40 +0200
+Subject: Fix volume key file if no LUKS2 keyslots are present.
+
+If all keyslots are removed, LUKS2 has no longer information about
+the volume key size (there is only key digest present).
+
+If user wants to open or add new keyslot, it must get information
+about key size externally.
+
+We do not want to guess key size from the file size (it does not
+work for block devices for example), so require explicit --keyfil
+option in these cases.
+
+Fixes #470.
+---
+ src/cryptsetup.c   |   18 ++++++++++++++++--
+ tests/compat-test2 |    7 ++++++-
+ 2 files changed, 22 insertions(+), 3 deletions(-)
+
+--- a/src/cryptsetup.c
++++ b/src/cryptsetup.c
+@@ -1249,6 +1249,13 @@ static int action_open_luks(void)
+ 
+ 	if (opt_master_key_file) {
+ 		keysize = crypt_get_volume_key_size(cd);
++		if (!keysize && !opt_key_size) {
++			log_err(_("Cannot dermine volume key size for LUKS without keyslots, please use --key-size option."));
++			r = -EINVAL;
++			goto out;
++		} else if (!keysize)
++			keysize = opt_key_size / 8;
++
+ 		r = tools_read_mk(opt_master_key_file, &key, keysize);
+ 		if (r < 0)
+ 			goto out;
+@@ -1553,6 +1560,13 @@ static int action_luksAddKey(void)
+ 	}
+ 
+ 	if (opt_master_key_file) {
++		if (!keysize && !opt_key_size) {
++			log_err(_("Cannot dermine volume key size for LUKS without keyslots, please use --key-size option."));
++			r = -EINVAL;
++			goto out;
++		} else if (!keysize)
++			keysize = opt_key_size / 8;
++
+ 		r = tools_read_mk(opt_master_key_file, &key, keysize);
+ 		if (r < 0)
+ 			goto out;
+@@ -2752,9 +2766,9 @@ int main(int argc, const char **argv)
+ 	   strcmp(aname, "luksFormat") &&
+ 	   strcmp(aname, "open") &&
+ 	   strcmp(aname, "benchmark") &&
+-	   (strcmp(aname, "luksAddKey") || !opt_unbound))
++	   strcmp(aname, "luksAddKey"))
+ 		usage(popt_context, EXIT_FAILURE,
+-		      _("Option --key-size is allowed only for luksFormat, luksAddKey (with --unbound),\n"
++		      _("Option --key-size is allowed only for luksFormat, luksAddKey,\n"
+ 			"open and benchmark actions. To limit read from keyfile use --keyfile-size=(bytes)."),
+ 		      poptGetInvocationName(popt_context));
+ 
+--- a/tests/compat-test2
++++ b/tests/compat-test2
+@@ -492,7 +492,7 @@ echo $PWD1 | $CRYPTSETUP luksOpen $LOOPD
+ $CRYPTSETUP  luksClose  $DEV_NAME || fail
+ 
+ prepare "[21] luksDump" wipe
+-echo $PWD1 | $CRYPTSETUP -q luksFormat $FAST_PBKDF_OPT --uuid $TEST_UUID --type luks2 $LOOPDEV $KEY1 || fail
++echo $PWD1 | $CRYPTSETUP -q luksFormat --key-size 256 $FAST_PBKDF_OPT --uuid $TEST_UUID --type luks2 $LOOPDEV $KEY1 || fail
+ echo $PWD1 | $CRYPTSETUP luksAddKey $FAST_PBKDF_OPT $LOOPDEV -d $KEY1 || fail
+ $CRYPTSETUP luksDump $LOOPDEV | grep -q "0: luks2" || fail
+ $CRYPTSETUP luksDump $LOOPDEV | grep -q $TEST_UUID || fail
+@@ -504,6 +504,11 @@ echo $PWD1 | $CRYPTSETUP luksDump -q $LO
+ fips_mode || {
+ 	echo $PWD1 | $CRYPTSETUP luksAddKey $FAST_PBKDF_OPT --master-key-file $VK_FILE $LOOPDEV || fail
+ }
++# Use volume key file without keyslots
++$CRYPTSETUP luksErase -q $LOOPDEV || fail
++$CRYPTSETUP luksOpen --master-key-file $VK_FILE --key-size 256 --test-passphrase $LOOPDEV || fail
++echo $PWD1 | $CRYPTSETUP luksAddKey $FAST_PBKDF_OPT --master-key-file $VK_FILE --key-size 256 $LOOPDEV || fail
++echo $PWD1 | $CRYPTSETUP luksOpen --test-passphrase $LOOPDEV || fail
+ 
+ prepare "[22] remove disappeared device" wipe
+ dmsetup create $DEV_NAME --table "0 39998 linear $LOOPDEV 2" || fail
diff -Nru cryptsetup-2.1.0/debian/patches/Mention-limitation-of-crypt_get_volume_key_size.patch cryptsetup-2.1.0/debian/patches/Mention-limitation-of-crypt_get_volume_key_size.patch
--- cryptsetup-2.1.0/debian/patches/Mention-limitation-of-crypt_get_volume_key_size.patch	1970-01-01 01:00:00.000000000 +0100
+++ cryptsetup-2.1.0/debian/patches/Mention-limitation-of-crypt_get_volume_key_size.patch	2019-08-16 19:18:10.000000000 +0200
@@ -0,0 +1,20 @@
+From fe4e1de56639f1e6851ff8e47729f703a25dece4 Mon Sep 17 00:00:00 2001
+From: Milan Broz <gmazyland@gmail.com>
+Date: Mon, 29 Jul 2019 14:32:13 +0200
+Subject: Mention limitation of crypt_get_volume_key_size().
+
+---
+ lib/libcryptsetup.h |    2 ++
+ 1 file changed, 2 insertions(+)
+
+--- a/lib/libcryptsetup.h
++++ b/lib/libcryptsetup.h
+@@ -1448,6 +1448,8 @@ uint64_t crypt_get_iv_offset(struct cryp
+  *
+  * @return volume key size
+  *
++ * @note For LUKS2, this function can be used only if there is at least
++ *       one keyslot assigned to data segment.
+  */
+ int crypt_get_volume_key_size(struct crypt_device *cd);
+ 
diff -Nru cryptsetup-2.1.0/debian/patches/series cryptsetup-2.1.0/debian/patches/series
--- cryptsetup-2.1.0/debian/patches/series	1970-01-01 01:00:00.000000000 +0100
+++ cryptsetup-2.1.0/debian/patches/series	2019-08-16 19:18:10.000000000 +0200
@@ -0,0 +1,3 @@
+Fix-getting-default-LUKS2-keyslot-encryption-paramet.patch
+Mention-limitation-of-crypt_get_volume_key_size.patch
+Fix-volume-key-file-if-no-LUKS2-keyslots-are-present.patch

Attachment: signature.asc
Description: PGP signature


--- End Message ---
--- Begin Message --- 
Version: 10.1

Hi,

The fixes referenced by each of these bugs were included in today's
buster point release.

Regards,

Adam

--- End Message ---
Reply to: