grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max num


From: Glenn Washburn
Subject: Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors
Date: Thu, 29 Oct 2020 14:53:34 -0500

On Tue, 27 Oct 2020 20:11:56 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Sat, Oct 03, 2020 at 12:42:55AM -0500, Glenn Washburn wrote:
> > On Mon, 21 Sep 2020 13:23:04 +0200
> > Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >
> > > On Mon, Sep 21, 2020 at 06:28:28AM +0000, Glenn Washburn wrote:
> > > > Sep 8, 2020 7:21:31 AM Daniel Kiper <daniel.kiper@oracle.com>:
> > > > > On Mon, Sep 07, 2020 at 05:27:46PM +0200, Patrick Steinhardt
> > > > > wrote:
> > > > >> From: Glenn Washburn <development@efficientek.com>
> > > > >>
> > > > >> The total_length field is named confusingly because length
> > > > >> usually refers to bytes, whereas in this case its really the
> > > > >> total number of sectors on the device. Also
> > > > >> counter-intuitively, grub_disk_get_size returns the total
> > > > >
> > > > > Could we change total_length name? Or should it stay as is
> > > > > because this name is used in other implementations too?
> > > >
> > > > I sent a patch which renamed total_length to total_sectors. I
> > > > believe Patrick chose not to include it because I did not fix a
> > > > bug in the code and this patch series was only patches he
> > > > thought essential to be included in the next release. I'll
> > > > include that patch again in a follow up patch series.
> > >
> > > Please do. I want to have this fixed before 2.06 release...
> > >
> > > > >> number of device native sectors sectors. We need to convert
> > > > >> the sectors from the size of the underlying device to the
> > > > >> cryptodisk sector size. And segment.size is in bytes which
> > > > >> need to be converted to cryptodisk sectors.
> > > > >>
> > > > >> Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > > >> Reviewed-by: Patrick Steinhardt <ps@pks.im>
> > > > >> ---
> > > > >> grub-core/disk/luks2.c | 7 ++++---
> > > > >> 1 file changed, 4 insertions(+), 3 deletions(-)
> > > > >>
> > > > >> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > > > >> index c4c6ac90c..5f15a4d2c 100644
> > > > >> --- a/grub-core/disk/luks2.c
> > > > >> +++ b/grub-core/disk/luks2.c
> > > > >> @@ -417,7 +417,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
> > > > >> grub_uint8_t salt[GRUB_CRYPTODISK_MAX_KEYLEN];
> > > > >> grub_uint8_t *split_key = NULL;
> > > > >> grub_size_t saltlen = sizeof (salt);
> > > > >> -  char cipher[32], *p;;
> > > > >> +  char cipher[32], *p;
> > > > >
> > > > > I am OK with changes like that but they should be mentioned
> > > > > shortly in the commit message.
> > > >
> > > > Noted, I'll put update the commit message.
> > > >
> > > > >> const gcry_md_spec_t *hash;
> > > > >> gcry_err_code_t gcry_ret;
> > > > >> grub_err_t ret;
> > > > >> @@ -603,9 +603,10 @@ luks2_recover_key (grub_disk_t disk,
> > > > >> crypt->log_sector_size = sizeof (unsigned int) * 8
> > > > >> - __builtin_clz ((unsigned int) segment.sector_size) - 1;
> > > > >> if (grub_strcmp (segment.size, "dynamic") == 0)
> > > > >> - crypt->total_length = grub_disk_get_size (disk) -
> > > > >> crypt->offset;
> > > > >> + crypt->total_length = (grub_disk_get_size (disk) >>
> > > > >> (crypt->log_sector_size - disk->log_sector_size))
> > > > >> +            - crypt->offset;
> > > > >> else
> > > > >> - crypt->total_length = grub_strtoull (segment.size, NULL,
> > > > >> 10);
> > > > >> + crypt->total_length = grub_strtoull (segment.size, NULL,
> > > > >> 10)
> > > > >> >> crypt->log_sector_size;
> > > > >
> > > > > I do not like that you ignore grub_strtoull() errors.
> > > > > Additionally, what will happen if segment.size is smaller than
> > > > > LUKS2 sector size? Should not you round segment.size up to the
> > > > > nearest multiple of LUKS2 sector size first? I think the same
> > > > > applies to the earlier change too.
> > > >
> > > > Again, I was making a minimal set of changes for this fix. Your
> > > > comments about grub_strtoull, while valid, don't apply to this
> > > > patch and should be addressed in a new patch.
> > >
> > > OK, please fix it then in separate patch.
> >
> > I've now looked in this more and feel that ignoring grub_strtoull()
> > errors is not a bad idea.  There are two error states where the
> > return value is either 0 if the first character is not a valid
> > digit or (1<<64)-1 in the case of overflow (actually could be more
> > depending on size of long long type). If grub_strtoull() returns 0
> > as an error, then the segment size string is not compliant with the
> > specification.  If grub_strtoull() returns an error because of
> > overflow, then the segment size is greater than 16 exbibytes or
> > 16777216 tebibytes.  If someone has that size storage capacity,
> > I'll wager they are not booting grub off that storage.  And even if
> > I'm wrong, its an even more astronomically improbable that they
> > would need to read past the 16th exbibyte.  As is, this error case
> > would still allow decrypting LUKS sectors up to the 16th exbibyte.
> >
> > Also, I looked at grub_strtoull() in hdparm.c, acpi.c, xnu_uuid.c,
> > and iorw.c and none of those do any error checking of
> > grub_strtoull() errors.  In fact, this could have serious
> > implications for a typo in iorw.
> 
> I know about these issues and I think they should be fixed at some
> point. Or at least there should be a comment added why it is safe here
> and there to ignore grub_strtoull() errors. Anyway, it is not an
> excuse to do things in wrong way if we are touching this code here.

Actually, I do think this is a good excuse to do things wrong _IF_ what
is wrong is also wrong in the original code.  Are you going to reject a
patch that _fixes_ a bug, because it does not fix a different bug around
it? So after rejecting the patch, then _both_ bugs will continue to
exist? This seems quite ridiculous to me.

> > My professional conclusion is that I see no reason to do any error
> > checking.  Do you have a suggestion on how you would like the
> > grub_strtoull() errors handled?
> 
> What is the worst scenario if somebody plays bad games with
> segment.size string? If nothing dangerous happens I am OK with the
> comment explaining why it is safe to ignore grub_strtoull() errors
> here.

I think part of my pushback on this is I don't see a good solution. How
do you know when grub_strtoull() errors here?  And even if it doesn't
error, how do you know that a segment.size of 3 or 128 won't cause a
crash? I don't have good answers to these questions.  If grub does
crash, then a bug has been exposed in grub which should be fixed.
Perhaps if we had functional testing (see my previous patch series
implementing functional testing), we could test this.  But even then
there's a problem, how do we know some grub file system or disk doesn't
have a crashing bug on too small of disks?

> > > > Your concern about rounding segment.size up, is also valid and
> > > > pertinent to this patch, I'll update that in a following patch
> > > > series. This may get more complicated if the last partial
> > > > sector is at the end of the disk.
> > >
> > > Yeah, but please try to fix it somehow...
> >
> > On second thought, this is an edge case for a nonexistent problem.
> > If segment.size is smaller than the LUKS2 sector size, then you
> > have a segment size of less than 4K, the current max supported
> > sector size. And having a filesystem smaller than 4K is
> > pathological and I dare say not supported by any filesystem
> > supported by linux.
> >
> > There's a more general problem of a segment size that is not a
> > multiple of the sector size.  In this case, there could be
> > unreadable (by grub) data at the end of the device.  But again,
> > this is not something we should worry about.  The cryptsetup
> > program will refuse to create LUKS2 devices where the disk size is
> > not a multiple of the sector size. It will give the error: "Device
> > size is not aligned to requested sector size." The only ways I can
> > think of where the segment size is not a multiple of sector size is
> > if the segment size string is corrupted or set incorrectly.  In
> > either case, reading the last partial sector isn't going to matter.
> >  The same logic holds for the case where sector size is "dynamic".
> >
> > So currently, I do not think we should support reading partial LUKS2
> > sectors at the end of a LUKS2 device.  And regardless, whether or
> > not we support reading partial sectors should not be something that
> > prevents this patch, which fixes a bug, from being merged.  Do you
> > disagree?
> 
> I am not saying we should care about such crazy scenarios. However,
> I care a lot if GRUB fails safely in cases where somebody feeds it
> with invalid data. So, please add code which protects against crashes
> or explain in the comments why such protections are not needed.
> 
> Daniel

Since you seem to have a clear idea of what should be done here,
perhaps you insert a patch to your liking?  Or just tell me exactly
what you think should be done to protect against crashes.  I can just
add a zero check if that's what you want.  But that just adding a
"check" to check a box and make people feel safe and comfortable that
something is being done, when in fact it may do little to fix a
potential crash.  When you say "somebody feeds it with invalid data", I
take you to be concerned about someone maliciously crafting data to
exploit grub, in which case a more in-depth audit of the use of
total_length and offset should be done.  Perhaps a compromise would be
a comment saying "FIXME: Verify that grub does not crash for any value
of total_length, offset, and sector_size combination."

Honestly, I'm frustrated at how much time this whole patch series is
requiring of me and dragging on. I feel like this patch is being held
hostage in order to strong-arm me in to fixing something unrelated to
my patch.

Glenn



reply via email to

[Prev in Thread] Current Thread [Next in Thread]