Laszlo, Yes, we have internal platforms using them. I'll file a BZ to remove those APIs in next stable tag.
Regards, Jian > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Laszlo Ersek > Sent: Tuesday, April 30, 2019 6:27 PM > To: Wang, Jian J <jian.j.w...@intel.com>; devel@edk2.groups.io; Lu, XiaoyuX > <xiaoyux...@intel.com> > Cc: Ye, Ting <ting...@intel.com> > Subject: Re: [edk2-devel] [PATCH 3/3] CryptoPkg/BaseCryptLib: updata > HMAC_ctx size > > On 04/30/19 03:16, Wang, Jian J wrote: > > Laszlo, > > > > > >> -----Original Message----- > >> From: Laszlo Ersek [mailto:ler...@redhat.com] > >> Sent: Tuesday, April 30, 2019 2:31 AM > >> To: devel@edk2.groups.io; Lu, XiaoyuX <xiaoyux...@intel.com> > >> Cc: Wang, Jian J <jian.j.w...@intel.com>; Ye, Ting <ting...@intel.com> > >> Subject: Re: [edk2-devel] [PATCH 3/3] CryptoPkg/BaseCryptLib: updata > >> HMAC_ctx size > >> > >> On 04/29/19 10:15, Xiaoyu lu wrote: > >>> From: Xiaoyu Lu <xiaoyux...@intel.com> > >>> > >>> Openssl internally redefines the size of HMAC_CTX, > >>> but there is no external definition. > >>> So add an additional nubmer. > >>> > >>> Cc: Jian J Wang <jian.j.w...@intel.com> > >>> Cc: Ting Ye <ting...@intel.com> > >>> Signed-off-by: Xiaoyu Lu <xiaoyux...@intel.com> > >>> --- > >>> 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 <openssl/hmac.h> > >>> > >>> +// > >>> +// 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 <openssl/hmac.h> > >>> > >>> -#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 <openssl/hmac.h> > >>> > >>> -#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.) > >> > > > > I agree that we need to clean up the deprecated macros and api. The problem > > is that XxxGetContextSize() are public APIs and there're dozens of modules > which > > still call them. This can't be done in short time. What about we file a BZ > > to > track > > this issue and target to next stable tag? > > I don't see any calls to either of HmacMd5GetContextSize, > HmacSha1GetContextSize, and HmacSha256GetContextSize in the open source > edk2 tree. > > Do you mean those dozens of calls are in the edk2-platforms project, or > in proprietary platfoms? > > I do see that the change affects the <BaseCryptLib.h> lib class header, > so it seems justified to postpone the real fix to the next stable tag. > Please file a BZ for this. > > Regarding this patch: > > (1) The commit message should explain the situation a lot more clearly. > It should reference both > > - https://github.com/openssl/openssl/pull/4338 > > and > > - OpenSSL commit e0810e3502bb ("Fix HMAC SHA3-224 and HMAC SHA3-256.", > 2018-09-04) > > (2) The commit message should also reference the new TianoCore BZ (about > removing these functions from the lib class). > > (3) The code comments are confusing, and should be deleted, in my > opinion. A good commit message will suffice. > > In particular, the following part of the comment: > > "we need to compatible with previous API" > > is very confusing, as it first made me think that we should stick with > the *previous* value, i.e. 128, for compatibility with callers of these > functions. > > (4) In the patch (= the new code), HMAC_MAX_MD_CBLOCK should be > eliminated completely, and the sum (HMAC_MAX_MD_CBLOCK + 16) should be > replaced with the constant 144. > > The fact that the new context size is 16 bytes larger than what it used > to be (due to the introduction of SHA3-224) is circumstantial. The old > value (128) is irrelevant, so we shouldn't express the new value > relative to the old value. The new size is 144 bytes because the new > size is 144 bytes. > > Thanks > Laszlo > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#39840): https://edk2.groups.io/g/devel/message/39840 Mute This Topic: https://groups.io/mt/31381055/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
