Re: [PATCH 2/9] luks: Fix out-of-bounds copy of UUID

[Denis 'GNUtoo' Carikli]

On Sun, 23 Aug 2020 12:59:57 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> When configuring a LUKS disk, we copy over the UUID from the LUKS
> header into the new `grub_cryptodisk_t` structure via `grub_memcpy
> ()`. As size we mistakenly use the size of the `grub_cryptodisk_t`
> UUID field, which is guaranteed to be strictly bigger than the LUKS
> UUID field we're copying. As a result, the copy always goes
> out-of-bounds and copies some garbage from other surrounding fields.
> During runtime, this isn't noticed due to the fact that we always
> NUL-terminate the UUID and thus never hit the trailing garbage.
> 
> Fix the issue by using the size of the local stripped UUID field.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  grub-core/disk/luks.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index 6ae162601..76f89dd29 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -125,7 +125,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, uuid, sizeof (uuid));

Is the fact that the real UUID size is 37 (36 + \0) instead of 40 an
issue?

In grub-core/disk/luks.c we have:
> /* On disk LUKS header */
> struct grub_luks_phdr
> {
>   [...]
>   char uuid[40];
>   [...]
> } GRUB_PACKED;
So here we use 40.

It's then used to define the size of the 'uuid' local variable that is
used grub_memcpy:
> static grub_cryptodisk_t
> luks_scan (grub_disk_t disk, const char *check_uuid, int check_boot,
>          grub_file_t hdr)
> {
>   [...]
>   char uuid[sizeof (header.uuid) + 1];
>   [...]
>   grub_memcpy (newdev->uuid, uuid, sizeof (newdev->uuid));
>   [...]
> }

However in lib/luks1/luks.h in cryptsetup source code we have:
> /* Actually we need only 37, but we don't want struct autoaligning to kick in 
> */
> #define UUID_STRING_L 40

And still in cryptsetup source code in the LUKS2_luks2_to_luks1 
function in lib/luks2/luks2_luks1_convert.c we have:
> strncpy(hdr1->uuid, hdr2->uuid, UUID_STRING_L); /* max 36 chars */
> hdr1->uuid[UUID_STRING_L-1] = '\0';

Denis.

pgpnOOeGprWSv.pgp
Description: OpenPGP digital signature