Re: [PATCH v8 14/18] luks2: Better error handling when setting up the cryptodisk

[Glenn Washburn]

On Sat, 12 Dec 2020 16:19:18 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Fri, Dec 11, 2020 at 07:10:18PM -0600, Glenn Washburn wrote:
> > On Thu, 10 Dec 2020 17:07:07 +0100
> > Daniel Kiper <dkiper@net-space.pl> wrote:
> >
> > > On Tue, Dec 08, 2020 at 04:45:45PM -0600, Glenn Washburn wrote:
> > > > First, check to make sure that source disk has a known size. If
> > > > not, print debug message and return error. There are 4 cases
> > > > where GRUB_DISK_SIZE_UNKNOWN is set (biosdisk, obdisk, ofdisk,
> > > > and uboot), and in all those cases processing continues. So
> > > > this is probably a bit conservative. However, 3 of the cases
> > > > seem pathological, and the other, biosdisk, happens when
> > > > booting from a cd. Since I doubt booting from a LUKS2 volume on
> > > > a cd is a big use case, we'll error until someone complains.
> > > >
> > > > Do some sanity checking on data coming from the luks header. If
> > > > segment.size is "dynamic", verify that the offset is not past
> > > > the end of disk. Otherwise, check for errors from grub_strtoull
> > > > when converting segment size from string. If a
> > > > GRUB_ERR_BAD_NUMBER error was returned, then the string was not
> > > > a valid parsable number, so skip the key. If
> > > > GRUB_ERR_OUT_OF_RANGE was returned, then there was an overflow
> > > > in converting to a 64-bit unsigned integer. So this could be a
> > > > very large disk (perhaps large raid array). In this case, we
> > > > want to continue to try to use this key, but only allow access
> > > > up to the end of the source disk.
> > > >
> > > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > > ---
> > > >  grub-core/disk/luks2.c | 84
> > > > ++++++++++++++++++++++++++++++++++++++++-- include/grub/disk.h
> > > >   | 17 +++++++++ 2 files changed, 98 insertions(+), 3
> > > > deletions(-)
> > > >
> > > > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > > > index 9abcb1c94..8cb11e899 100644
> > > > --- a/grub-core/disk/luks2.c
> > > > +++ b/grub-core/disk/luks2.c
> > > > @@ -600,12 +600,26 @@ luks2_recover_key (grub_disk_t source,
> > > >        goto err;
> > > >      }
> > > >
> > > > +  if (source->total_sectors == GRUB_DISK_SIZE_UNKNOWN)
> > > > +    {
> > > > +      /* FIXME: Allow use of source disk, and maybe cause
> > > > errors in read. */
> > > > +      grub_dprintf ("luks2", "Source disk %s has an unknown
> > > > size, "
> > > > +                            "conservatively returning
> > > > error\n", source->name);
> > > > +      ret = grub_error (GRUB_ERR_BUG, "Unknown size of luks2
> > > > source device");
> > > > +      goto err;
> > > > +    }
> > > > +
> > > >    /* Try all keyslot */
> > > >    for (json_idx = 0; json_idx < size; json_idx++)
> > > >      {
> > > > +      typeof(source->total_sectors) max_crypt_sectors = 0;
> > > > +
> > > > +      grub_errno = GRUB_ERR_NONE;
> > > >        ret = luks2_get_keyslot (&keyslot, &digest, &segment,
> > > > json, json_idx); if (ret)
> > > >         goto err;
> > > > +      if (grub_errno != GRUB_ERR_NONE)
> > > > +         grub_dprintf ("luks2", "Ignoring unhandled error %d
> > > > from luks2_get_keyslot\n", grub_errno);
> > > >
> > > >        if (keyslot.priority == 0)
> > > >         {
> > > > @@ -619,11 +633,75 @@ luks2_recover_key (grub_disk_t source,
> > > >        crypt->offset_sectors = grub_divmod64 (segment.offset,
> > > > segment.sector_size, NULL); crypt->log_sector_size = sizeof
> > > > (unsigned int) * 8
> > > >                 - __builtin_clz ((unsigned int)
> > > > segment.sector_size) - 1;
> > > > +      /* Set to the source disk size, which is the maximum we
> > > > allow. */
> > > > +      max_crypt_sectors = grub_disk_convert_sector(source,
> > > > +
> > > > source->total_sectors,
> > > > +
> > > > crypt->log_sector_size); +
> > > > +      if (max_crypt_sectors < crypt->offset_sectors)
> > > > +       {
> > > > +         grub_dprintf ("luks2", "Segment
> > > > \"%"PRIuGRUB_UINT64_T"\" has offset"
> > > > +                                " %"PRIuGRUB_UINT64_T" which
> > > > is greater than"
> > > > +                                " source disk size
> > > > %"PRIuGRUB_UINT64_T","
> > > > +                                " skipping\n",
> > > > +                                segment.idx,
> > > > crypt->offset_sectors,
> > > > +                                max_crypt_sectors);
> > > > +         continue;
> > > > +       }
> > > > +
> > > >        if (grub_strcmp (segment.size, "dynamic") == 0)
> > > > -       crypt->total_sectors = (grub_disk_get_size (source) >>
> > > > (crypt->log_sector_size - source->log_sector_size))
> > > > -                              - crypt->offset_sectors;
> > > > +       crypt->total_sectors = max_crypt_sectors -
> > > > crypt->offset_sectors; else
> > > > -       crypt->total_sectors = grub_strtoull (segment.size,
> > > > NULL, 10) >> crypt->log_sector_size;
> > > > +       {
> > > > +         grub_errno = GRUB_ERR_NONE;
> > > > +         /* Convert segment.size to sectors, rounding up to
> > > > nearest sector */
> > > > +         crypt->total_sectors = grub_strtoull (segment.size,
> > > > NULL, 10);
> > > > +         crypt->total_sectors = ALIGN_UP
> > > > (crypt->total_sectors,
> > > > +                                          1 <<
> > > > crypt->log_sector_size);
> > > > +         crypt->total_sectors >>= crypt->log_sector_size;
> > > > +
> > > > +         if (grub_errno == GRUB_ERR_NONE)
> > > > +           ;
> > > > +         else if (grub_errno == GRUB_ERR_BAD_NUMBER)
> > > > +           {
> > > > +             grub_dprintf ("luks2", "Segment
> > > > \"%"PRIuGRUB_UINT64_T"\" size"
> > > > +                                    " \"%s\" is not a parsable
> > > > number\n",
> > > > +                                    segment.idx,
> > > > segment.size);
> > > > +             continue;
> > > > +           }
> > > > +         else if (grub_errno == GRUB_ERR_OUT_OF_RANGE)
> > > > +           {
> > > > +             /*
> > > > +              * There was an overflow in parsing
> > > > segment.size, so disk must
> > > > +              * be very large or the string is incorrect.
> > > > +              */
> > > > +             grub_dprintf ("luks2", "Segment
> > > > \"%"PRIuGRUB_UINT64_T"\" size"
> > > > +                                    " %s overflowed 64-bit
> > > > unsigned integer,"
> > > > +                                    " the end of the crypto
> > > > device will be"
> > > > +                                    " inaccessible\n",
> > > > +                                    segment.idx,
> > > > segment.size);
> > > > +             if (crypt->total_sectors > max_crypt_sectors)
> > >
> > > I think this if is bogus. You should clamp crypt->total_sectors
> > > without any checks here.
> >
> > Actually, I wouldn't call this a clamp because in the overflow case
> > crypt->total_sectors always equals 0.  I just realized this, and its
> > because grub_strtoull will return 2^64-1 thus causing the following
> > ALIGN_UP to overflow returning 0.  Suffice to say that's not what we
> > want. My original intent was what happened before the ALIGN_UP code
> > was introduced, which would ALIGN_DOWN.
> 
> Ugh... Right. That is why I still think you should do further
> calculations on value returned from grub_strtoull() if grub_errno ==
> GRUB_ERR_NONE only. I understand that then crypt->total_sectors does
> not contain total sectors for a while but you can rectify this a bit
> by putting short comment before grub_strtoull() call.
> 
> > Here's an example illustrating why I wanted and still think the
> > intent of this check is reasonable. Suppose we have a disk size
> > 2^67 bytes with 512 byte (2^9) sized sectors. It will have 2^58
> > sectors. Further suppose, there is a LUKS volume with size 2^65
> > bytes starting at the beginning and a sector size of 4096 bytes
> > (2^12). This will cause an overflow, so grub_strtoull will return
> > 2^64-1, this should have us set crypt->total_sectors to 2^52-1.
> > Since we don't know how much the overflow is (1 byte or 1
> > terabyte), we don't know how many more sectors til the end of the
> > LUKS encrypted area. In this case max_crypt_sectors will be
> > 2^(58+9-12) => 2^57 sectors.  So here we see that
> > crypt->total_sectors < max_crypt_sectors in the overflow case.  If
> > we do as you suggest crypt->total_sectors will be set to 2^57, and
> > thus it will be valid to read past the end of the encrypted data
> > (ie. block 2^56 of the 4k sector crypt will be a sector starting at
> > byte 2^68, which is more than the 2^65 byte size volume).
> >
> > On the one hand, I like your suggestion because it allows reading
> > all possible encrypted data, at the cost of reading, decrypting, and
> > returning non-encrypted data (ie random garbage).  While keeping the
> > check, will prevent returning garbage at the cost of not allowing
> > access to all encrypted sectors. I think we should keep the check
> > and document a known limitation of 2^64 byte maximum sized LUKS
> > volumes. And that larger sized volumes can be read only up to byte
> > 2^64.
> 
> If think the code should look like this:
> 
>   /*
>    * ...a comment saying what crypt->total_sectors contains
>    * and why LUKS2 volumes larger than 2^64 does not work...
>    */
>   crypt->total_sectors = grub_strtoull (segment.size,  NULL, 10);
> 
>   if (grub_errno == GRUB_ERR_NONE)
>     {
>       crypt->total_sectors = ALIGN_UP (crypt->total_sectors, 1 <<
> crypt->log_sector_size); crypt->total_sectors >>=
> crypt->log_sector_size; }

Now that ALIGN_UP is being used which we don't want in the
GRUB_ERR_OUT_OF_RANGE case, I'm ok with this. Although, if we just
continue in grub_strtoull error case as you suggest below, it doesn't
matter. And this actually doesn't prevent ALIGN_UP from potentially
overflowing crypt->total_sectors either, when segment.size is very
large. Perhaps we should check for that.

>   else if (grub_errno == GRUB_ERR_BAD_NUMBER)
>     {
>       ...
>       continue;
>     }
> 
>   if (grub_errno == GRUB_ERR_OUT_OF_RANGE ||
>       !crypt->total_sectors ||
>       crypt->total_sectors > max_crypt_sectors)
>     {
>       ...
>       continue;
>       /*
>        * Yes, I think we should not guess crypt->total_sectors
>        * value and always fail. It seems safer.
>        */

I agree this is safer, but its at the cost of using volumes over 2^64
bytes working at all. I think the question is how safe is it to allow
arbitrary disk read failures in grub. Ultimately I don't really like
continuing here, which is why I originally had the TODO, because it
prevents otherwise accessible drives from being accessible (admittedly
in an extremely rare case). Think about a 2^65 sized raid with LUKS on
top and LVM on top of that. But really you should probably not be
booting from that anyway. I think I'll add a TODO here, with a work
around suggesting not to use such large devices to boot from.

>     }

Glenn