From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 29 Apr 2019 11:31:21 -0700 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 46C583199366; Mon, 29 Apr 2019 18:31:21 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-31.rdu2.redhat.com [10.10.121.31]) by smtp.corp.redhat.com (Postfix) with ESMTP id E2DAC6C1E1; Mon, 29 Apr 2019 18:31:19 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 3/3] CryptoPkg/BaseCryptLib: updata HMAC_ctx size To: devel@edk2.groups.io, xiaoyux.lu@intel.com Cc: Jian J Wang , Ting Ye References: <1556525727-14875-1-git-send-email-xiaoyux.lu@intel.com> <1556525727-14875-4-git-send-email-xiaoyux.lu@intel.com> From: "Laszlo Ersek" Message-ID: Date: Mon, 29 Apr 2019 20:31:18 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <1556525727-14875-4-git-send-email-xiaoyux.lu@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.41]); Mon, 29 Apr 2019 18:31:21 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/29/19 10:15, Xiaoyu lu wrote: > From: Xiaoyu Lu > > Openssl internally redefines the size of HMAC_CTX, > but there is no external definition. > So add an additional nubmer. > > Cc: Jian J Wang > Cc: Ting Ye > Signed-off-by: Xiaoyu Lu > --- > CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c | 11 ++++++++++- > CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c | 12 ++++++++++-- > CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c | 12 ++++++++++-- > 3 files changed, 30 insertions(+), 5 deletions(-) > > diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c > index 3134806..3ffb8e2 100644 > --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c > +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c > @@ -9,8 +9,17 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #include "InternalCryptLib.h" > #include > > +// > +// NOTE: HMAC_MAX_MD_CBLOCK is deprecated. > +// #define HMAC_MAX_MD_CBLOCK 128 > +// Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h > +// #define HMAC_MAX_MD_CBLOCK_SIZE 144 > +// But we need to compatible with previous API. > +// So fix it with correct size 144-128 = 16. > +// > #define HMAC_MD5_CTX_SIZE sizeof(void *) * 4 + sizeof(unsigned int) + \ > - sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK > + sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16) > + > > /** > Retrieves the size, in bytes, of the context buffer required for HMAC-MD5 operations. > diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c > index bbe3df4..e59602e 100644 > --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c > +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c > @@ -9,8 +9,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #include "InternalCryptLib.h" > #include > > -#define HMAC_SHA1_CTX_SIZE sizeof(void *) * 4 + sizeof(unsigned int) + \ > - sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK > +// > +// NOTE: HMAC_MAX_MD_CBLOCK is deprecated. > +// #define HMAC_MAX_MD_CBLOCK 128 > +// Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h > +// #define HMAC_MAX_MD_CBLOCK_SIZE 144 > +// But we need to compatible with previous API. > +// So fix it with correct size 144-128 = 16. > +// > +#define HMAC_SHA1_CTX_SIZE sizeof(void *) * 4 + sizeof(unsigned int) + \ > + sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16) > > /** > Retrieves the size, in bytes, of the context buffer required for HMAC-SHA1 operations. > diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c > index ac9084f..8d0570b 100644 > --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c > +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c > @@ -9,8 +9,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #include "InternalCryptLib.h" > #include > > -#define HMAC_SHA256_CTX_SIZE sizeof(void *) * 4 + sizeof(unsigned int) + \ > - sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK > +// > +// NOTE: HMAC_MAX_MD_CBLOCK is deprecated. > +// #define HMAC_MAX_MD_CBLOCK 128 > +// Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h > +// #define HMAC_MAX_MD_CBLOCK_SIZE 144 > +// But we need to compatible with previous API. > +// So fix it with correct size 144-128 = 16. > +// > +#define HMAC_SHA256_CTX_SIZE sizeof(void *) * 4 + sizeof(unsigned int) + \ > + sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16) > > /** > Retrieves the size, in bytes, of the context buffer required for HMAC-SHA256 operations. > I believe I disagree with this patch. The issue is well described here: https://github.com/openssl/openssl/pull/4338 And the real solution, for edk2, is apparently described in edk2 already: [CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c]: > /** > Retrieves the size, in bytes, of the context buffer required for HMAC-MD5 operations. > (NOTE: This API is deprecated. > Use HmacMd5New() / HmacMd5Free() for HMAC-MD5 Context operations.) > > @return The size, in bytes, of the context buffer required for HMAC-MD5 operations. > > **/ > UINTN > EFIAPI > HmacMd5GetContextSize ( > VOID > ) > { > // > // Retrieves the OpenSSL HMAC-MD5 Context Size > // NOTE: HMAC_CTX object was made opaque in openssl-1.1.x, here we just use the > // fixed size as a workaround to make this API work for compatibility. > // We should retire HmacMd5GetContextSize() in future, and use HmacMd5New() > // and HmacMd5Free() for context allocation and release. > // > return (UINTN) HMAC_MD5_CTX_SIZE; > } [CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c]: > /** > Retrieves the size, in bytes, of the context buffer required for HMAC-SHA1 operations. > (NOTE: This API is deprecated. > Use HmacSha1New() / HmacSha1Free() for HMAC-SHA1 Context operations.) > > @return The size, in bytes, of the context buffer required for HMAC-SHA1 operations. > > **/ > UINTN > EFIAPI > HmacSha1GetContextSize ( > VOID > ) > { > // > // Retrieves the OpenSSL HMAC-SHA1 Context Size > // NOTE: HMAC_CTX object was made opaque in openssl-1.1.x, here we just use the > // fixed size as a workaround to make this API work for compatibility. > // We should retire HmacSha15GetContextSize() in future, and use HmacSha1New() > // and HmacSha1Free() for context allocation and release. > // > return (UINTN) HMAC_SHA1_CTX_SIZE; > } [CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c]: > /** > Retrieves the size, in bytes, of the context buffer required for HMAC-SHA256 operations. > (NOTE: This API is deprecated. > Use HmacSha256New() / HmacSha256Free() for HMAC-SHA256 Context operations.) > > @return The size, in bytes, of the context buffer required for HMAC-SHA256 operations. > > **/ > UINTN > EFIAPI > HmacSha256GetContextSize ( > VOID > ) > { > // > // Retrieves the OpenSSL HMAC-SHA256 Context Size > // NOTE: HMAC_CTX object was made opaque in openssl-1.1.x, here we just use the > // fixed size as a workaround to make this API work for compatibility. > // We should retire HmacSha256GetContextSize() in future, and use HmacSha256New() > // and HmacSha256Free() for context allocation and release. > // > return (UINTN)HMAC_SHA256_CTX_SIZE; > } Is there any reason why we can't clean up this code first, in a separate patch series, before moving to OpenSSL 1.1.1? Because, the "NOTE"s are more than two years old now... They come from commit 4c270243995a ("CryptoPkg: Update HMAC Wrapper with opaque HMAC_CTX object.", 2017-03-29). This has been our technical debt ever since, violating the OpenSSL effort to hide implementation details. I think we should eliminate this debt, rather than make it worse. (Obviously this is just my opinion, and I'm not a CryptoPkg co-maintainer.) Thanks Laszlo