From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1Z6AnG-0006PW-Rr for mharc-grub-devel@gnu.org; Sat, 20 Jun 2015 00:54:14 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56552) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z6AnD-0006O1-O4 for grub-devel@gnu.org; Sat, 20 Jun 2015 00:54:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z6AnA-0001M0-F1 for grub-devel@gnu.org; Sat, 20 Jun 2015 00:54:11 -0400 Received: from mail-la0-x22a.google.com ([2a00:1450:4010:c03::22a]:36809) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z6AnA-0001Km-1a for grub-devel@gnu.org; Sat, 20 Jun 2015 00:54:08 -0400 Received: by lacny3 with SMTP id ny3so84447809lac.3 for ; Fri, 19 Jun 2015 21:54:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-type:content-transfer-encoding; bh=wInQg4pbwGh2E1UNgPNfft6LGRqXPOA02UOXsth/Czg=; b=ZFHE/VWIIAQT94lA/n2DatGfadtPek+ON94Sm9f5zWKfYkh8+cEYLdp9hWRJAEUGyb 9KgJWtKGj5msXP9NnCoDF2wstMVAc55XIkbh4Fvet5di0svUvVA9GzVAorVXP6gKZf7W ZVMCfMS25CHwVfg7ad69Qpm9b9fDtXNyWqaVuRQ91K3k8SNOYYyIa1DJ++AecdrXs9BO 5kVDS6fMZ0nbXM7GmQjph5mLMmBq0OAd5v3as1BOZa3xu1mWZSWpjmrIO+I8+3vJz3mo jIF5yHfFH0Myg5aK/xzSzj+pqRYYHzF3oqYxE4MP794u3Q2XQuJ5LLR2MPaV65kirB8A GK/g== X-Received: by 10.112.41.196 with SMTP id h4mr20855055lbl.36.1434776046802; Fri, 19 Jun 2015 21:54:06 -0700 (PDT) Received: from opensuse.site (ppp91-76-14-38.pppoe.mtu-net.ru. [91.76.14.38]) by mx.google.com with ESMTPSA id lb8sm2965764lbc.31.2015.06.19.21.54.05 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 19 Jun 2015 21:54:05 -0700 (PDT) Date: Sat, 20 Jun 2015 07:54:04 +0300 From: Andrei Borzenkov To: John Lane Subject: Re: [PATCH 2/4] Cryptomount support key files Message-ID: <20150620075404.1efc15d5@opensuse.site> In-Reply-To: <1434445875-6846-3-git-send-email-john@lane.uk.net> References: <1434445875-6846-1-git-send-email-john@lane.uk.net> <1434445875-6846-3-git-send-email-john@lane.uk.net> X-Mailer: Claws Mail 3.11.0 (GTK+ 2.24.28; x86_64-suse-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:4010:c03::22a 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: Sat, 20 Jun 2015 04:54:13 -0000 =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(-) >=20 > 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_STR= ING}, > + {"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 > + {"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 > @@ -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_SIZ= E]; > +static grub_size_t keyfile_size; > =20 Someone should really get rid of static variables and pass them as callback data. > static void > cryptodisk_close (grub_cryptodisk_t dev) > @@ -835,7 +841,7 @@ grub_cryptodisk_scan_device_real (const char *name, g= rub_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. > if (err) > { > cryptodisk_close (dev); > @@ -938,12 +944,36 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, i= nt 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? > } > 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? > + } > + 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 *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]; > 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 *chec= k_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? > ciph =3D grub_crypto_lookup_cipher_by_name (ciphername); > if (!ciph) > @@ -322,12 +324,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; > @@ -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->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 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. > + { > + /* 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); > + > } > =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 *) passp= hrase, > - 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?=20 > 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, gr= ub_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