From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1kavRM-0003NX-3L for mharc-grub-devel@gnu.org; Fri, 06 Nov 2020 01:41:40 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:57906) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kavRK-0003NM-8n for grub-devel@gnu.org; Fri, 06 Nov 2020 01:41:38 -0500 Received: from mail-oo1-xc44.google.com ([2607:f8b0:4864:20::c44]:41074) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kavRG-0000mR-VG for grub-devel@gnu.org; Fri, 06 Nov 2020 01:41:37 -0500 Received: by mail-oo1-xc44.google.com with SMTP id n2so96187ooo.8 for ; Thu, 05 Nov 2020 22:41:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=efficientek-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:message-id:in-reply-to:references:subject :mime-version:content-transfer-encoding; bh=ehwcduMQjcvThNjmWKby/CO9kf3DnSvqBnCd+Tzm8Ow=; b=da019a5HLfIpMO0sHmEEXeyy00ccbiJxUtDu8DapOZ6ukXQPmNlNUaQ1JEwsLX8kZV g3gC/LXb7NcqEJfYPSwRRfLg6IbyUQXT5hGYYkb62utc1+wb5LRyz2uAhdKnRPABOy2+ R8cqB5FQnlA20uHCgm7YhZG9x9ZTcTIwwS06ybmZiPKgqBXerkjWJM18AfZbNIosWgek LFEpLm7gUYUO193qmi5jfM9Xu1VG3gzGTH7KIK0lO3SvWG0nMZcHKybk9hV1VDZMEB5X Qg+9886glUiBJsCHeWv7XkMc85iuDouafRtfQbO4RI6QhJ1Xz8mMTkcyE164ncgrp1nG pSPQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:message-id:in-reply-to :references:subject:mime-version:content-transfer-encoding; bh=ehwcduMQjcvThNjmWKby/CO9kf3DnSvqBnCd+Tzm8Ow=; b=pbw72wiXhGryZb+WRqhAJ4nm8jLEqS0qdAcB36wwOZOOsUkvkaiVet9/msLXjf1JO7 RTDylE2n69Olk/5sYWy4XuO9VJEGvFen54PZkLOhpDb/q8IGoSHwXYoVeSlydyygp5O2 yRE9WCZg9U/ObHrMECJhH1MVNYn9V+Efl7gBVe+h+Tgu0WScLop0qSYqaBKhxZT66jtv OQKYl0q2ucfc4pADRKlPNs0MKXblZCOgZbJoA7rY2Y+KNfCoaZGQTSUq0d+t2bTPbsA9 WegtcendxTVXSggpA66l8VyZjf3Vt4mXyr1bwxNMxgbmZ6XW3Se4LI/Cn4Bf0k02Fnc0 44bg== X-Gm-Message-State: AOAM531CGkADlmzEJRt/unWuwz0T4M/lemhtMK66SoWexuDfiw9cbaS9 CHZGD5dcu8S9TVTMG5eVd9OAvA== X-Google-Smtp-Source: ABdhPJxgJpsXydIqk1JbViViP2BvDexDtjSluyhWyySfxOMjqwJJbp0aA2pSgIGPfpxPDB7IXDJKug== X-Received: by 2002:a05:6820:100b:: with SMTP id v11mr288887oor.87.1604644892568; Thu, 05 Nov 2020 22:41:32 -0800 (PST) Received: from ?IPv6:::1? ([2806:103e:1d:44c7:f861:e67a:d10e:f7f9]) by smtp.gmail.com with ESMTPSA id m10sm117538oon.27.2020.11.05.22.41.30 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 05 Nov 2020 22:41:31 -0800 (PST) Date: Fri, 6 Nov 2020 06:41:31 +0000 (UTC) From: Glenn Washburn To: Daniel Kiper Cc: Daniel Kiper , Patrick Steinhardt , grub-devel@gnu.org, Denis GNUtoo Carikli Message-ID: <177ff80a-4bb8-4876-873a-364127e082f6@efficientek.com> In-Reply-To: <20201104131521.2wttovsxfygbregz@tomti.i.net-space.pl> References: <20200908132119.6o4xaaubq6oe2vxh@tomti.i.net-space.pl> <94e72828-212b-49ed-8a59-e33a85f7afce@efficientek.com> <20200921112304.4wz3gbz5j3lfqhux@tomti.i.net-space.pl> <20201003004224.2e057dd4@crass-HP-ZBook-15-G2> <20201027191156.yfizbvbzjmkhjedd@tomti.i.net-space.pl> <20201029145334.2aa22c50@crass-HP-ZBook-15-G2> <20201030124926.svq2l4y7s7zdzfpj@tomti.i.net-space.pl> <25abba49-2f02-4f17-924a-64a96efdd2da@efficientek.com> <20201104131521.2wttovsxfygbregz@tomti.i.net-space.pl> Subject: Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Correlation-ID: <177ff80a-4bb8-4876-873a-364127e082f6@efficientek.com> Received-SPF: pass client-ip=2607:f8b0:4864:20::c44; envelope-from=development@efficientek.com; helo=mail-oo1-xc44.google.com X-detected-operating-system: by eggs.gnu.org: No matching host in p0f cache. That's all we know. X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 Nov 2020 06:41:38 -0000 Nov 4, 2020 7:15:30 AM Daniel Kiper : > On Tue, Nov 03, 2020 at 08:21:15PM +0000, Glenn Washburn wrote: >> Oct 30, 2020 7:50:08 AM Daniel Kiper : >>> On Thu, Oct 29, 2020 at 02:53:34PM -0500, Glenn Washburn wrote: >>>> On Tue, 27 Oct 2020 20:11:56 +0100 >>>> Daniel Kiper 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 wrote: >>>>>>> On Mon, Sep 21, 2020 at 06:28:28AM +0000, Glenn Washburn wrote: >>>>>>>> Sep 8, 2020 7:21:31 AM Daniel Kiper : >>>>>>>>> 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. Ho= w >>> >>> OK... >>> >>>> do you know when grub_strtoull() errors here?=C2=A0 And even if it doe= sn'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 chec= king 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 se= e now that man strtoull suggests setting errno=3D0 before calling strtoull(= ). Wrapping the call in a grub_error_push/pop makes better sense here, so w= e 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 purpos= e 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.=C2=A0 If grub doe= s >>> >>> 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?=C2=A0 Or just tell me exact= ly >>>> what you think should be done to protect against crashes.=C2=A0 I can = just >>>> add a zero check if that's what you want.=C2=A0 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.=C2=A0 When you say "somebody feeds it with invalid da= ta", 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.=C2=A0 Perhaps a compromise wou= ld 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 tha= t >>> 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 does= n't actually verify the checksum, so we probably don't need to change it. B= ut we will want a json parser. Since the build already requires python, I s= uppose 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 =3D=3D 0. And to add a=C2=A0 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