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. 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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel