Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors

[Glenn Washburn]

Oct 30, 2020 7:50:08 AM Daniel Kiper <dkiper@net-space.pl>:

> On Thu, Oct 29, 2020 at 02:53:34PM -0500, Glenn Washburn wrote:
>> On Tue, 27 Oct 2020 20:11:56 +0100
>> Daniel Kiper <dkiper@net-space.pl> wrote:
>>> On Sat, Oct 03, 2020 at 12:42:55AM -0500, Glenn Washburn wrote:
>>>> On Mon, 21 Sep 2020 13:23:04 +0200
>>>> Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>>>> On Mon, Sep 21, 2020 at 06:28:28AM +0000, Glenn Washburn wrote:
>>>>>> Sep 8, 2020 7:21:31 AM Daniel Kiper <daniel.kiper@oracle.com>:
>>>>>>> On Mon, Sep 07, 2020 at 05:27:46PM +0200, Patrick Steinhardt
>>>>>>> wrote:
>>>>>>>> From: Glenn Washburn <development@efficientek.com>
>>>>>>>>
>>>>>>>> The total_length field is named confusingly because length
>>>>>>>> usually refers to bytes, whereas in this case its really the
>>>>>>>> total number of sectors on the device. Also
>>>>>>>> counter-intuitively, grub_disk_get_size returns the total
>>>>>>>
>>>>>>> Could we change total_length name? Or should it stay as is
>>>>>>> because this name is used in other implementations too?
>>>>>>
>>>>>> I sent a patch which renamed total_length to total_sectors. I
>>>>>> believe Patrick chose not to include it because I did not fix a
>>>>>> bug in the code and this patch series was only patches he
>>>>>> thought essential to be included in the next release. I'll
>>>>>> include that patch again in a follow up patch series.
>>>>>
>>>>> Please do. I want to have this fixed before 2.06 release...
>>>>>
>>>>>>>> number of device native sectors sectors. We need to convert
>>>>>>>> the sectors from the size of the underlying device to the
>>>>>>>> cryptodisk sector size. And segment.size is in bytes which
>>>>>>>> need to be converted to cryptodisk sectors.
>>>>>>>>
>>>>>>>> Signed-off-by: Glenn Washburn <development@efficientek.com>
>>>>>>>> Reviewed-by: Patrick Steinhardt <ps@pks.im>
>>>>>>>> ---
>>>>>>>> grub-core/disk/luks2.c | 7 ++++---
>>>>>>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
>>>>>>>> index c4c6ac90c..5f15a4d2c 100644
>>>>>>>> --- a/grub-core/disk/luks2.c
>>>>>>>> +++ b/grub-core/disk/luks2.c
>>>>>>>> @@ -417,7 +417,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
>>>>>>>> grub_uint8_t salt[GRUB_CRYPTODISK_MAX_KEYLEN];
>>>>>>>> grub_uint8_t *split_key = NULL;
>>>>>>>> grub_size_t saltlen = sizeof (salt);
>>>>>>>> -  char cipher[32], *p;;
>>>>>>>> +  char cipher[32], *p;
>>>>>>>
>>>>>>> I am OK with changes like that but they should be mentioned
>>>>>>> shortly in the commit message.
>>>>>>
>>>>>> Noted, I'll put update the commit message.
>>>>>>
>>>>>>>> const gcry_md_spec_t *hash;
>>>>>>>> gcry_err_code_t gcry_ret;
>>>>>>>> grub_err_t ret;
>>>>>>>> @@ -603,9 +603,10 @@ luks2_recover_key (grub_disk_t disk,
>>>>>>>> crypt->log_sector_size = sizeof (unsigned int) * 8
>>>>>>>> - __builtin_clz ((unsigned int) segment.sector_size) - 1;
>>>>>>>> if (grub_strcmp (segment.size, "dynamic") == 0)
>>>>>>>> - crypt->total_length = grub_disk_get_size (disk) -
>>>>>>>> crypt->offset;
>>>>>>>> + crypt->total_length = (grub_disk_get_size (disk) >>
>>>>>>>> (crypt->log_sector_size - disk->log_sector_size))
>>>>>>>> +            - crypt->offset;
>>>>>>>> else
>>>>>>>> - crypt->total_length = grub_strtoull (segment.size, NULL,
>>>>>>>> 10);
>>>>>>>> + crypt->total_length = grub_strtoull (segment.size, NULL,
>>>>>>>> 10)
>>>>>>>>>> crypt->log_sector_size;
>>>>>>>
>>>>>>> I do not like that you ignore grub_strtoull() errors.
>>>>>>> Additionally, what will happen if segment.size is smaller than
>>>>>>> LUKS2 sector size? Should not you round segment.size up to the
>>>>>>> nearest multiple of LUKS2 sector size first? I think the same
>>>>>>> applies to the earlier change too.
>>>>>>
>>>>>> Again, I was making a minimal set of changes for this fix. Your
>>>>>> comments about grub_strtoull, while valid, don't apply to this
>>>>>> patch and should be addressed in a new patch.
>>>>>
>>>>> OK, please fix it then in separate patch.
>>>>
>>>> I've now looked in this more and feel that ignoring grub_strtoull()
>>>> errors is not a bad idea.  There are two error states where the
>>>> return value is either 0 if the first character is not a valid
>>>> digit or (1<<64)-1 in the case of overflow (actually could be more
>>>> depending on size of long long type). If grub_strtoull() returns 0
>>>> as an error, then the segment size string is not compliant with the
>>>> specification.  If grub_strtoull() returns an error because of
>>>> overflow, then the segment size is greater than 16 exbibytes or
>>>> 16777216 tebibytes.  If someone has that size storage capacity,
>>>> I'll wager they are not booting grub off that storage.  And even if
>>>> I'm wrong, its an even more astronomically improbable that they
>>>> would need to read past the 16th exbibyte.  As is, this error case
>>>> would still allow decrypting LUKS sectors up to the 16th exbibyte.
>>>>
>>>> Also, I looked at grub_strtoull() in hdparm.c, acpi.c, xnu_uuid.c,
>>>> and iorw.c and none of those do any error checking of
>>>> grub_strtoull() errors.  In fact, this could have serious
>>>> implications for a typo in iorw.
>>>
>>> I know about these issues and I think they should be fixed at some
>>> point. Or at least there should be a comment added why it is safe here
>>> and there to ignore grub_strtoull() errors. Anyway, it is not an
>>> excuse to do things in wrong way if we are touching this code here.
>>
>> Actually, I do think this is a good excuse to do things wrong _IF_ what
>> is wrong is also wrong in the original code.  Are you going to reject a
>> patch that _fixes_ a bug, because it does not fix a different bug around
>> it? So after rejecting the patch, then _both_ bugs will continue to
>> exist? This seems quite ridiculous to me.
>
> No, this was never my goal. I am just trying to find the best solution here...
>
>>>> My professional conclusion is that I see no reason to do any error
>>>> checking.  Do you have a suggestion on how you would like the
>>>> grub_strtoull() errors handled?
>>>
>>> What is the worst scenario if somebody plays bad games with
>>> segment.size string? If nothing dangerous happens I am OK with the
>>> comment explaining why it is safe to ignore grub_strtoull() errors
>>> here.
>>
>> I think part of my pushback on this is I don't see a good solution. How
>
> OK...
>
>> do you know when grub_strtoull() errors here?  And even if it doesn't
>
> Just check values/errors returned by it?

There are two error values returned by grub_strtoull(), 0 and ~0ULL for 
unrecognized number and overflow. However, these are _both_ valid non-error 
return values. So was it an overflow condition or a valid return when ~0ULL is 
returned? Same for 0. In the case of 0 while it may be valid, it wouldn't 
reflect a usable segment, so we can filter out those.

>
>> error, how do you know that a segment.size of 3 or 128 won't cause a
>> crash? I don't have good answers to these questions.  If grub does
>
> This question is more difficult and I am afraid that you are right.
>
>> crash, then a bug has been exposed in grub which should be fixed.
>
> If possible we should prevent against the crashes but I am also aware
> that it is not always feasible to predict when the GRUB will crash.

It would be good to detect where grub is crashing because there might be other 
ways to trigger such a crash (perhaps through loopback?)

>> Perhaps if we had functional testing (see my previous patch series
>> implementing functional testing), we could test this.  But even then
>
> I will try to take a look at it next week...
>
>> there's a problem, how do we know some grub file system or disk doesn't
>> have a crashing bug on too small of disks?
>
> We can improve a situation a bit here by running some tests you are
> mentioning above.
>
>>>>>> Your concern about rounding segment.size up, is also valid and
>>>>>> pertinent to this patch, I'll update that in a following patch
>>>>>> series. This may get more complicated if the last partial
>>>>>> sector is at the end of the disk.
>>>>>
>>>>> Yeah, but please try to fix it somehow...
>>>>
>>>> On second thought, this is an edge case for a nonexistent problem.
>>>> If segment.size is smaller than the LUKS2 sector size, then you
>>>> have a segment size of less than 4K, the current max supported
>>>> sector size. And having a filesystem smaller than 4K is
>>>> pathological and I dare say not supported by any filesystem
>>>> supported by linux.
>>>>
>>>> There's a more general problem of a segment size that is not a
>>>> multiple of the sector size.  In this case, there could be
>>>> unreadable (by grub) data at the end of the device.  But again,
>>>> this is not something we should worry about.  The cryptsetup
>>>> program will refuse to create LUKS2 devices where the disk size is
>>>> not a multiple of the sector size. It will give the error: "Device
>>>> size is not aligned to requested sector size." The only ways I can
>>>> think of where the segment size is not a multiple of sector size is
>>>> if the segment size string is corrupted or set incorrectly.  In
>>>> either case, reading the last partial sector isn't going to matter.
>>>> The same logic holds for the case where sector size is "dynamic".
>>>>
>>>> So currently, I do not think we should support reading partial LUKS2
>>>> sectors at the end of a LUKS2 device.  And regardless, whether or
>>>> not we support reading partial sectors should not be something that
>>>> prevents this patch, which fixes a bug, from being merged.  Do you
>>>> disagree?
>>>
>>> I am not saying we should care about such crazy scenarios. However,
>>> I care a lot if GRUB fails safely in cases where somebody feeds it
>>> with invalid data. So, please add code which protects against crashes
>>> or explain in the comments why such protections are not needed.
>>>
>>> Daniel
>>
>> Since you seem to have a clear idea of what should be done here,
>> perhaps you insert a patch to your liking?  Or just tell me exactly
>> what you think should be done to protect against crashes.  I can just
>> add a zero check if that's what you want.  But that just adding a
>> "check" to check a box and make people feel safe and comfortable that
>
> I am not interested in adding `"check" to check a box`...
>
>> something is being done, when in fact it may do little to fix a
>> potential crash.  When you say "somebody feeds it with invalid data", I
>> take you to be concerned about someone maliciously crafting data to
>> exploit grub, in which case a more in-depth audit of the use of
>
> Yep...
>
>> total_length and offset should be done.  Perhaps a compromise would be
>
> That would be perfect.
>
>> a comment saying "FIXME: Verify that grub does not crash for any value
>> of total_length, offset, and sector_size combination."
>
> OK but I would be more happy if you add to the compromise a promise that
> you will continue the work on the functional testing mentioned above... :-)

Hmm, let me look into it before making a promise. The part with the most work 
will be adding the ability to create LUKS2 devices that cryptsetup does not 
currently allow (eg. One with a zero length segment or something 
grub_strtoull() will error on).

>> Honestly, I'm frustrated at how much time this whole patch series is
>> requiring of me and dragging on. I feel like this patch is being held
>
> This is partially by my lack of time. However, I hope it will be
> changing. Anyway, sorry about the delays on my side.
>
>> hostage in order to strong-arm me in to fixing something unrelated to
>> my patch.
>
> I think it is related to some extend. Anyway, I am open to discuss any
> solution to this issue except ignoring it. Though I think we are close
> to the compromise... :-)

I think a good compromise would be to error on segments where offset > 
segment.size and crypt->total_length
== 0. And to add a  fixme comment to handle the overflow case for 
grub_strtoull() better. Overflow won't cause a crash, just the area larger than 
the overflow amount to be inaccessible. And I don't think we need the 
previously mentioned fixme, but I'm not opposed to adding it.

Glenn