Re: [CRYPTO-LUKS v1 08/19] cryptodisk, luks: Allow special processing for comparing UUIDs.

[Patrick Steinhardt]

On Fri, Jul 31, 2020 at 07:01:49AM -0500, Glenn Washburn wrote:
> Create grub_uuidcasecmp to compare UUIDs in a case-insensitive manner and
> that ignores '-' characters. This is backwards compatible with the old LUKS1
> code that stored and compared against UUIDs without dashes. However, the new
> LUKS2 code stores and compares UUIDs that contain dashes. Really, the UUID
> comparison shouldn't care about the dashes, as this change implements. Now
> your old scripts will continue to work with UUIDs without dashes, but you
> may choose to use UUIDs with dashes now too for both LUKS1 and LUKS2.
> 
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c |  4 ++--
>  grub-core/disk/luks.c       | 20 ++++----------------
>  grub-core/disk/luks2.c      |  2 +-
>  include/grub/misc.h         | 21 +++++++++++++++++++++
>  4 files changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 847337072..e7a4b8082 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -660,7 +660,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk)
>    if (grub_memcmp (name, "cryptouuid/", sizeof ("cryptouuid/") - 1) == 0)
>      {
>        for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
> -     if (grub_strcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid) == 0)
> +     if (grub_uuidcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid) == 
> 0)
>         break;
>      }
>    else
> @@ -897,7 +897,7 @@ grub_cryptodisk_get_by_uuid (const char *uuid)
>  {
>    grub_cryptodisk_t dev;
>    for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
> -    if (grub_strcasecmp (dev->uuid, uuid) == 0)
> +    if (grub_uuidcasecmp (dev->uuid, uuid) == 0)
>        return dev;
>    return NULL;
>  }
> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index 6ae162601..ea54a9d10 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -69,10 +69,7 @@ configure_ciphers (grub_disk_t disk, const char 
> *check_uuid,
>                  int check_boot)
>  {
>    grub_cryptodisk_t newdev;
> -  const char *iptr;
>    struct grub_luks_phdr header;
> -  char *optr;
> -  char uuid[sizeof (header.uuid) + 1];
>    char ciphername[sizeof (header.cipherName) + 1];
>    char ciphermode[sizeof (header.cipherMode) + 1];
>    char hashspec[sizeof (header.hashSpec) + 1];
> @@ -95,18 +92,9 @@ configure_ciphers (grub_disk_t disk, const char 
> *check_uuid,
>        || grub_be_to_cpu16 (header.version) != 1)
>      return NULL;
>  
> -  optr = uuid;
> -  for (iptr = header.uuid; iptr < &header.uuid[ARRAY_SIZE (header.uuid)];
> -       iptr++)
> +  if (check_uuid && grub_uuidcasecmp (check_uuid, header.uuid) != 0)

I don't think the header's UUID is NUL-terminated. So if
`strlen(check_uuid) > sizeof(header.uuid)`, we'll overflow the header.
We should probably add a size parameter to `grub_uuidcasecmp` to fix
this.

>      {
> -      if (*iptr != '-')
> -     *optr++ = *iptr;
> -    }
> -  *optr = 0;
> -
> -  if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
> -    {
> -      grub_dprintf ("luks", "%s != %s\n", uuid, check_uuid);
> +      grub_dprintf ("luks", "%s != %s\n", header.uuid, check_uuid);
>        return NULL;
>      }
>  
> @@ -125,7 +113,7 @@ configure_ciphers (grub_disk_t disk, const char 
> *check_uuid,
>    newdev->source_disk = NULL;
>    newdev->log_sector_size = 9;
>    newdev->total_length = grub_disk_get_size (disk) - newdev->offset;
> -  grub_memcpy (newdev->uuid, uuid, sizeof (newdev->uuid));
> +  grub_memcpy (newdev->uuid, header.uuid, sizeof (newdev->uuid));
>    newdev->modname = "luks";
>  
>    /* Configure the hash used for the AF splitter and HMAC.  */
> @@ -145,7 +133,7 @@ configure_ciphers (grub_disk_t disk, const char 
> *check_uuid,
>        return NULL;
>      }
>  
> -  COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (uuid));
> +  COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (header.uuid));
>    return newdev;
>  }
>  
> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> index 41329f745..7f5deb469 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -356,7 +356,7 @@ luks2_scan (grub_disk_t disk, const char *check_uuid, int 
> check_boot)
>        return NULL;
>      }
>  
> -  if (check_uuid && grub_strcasecmp (check_uuid, header.uuid) != 0)
> +  if (check_uuid && grub_uuidcasecmp (check_uuid, header.uuid) != 0)
>      return NULL;

Same over here.

Patrick

signature.asc
Description: PGP signature