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

[Glenn Washburn <development@efficientek.com>]

Nov 4, 2020 7:15:30 AM Daniel Kiper <dkiper@net-space.pl>:

> On Tue, Nov 03, 2020 at 08:21:15PM +0000, Glenn Washburn wrote:
>> 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:
>
> [...]
>
>>>>> 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.
>
> That is why you should check grub_errno too. In general "man strtoull"
> is your friend. However, I agree that this does not prevent against
> overflows later. So, I think we should have error checks for
> grub_strtoull() to detect incorrect input and further some other checks
> for overflows in math if needed. Potentially we can use only the latter
> if we put these protection properly.

Normally you only check errno after a function that sets it fails. In this case, we know it sets it, but we don't know it fails. The problem with checking grub_errno when grub_strtoull() has not failed is that grub_errno can be non-zero from being by a failing function prior to grub_strtoull(). I see now that man strtoull suggests setting errno=0 before calling strtoull(). Wrapping the call in a grub_error_push/pop makes better sense here, so we can preserve any previous errors.

As an aside, grub_strtoull() doesn't handle strings prefixed by '-', while strtoull does. I think grub's makes more sense and suits the current purpose better.

>>>> 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?)
>
> Yeah, but I think we get back to tests here...
>
> [...]
>
>>>> 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).
>
> Could not you create correct image and break it later by overwriting
> some parts of it?

Yes, and I've written python code to do that. But it's not trivial because the header has a checksum and the part we'd be modifying is json. Grub doesn't actually verify the checksum, so we probably don't need to change it. But we will want a json parser. Since the build already requires python, I suppose there's no harm in having it as a dependency of tests too.

>>>> 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.
>
> Make sense for me.

Ok, I'll combine this with grub_error_push/pop

Glenn