From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1Z7RzT-0003CE-NG for mharc-grub-devel@gnu.org; Tue, 23 Jun 2015 13:28:07 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47626) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z7RzQ-00037e-J7 for grub-devel@gnu.org; Tue, 23 Jun 2015 13:28:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z7RzN-00046I-9r for grub-devel@gnu.org; Tue, 23 Jun 2015 13:28:04 -0400 Received: from johnlane.plus.com ([212.159.104.145]:62385 helo=sodium.amajohn.co.uk) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z7RzM-000460-QC for grub-devel@gnu.org; Tue, 23 Jun 2015 13:28:01 -0400 Received: by sodium.amajohn.co.uk (Postfix, from userid 1000) id 5E84040; Tue, 23 Jun 2015 18:27:58 +0100 (BST) Received: from [10.0.200.1] (hydrogen.amajohn.co.uk [10.0.200.1]) by sodium.amajohn.co.uk (Postfix) with ESMTPSA id 8F52028; Tue, 23 Jun 2015 18:27:57 +0100 (BST) Message-ID: <5589971C.3070207@jelmail.com> Date: Tue, 23 Jun 2015 18:27:56 +0100 From: John Lane User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.2 MIME-Version: 1.0 To: Andrei Borzenkov Subject: Re: [PATCH 2/4] Cryptomount support key files References: <1434445875-6846-1-git-send-email-john@lane.uk.net> <1434445875-6846-3-git-send-email-john@lane.uk.net> <20150620075404.1efc15d5@opensuse.site> In-Reply-To: <20150620075404.1efc15d5@opensuse.site> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Outbound-Checked: Yes X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 212.159.104.145 Cc: grub-devel@gnu.org X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Jun 2015 17:28:06 -0000 -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Comments inline. I'll resubmit the patch set with changes as per comments= =2E On 20/06/15 05:54, Andrei Borzenkov wrote: > =D0=92 Tue, 16 Jun 2015 10:11:13 +0100 > John Lane =D0=BF=D0=B8=D1=88=D0=B5=D1=82: > >> --- >> grub-core/disk/cryptodisk.c | 34 +++++++++++++++++++++++++++++++-- >> grub-core/disk/geli.c | 4 +++- >> grub-core/disk/luks.c | 46 +++++++++++++++++++++++++++++++-------------- >> include/grub/cryptodisk.h | 5 ++++- >> 4 files changed, 71 insertions(+), 18 deletions(-) >> >> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c= >> index 65b3a01..fbe7db9 100644 >> --- a/grub-core/disk/cryptodisk.c >> +++ b/grub-core/disk/cryptodisk.c >> @@ -41,6 +41,10 @@ static const struct grub_arg_option options[] =3D >> {"all", 'a', 0, N_("Mount all."), 0, 0}, >> {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0}, >> {"header", 'H', 0, N_("Read LUKS header from file"), 0, ARG_TYPE_STRING}, >> + {"keyfile", 'k', 0, N_("Key file"), 0, ARG_TYPE_STRING}, >> + {"key-size", 'K', 0, N_("Set key size (bits)"), 0, ARG_TYPE_INT},= > It is unused That's a mistake from when I split the patch in two. The key size argument is only used by plain mode. (in LUKS mode it's obtained from the header). I'll re-do the patches with that in the plain-mode one. > >> + {"keyfile-offset", 'O', 0, N_("Key file offset (bytes)"), 0, ARG_TYPE_INT}, >> + {"keyfile-size", 'S', 0, N_("Key file data size (bytes)"), 0, ARG_TYPE_INT}, >> {0, 0, 0, 0, 0, 0} >> }; >>=20 >> @@ -805,6 +809,8 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk) >> static int check_boot, have_it; >> static char *search_uuid; >> static grub_file_t hdr; >> +static grub_uint8_t *key, keyfile_buffer[GRUB_CRYPTODISK_MAX_KEYFILE_SIZE]; >> +static grub_size_t keyfile_size; >>=20 > Someone should really get rid of static variables and pass them as > callback data. I just followed the conventions used by the existing code. I am not familiar enough with the code-base to change how it does things. > >> static void >> cryptodisk_close (grub_cryptodisk_t dev) >> @@ -835,7 +841,7 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source) >> if (!dev) >> continue; >> =20 >> - err =3D cr->recover_key (source, dev, hdr); >> + err =3D cr->recover_key (source, dev, hdr, key, keyfile_size); > You never clear key variable, so after the first call all subsequent > invocations will always use it for any device. Moving to proper > callback data would make such errors less likely. It is cleared when the arguments are processed, specifically "grub_cmd_cryptomount" sets "key =3D NULL". I have tested multiple uses and it does work as expected. > >> if (err) >> { >> cryptodisk_close (dev); >> @@ -938,12 +944,36 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args) >> hdr =3D grub_file_open (state[3].arg); >> if (!hdr) >> return grub_errno; >> - grub_printf ("\nUsing detached header %s\n", state[3].arg); > You remove line just added in previous patch? Why previous patch added > it then? I thought I'd removed that. Will re-do. > >> } >> else >> hdr =3D NULL; >>=20 >> have_it =3D 0; >> + >> + if (state[4].set) /* Key file */ >> + { >> + grub_file_t keyfile; >> + int keyfile_offset; >> + >> + key =3D keyfile_buffer; >> + keyfile_offset =3D state[6].set ? grub_strtoul (state[6].arg, 0= , 0) : 0; >> + keyfile_size =3D state[7].set ? grub_strtoul (state[7].arg, 0, 0) : \ >> + GRUB_CRYPTODISK_MAX_KEYFILE_SIZE= ; >> + >> + keyfile =3D grub_file_open (state[4].arg); >> + if (!keyfile) >> + return grub_errno; >> + >> + if (grub_file_seek (keyfile, keyfile_offset) =3D=3D (grub_off_t= )-1) >> + return grub_errno; >> + >> + keyfile_size =3D grub_file_read (keyfile, key, keyfile_size); >> + if (keyfile_size =3D=3D (grub_size_t)-1) >> + return grub_errno; > If keyfile size is explicitly given, I expect that short read should be= > considered an error. Otherwise what is the point of explicitly giving > size? A short read is accepted by the upstream "cryptsetup" tool. Its man page describes its "--keyfile-size" argument as "Read a maximum of value bytes from the key file. Default is to read the whole file up to the compiled-in maximum. The cryptomount command follows that rule. > >> + } >> + else >> + key =3D NULL; >> + >> if (state[0].set) >> { >> grub_cryptodisk_t dev; >> diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c >> index f4394eb..da6aa6a 100644 >> --- a/grub-core/disk/geli.c >> +++ b/grub-core/disk/geli.c >> @@ -401,7 +401,9 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid, >>=20 >> static grub_err_t >> recover_key (grub_disk_t source, grub_cryptodisk_t dev, >> - grub_file_t hdr __attribute__ ((unused)) ) >> + grub_file_t hdr __attribute__ ((unused)), >> + grub_uint8_t *key __attribute__ ((unused)), >> + grub_size_t keyfile_size __attribute__ ((unused)) ) >> { >> grub_size_t keysize; >> grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN]; >> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c >> index 66e64c0..0d0db9d 100644 >> --- a/grub-core/disk/luks.c >> +++ b/grub-core/disk/luks.c >> @@ -136,6 +136,8 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid, >> ciphermode[sizeof (header.cipherMode)] =3D 0; >> grub_memcpy (hashspec, header.hashSpec, sizeof (header.hashSpec)); >> hashspec[sizeof (header.hashSpec)] =3D 0; >> + grub_memcpy (uuid, header.uuid, sizeof (header.uuid)); >> + uuid[sizeof (header.uuid)] =3D 0; >>=20 > How exactly this is related to keyfile support? it isn't. it belongs in one of the other patches. will fix. > > >> ciph =3D grub_crypto_lookup_cipher_by_name (ciphername); >> if (!ciph) >> @@ -322,12 +324,16 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid, >> static grub_err_t >> luks_recover_key (grub_disk_t source, >> grub_cryptodisk_t dev, >> - grub_file_t hdr) >> + grub_file_t hdr, >> + grub_uint8_t *keyfile_bytes, >> + grub_size_t keyfile_bytes_size) >> { >> struct grub_luks_phdr header; >> grub_size_t keysize; >> grub_uint8_t *split_key =3D NULL; >> - char passphrase[MAX_PASSPHRASE] =3D ""; >> + char interactive_passphrase[MAX_PASSPHRASE] =3D ""; >> + grub_uint8_t *passphrase; >> + grub_size_t passphrase_length; >> grub_uint8_t candidate_digest[sizeof (header.mkDigest)]; >> unsigned i; >> grub_size_t length; >> @@ -364,18 +370,30 @@ luks_recover_key (grub_disk_t source, >> if (!split_key) >> return grub_errno; >>=20 >> - /* Get the passphrase from the user. */ >> - tmp =3D NULL; >> - if (source->partition) >> - tmp =3D grub_partition_get_name (source->partition); >> - grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->nam= e, >> - source->partition ? "," : "", tmp ? : "", >> - dev->uuid); >> - grub_free (tmp); >> - if (!grub_password_get (passphrase, MAX_PASSPHRASE)) >> + if (keyfile_bytes) >> { >> - grub_free (split_key); >> - return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied"); >> + /* Use bytestring from key file as passphrase */ >> + passphrase =3D keyfile_bytes; >> + passphrase_length =3D keyfile_bytes_size; >> + } >> + else > I'm not sure if this should be "else". I think normal behavior of user > space is to ask for password then. If keyfile fails we cannot continue > anyway. Not sure I follow you. This is saying that the key file data should be used if given ELSE ask the user for a passphrase. > >> + { >> + /* Get the passphrase from the user. */ >> + tmp =3D NULL; >> + if (source->partition) >> + tmp =3D grub_partition_get_name (source->partition); >> + grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name, >> + source->partition ? "," : "", tmp ? : "", dev->uuid); >> + grub_free (tmp); >> + if (!grub_password_get (interactive_passphrase, MAX_PASSPHRASE)= ) >> + { >> + grub_free (split_key); >> + return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied"); >> + } >> + >> + passphrase =3D (grub_uint8_t *)interactive_passphrase; >> + passphrase_length =3D grub_strlen (interactive_passphrase); >> + >> } >>=20 >> /* Try to recover master key from each active keyslot. */ >> @@ -393,7 +411,7 @@ luks_recover_key (grub_disk_t source, >>=20 >> /* Calculate the PBKDF2 of the user supplied passphrase. */ >> gcry_err =3D grub_crypto_pbkdf2 (dev->hash, (grub_uint8_t *) passphrase, >> - grub_strlen (passphrase), >> + passphrase_length, >> header.keyblock[i].passwordSalt, >> sizeof (header.keyblock[i].passwordSalt), >> grub_be_to_cpu32 (header.keyblock[i]. >> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h >> index 16dee3c..0299625 100644 >> --- a/include/grub/cryptodisk.h >> +++ b/include/grub/cryptodisk.h >> @@ -55,6 +55,8 @@ typedef enum >> #define GRUB_CRYPTODISK_GF_BYTES (1U << GRUB_CRYPTODISK_GF_LOG_BYTES)= >> #define GRUB_CRYPTODISK_MAX_KEYLEN 128 >>=20 >> +#define GRUB_CRYPTODISK_MAX_KEYFILE_SIZE 8192 >> + > Why it is different from MAX_KEYLEN? A keyfile can be bigger than a key. A offset into the keyfile gives the start of the key. The given value is what is implemented by cryptsetup (as reported by "cryptsetup --help"). The value of MAX_KEYLEN is what existed in Grub prior to patching. > >> struct grub_cryptodisk; >>=20 >> typedef gcry_err_code_t >> @@ -108,7 +110,8 @@ struct grub_cryptodisk_dev >>=20 >> grub_cryptodisk_t (*scan) (grub_disk_t disk, const char *check_uuid= , >> int boot_only, grub_file_t hdr); >> - grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev, grub_file_t hdr); >> + grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev,= >> + grub_file_t hdr, grub_uint8_t *key, grub_size_t keyfile_size); >> }; >> typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t; >>=20 -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJViZccAAoJEGnTYCRabxPG3FgP/3v62hWS/5fH9z4KGgRJvaCA gJSkuy8HcLwBurH8xLqaAwEZe9NVEMeDsbtjS5jeNFiylhYp2kYa1PAgOGb0aAQS I+i2Ek4soIgPQyC212JzpCot9GI+WUCXObQ/unXVaWrViqL3/DJRoo0Nhes1g0Gh qvLYJjfBb3Ujl2Ldo9ANWIGATzUSc/wi8oXl/mGWUQ0Gz3hBL3VDKcsjYq3Y8eQD JpSTr2dJrTKvY+3lnwTlQt4YSbKkOH+PvEdiB/jKGqkw0r7K3BQKGCiIpgeS8bbS bhLn24HuGIWfCKdZlqvBsmNuB6elUucTUIYkbvLqwD7Q6d/2a/30AzsgOpBgmCR0 dw6EUc0loTBqmpeeChS0Z0+JLMaFzRk8Yxq/FjrASejOXVL3sXLXO/2YW2LyvNTk PIHSuZ0u8juqwM12xI5eZ94pRq+LNyDvwPcdPqJHsiFyXYTn3JYlPAgD+4R0IHpV NWWQMIxTR5RdLkVoxGtyAIsKYGcxjd9nY4A0mRvmLZYx8jBBMi0L7XITLpoRy9ZX ib3a05pfSh6cM1dGHXPXnYjNXga2QBJ7MUrL3HdE48jLSbVd7LtBBpAFczBjrfyJ Xw5tFvRUbHM5qFBdwCMiFJTIOiQxinBQQfVQ+U0zKH+OPfTC+2svtpNLFr4hGVPS PblMr2bI0cZAv4zhD96j =3DyRSR -----END PGP SIGNATURE-----