[PATCH v3 0/2] cryptodisk: Allows UUIDs to be compared in a dash-insensitive manner

[PATCH v3 0/2] cryptodisk: Allows UUIDs to be compared in a dash-insensitive manner

[Glenn Washburn]

This patch series has been updated to reflect changes suggested by Mihai.
grub_uuidcasecmp() has been completely rewritten to basically a copy of
grub_strncasecmp() which ignores dashes. The rest is the same minus changes
needed due to rebasing onto master.

Glenn

Glenn Washburn (2):
  luks, luks2: Force header.uuid to be NULL terminated
  cryptodisk: Allows UUIDs to be compared in a dash-insensitive manner

 grub-core/disk/cryptodisk.c |  4 ++--
 grub-core/disk/geli.c       |  2 +-
 grub-core/disk/luks.c       | 24 +++++++-----------------
 grub-core/disk/luks2.c      | 18 +++++++-----------
 include/grub/misc.h         | 27 +++++++++++++++++++++++++++
 5 files changed, 44 insertions(+), 31 deletions(-)

-- 
2.27.0

[PATCH v3 1/2] luks, luks2: Force header.uuid to be NULL terminated

[Glenn Washburn]

According to the LUKS specification the uuid header field is of data type
"char[]", which is defined as "a string stored as null terminated sequence
of 8-bit characters". So enforce this by adding a null byte as the last byte
of the uuid. The LUKS2 specification defers to the LUKS1 specification in
this regard.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/luks.c  | 3 +++
 grub-core/disk/luks2.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index f0feb3844..d74f004b6 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -87,6 +87,9 @@ configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
       return NULL;
     }
 
+  /* According to the spec the uuid field is NULL terminated, so enforce it. */
+  header.uuid[sizeof(header.uuid)-1] = '\0';
+
   /* Look for LUKS magic sequence.  */
   if (grub_memcmp (header.magic, LUKS_MAGIC, sizeof (header.magic))
       || grub_be_to_cpu16 (header.version) != 1)
diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index ccfacb63a..2c13246f2 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -342,6 +342,9 @@ luks2_read_header (grub_disk_t disk, grub_luks2_header_t *outhdr)
       header = &secondary;
   grub_memcpy (outhdr, header, sizeof (*header));
 
+  /* According to the spec the uuid field is NULL terminated, so enforce it. */
+  header->uuid[sizeof(header->uuid)-1] = '\0';
+
   return GRUB_ERR_NONE;
 }
 
-- 
2.27.0

[PATCH v3 2/2] cryptodisk: Allows UUIDs to be compared in a dash-insensitive manner

[Glenn Washburn]

A user can now specify UUID strings with dashes, instead of having to remove
dashes. This is backwards-compatability preserving and also fixes a source
of user confusion over the inconsistency with how UUIDs are specified
between file system UUIDs and cryptomount UUIDs. Since cryptsetup, the
reference implementation for LUKS, displays and generates UUIDs with dashes
there has been additional confusion when using the UUID strings from
cryptsetup as exact input into GRUB does not find the expected cryptodisk.

A new function grub_uuidcasecmp is added that is general enough to be used
other places where UUIDs are being compared.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/cryptodisk.c |  4 ++--
 grub-core/disk/geli.c       |  2 +-
 grub-core/disk/luks.c       | 21 ++++-----------------
 grub-core/disk/luks2.c      | 15 ++++-----------
 include/grub/misc.h         | 27 +++++++++++++++++++++++++++
 5 files changed, 38 insertions(+), 31 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 497097394..3015e3bd5 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -679,7 +679,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, sizeof (dev->uuid)) == 0)
 	  break;
     }
   else
@@ -909,7 +909,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, sizeof (dev->uuid)) == 0)
       return dev;
   return NULL;
 }
diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index 23789c43f..43a63cef8 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -301,7 +301,7 @@ configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
       return NULL;
     }
 
