From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1ka2o0-0002KX-Ri for mharc-grub-devel@gnu.org; Tue, 03 Nov 2020 15:21:24 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:39818) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ka2ny-0002JX-0Y for grub-devel@gnu.org; Tue, 03 Nov 2020 15:21:22 -0500 Received: from mail-ot1-x343.google.com ([2607:f8b0:4864:20::343]:35298) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1ka2nu-000530-VG for grub-devel@gnu.org; Tue, 03 Nov 2020 15:21:21 -0500 Received: by mail-ot1-x343.google.com with SMTP id n11so17225592ota.2 for ; Tue, 03 Nov 2020 12:21:18 -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=6+6fOw8L/XfveBkcPA5jZdswRYv31Xcf2u01p4qPIYI=; b=dA3EX17WFuwpD6MFADmcGGzOeZ18vf/swQnL9B8OAEv817AQ7qI2XHimYwnjVNpos9 p4Re7BHWz9bOm4cV+sjAgRlPk1GsWgPgzmN/hw89FDSW+7tT3ccwFwmPJvLEmk7tHaF6 sJ9m4UPitYwmuo4WCL0wPUtu/9DdBZQnu87td3Kd8hhXIkh+7sSzzZLE3Qg05an9j7F1 wxMpsGhWHqQmGw/f49lWVQ8VLukwLUQNH88ncY0BHQKFuM5J+JRyV/Qawh5FG1OHQnMB jYRBt225frzZI9CIf57lJ4xOBAXBGcErlu44NkpslrGu3wfwzIrxwNy8vtiIdnbuE2YN JZyQ== 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=6+6fOw8L/XfveBkcPA5jZdswRYv31Xcf2u01p4qPIYI=; b=hWS4NK0Naj3eWvkZ4GpAo536gYWiSflOslwPtS9o/pRg5UCAtcHCNbeOAp2JrfBMAl 8G8r3qYq2qoDa0nD1I4NsiSbr1XcIfZfi3syR7jWKgmVHeeSH9IpwV+0ZbLN0DsNbZYX 9QqWSzrJ4YP/YZV5m2/jac3+9kxwFm9iz2XztUAbpPLfeBzE2rPhrNMLJiJoUzrqo+pg H1uXPyooOfh5KXE2akBOIr+/CWUaIYvxFgIFN0dRcgL8TFOp0WwgJzDquAh8T+f+F7Jy a34UR5gA5n6V7p4570sUKH78VBgIBqbI0N04qM536BC/hEa0R75Le8huwhEe3Wiyv0lT 8Dww== X-Gm-Message-State: AOAM533NorjP9/tbim5WOsKbbRa6ef+j7UGr7nw86TyrjxYCoZs51Xb2 ES/WhWj+OFoswjavwM23aT21Ig== X-Google-Smtp-Source: ABdhPJwMNz/MNPUhszAQep4jwzxCSq2ducBqMHWVdwIRyFGmcigtihC5His+A/VhPM5R98dhQv5l4w== X-Received: by 2002:a9d:4588:: with SMTP id x8mr1985117ote.169.1604434877182; Tue, 03 Nov 2020 12:21:17 -0800 (PST) Received: from ?IPv6:::1? ([2806:103e:1d:44c7:ece2:4187:f0ae:ca36]) by smtp.gmail.com with ESMTPSA id t5sm4371228ool.8.2020.11.03.12.21.15 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Tue, 03 Nov 2020 12:21:16 -0800 (PST) Date: Tue, 3 Nov 2020 20:21:15 +0000 (UTC) From: Glenn Washburn To: Daniel Kiper Cc: Daniel Kiper , Patrick Steinhardt , grub-devel@gnu.org, Denis GNUtoo Carikli Message-ID: <25abba49-2f02-4f17-924a-64a96efdd2da@efficientek.com> In-Reply-To: <20201030124926.svq2l4y7s7zdzfpj@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> 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: <25abba49-2f02-4f17-924a-64a96efdd2da@efficientek.com> Received-SPF: pass client-ip=2607:f8b0:4864:20::343; envelope-from=development@efficientek.com; helo=mail-ot1-x343.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: Tue, 03 Nov 2020 20:21:22 -0000 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: >>>>>>>> From: Glenn Washburn >>>>>>>> >>>>>>>> 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 >>>>>>>> Reviewed-by: Patrick Steinhardt >>>>>>>> --- >>>>>>>> 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 =3D NULL; >>>>>>>> grub_size_t saltlen =3D sizeof (salt); >>>>>>>> -=C2=A0 char cipher[32], *p;; >>>>>>>> +=C2=A0 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 =3D sizeof (unsigned int) * 8 >>>>>>>> - __builtin_clz ((unsigned int) segment.sector_size) - 1; >>>>>>>> if (grub_strcmp (segment.size, "dynamic") =3D=3D 0) >>>>>>>> - crypt->total_length =3D grub_disk_get_size (disk) - >>>>>>>> crypt->offset; >>>>>>>> + crypt->total_length =3D (grub_disk_get_size (disk) >> >>>>>>>> (crypt->log_sector_size - disk->log_sector_size)) >>>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 - crypt->offset; >>>>>>>> else >>>>>>>> - crypt->total_length =3D grub_strtoull (segment.size, NULL, >>>>>>>> 10); >>>>>>>> + crypt->total_length =3D 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.=C2=A0 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.=C2=A0 If grub_strtoull() returns an error because of >>>> overflow, then the segment size is greater than 16 exbibytes or >>>> 16777216 tebibytes.=C2=A0 If someone has that size storage capacity, >>>> I'll wager they are not booting grub off that storage.=C2=A0 And even = if >>>> I'm wrong, its an even more astronomically improbable that they >>>> would need to read past the 16th exbibyte.=C2=A0 As is, this error cas= e >>>> 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.=C2=A0 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.=C2=A0 Are you going to reje= ct 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 he= re... > >>>> My professional conclusion is that I see no reason to do any error >>>> checking.=C2=A0 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?=C2=A0 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 unr= ecognized number and overflow. However, these are _both_ valid non-error re= turn values. So was it an overflow condition or a valid return when ~0ULL i= s 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.=C2=A0 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 ot= her 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.=C2=A0 But even the= n > > 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.=C2=A0 In this case, there could be >>>> unreadable (by grub) data at the end of the device.=C2=A0 But again, >>>> this is not something we should worry about.=C2=A0 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.=C2=A0 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.=C2=A0 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.=C2=A0 Do yo= u >>>> 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?=C2=A0 Or just tell me exactly >> what you think should be done to protect against crashes.=C2=A0 I can ju= st >> 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 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.=C2=A0 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 wo= rk 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_s= trtoull() 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 > segm= ent.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. Glenn