From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1j8K8T-0005Nw-SM for mharc-grub-devel@gnu.org; Sun, 01 Mar 2020 03:39:41 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:43412) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1j8K8Q-0005Ng-E0 for grub-devel@gnu.org; Sun, 01 Mar 2020 03:39:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1j8K8O-0000PZ-PH for grub-devel@gnu.org; Sun, 01 Mar 2020 03:39:38 -0500 Received: from out4-smtp.messagingengine.com ([66.111.4.28]:35067) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1j8K8O-0000PQ-Av for grub-devel@gnu.org; Sun, 01 Mar 2020 03:39:36 -0500 Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailout.nyi.internal (Postfix) with ESMTP id AA47C224E4; Sun, 1 Mar 2020 03:39:35 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute4.internal (MEProxy); Sun, 01 Mar 2020 03:39:35 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm3; bh=po+QFqgh2DU2f4WhjatpHFp3yI5 0zhGHizj8FSVyILA=; b=cHVEKCv0ROVll/8Mar5dG8nqNPV9bU8PU4dKFmkboFk FzHWgPawtLdSruJ0aNL6M/RYOSAfliqG7VVtORKflAkRARBwpqpPbPohc0VfZ4vp q33ugNFbTevCrqAcz8asxQe6D6Jo4IckIqtbQMagF4/lqOWBZh//TvgE70PirLww JaFwDZLDEFQ/W7kfn+xFatAEMCeUP2NR2vFNqFnS3/sujS60k1lW7IxtmciODRA2 zIvOQ1Q1pK07e63H5yZ93P5WXybyE3BTZHxGkAsbtYkhqogt3jjrCMaJva+k73aL 6gie1d8yC7VU7fUjPbXXULZd3ieInclGPvNB4kC8pLw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm2; bh=po+QFq gh2DU2f4WhjatpHFp3yI50zhGHizj8FSVyILA=; b=yZR31FgM8L1lmtSu3wYMhY z6R3r8Skk9z7mc+YdbzwGr4SEXWYlXSfN/7tuJzVn92uvjJnSTq2zaJBeR5Vbocr o/YfUHCmzP/+LRyUzbNOvXIR9N5FTxGTsxzyh6d/2TITgrpoTk1iTGDk7Ya7i5t4 Cp+1AvB4dMz3LrXBjs0Z0GzlBU+588K4mUU1YzmyoCyGUC1qlNp7WUh1gapEjEsJ z2fK36BAncUztDKAZT1x4YM40aOdn0BnOi9L/aPqHt6e5ggjzUTHW2b9NMbUWHQi O4XSxLpukiPHwSG4tWRUN9tvJneSUF8ujJFBwyu7v0dP0L7d1Mbku3SiTTXf8IMw == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedugedruddtuddguddulecutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpeffhffvuffkfhggtggujgesghdtreertddtvdenucfhrhhomheprfgrthhr ihgtkhcuufhtvghinhhhrghrughtuceophhssehpkhhsrdhimheqnecukfhppeejjedrud efrddvfeekrddunecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhf rhhomhepphhssehpkhhsrdhimh X-ME-Proxy: Received: from vm-mail (x4d0dee01.dyn.telefonica.de [77.13.238.1]) by mail.messagingengine.com (Postfix) with ESMTPA id 9C91D328005D; Sun, 1 Mar 2020 03:39:34 -0500 (EST) Received: from localhost (ncase [10.192.0.11]) by vm-mail (OpenSMTPD) with ESMTPSA id 744403f9 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Sun, 1 Mar 2020 08:39:33 +0000 (UTC) Date: Sun, 1 Mar 2020 09:39:32 +0100 From: Patrick Steinhardt To: The development of GNU GRUB Cc: John Lane , Denis 'GNUtoo' Carikli Subject: Re: [PATCH 2/2] Cryptomount support key files Message-ID: <20200301083932.GB9034@ncase> References: <20200221210349.22490-1-GNUtoo@cyberdimension.org> <20200221210349.22490-2-GNUtoo@cyberdimension.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="7ZAtKRhVyVSsbBD2" Content-Disposition: inline In-Reply-To: <20200221210349.22490-2-GNUtoo@cyberdimension.org> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 66.111.4.28 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 01 Mar 2020 08:39:40 -0000 --7ZAtKRhVyVSsbBD2 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Feb 21, 2020 at 10:03:49PM +0100, Denis 'GNUtoo' Carikli wrote: > From: John Lane >=20 > Signed-off-by: John Lane > GNUtoo@cyberdimension.org: rebase > Signed-off-by: Denis 'GNUtoo' Carikli Same remarks with regards to commit subject and message as for the other patch. > --- > grub-core/disk/cryptodisk.c | 46 ++++++++++++++++++++++++++++++++++++- > grub-core/disk/geli.c | 4 +++- > grub-core/disk/luks.c | 44 ++++++++++++++++++++++++----------- > include/grub/cryptodisk.h | 5 +++- > include/grub/file.h | 2 ++ > 5 files changed, 84 insertions(+), 17 deletions(-) >=20 > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c > index 6d4befc6f..ee2f300dd 100644 > --- a/grub-core/disk/cryptodisk.c > +++ b/grub-core/disk/cryptodisk.c > @@ -42,6 +42,9 @@ 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_STR= ING}, > + {"keyfile", 'k', 0, N_("Key file"), 0, ARG_TYPE_STRING}, > + {"keyfile-offset", 'O', 0, N_("Key file offset (bytes)"), 0, ARG_TYP= E_INT}, > + {"keyfile-size", 'S', 0, N_("Key file data size (bytes)"), 0, ARG_TY= PE_INT}, > {0, 0, 0, 0, 0, 0} > }; > =20 > @@ -972,6 +975,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_SIZ= E]; > +static grub_size_t keyfile_size; > =20 > static void > cryptodisk_close (grub_cryptodisk_t dev) > @@ -1002,7 +1007,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); > if (err) > { > cryptodisk_close (dev); > @@ -1110,6 +1115,45 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, = int argc, char **args) > hdr =3D NULL; > =20 > have_it =3D 0; > + key =3D NULL; > + > + if (state[4].set) /* Key file; fails back to passphrase entry */ > + { > + grub_file_t keyfile; > + int keyfile_offset; > + grub_size_t requested_keyfile_size; > + > + requested_keyfile_size =3D state[6].set ? grub_strtoul(state[6].ar= g, 0, 0) : 0; I think we should check for conversion errors here when calling `grub_strtoul()`. Defaulting to `GRUB_CRYPTODISK_MAX_KEYFILE_SIZE` in case the user has typoed seems surprising to me. > + if (requested_keyfile_size > GRUB_CRYPTODISK_MAX_KEYFILE_SIZE) > + grub_printf (N_("Key file size exceeds maximum (%llu)\n"), \ > + (unsigned long long) GRUB_CRYPTODISK_MAX_KEYFILE_S= IZE); Shouldn't we error out in this case? Same for the below error cases. If we do so then we could also get rid of the cascading if statements by returning early in case any error occurs. > + else > + { > + keyfile_offset =3D state[5].set ? grub_strtoul (state[5].arg, = 0, 0) : 0; > + keyfile_size =3D requested_keyfile_size ? requested_keyfile_si= ze : \ > + GRUB_CRYPTODISK_MAX_KEYFILE_SIZE; Hum. So if we're given a keyfile size, then we obviously should read exactly that many bytes. I'd argue in the case where the user didn't pass a size that we should not use GRUB_CRYPTODISK_MAX_KEYFILE_SIZE, but instead stat the file and read it in full. The benefit would be that we know how many bytes to expect when calling `grub_file_read()`. > + keyfile =3D grub_file_open (state[4].arg, GRUB_FILE_TYPE_LUKS_= KEY_FILE); > + if (!keyfile) > + grub_printf (N_("Unable to open key file %s\n"), state[4].ar= g); > + else if (grub_file_seek (keyfile, keyfile_offset) =3D=3D (grub= _off_t)-1) > + grub_printf (N_("Unable to seek to offset %d in key file\n")= , keyfile_offset); > + else > + { > + keyfile_size =3D grub_file_read (keyfile, keyfile_buffer, = keyfile_size); > + if (keyfile_size =3D=3D (grub_size_t)-1) > + grub_printf (N_("Error reading key file\n")); > + else if (requested_keyfile_size && (keyfile_size !=3D requested_k= eyfile_size)) > + grub_printf (N_("Cannot read %llu bytes for key file (r= ead %llu bytes)\n"), > + (unsigned long long) req= uested_keyfile_size, > + (unsigned long long) keyfile_size); > + else > + key =3D keyfile_buffer; > + } > + } > + } > + Indentation in the above block is wrong, eight consecutive spaces should be converted to a tab. > if (state[0].set) > { > grub_cryptodisk_t dev; > diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c > index f4394eb42..da6aa6a63 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 *chec= k_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]; Same as with the other patch, you'll also have to adjust "grub-core/disk/luks2.c". > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c > index 950e89237..54b1cfe70 100644 > --- a/grub-core/disk/luks.c > +++ b/grub-core/disk/luks.c > @@ -164,12 +164,16 @@ configure_ciphers (grub_disk_t disk, const char *ch= eck_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; > @@ -206,18 +210,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->name, > - 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 > + { > + /* 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->na= me, > + 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 supp= lied"); > + } > + > + passphrase =3D (grub_uint8_t *)interactive_passphrase; > + passphrase_length =3D grub_strlen (interactive_passphrase); > + > } Please remove the newline before the brace. The rest looks fine to me. Patrick --7ZAtKRhVyVSsbBD2 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEtmscHsieVjl9VyNUEXxntp6r8SwFAl5bdMQACgkQEXxntp6r 8SwbQxAAsa3/65HhqtsIY1Xtm223nXwC4D+GI5AcjM9YBet/lOsy4QZAwCLTd6ef Lfyb2PqLQIGPswoQIdbXaZrUa5t8XBrial3CakiCd/HJrL89N512DDz6JEPdxq7W Rm2Nl+rSoHa7E9uLDJu+jPLSey4V1TGNp5ZSfxQXbwvEX3CNIQwRb1Rvewci5tV0 zkCVHtSg5qqG1CCZhbGahnIwNHejNwoHsuthcTWiruxxTQ6e/vsckDwYax+sTTFH SCXyNZnGPcijoyKdD8v4wp3m3h1i+no6LtAfLCuJXrdpcgMZ2b2xm2Ff+TD4+etl 32YDVsDVVEc+FTyiEYsP4pfptDZV6SYY1+HPVSf1wY27YGAXG444wU7bb59qkcms F3K8f/lAYKZGBYw/7im3cxfYj+I7ySGQp2KaazXfWzSIu2e70VQUFFKb8vkiHhR6 GS7AsXhuzzs1p9oo1IeLLfkNfdwzFpNP+fbwtPaVmZcOlrtlH3YoOa+FHk9qj4hp 2Vu9GCelkvOjBENtJdgSl+dB7EBg/wcY9CT8+cmFpdmuDytC235gucSnqAwHsIhg IDy2m0B1QN0c00fYoNSvvivwc9kDOpsuLSvbW8jexjyR5Pue5nyAv+301hdy52ak 3TMSiFm/wDY/yWji9p7bzJzLi1VWuxITf4wyeh0hf+vgAa84QtE= =z4v4 -----END PGP SIGNATURE----- --7ZAtKRhVyVSsbBD2--