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

[Glenn Washburn]

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.

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. 

> > +           crypt->total_sectors = max_crypt_sectors;
> > +       }
> > +   }
> > +
> > +      if (crypt->total_sectors == 0)
> > +   {
> > +     grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\"
> > has zero"
> > +                            " sectors, skipping\n",
> > +                            segment.idx);
> > +     continue;
> > +   }
> > +      else if (max_crypt_sectors < (crypt->offset_sectors +
> > crypt->total_sectors))
> > +   {
> > +     grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\"
> > has last"
> > +                            " data position greater than
> > source disk size,"
> > +                            " the end of the crypto device
> > will be"
> > +                            " inaccessible\n",
> > +                            segment.idx);
> > +     /* Allow decryption up to the end of the source disk. */
> > +     crypt->total_sectors = max_crypt_sectors -
> > crypt->offset_sectors;
> > +   }
> >
> >        ret = luks2_decrypt_key (candidate_key, source, crypt,
> > &keyslot, (const grub_uint8_t *) passphrase, grub_strlen
> > (passphrase)); diff --git a/include/grub/disk.h
> > b/include/grub/disk.h index 0fb727d3d..f9227f285 100644
> > --- a/include/grub/disk.h
> > +++ b/include/grub/disk.h
> > @@ -27,6 +27,8 @@
> >  #include <grub/device.h>
> >  /* For NULL.  */
> >  #include <grub/mm.h>
> > +/* For ALIGN_UP.  */
> > +#include <grub/misc.h>
> >
> >  /* These are used to set a device id. When you add a new disk
> > device, you must define a new id for it here.  */
> > @@ -174,6 +176,21 @@ typedef struct grub_disk_memberlist
> > *grub_disk_memberlist_t; /* Return value of
> > grub_disk_native_sectors() in case disk size is unknown. */ #define
> > GRUB_DISK_SIZE_UNKNOWN       0xffffffffffffffffULL
> >
> > +/* Convert sector number from disk sized sectors to a log-size
> > sized sector. */ +static inline grub_disk_addr_t
> > +grub_disk_convert_sector (grub_disk_t disk,
> > +                     grub_disk_addr_t sector,
> > +                     grub_size_t log_sector_size)
> > +{
> > +  if (disk->log_sector_size < log_sector_size)
> > +    {
> > +      sector = ALIGN_UP (sector, 1 << (log_sector_size /
> > disk->log_sector_size));
> 
> s#/#-#?

Yep, looks like this passed my tests because an overflow is caused and
crypt->total_sectors is set to a very large value, which doesn't affect
the tests. I'll fix it.

Glenn

> > +      return sector >> (log_sector_size - disk->log_sector_size);
> > +    }
> > +  else
> > +    return sector << (disk->log_sector_size - log_sector_size);
> > +}
> 
> Daniel