From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1keFsH-0003wQ-1k for mharc-grub-devel@gnu.org; Sun, 15 Nov 2020 06:07:13 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:52890) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1keFsF-0003wB-Dz for grub-devel@gnu.org; Sun, 15 Nov 2020 06:07:11 -0500 Received: from new3-smtp.messagingengine.com ([66.111.4.229]:57993) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1keFsC-0006P7-T8 for grub-devel@gnu.org; Sun, 15 Nov 2020 06:07:11 -0500 Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailnew.nyi.internal (Postfix) with ESMTP id B64B958028D; Sun, 15 Nov 2020 06:07:07 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Sun, 15 Nov 2020 06:07:07 -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=fm2; bh=b88vYoRa/eDOu4hcFSxe2d25Kpo rmq5zBJoYh3CTkbY=; b=SM+iAKljJrCZqJ76skpLUPm+cMmK6orJhpTQnhn5Fsg 6CekjMxkCImFTE08VOXXI8cP7NBvtYYThIk80sFNSUUhuWsAz0WyGmLhp1u/LOH+ ifsMxaF/I+BMQpb5ccGm5/B4trcvbPXX9tXwSh3hDodV7wtmznbIlDVTurjtuvtp wL6CuoAF0XIvkIgZdkQRAka0GX38Nb1ni9nDBszgf7H8JffES3d9+GE3BLPJoz86 /SYDHYXc1zUqG2hwlWAUE5IZE3qfp6ZEQJYUmUQAhBewG6vRkocjx1omPEMjRIlP mZ5EtsN0h2f4UrZaiPs15XgV1jAJ7DThRds1WWA3p8Q== 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=fm1; bh=b88vYo Ra/eDOu4hcFSxe2d25Kpormq5zBJoYh3CTkbY=; b=QUINzJb4wZ9XKCfqtHi8By 9GH8qZ4Q7wprOwyQhx9MJ9/6fGnFkPhAPR+LgY4Ie9uwI1qWiXGc1Phw2uaSvPtF 0gTjoqsV8NQ4Oj0P/JQgQFv3krAsIWlXfZ2f2X4EGnKMvE3oIASkjXhGqglySfTG H1n0mc5arIIYQGe3Z3JOWcDFnjzIB9qV7ufDdqngWuiAKsDgJkn+0DG7p7PthmXV vPss5CPXaBrgKkLcSWQtB7HGWFV5iAfE0/CWa5aMoVKZD2SFpN/X5dYXgcP9Om/m gj9cKp4s5yJsnuoW1Z0UpPTO4DOMIcSaTwP3BO48ATsPkTJH2bejwy/hfRQcJ7bw == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedruddvledgvdeiucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepfffhvffukfhfgggtuggjsehgtderredttddvnecuhfhrohhmpefrrghtrhhi tghkucfuthgvihhnhhgrrhguthcuoehpshesphhkshdrihhmqeenucggtffrrghtthgvrh hnpedugeeiuefffefggeduieeuudfhvdffledvhfekvdelteefffdugeevkeeijeeiieen ucffohhmrghinhepghhnuhdrohhrghenucfkphepjeekrdehgedrvddurddvtdeinecuve hluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepphhssehpkhhs rdhimh X-ME-Proxy: Received: from vm-mail.pks.im (dynamic-078-054-021-206.78.54.pool.telefonica.de [78.54.21.206]) by mail.messagingengine.com (Postfix) with ESMTPA id 872783280059; Sun, 15 Nov 2020 06:07:04 -0500 (EST) Received: from localhost (ncase [10.192.0.11]) by vm-mail.pks.im (OpenSMTPD) with ESMTPSA id d875a774 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Sun, 15 Nov 2020 11:07:02 +0000 (UTC) Date: Sun, 15 Nov 2020 12:07:01 +0100 From: Patrick Steinhardt To: The development of GNU GRUB Cc: dovmurik@linux.vnet.ibm.com, Dov.Murik1@il.ibm.com, ashish.kalra@amd.com, brijesh.singh@amd.com, tobin@ibm.com, david.kaplan@amd.com, jon.grimm@amd.com, thomas.lendacky@amd.com, jejb@linux.ibm.com, frankeh@us.ibm.com, "Dr . David Alan Gilbert" , GNUtoo@cyberdimension.org Subject: Re: [PATCH v2 1/3] cryptodisk: make the password getter and additional argument to recover_key Message-ID: References: <20201113222510.16958-1-jejb@linux.ibm.com> <20201113222510.16958-2-jejb@linux.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="JaMHE56qv7YLbGYC" Content-Disposition: inline In-Reply-To: <20201113222510.16958-2-jejb@linux.ibm.com> Received-SPF: pass client-ip=66.111.4.229; envelope-from=ps@pks.im; helo=new3-smtp.messagingengine.com X-detected-operating-system: by eggs.gnu.org: First seen = 2020/11/15 06:07:07 X-ACL-Warn: Detected OS = Linux 2.2.x-3.x [generic] [fuzzy] X-Spam_score_int: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action 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, 15 Nov 2020 11:07:11 -0000 --JaMHE56qv7YLbGYC Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Nov 13, 2020 at 02:25:08PM -0800, James Bottomley wrote: > For AMD SEV environments, the grub boot password has to be retrieved > from a given memory location rather than prompted for. This means > that the standard password getter needs to be replaced with one that > gets the passphrase from the SEV area and uses that instead. Adding > the password getter as a passed in argument to recover_key() makes > this possible. >=20 > Signed-off-by: James Bottomley I actually like this approach of having a callback to retrieve passwords. Note though that this conflicts with the in-flight support for detached headers and key files [1]. It's been a while since the last revision has been posted, but this can mostly be attributed to the fact that GRUB has been in a code freeze for some months in order to get v2.06 out of the door. In hindsight I'd have preferred your approach to the one in the mentioned patch series. I'm not quite sure about a proper way forward though -- depending on which series lands first, the other one'll have to adjust code to make both play nicely with each other. Anyway, I'm adding Denis to Cc. Patrick [1]: https://lists.gnu.org/archive/html/grub-devel/2020-08/msg00028.html > --- >=20 > v2: add conditional prompting to geli.c > --- > grub-core/disk/cryptodisk.c | 2 +- > grub-core/disk/geli.c | 12 +++++++----- > grub-core/disk/luks.c | 12 +++++++----- > grub-core/disk/luks2.c | 12 +++++++----- > include/grub/cryptodisk.h | 6 +++++- > 5 files changed, 27 insertions(+), 17 deletions(-) >=20 > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c > index a3d672f68..682f5a55d 100644 > --- a/grub-core/disk/cryptodisk.c > +++ b/grub-core/disk/cryptodisk.c > @@ -997,7 +997,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); > + err =3D cr->recover_key (source, dev, grub_password_get); > if (err) > { > cryptodisk_close (dev); > diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c > index e9d23299a..3fece3f4a 100644 > --- a/grub-core/disk/geli.c > +++ b/grub-core/disk/geli.c > @@ -398,7 +398,8 @@ 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) > +recover_key (grub_disk_t source, grub_cryptodisk_t dev, > + grub_passwd_cb *password_get) > { > grub_size_t keysize; > grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN]; > @@ -438,11 +439,12 @@ recover_key (grub_disk_t source, grub_cryptodisk_t = dev) > 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); > + if (password_get =3D=3D grub_password_get) > + grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name, > + source->partition ? "," : "", tmp ? : "", > + dev->uuid); Ideally we'd be moving this print into `grub_password_get`, but it's not that easy. First, it gets called by others, so an unconditional print would change output for those. Second, the interface would get a bit weird if we just pass the message to any `grub_passwd_cb`. One thing that does sound generic enough though would be to pass something like a "subject" to the callback, where subject is the thing which we want to retrieve the secret for. If it's set, the callback will print above message, otherwise it'll stay silent. This'd also allow other callbacks to print nice messages if they require user input. Furthermore, it would nicely deduplicate those messages across all cryptodisk backends. > grub_free (tmp); > - if (!grub_password_get (passphrase, MAX_PASSPHRASE)) > + if (!password_get (passphrase, MAX_PASSPHRASE)) > return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied"); > =20 > /* Calculate the PBKDF2 of the user supplied passphrase. */ > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c > index 59702067a..165f4a6bd 100644 > --- a/grub-core/disk/luks.c > +++ b/grub-core/disk/luks.c > @@ -152,7 +152,8 @@ configure_ciphers (grub_disk_t disk, const char *chec= k_uuid, > =20 > static grub_err_t > luks_recover_key (grub_disk_t source, > - grub_cryptodisk_t dev) > + grub_cryptodisk_t dev, > + grub_passwd_cb *password_get) > { > struct grub_luks_phdr header; > grub_size_t keysize; > @@ -187,11 +188,12 @@ luks_recover_key (grub_disk_t source, > 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); > + if (password_get =3D=3D grub_password_get) > + 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 (!password_get (passphrase, MAX_PASSPHRASE)) > { > grub_free (split_key); > return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied= "); > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c > index 31d7166fc..984182aa9 100644 > --- a/grub-core/disk/luks2.c > +++ b/grub-core/disk/luks2.c > @@ -531,7 +531,8 @@ luks2_decrypt_key (grub_uint8_t *out_key, > =20 > static grub_err_t > luks2_recover_key (grub_disk_t disk, > - grub_cryptodisk_t crypt) > + grub_cryptodisk_t crypt, > + grub_passwd_cb *password_get) > { > grub_uint8_t candidate_key[GRUB_CRYPTODISK_MAX_KEYLEN]; > char passphrase[MAX_PASSPHRASE], cipher[32]; > @@ -573,10 +574,11 @@ luks2_recover_key (grub_disk_t disk, > /* Get the passphrase from the user. */ > if (disk->partition) > part =3D grub_partition_get_name (disk->partition); > - grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), disk->name, > - disk->partition ? "," : "", part ? : "", > - crypt->uuid); > - if (!grub_password_get (passphrase, MAX_PASSPHRASE)) > + if (password_get =3D=3D grub_password_get) > + grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), disk->name, > + disk->partition ? "," : "", part ? : "", > + crypt->uuid); > + if (!password_get (passphrase, MAX_PASSPHRASE)) > { > ret =3D grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplie= d"); > goto err; > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h > index e1b21e785..45dae5483 100644 > --- a/include/grub/cryptodisk.h > +++ b/include/grub/cryptodisk.h > @@ -101,6 +101,9 @@ struct grub_cryptodisk > }; > typedef struct grub_cryptodisk *grub_cryptodisk_t; > =20 > +/* must match prototype for grub_password_get */ > +typedef int (grub_passwd_cb)(char buf[], unsigned buf_size); > + Considering we have the keyfiles patch series in flight, we might want to try make this function work for both. While `grub_password_get` wouldn't require any additional input, the keyfile callback would need to know where the file is located. So we might want to add an optional payload here which is specific to the function. Also, I'd maybe rename this to something like `grub_credentials_cb` or `grub_secret_cb` to make it generic. Patrick > struct grub_cryptodisk_dev > { > struct grub_cryptodisk_dev *next; > @@ -108,7 +111,8 @@ struct grub_cryptodisk_dev > =20 > grub_cryptodisk_t (*scan) (grub_disk_t disk, const char *check_uuid, > int boot_only); > - grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev); > + grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev, > + grub_passwd_cb *get_password); > }; > typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t; > =20 > --=20 > 2.26.2 >=20 >=20 > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel --JaMHE56qv7YLbGYC Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEF9hrgiFbCdvenl/rVbJhu7ckPpQFAl+xC9QACgkQVbJhu7ck PpTQzg/+OnVYQdfCeFIJMZ94zyxAx0XeuTDmavrv249mfdVmRiKrXjAIGnQsOpOv FeEG8p/gKIXx/EqBTQwGXx0uGjmw187hSHM42iZtUrNZoBGlHZeTzbYr0oqNSnxp Kc3U9WycLdcUlzh6iiyJH/t44B2wo76PCieixwyTx3vtGd0MTlX/wpUPaCE3OxpQ R9pmvZyknnkYVDEVRvwsVJNgHdf+rMMMVHRtY14fNNnABZLA6GNtGUo0cfmsjUv7 Krt21/8MO8t8UC0YmxjWEpYOhJzpjj11/j7ML+5Wlrizv6mmRbbiHIO5AnG8zKhK fkzjKpbeG4f+YUYKWJJ4hJ/Q9diGIvVDA8tINI2eTu8oviEDaXhIQfX9jkXuZt8c 9LSoT/YWHMVvz9RWdcPEda3mH4UeQjM3k3aXhBgcYf8+Au1hqtXl+vhRQUcmVcdW csW6t3RH0s6pavUQ6IDmaVa6kLourrfWzPGni4V53Mx5MC9XEAMUczXILOv+0KB5 PpbN11S/xLMoEyExy4r1wJFekGOMYucp9rA+qes4PgaKRwRZlyjjd/VlJMkFmaBb gdtBehR8u7Ao5slXej1y2gM764Cn9je8F+rBxFjEMmnY2xKcLzDkcoyIyr1pU9UI hsUqV483aOqaGJcu6k9be//txHW2WEs53C4s5aVrJJnqD367f3U= =QmTP -----END PGP SIGNATURE----- --JaMHE56qv7YLbGYC--