[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v8 14/18] luks2: Better error handling when setting up the cr
From: |
Daniel Kiper |
Subject: |
Re: [PATCH v8 14/18] luks2: Better error handling when setting up the cryptodisk |
Date: |
Sat, 12 Dec 2020 16:19:18 +0100 |
User-agent: |
NeoMutt/20170113 (1.7.2) |
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;
}
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.
*/
}
Daniel
- [PATCH v8 02/18] misc: Add parentheses around ALIGN_UP and ALIGN_DOWN arguments, (continued)
[PATCH v8 15/18] luks2: Error check segment.sector_size, Glenn Washburn, 2020/12/08
[PATCH v8 16/18] mips: Enable __clzdi2(), Glenn Washburn, 2020/12/08
[PATCH v8 18/18] luks2: Use grub_log2ull to calculate log_sector_size and improve readability, Glenn Washburn, 2020/12/08
Re: [PATCH v8 00/18] Cryptodisk fixes for v2.06 redux, Daniel Kiper, 2020/12/10