-  if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
+  if (cargs->search_uuid != NULL && grub_uuidcasecmp (cargs->search_uuid, uuid, sizeof (uuid)) != 0)
     {
       grub_dprintf ("geli", "%s != %s\n", uuid, cargs->search_uuid);
       return NULL;
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index d74f004b6..e522b5162 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -66,10 +66,7 @@ static grub_cryptodisk_t
 configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
 {
   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,19 +92,9 @@ configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
       || grub_be_to_cpu16 (header.version) != 1)
     return NULL;
 
-  grub_memset (uuid, 0, sizeof (uuid));
-  optr = uuid;
-  for (iptr = header.uuid; iptr < &header.uuid[ARRAY_SIZE (header.uuid)];
-       iptr++)
+  if (cargs->search_uuid != NULL && grub_uuidcasecmp (cargs->search_uuid, header.uuid, sizeof (header.uuid)) != 0)
     {
-      if (*iptr != '-')
-	*optr++ = *iptr;
-    }
-  *optr = 0;
-
-  if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
-    {
-      grub_dprintf ("luks", "%s != %s\n", uuid, cargs->search_uuid);
+      grub_dprintf ("luks", "%s != %s\n", header.uuid, cargs->search_uuid);
       return NULL;
     }
 
@@ -126,7 +113,7 @@ configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
   newdev->source_disk = NULL;
   newdev->log_sector_size = GRUB_LUKS1_LOG_SECTOR_SIZE;
   newdev->total_sectors = grub_disk_native_sectors (disk) - newdev->offset_sectors;
-  grub_memcpy (newdev->uuid, uuid, sizeof (uuid));
+  grub_memcpy (newdev->uuid, header.uuid, sizeof (header.uuid));
   newdev->modname = "luks";
 
   /* Configure the hash used for the AF splitter and HMAC.  */
@@ -146,7 +133,7 @@ configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
       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 2c13246f2..6509010e9 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -353,8 +353,6 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
 {
   grub_cryptodisk_t cryptodisk;
   grub_luks2_header_t header;
-  char uuid[sizeof (header.uuid) + 1];
-  grub_size_t i, j;
 
   if (cargs->check_boot)
     return NULL;
@@ -365,14 +363,9 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
       return NULL;
     }
 
-  for (i = 0, j = 0; i < sizeof (header.uuid); i++)
-    if (header.uuid[i] != '-')
-      uuid[j++] = header.uuid[i];
-  uuid[j] = '\0';
-
-  if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
+  if (cargs->search_uuid != NULL && grub_uuidcasecmp (cargs->search_uuid, header.uuid, sizeof (header.uuid)) != 0)
     {
-      grub_dprintf ("luks2", "%s != %s\n", uuid, cargs->search_uuid);
+      grub_dprintf ("luks2", "%s != %s\n", header.uuid, cargs->search_uuid);
       return NULL;
     }
 
@@ -380,8 +373,8 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
   if (!cryptodisk)
     return NULL;
 
-  COMPILE_TIME_ASSERT (sizeof (cryptodisk->uuid) >= sizeof (uuid));
-  grub_memcpy (cryptodisk->uuid, uuid, sizeof (uuid));
+  COMPILE_TIME_ASSERT (sizeof (cryptodisk->uuid) >= sizeof (header.uuid));
+  grub_memcpy (cryptodisk->uuid, header.uuid, sizeof (header.uuid));
 
   cryptodisk->modname = "luks2";
   return cryptodisk;
diff --git a/include/grub/misc.h b/include/grub/misc.h
index 7d2b55196..41d64a5c3 100644
--- a/include/grub/misc.h
+++ b/include/grub/misc.h
@@ -243,6 +243,33 @@ grub_strncasecmp (const char *s1, const char *s2, grub_size_t n)
     - (int) grub_tolower ((grub_uint8_t) *s2);
 }
 
+/* Do a case insensitive compare of two UUID strings by ignoring all dashes */
+static inline int
+grub_uuidcasecmp (const char *uuid1, const char *uuid2, grub_size_t n)
+{
+  if (n == 0)
+    return 0;
+
+  while (*s1 && *s2 && --n)
+    {
+      /* Skip forward to non-dash on both UUIDs. */
+      while ('-' == *s1)
+        ++s1;
+
+      while ('-' == *s2)
+        ++s2;
+
+      if (grub_tolower (*s1) != grub_tolower (*s2))
+	break;
+
+      s1++;
+      s2++;
+    }
+
+  return (int) grub_tolower ((grub_uint8_t) *s1)
+    - (int) grub_tolower ((grub_uint8_t) *s2);
+}
+
 /*
  * Note that these differ from the C standard's definitions of strtol,
  * strtoul(), and strtoull() by the addition of two const qualifiers on the end
-- 
2.27.0