From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1k9xls-0007qd-26 for mharc-grub-devel@gnu.org; Sun, 23 Aug 2020 17:43:24 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:58028) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1k9xlq-0007qU-4R for grub-devel@gnu.org; Sun, 23 Aug 2020 17:43:22 -0400 Received: from cyberdimension.org ([80.67.179.20]:50406 helo=gnutoo.cyberdimension.org) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_CHACHA20_POLY1305:256) (Exim 4.90_1) (envelope-from ) id 1k9xlm-0001MF-3A for grub-devel@gnu.org; Sun, 23 Aug 2020 17:43:21 -0400 Received: from gnutoo.cyberdimension.org (localhost [127.0.0.1]) by cyberdimension.org (OpenSMTPD) with ESMTP id fbb414e4; Sun, 23 Aug 2020 21:41:11 +0000 (UTC) Received: from primarylaptop.localdomain (localhost.localdomain [::1]) by gnutoo.cyberdimension.org (OpenSMTPD) with ESMTP id b9c7a8b3; Sun, 23 Aug 2020 21:41:11 +0000 (UTC) Date: Sun, 23 Aug 2020 23:34:51 +0200 From: Denis 'GNUtoo' Carikli To: Patrick Steinhardt Cc: grub-devel@gnu.org, Glenn Washburn , Daniel Kiper Subject: Re: [PATCH 2/9] luks: Fix out-of-bounds copy of UUID Message-ID: <20200823233451.66534185@primarylaptop.localdomain> In-Reply-To: <8668b265f6b1f51c04b0528a287abaf2daaf8d79.1598179677.git.ps@pks.im> References: <8668b265f6b1f51c04b0528a287abaf2daaf8d79.1598179677.git.ps@pks.im> X-Mailer: Claws Mail 3.17.5 (GTK+ 2.24.32; i686-pc-linux-gnu) MIME-Version: 1.0 Content-Type: multipart/signed; boundary="Sig_/q=WrdWKjUdUUrqjCkt76zO8"; protocol="application/pgp-signature"; micalg=pgp-sha256 Received-SPF: pass client-ip=80.67.179.20; envelope-from=GNUtoo@cyberdimension.org; helo=gnutoo.cyberdimension.org X-detected-operating-system: by eggs.gnu.org: First seen = 2020/08/23 17:43:15 X-ACL-Warn: Detected OS = Linux 2.2.x-3.x [generic] 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, SPF_HELO_PASS=-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: Sun, 23 Aug 2020 21:43:22 -0000 --Sig_/q=WrdWKjUdUUrqjCkt76zO8 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable On Sun, 23 Aug 2020 12:59:57 +0200 Patrick Steinhardt wrote: > When configuring a LUKS disk, we copy over the UUID from the LUKS > header into the new `grub_cryptodisk_t` structure via `grub_memcpy > ()`. As size we mistakenly use the size of the `grub_cryptodisk_t` > UUID field, which is guaranteed to be strictly bigger than the LUKS > UUID field we're copying. As a result, the copy always goes > out-of-bounds and copies some garbage from other surrounding fields. > During runtime, this isn't noticed due to the fact that we always > NUL-terminate the UUID and thus never hit the trailing garbage. >=20 > Fix the issue by using the size of the local stripped UUID field. >=20 > Signed-off-by: Patrick Steinhardt > --- > grub-core/disk/luks.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) >=20 > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c > index 6ae162601..76f89dd29 100644 > --- a/grub-core/disk/luks.c > +++ b/grub-core/disk/luks.c > @@ -125,7 +125,7 @@ configure_ciphers (grub_disk_t disk, const char > *check_uuid, newdev->source_disk =3D NULL; > newdev->log_sector_size =3D 9; > newdev->total_length =3D grub_disk_get_size (disk) - newdev->offset; > - grub_memcpy (newdev->uuid, uuid, sizeof (newdev->uuid)); > + grub_memcpy (newdev->uuid, uuid, sizeof (uuid)); Is the fact that the real UUID size is 37 (36 + \0) instead of 40 an issue? In grub-core/disk/luks.c we have: > /* On disk LUKS header */ > struct grub_luks_phdr > { > [...] > char uuid[40]; > [...] > } GRUB_PACKED; So here we use 40. It's then used to define the size of the 'uuid' local variable that is used grub_memcpy: > static grub_cryptodisk_t > luks_scan (grub_disk_t disk, const char *check_uuid, int check_boot, > grub_file_t hdr) > { > [...] > char uuid[sizeof (header.uuid) + 1]; > [...] > grub_memcpy (newdev->uuid, uuid, sizeof (newdev->uuid)); > [...] > } However in lib/luks1/luks.h in cryptsetup source code we have: > /* Actually we need only 37, but we don't want struct autoaligning to kic= k in */ > #define UUID_STRING_L 40 And still in cryptsetup source code in the LUKS2_luks2_to_luks1=20 function in lib/luks2/luks2_luks1_convert.c we have: > strncpy(hdr1->uuid, hdr2->uuid, UUID_STRING_L); /* max 36 chars */ > hdr1->uuid[UUID_STRING_L-1] =3D '\0'; Denis. --Sig_/q=WrdWKjUdUUrqjCkt76zO8 Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEeC+d2+Nrp/PU3kkGX138wUF34mMFAl9C4PsACgkQX138wUF3 4mMzIQ/+O4l6OAEENYyAiMU1LU2oLH1ilWOskIWB1IpU/mGjBvro9the7ummQyVP sW47e7TQnmNp4D+4YndDl+Bm2ERUb2FZtP747vF8VhlYRAkE9JXUzUGsa560BsLN VahI6zNaB6+RfhZBTcS1y5wE0mHK9lcrHsrwFNu4Ijr3V0jhrZ3tbwZirjoYmdvT FEPjlnn4fq90r7B1J/WML1EJqmDvyxQfT5E5e31JJh50xhC53vP/VK8/lBhMm8Gg RYN6l0RiJJX04XI75LzF301mHp/hfux5RQAA49uhT13QqpAVDdITlv44w17k6EQ1 XpC5rZ10+wkh+L/xhuuvib2zSHPAeTWTwoMNd5VSzQgqrsuzNHanM49bWFB3U28b kQAlAesAgTSV+Bj+EOaMc+aTPYE8cCV56Ffrqqbd3GpEUhgCuHI3oC2RewyxMb29 4+S0rVxqtORkn9AJ01M6lD4FSBC14W7MyCsFhCPwjnJSX4XBzfIp7nfmDQ7hrE6K FXfaw+5y1FwSX4V9B2gDDoQIB7IHEFwwRd84VbKTx4zXIR3BJXggC0Zmv6nZ+2RL 5SuVv2/6rVhW5b+/vmgIxEPZjibiES+yfnx0vrfEykcMgBCoxAHFgyKGh/nX1WYn pcY7GHHdAEabJ7SIxrSTyCTQHOB1CVKUkg8+y/PUYA09dfjoNtI= =ndrf -----END PGP SIGNATURE----- --Sig_/q=WrdWKjUdUUrqjCkt76zO8--