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

From: Patrick Steinhardt <ps@pks.im>
To: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
Cc: grub-devel@gnu.org, Glenn Washburn <development@efficientek.com>,
	Daniel Kiper <daniel.kiper@oracle.com>
Subject: Re: [PATCH 2/9] luks: Fix out-of-bounds copy of UUID
Date: Wed, 26 Aug 2020 09:18:04 +0200	[thread overview]
Message-ID: <20200826071804.GB757@xps> (raw)
In-Reply-To: <20200823233451.66534185@primarylaptop.localdomain>

[-- Attachment #1: Type: text/plain, Size: 3040 bytes --]

On Sun, Aug 23, 2020 at 11:34:51PM +0200, Denis 'GNUtoo' Carikli wrote:
> 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?

I think you're right. When copying `header.uuid` into the local
variable, we strip all dashes and will thus only copy 36 bytes plus the
trailing NUL byte. Which effectively means that the last 4 bytes of the
local variable aren't initialized, but we still copy them over into the
new device.

It's probably not going to cause any problems, but we should still do
the right thing and just zero-initialize the UUID variable.

Patrick

> 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]