Re: [PATCH v12 18/20] diskfilter: look up cryptodisk devices first

[Stefan Berger]

On 4/19/24 04:31, 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 th malicious root and further dump memory to

s/th /the /

> steal the unsealed key.
>
> To defend such attack, we can specify the hint provided by 'grub-probe'
Do defend against such an attack

> to search the encrypted partition first:
>
> search --fs-uuid --set=root --hint='cryptouuid/<PART-UUID>' <FS-UUID>
>
> However, for LVM on a encrypted partition, the search hint provided by

s/a /an /

> '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 mininize the attack surface, this commit tweaks the disk device search

minimize

> 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.
>
> Signed-off-by: Gary Lin <glin@suse.com>
> Cc: Fabian Vogt <fvogt@suse.com>

Reviewed-by: Stefan Berger <stefanb@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
> +           && p->disk_iterate)
> +         {
> +           if ((p->disk_iterate) (scan_disk_hook, NULL, pull))
> +             return;
> +           if (arname && is_lv_readable (find_lv (arname), 1))
> +             return;
> +         }
> +    }
>   
>     scan_depth = 0;
>     need_rescan = 1;