Nov 4, 2020 7:15:30 AM Daniel Kiper <dki...@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 <dki...@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 <dki...@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.ki...@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.ki...@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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
