On Fri, Aug 30, 2024 at 06:31:50PM +0200, Daniel Kiper wrote:
> On Fri, Jun 28, 2024 at 04:19:06PM +0800, Gary Lin via Grub-devel wrote:
> > When using disk auto-unlocking with TPM 2.0, the typical grub.cfg may
> > look like this:
> >
> > tpm2_key_protector_init --tpm2key=(hd0,gpt1)/boot/grub2/sealed.tpm
> > cryptomount -u <PART-UUID> -P tpm2
> > search --fs-uuid --set=root <FS-UUID>
> >
> > Since the disk search order is based on the order of module loading, the
> > attacker could insert a malicious disk with the same FS-UUID root to
> > trick grub2 to boot into the malicious root and further dump memory to
> > steal the unsealed key.
> >
> > Do defend against such an attack, we can specify the hint provided by
> > 'grub-probe' to search the encrypted partition first:
> >
> > search --fs-uuid --set=root --hint='cryptouuid/<PART-UUID>' <FS-UUID>
> >
> > However, for LVM on an encrypted partition, the search hint provided by
> > 'grub-probe' is:
> >
> > --hint='lvmid/<VG-UUID>/<LV-UUID>'
> >
> > It doesn't guarantee to look up the logical volume from the encrypted
> > partition, so the attacker may have the chance to fool grub2 to boot
> > into the malicious disk.
> >
> > To minimize the attack surface, this commit tweaks the disk device search
> > in diskfilter to look up cryptodisk devices first and then others, so
> > that the auto-unlocked disk will be found first, not the attacker's disk.
> >
> > Cc: Fabian Vogt <fv...@suse.com>
> > Signed-off-by: Gary Lin <g...@suse.com>
> > Reviewed-by: Stefan Berger <stef...@linux.ibm.com>
> > ---
> > grub-core/disk/diskfilter.c | 35 ++++++++++++++++++++++++++---------
> > 1 file changed, 26 insertions(+), 9 deletions(-)
> >
> > diff --git a/grub-core/disk/diskfilter.c b/grub-core/disk/diskfilter.c
> > index 21e239511..df1992305 100644
> > --- a/grub-core/disk/diskfilter.c
> > +++ b/grub-core/disk/diskfilter.c
> > @@ -226,15 +226,32 @@ scan_devices (const char *arname)
> > int need_rescan;
> >
> > for (pull = 0; pull < GRUB_DISK_PULL_MAX; pull++)
> > - for (p = grub_disk_dev_list; p; p = p->next)
> > - if (p->id != GRUB_DISK_DEVICE_DISKFILTER_ID
> > - && p->disk_iterate)
> > - {
> > - if ((p->disk_iterate) (scan_disk_hook, NULL, pull))
> > - return;
> > - if (arname && is_lv_readable (find_lv (arname), 1))
> > - return;
> > - }
> > + {
> > + /* look up the crytodisk devices first */
> > + for (p = grub_disk_dev_list; p; p = p->next)
> > + if (p->id == GRUB_DISK_DEVICE_CRYPTODISK_ID
> > + && p->disk_iterate)
> > + {
> > + if ((p->disk_iterate) (scan_disk_hook, NULL, pull))
> > + return;
> > + if (arname && is_lv_readable (find_lv (arname), 1))
> > + return;
> > + break;
> > + }
> > +
> > + /* check the devices other than crytodisk */
> > + for (p = grub_disk_dev_list; p; p = p->next)
> > + if (p->id == GRUB_DISK_DEVICE_CRYPTODISK_ID)
> > + continue;
> > + else if (p->id != GRUB_DISK_DEVICE_DISKFILTER_ID
>
> I think you can drop "if (p->id == GRUB_DISK_DEVICE_CRYPTODISK_ID)"...
>
Ok, I'll drop the if statement.
Gary Lin
> > + && p->disk_iterate)
> > + {
> > + if ((p->disk_iterate) (scan_disk_hook, NULL, pull))
> > + return;
> > + if (arname && is_lv_readable (find_lv (arname), 1))
> > + return;
> > + }
> > + }
>
> Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
