From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1kApfO-0001xx-Im for mharc-grub-devel@gnu.org; Wed, 26 Aug 2020 03:16:18 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:46544) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kApfM-0001xb-Nh for grub-devel@gnu.org; Wed, 26 Aug 2020 03:16:16 -0400 Received: from wout1-smtp.messagingengine.com ([64.147.123.24]:38607) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kApfK-0006i3-Ec for grub-devel@gnu.org; Wed, 26 Aug 2020 03:16:16 -0400 Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.west.internal (Postfix) with ESMTP id A2C9CFE9; Wed, 26 Aug 2020 03:16:07 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute3.internal (MEProxy); Wed, 26 Aug 2020 03:16:07 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm1; bh=Hp0O4ASecCQkvvwdm5Dz9D7L8zq 8rmKh/+k4PP3n9HY=; b=v+heB05bsBQuuc6nP9/IcepYMZD+ee1zxBSe75vMIJW e96DhQdTI4MTA44BjCI5lsE+AawK6XU5BMPIw/C7MwVgWM8LtZC0KXuTJbpQU9Oi E27Vv8g14sfUOQ0qABrrZu2/0LZDOI236sk2ujCzT+A0m0dsAedO0EsemAu1Rnbz Rj8NYPTL2/Uw1kkWltE+aviQi2aVl2WSoLs3WKlPaYJXcwwpi+sblQBnGtWNa26T Ez0pYz6dVde4CQ0gxgZUwEDl6E2aXpiCGSQ4Q9GFQN5bOJUuZDb/PiYpd8GYJlJD PrLGGkW3+7YJDASHmB3pS6xnnMr7GgnrBUgVm31skYA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm3; bh=Hp0O4A SecCQkvvwdm5Dz9D7L8zq8rmKh/+k4PP3n9HY=; b=rKlpEFtytLjW54pSE/+7Cu Qu9AR5F5cMLdvOgAyV1v67pDCgMpFTXvlm0rdKXIrmE79S7aad81VT3QZtbj3kxm Z1V2mr2k0/kM/GWwlL7L0oPKXFv4t8ITWBAF+GG6KimLw6AyzEfau60chaA0WdOV kOPc3QKw7AfDbGm1GED2639ldIPEb4agw5+jalmcoPn0S+g5tdNxJ3KLS6RsZgM3 uYTgkmB+83ZSo2RYY47OEQqQFSQhoaLsVu2zvoc2gn7jv+ibe/XIRXJLer+RE/kD CT8l9FY/o295m9qNBERSOUjUlByAihf4DfPufszz1SZ4zS8AQqk8uaj3pdU+clkA == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduiedruddvuddguddvudcutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpeffhffvuffkfhggtggujgesghdtreertddtvdenucfhrhhomheprfgrthhr ihgtkhcuufhtvghinhhhrghrughtuceophhssehpkhhsrdhimheqnecuggftrfgrthhtvg hrnhepheeghfdtfeeuffehkefgffduleffjedthfdvjeektdfhhedvlefgtefgvdettdfh necukfhppeekledruddvrdefkedrvdefieenucevlhhushhtvghrufhiiigvpedtnecurf grrhgrmhepmhgrihhlfhhrohhmpehpshesphhkshdrihhm X-ME-Proxy: Received: from vm-mail.pks.im (x590c26ec.dyn.telefonica.de [89.12.38.236]) by mail.messagingengine.com (Postfix) with ESMTPA id 7DB293280059; Wed, 26 Aug 2020 03:16:05 -0400 (EDT) Received: from localhost (xps [10.192.0.12]) by vm-mail.pks.im (OpenSMTPD) with ESMTPSA id 78fb86c0 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Wed, 26 Aug 2020 07:16:04 +0000 (UTC) Date: Wed, 26 Aug 2020 09:18:04 +0200 From: Patrick Steinhardt To: Denis 'GNUtoo' Carikli Cc: grub-devel@gnu.org, Glenn Washburn , Daniel Kiper Subject: Re: [PATCH 2/9] luks: Fix out-of-bounds copy of UUID Message-ID: <20200826071804.GB757@xps> References: <8668b265f6b1f51c04b0528a287abaf2daaf8d79.1598179677.git.ps@pks.im> <20200823233451.66534185@primarylaptop.localdomain> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="Bn2rw/3z4jIqBvZU" Content-Disposition: inline In-Reply-To: <20200823233451.66534185@primarylaptop.localdomain> Received-SPF: pass client-ip=64.147.123.24; envelope-from=ps@pks.im; helo=wout1-smtp.messagingengine.com X-detected-operating-system: by eggs.gnu.org: First seen = 2020/08/26 03:16:08 X-ACL-Warn: Detected OS = Linux 2.2.x-3.x [generic] [fuzzy] X-Spam_score_int: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H4=0.001, RCVD_IN_MSPIKE_WL=0.001, 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: Wed, 26 Aug 2020 07:16:17 -0000 --Bn2rw/3z4jIqBvZU Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, Aug 23, 2020 at 11:34:51PM +0200, Denis 'GNUtoo' Carikli wrote: > On Sun, 23 Aug 2020 12:59:57 +0200 > Patrick Steinhardt wrote: >=20 > > 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)); >=20 > Is the fact that the real UUID size is 37 (36 + \0) instead of 40 an > issue? I think you're right. When copying `header.uuid` into the local variable, we strip all dashes and will thus only copy 36 bytes plus the trailing NUL byte. Which effectively means that the last 4 bytes of the local variable aren't initialized, but we still copy them over into the new device. It's probably not going to cause any problems, but we should still do the right thing and just zero-initialize the UUID variable. Patrick > 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. >=20 > 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)); > > [...] > > } >=20 > 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 k= ick in */ > > #define UUID_STRING_L 40 >=20 > 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'; >=20 > Denis. --Bn2rw/3z4jIqBvZU Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEF9hrgiFbCdvenl/rVbJhu7ckPpQFAl9GDKsACgkQVbJhu7ck PpSOAQ//XxOTH/hgjTijeg/wDeDKlfr6WXFIDtPwBKrv7hWfyLY4qi84rlU+R/RO YreQTBgVL1gqCVEwTmbF3jN7Fadv1JCQ550/pQLyF8m4UWb8i+54lslLUGvPEcMU Fqyt4ZBIrHBWRn9M+VkjMF1KMeHJlRkdisH1Olv7xzJOAGvQf4Q2SqsbycZBwOI3 YPuw134YMstVAyCmdtm6lGUeR8bIo7L029xcKuMOH4o8vmV+b76oNTdyYiVf0cgD CQsGacbV61yRr5y9XNRcKgzDwFQtI3HNpdwZzU+GEhQFjvacCb3ssAFg2lyt3DgI 0pgxxcvfjxxzX6O1eNqURdmSDC4BB1CmRsx/46fv/enCL8VEHyggFHpPQ+7OVF2w Ya4LWySDxYuzxlqNcFPoR4EPcnmU4xNhUn+PgQSWspl9pWG8Ozmm3u4Wid9Yae3w lqImGgzMcuVctdFYsMpWIZLz48QU+BvRaNgk+XgxiaAo+kf5fgq0IclHFZ74bu7w tqPjxgdr1NlNj4sF6Pz3jOmLraBQcjCWygtUnXFL+EHxkKmxGd5rKDVo3xEmufE1 fOqbatPrVZdcCKd+0Arh0i1z+r4w3JzxIrO5wTxDtTbzvMgf+Y4D4beG3ymNH4my Bpk0zVp7yTHmUXiSo7eAyIBbf1zMRLXlWnxb2B7P/C6ZRQvZTfk= =boNw -----END PGP SIGNATURE----- --Bn2rw/3z4jIqBvZU--