From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=marcandre.lureau@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A23FE22135D48 for ; Wed, 7 Mar 2018 03:18:50 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0E880404085A; Wed, 7 Mar 2018 11:25:05 +0000 (UTC) Received: from localhost (ovpn-112-24.ams2.redhat.com [10.36.112.24]) by smtp.corp.redhat.com (Postfix) with ESMTP id EF9AB7C48; Wed, 7 Mar 2018 11:25:01 +0000 (UTC) From: marcandre.lureau@redhat.com To: edk2-devel@lists.01.org Cc: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= , Jiewen Yao , Chao Zhang , Star Zeng , Laszlo Ersek Date: Wed, 7 Mar 2018 12:24:41 +0100 Message-Id: <20180307112441.20278-1-marcandre.lureau@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Wed, 07 Mar 2018 11:25:05 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Wed, 07 Mar 2018 11:25:05 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'marcandre.lureau@redhat.com' RCPT:'' Subject: [PATCH v2 1/1] SecurityPkg: fix ZeroMem HashInterfaceHob X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 07 Mar 2018 11:18:51 -0000 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From: Marc-André Lureau The ZeroMem() call goes beyond the HashInterfaceHob structure, causing HOB list corruption. The intent was to clear all but the Identifier, that is starting from HashInterfaceCount. Quoting Laszlo Ersek: Therefore I think the *first* approach to actually fixing this bug would be to simply add the "Count" suffix: > diff --git a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c > index dbee0f2531bc..a869fffae3fe 100644 > --- a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c > +++ b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c > @@ -424,7 +424,10 @@ HashLibBaseCryptoRouterPeiConstructor ( > // This is the second execution of this module, clear the hash interface > // information registered at its first execution. > // > - ZeroMem (&HashInterfaceHob->HashInterface, sizeof (*HashInterfaceHob) - sizeof (EFI_GUID)); > + ZeroMem ( > + &HashInterfaceHob->HashInterfaceCount, > + sizeof (*HashInterfaceHob) - sizeof (EFI_GUID) > + ); > } > > // On the other hand, I don't think such a fix would be great. First of all, the HASH_INTERFACE_HOB struct definition is not wrapped in #pragma pack (1) ... #pragma pack () in other words, HASH_INTERFACE_HOB is not packed, hence there could be an unspecified amount of padding between "Identifier" and "HashInterfaceCount". That would lead to the same kind of buffer overflow, because "Length" includes the padding, but the "Buffer" argument doesn't. Second, even if there were no padding (and thus the byte count would be a perfect match for the full HOB structure), in the "Buffer" argument we take the address of the "HashInterfaceCount" field, and with the "Length" field, we still overflow that *individual* field. Although we wouldn't overflow the containing HOB structure, this is still (arguably) undefined behavior -- I seem to remember that some compilers actually blow up on this when you use the standard ISO C memcpy() or memset() functions in such contexts. This patch takes Laszlo suggestion to fix the issue. Cc: Jiewen Yao Cc: Chao Zhang Cc: Star Zeng Cc: Laszlo Ersek Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Marc-André Lureau --- .../HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c index dbee0f2531bc..84c1aaae70eb 100644 --- a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c +++ b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c @@ -397,6 +397,7 @@ HashLibBaseCryptoRouterPeiConstructor ( { EFI_STATUS Status; HASH_INTERFACE_HOB *HashInterfaceHob; + UINTN ClearStartOffset; HashInterfaceHob = InternalGetHashInterfaceHob (&gZeroGuid); if (HashInterfaceHob == NULL) { @@ -424,7 +425,11 @@ HashLibBaseCryptoRouterPeiConstructor ( // This is the second execution of this module, clear the hash interface // information registered at its first execution. // - ZeroMem (&HashInterfaceHob->HashInterface, sizeof (*HashInterfaceHob) - sizeof (EFI_GUID)); + ClearStartOffset = OFFSET_OF (HASH_INTERFACE_HOB, HashInterfaceCount); + ZeroMem ( + (UINT8 *)HashInterfaceHob + ClearStartOffset, + sizeof (*HashInterfaceHob) - ClearStartOffset + ); } // -- 2.16.2.346.g9779355e34