On Sat, 12 Dec 2020 16:19:18 +0100 Daniel Kiper <dki...@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 <dki...@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 <developm...@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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel