* [PATCH] CryptoPkg/BaseCryptLib: deprecate HmacXxxGetContextSize interface @ 2020-01-08 7:26 Wang, Jian J 2020-01-08 10:24 ` Laszlo Ersek 0 siblings, 1 reply; 5+ messages in thread From: Wang, Jian J @ 2020-01-08 7:26 UTC (permalink / raw) To: devel; +Cc: Xiaoyu Lu, Laszlo Ersek REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1792 Hmac(Md5|Sha1|Sha256)GetContextSize() use a deprecated macro HMAC_MAX_MD_CBLOCK defined in openssl. They should be dropped to avoid misuses in the future. For context allocation and release, use HmacXxxNew() and HmacXxxFree() instead. Since HmacXxxNew will zero allocated context buffer, the calling to memset() in HmacXxxInit is safe to be removed. Cc: Xiaoyu Lu <xiaoyux.lu@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Jian J Wang <jian.j.wang@intel.com> --- CryptoPkg/Include/Library/BaseCryptLib.h | 51 ------------------- .../Library/BaseCryptLib/Hmac/CryptHmacMd5.c | 32 ------------ .../BaseCryptLib/Hmac/CryptHmacMd5Null.c | 20 -------- .../Library/BaseCryptLib/Hmac/CryptHmacSha1.c | 33 ------------ .../BaseCryptLib/Hmac/CryptHmacSha1Null.c | 20 -------- .../BaseCryptLib/Hmac/CryptHmacSha256.c | 32 ------------ .../BaseCryptLib/Hmac/CryptHmacSha256Null.c | 20 -------- .../BaseCryptLibNull/Hmac/CryptHmacMd5Null.c | 20 -------- .../BaseCryptLibNull/Hmac/CryptHmacSha1Null.c | 20 -------- .../Hmac/CryptHmacSha256Null.c | 20 -------- 10 files changed, 268 deletions(-) diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h b/CryptoPkg/Include/Library/BaseCryptLib.h index 8fe303a0b3..ffe606fa3f 100644 --- a/CryptoPkg/Include/Library/BaseCryptLib.h +++ b/CryptoPkg/Include/Library/BaseCryptLib.h @@ -1025,23 +1025,6 @@ Sm3HashAll ( // MAC (Message Authentication Code) Primitive //===================================================================================== -/** - 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.) - - If this interface is not supported, then return zero. - - @return The size, in bytes, of the context buffer required for HMAC-MD5 operations. - @retval 0 This interface is not supported. - -**/ -UINTN -EFIAPI -HmacMd5GetContextSize ( - VOID - ); - /** Allocates and initializes one HMAC_CTX context for subsequent HMAC-MD5 use. @@ -1175,23 +1158,6 @@ HmacMd5Final ( OUT UINT8 *HmacValue ); -/** - 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.) - - If this interface is not supported, then return zero. - - @return The size, in bytes, of the context buffer required for HMAC-SHA1 operations. - @retval 0 This interface is not supported. - -**/ -UINTN -EFIAPI -HmacSha1GetContextSize ( - VOID - ); - /** Allocates and initializes one HMAC_CTX context for subsequent HMAC-SHA1 use. @@ -1325,23 +1291,6 @@ HmacSha1Final ( OUT UINT8 *HmacValue ); -/** - 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.) - - If this interface is not supported, then return zero. - - @return The size, in bytes, of the context buffer required for HMAC-SHA256 operations. - @retval 0 This interface is not supported. - -**/ -UINTN -EFIAPI -HmacSha256GetContextSize ( - VOID - ); - /** Allocates and initializes one HMAC_CTX context for subsequent HMAC-SHA256 use. diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c index 19e9fbeae6..819842392b 100644 --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c @@ -9,37 +9,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include "InternalCryptLib.h" #include <openssl/hmac.h> -// -// NOTE: OpenSSL redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h -// #define HMAC_MAX_MD_CBLOCK_SIZE 144 -// -#define HMAC_MD5_CTX_SIZE (sizeof(void *) * 4 + sizeof(unsigned int) + \ - sizeof(unsigned char) * 144) - -/** - 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; -} - /** Allocates and initializes one HMAC_CTX context for subsequent HMAC-MD5 use. @@ -109,7 +78,6 @@ HmacMd5Init ( // // OpenSSL HMAC-MD5 Context Initialization // - memset(HmacMd5Context, 0, HMAC_MD5_CTX_SIZE); if (HMAC_CTX_reset ((HMAC_CTX *)HmacMd5Context) != 1) { return FALSE; } diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5Null.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5Null.c index 3aafed874b..205dc9e474 100644 --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5Null.c +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5Null.c @@ -8,26 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include "InternalCryptLib.h" -/** - 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 zero to indicate this interface is not supported. - - @retval 0 This interface is not supported. - -**/ -UINTN -EFIAPI -HmacMd5GetContextSize ( - VOID - ) -{ - ASSERT (FALSE); - return 0; -} - /** Allocates and initializes one HMAC_CTX context for subsequent HMAC-MD5 use. diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c index 7d7df9640e..f45ecebc6d 100644 --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c @@ -9,38 +9,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include "InternalCryptLib.h" #include <openssl/hmac.h> -// -// NOTE: OpenSSL redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h -// #define HMAC_MAX_MD_CBLOCK_SIZE 144 -// -// -#define HMAC_SHA1_CTX_SIZE (sizeof(void *) * 4 + sizeof(unsigned int) + \ - sizeof(unsigned char) * 144) - -/** - 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; -} - /** Allocates and initializes one HMAC_CTX context for subsequent HMAC-SHA1 use. @@ -110,7 +78,6 @@ HmacSha1Init ( // // OpenSSL HMAC-SHA1 Context Initialization // - memset(HmacSha1Context, 0, HMAC_SHA1_CTX_SIZE); if (HMAC_CTX_reset ((HMAC_CTX *)HmacSha1Context) != 1) { return FALSE; } diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1Null.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1Null.c index 547aa484ea..542350f15a 100644 --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1Null.c +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1Null.c @@ -8,26 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include "InternalCryptLib.h" -/** - 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 zero to indicate this interface is not supported. - - @retval 0 This interface is not supported. - -**/ -UINTN -EFIAPI -HmacSha1GetContextSize ( - VOID - ) -{ - ASSERT (FALSE); - return 0; -} - /** Allocates and initializes one HMAC_CTX context for subsequent HMAC-SHA1 use. diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c index f24443e745..446d629d74 100644 --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c @@ -9,37 +9,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include "InternalCryptLib.h" #include <openssl/hmac.h> -// -// NOTE: OpenSSL redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h -// #define HMAC_MAX_MD_CBLOCK_SIZE 144 -// -#define HMAC_SHA256_CTX_SIZE (sizeof(void *) * 4 + sizeof(unsigned int) + \ - sizeof(unsigned char) * 144) - -/** - 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; -} - /** Allocates and initializes one HMAC_CTX context for subsequent HMAC-SHA256 use. @@ -109,7 +78,6 @@ HmacSha256Init ( // // OpenSSL HMAC-SHA256 Context Initialization // - memset(HmacSha256Context, 0, HMAC_SHA256_CTX_SIZE); if (HMAC_CTX_reset ((HMAC_CTX *)HmacSha256Context) != 1) { return FALSE; } diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256Null.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256Null.c index f0a4420e27..f8074cc617 100644 --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256Null.c +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256Null.c @@ -8,26 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include "InternalCryptLib.h" -/** - 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 zero to indicate this interface is not supported. - - @retval 0 This interface is not supported. - -**/ -UINTN -EFIAPI -HmacSha256GetContextSize ( - VOID - ) -{ - ASSERT (FALSE); - return 0; -} - /** Allocates and initializes one HMAC_CTX context for subsequent HMAC-SHA256 use. diff --git a/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacMd5Null.c b/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacMd5Null.c index 3aafed874b..205dc9e474 100644 --- a/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacMd5Null.c +++ b/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacMd5Null.c @@ -8,26 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include "InternalCryptLib.h" -/** - 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 zero to indicate this interface is not supported. - - @retval 0 This interface is not supported. - -**/ -UINTN -EFIAPI -HmacMd5GetContextSize ( - VOID - ) -{ - ASSERT (FALSE); - return 0; -} - /** Allocates and initializes one HMAC_CTX context for subsequent HMAC-MD5 use. diff --git a/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha1Null.c b/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha1Null.c index 547aa484ea..542350f15a 100644 --- a/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha1Null.c +++ b/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha1Null.c @@ -8,26 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include "InternalCryptLib.h" -/** - 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 zero to indicate this interface is not supported. - - @retval 0 This interface is not supported. - -**/ -UINTN -EFIAPI -HmacSha1GetContextSize ( - VOID - ) -{ - ASSERT (FALSE); - return 0; -} - /** Allocates and initializes one HMAC_CTX context for subsequent HMAC-SHA1 use. diff --git a/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha256Null.c b/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha256Null.c index f0a4420e27..f8074cc617 100644 --- a/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha256Null.c +++ b/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha256Null.c @@ -8,26 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include "InternalCryptLib.h" -/** - 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 zero to indicate this interface is not supported. - - @retval 0 This interface is not supported. - -**/ -UINTN -EFIAPI -HmacSha256GetContextSize ( - VOID - ) -{ - ASSERT (FALSE); - return 0; -} - /** Allocates and initializes one HMAC_CTX context for subsequent HMAC-SHA256 use. -- 2.24.0.windows.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] CryptoPkg/BaseCryptLib: deprecate HmacXxxGetContextSize interface 2020-01-08 7:26 [PATCH] CryptoPkg/BaseCryptLib: deprecate HmacXxxGetContextSize interface Wang, Jian J @ 2020-01-08 10:24 ` Laszlo Ersek 2020-01-09 2:40 ` Wang, Jian J 0 siblings, 1 reply; 5+ messages in thread From: Laszlo Ersek @ 2020-01-08 10:24 UTC (permalink / raw) To: Jian J Wang, devel; +Cc: Xiaoyu Lu On 01/08/20 08:26, Jian J Wang wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1792 > > Hmac(Md5|Sha1|Sha256)GetContextSize() use a deprecated macro > HMAC_MAX_MD_CBLOCK defined in openssl. They should be dropped to > avoid misuses in the future. For context allocation and release, > use HmacXxxNew() and HmacXxxFree() instead. This sounds good, but the subject line is incorrect. We are deleting the Hmac(Md5|Sha1|Sha256)GetContextSize() functions right now, because they have been deprecated for a long time already. (1) Therefore, the subject should not say "deprecate", but "delete". > Since HmacXxxNew will zero allocated context buffer, the calling > to memset() in HmacXxxInit is safe to be removed. This is wrong, the memset() is not safe to remove. The Hmac(Md5|Sha1|Sha256)Init functions are *alternatives* to Hmac(Md5|Sha1|Sha256)New. Consider the (recommended, modern) HmacSha256New() function. The non-Null implementation is in "CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c": > VOID * > EFIAPI > HmacSha256New ( > VOID > ) > { > // > // Allocates & Initializes HMAC_CTX Context by OpenSSL HMAC_CTX_new() > // > return (VOID *) HMAC_CTX_new (); > } Let's see what HMAC_CTX_new() does -- it is implemented in "CryptoPkg/Library/OpensslLib/openssl/crypto/hmac/hmac.c": > HMAC_CTX *HMAC_CTX_new(void) > { > HMAC_CTX *ctx = OPENSSL_zalloc(sizeof(HMAC_CTX)); > > if (ctx != NULL) { > if (!HMAC_CTX_reset(ctx)) { > HMAC_CTX_free(ctx); > return NULL; > } > } > return ctx; > } Okay, so this is safe: we have first an OPENSSL_zalloc() call, which clears the allocated memory, and then we have a HMAC_CTX_reset() function call. Good. Now compare the HmacSha256Init() function (again in "CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c"): > /** > Initializes user-supplied memory pointed by HmacSha256Context as HMAC-SHA256 context for > subsequent use. > > If HmacSha256Context is NULL, then return FALSE. > > @param[out] HmacSha256Context Pointer to HMAC-SHA256 context being initialized. > @param[in] Key Pointer to the user-supplied key. > @param[in] KeySize Key size in bytes. > > @retval TRUE HMAC-SHA256 context initialization succeeded. > @retval FALSE HMAC-SHA256 context initialization failed. > > **/ > BOOLEAN > EFIAPI > HmacSha256Init ( > OUT VOID *HmacSha256Context, > IN CONST UINT8 *Key, > IN UINTN KeySize > ) > { > // > // Check input parameters. > // > if (HmacSha256Context == NULL || KeySize > INT_MAX) { > return FALSE; > } > > // > // OpenSSL HMAC-SHA256 Context Initialization > // > memset(HmacSha256Context, 0, HMAC_SHA256_CTX_SIZE); > if (HMAC_CTX_reset ((HMAC_CTX *)HmacSha256Context) != 1) { > return FALSE; > } > if (HMAC_Init_ex ((HMAC_CTX *)HmacSha256Context, Key, (UINT32) KeySize, EVP_sha256(), NULL) != 1) { > return FALSE; > } > > return TRUE; > } As the leading comment says, "HmacSha256Context" is user-supplied memory. If you remove the memset() call from the function, then HMAC_CTX_reset() will be invoked on user-supplied memory that may not have been cleared. Then HMAC_CTX_reset() will be called on garbage. (2) The only way that I can see for fixing this problem is to remove the Hmac(Md5|Sha1|Sha256)Init functions too. I think that is safe to do, because I can't see any callers in the edk2 codebase. One tricky part is that the leading comments of the Hmac(Md5|Sha1|Sha256)(Update|Final) functions refer to Hmac(Md5|Sha1|Sha256)Init. In other words, we do not have code references to Hmac(Md5|Sha1|Sha256)Init, but we have documentation references. This means that those comments should be updated as well -- they should refer to Hmac(Md5|Sha1|Sha256)New instead. (3) In case we'd like to continue providing functions that accept "Key" and "KeySize", for HMAC context initialization, then those functions will have to call HMAC_CTX_new() internally. Meaning that they can no longer take user-supplied memory; the context will have to be allocated inside OpenSSL, and returned to the caller. Thanks Laszlo > > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com> > --- > CryptoPkg/Include/Library/BaseCryptLib.h | 51 ------------------- > .../Library/BaseCryptLib/Hmac/CryptHmacMd5.c | 32 ------------ > .../BaseCryptLib/Hmac/CryptHmacMd5Null.c | 20 -------- > .../Library/BaseCryptLib/Hmac/CryptHmacSha1.c | 33 ------------ > .../BaseCryptLib/Hmac/CryptHmacSha1Null.c | 20 -------- > .../BaseCryptLib/Hmac/CryptHmacSha256.c | 32 ------------ > .../BaseCryptLib/Hmac/CryptHmacSha256Null.c | 20 -------- > .../BaseCryptLibNull/Hmac/CryptHmacMd5Null.c | 20 -------- > .../BaseCryptLibNull/Hmac/CryptHmacSha1Null.c | 20 -------- > .../Hmac/CryptHmacSha256Null.c | 20 -------- > 10 files changed, 268 deletions(-) > > diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h b/CryptoPkg/Include/Library/BaseCryptLib.h > index 8fe303a0b3..ffe606fa3f 100644 > --- a/CryptoPkg/Include/Library/BaseCryptLib.h > +++ b/CryptoPkg/Include/Library/BaseCryptLib.h > @@ -1025,23 +1025,6 @@ Sm3HashAll ( > // MAC (Message Authentication Code) Primitive > //===================================================================================== > > -/** > - 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.) > - > - If this interface is not supported, then return zero. > - > - @return The size, in bytes, of the context buffer required for HMAC-MD5 operations. > - @retval 0 This interface is not supported. > - > -**/ > -UINTN > -EFIAPI > -HmacMd5GetContextSize ( > - VOID > - ); > - > /** > Allocates and initializes one HMAC_CTX context for subsequent HMAC-MD5 use. > > @@ -1175,23 +1158,6 @@ HmacMd5Final ( > OUT UINT8 *HmacValue > ); > > -/** > - 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.) > - > - If this interface is not supported, then return zero. > - > - @return The size, in bytes, of the context buffer required for HMAC-SHA1 operations. > - @retval 0 This interface is not supported. > - > -**/ > -UINTN > -EFIAPI > -HmacSha1GetContextSize ( > - VOID > - ); > - > /** > Allocates and initializes one HMAC_CTX context for subsequent HMAC-SHA1 use. > > @@ -1325,23 +1291,6 @@ HmacSha1Final ( > OUT UINT8 *HmacValue > ); > > -/** > - 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.) > - > - If this interface is not supported, then return zero. > - > - @return The size, in bytes, of the context buffer required for HMAC-SHA256 operations. > - @retval 0 This interface is not supported. > - > -**/ > -UINTN > -EFIAPI > -HmacSha256GetContextSize ( > - VOID > - ); > - > /** > Allocates and initializes one HMAC_CTX context for subsequent HMAC-SHA256 use. > > diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c > index 19e9fbeae6..819842392b 100644 > --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c > +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c > @@ -9,37 +9,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #include "InternalCryptLib.h" > #include <openssl/hmac.h> > > -// > -// NOTE: OpenSSL redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h > -// #define HMAC_MAX_MD_CBLOCK_SIZE 144 > -// > -#define HMAC_MD5_CTX_SIZE (sizeof(void *) * 4 + sizeof(unsigned int) + \ > - sizeof(unsigned char) * 144) > - > -/** > - 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; > -} > - > /** > Allocates and initializes one HMAC_CTX context for subsequent HMAC-MD5 use. > > @@ -109,7 +78,6 @@ HmacMd5Init ( > // > // OpenSSL HMAC-MD5 Context Initialization > // > - memset(HmacMd5Context, 0, HMAC_MD5_CTX_SIZE); > if (HMAC_CTX_reset ((HMAC_CTX *)HmacMd5Context) != 1) { > return FALSE; > } > diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5Null.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5Null.c > index 3aafed874b..205dc9e474 100644 > --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5Null.c > +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5Null.c > @@ -8,26 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include "InternalCryptLib.h" > > -/** > - 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 zero to indicate this interface is not supported. > - > - @retval 0 This interface is not supported. > - > -**/ > -UINTN > -EFIAPI > -HmacMd5GetContextSize ( > - VOID > - ) > -{ > - ASSERT (FALSE); > - return 0; > -} > - > /** > Allocates and initializes one HMAC_CTX context for subsequent HMAC-MD5 use. > > diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c > index 7d7df9640e..f45ecebc6d 100644 > --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c > +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c > @@ -9,38 +9,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #include "InternalCryptLib.h" > #include <openssl/hmac.h> > > -// > -// NOTE: OpenSSL redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h > -// #define HMAC_MAX_MD_CBLOCK_SIZE 144 > -// > -// > -#define HMAC_SHA1_CTX_SIZE (sizeof(void *) * 4 + sizeof(unsigned int) + \ > - sizeof(unsigned char) * 144) > - > -/** > - 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; > -} > - > /** > Allocates and initializes one HMAC_CTX context for subsequent HMAC-SHA1 use. > > @@ -110,7 +78,6 @@ HmacSha1Init ( > // > // OpenSSL HMAC-SHA1 Context Initialization > // > - memset(HmacSha1Context, 0, HMAC_SHA1_CTX_SIZE); > if (HMAC_CTX_reset ((HMAC_CTX *)HmacSha1Context) != 1) { > return FALSE; > } > diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1Null.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1Null.c > index 547aa484ea..542350f15a 100644 > --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1Null.c > +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1Null.c > @@ -8,26 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include "InternalCryptLib.h" > > -/** > - 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 zero to indicate this interface is not supported. > - > - @retval 0 This interface is not supported. > - > -**/ > -UINTN > -EFIAPI > -HmacSha1GetContextSize ( > - VOID > - ) > -{ > - ASSERT (FALSE); > - return 0; > -} > - > /** > Allocates and initializes one HMAC_CTX context for subsequent HMAC-SHA1 use. > > diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c > index f24443e745..446d629d74 100644 > --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c > +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c > @@ -9,37 +9,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #include "InternalCryptLib.h" > #include <openssl/hmac.h> > > -// > -// NOTE: OpenSSL redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h > -// #define HMAC_MAX_MD_CBLOCK_SIZE 144 > -// > -#define HMAC_SHA256_CTX_SIZE (sizeof(void *) * 4 + sizeof(unsigned int) + \ > - sizeof(unsigned char) * 144) > - > -/** > - 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; > -} > - > /** > Allocates and initializes one HMAC_CTX context for subsequent HMAC-SHA256 use. > > @@ -109,7 +78,6 @@ HmacSha256Init ( > // > // OpenSSL HMAC-SHA256 Context Initialization > // > - memset(HmacSha256Context, 0, HMAC_SHA256_CTX_SIZE); > if (HMAC_CTX_reset ((HMAC_CTX *)HmacSha256Context) != 1) { > return FALSE; > } > diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256Null.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256Null.c > index f0a4420e27..f8074cc617 100644 > --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256Null.c > +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256Null.c > @@ -8,26 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include "InternalCryptLib.h" > > -/** > - 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 zero to indicate this interface is not supported. > - > - @retval 0 This interface is not supported. > - > -**/ > -UINTN > -EFIAPI > -HmacSha256GetContextSize ( > - VOID > - ) > -{ > - ASSERT (FALSE); > - return 0; > -} > - > /** > Allocates and initializes one HMAC_CTX context for subsequent HMAC-SHA256 use. > > diff --git a/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacMd5Null.c b/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacMd5Null.c > index 3aafed874b..205dc9e474 100644 > --- a/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacMd5Null.c > +++ b/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacMd5Null.c > @@ -8,26 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include "InternalCryptLib.h" > > -/** > - 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 zero to indicate this interface is not supported. > - > - @retval 0 This interface is not supported. > - > -**/ > -UINTN > -EFIAPI > -HmacMd5GetContextSize ( > - VOID > - ) > -{ > - ASSERT (FALSE); > - return 0; > -} > - > /** > Allocates and initializes one HMAC_CTX context for subsequent HMAC-MD5 use. > > diff --git a/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha1Null.c b/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha1Null.c > index 547aa484ea..542350f15a 100644 > --- a/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha1Null.c > +++ b/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha1Null.c > @@ -8,26 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include "InternalCryptLib.h" > > -/** > - 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 zero to indicate this interface is not supported. > - > - @retval 0 This interface is not supported. > - > -**/ > -UINTN > -EFIAPI > -HmacSha1GetContextSize ( > - VOID > - ) > -{ > - ASSERT (FALSE); > - return 0; > -} > - > /** > Allocates and initializes one HMAC_CTX context for subsequent HMAC-SHA1 use. > > diff --git a/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha256Null.c b/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha256Null.c > index f0a4420e27..f8074cc617 100644 > --- a/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha256Null.c > +++ b/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha256Null.c > @@ -8,26 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include "InternalCryptLib.h" > > -/** > - 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 zero to indicate this interface is not supported. > - > - @retval 0 This interface is not supported. > - > -**/ > -UINTN > -EFIAPI > -HmacSha256GetContextSize ( > - VOID > - ) > -{ > - ASSERT (FALSE); > - return 0; > -} > - > /** > Allocates and initializes one HMAC_CTX context for subsequent HMAC-SHA256 use. > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] CryptoPkg/BaseCryptLib: deprecate HmacXxxGetContextSize interface 2020-01-08 10:24 ` Laszlo Ersek @ 2020-01-09 2:40 ` Wang, Jian J 2020-01-09 14:19 ` Laszlo Ersek 0 siblings, 1 reply; 5+ messages in thread From: Wang, Jian J @ 2020-01-09 2:40 UTC (permalink / raw) To: Laszlo Ersek, devel@edk2.groups.io; +Cc: Lu, XiaoyuX Laszlo, > -----Original Message----- > From: Laszlo Ersek <lersek@redhat.com> > Sent: Wednesday, January 08, 2020 6:24 PM > To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io > Cc: Lu, XiaoyuX <xiaoyux.lu@intel.com> > Subject: Re: [PATCH] CryptoPkg/BaseCryptLib: deprecate > HmacXxxGetContextSize interface > > On 01/08/20 08:26, Jian J Wang wrote: > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1792 > > > > Hmac(Md5|Sha1|Sha256)GetContextSize() use a deprecated macro > > HMAC_MAX_MD_CBLOCK defined in openssl. They should be dropped to > > avoid misuses in the future. For context allocation and release, > > use HmacXxxNew() and HmacXxxFree() instead. > > This sounds good, but the subject line is incorrect. > > We are deleting the Hmac(Md5|Sha1|Sha256)GetContextSize() functions > right now, because they have been deprecated for a long time already. > > (1) Therefore, the subject should not say "deprecate", but "delete". You're right. I'll change it. > > > Since HmacXxxNew will zero allocated context buffer, the calling > > to memset() in HmacXxxInit is safe to be removed. > > This is wrong, the memset() is not safe to remove. The > Hmac(Md5|Sha1|Sha256)Init functions are *alternatives* to > Hmac(Md5|Sha1|Sha256)New. > > Consider the (recommended, modern) HmacSha256New() function. The > non-Null implementation is in > "CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c": > > > VOID * > > EFIAPI > > HmacSha256New ( > > VOID > > ) > > { > > // > > // Allocates & Initializes HMAC_CTX Context by OpenSSL HMAC_CTX_new() > > // > > return (VOID *) HMAC_CTX_new (); > > } > > Let's see what HMAC_CTX_new() does -- it is implemented in > "CryptoPkg/Library/OpensslLib/openssl/crypto/hmac/hmac.c": > > > HMAC_CTX *HMAC_CTX_new(void) > > { > > HMAC_CTX *ctx = OPENSSL_zalloc(sizeof(HMAC_CTX)); > > > > if (ctx != NULL) { > > if (!HMAC_CTX_reset(ctx)) { > > HMAC_CTX_free(ctx); > > return NULL; > > } > > } > > return ctx; > > } > > Okay, so this is safe: we have first an OPENSSL_zalloc() call, which > clears the allocated memory, and then we have a HMAC_CTX_reset() > function call. Good. > > Now compare the HmacSha256Init() function (again in > "CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c"): > > > /** > > Initializes user-supplied memory pointed by HmacSha256Context as HMAC- > SHA256 context for > > subsequent use. > > > > If HmacSha256Context is NULL, then return FALSE. > > > > @param[out] HmacSha256Context Pointer to HMAC-SHA256 context being > initialized. > > @param[in] Key Pointer to the user-supplied key. > > @param[in] KeySize Key size in bytes. > > > > @retval TRUE HMAC-SHA256 context initialization succeeded. > > @retval FALSE HMAC-SHA256 context initialization failed. > > > > **/ > > BOOLEAN > > EFIAPI > > HmacSha256Init ( > > OUT VOID *HmacSha256Context, > > IN CONST UINT8 *Key, > > IN UINTN KeySize > > ) > > { > > // > > // Check input parameters. > > // > > if (HmacSha256Context == NULL || KeySize > INT_MAX) { > > return FALSE; > > } > > > > // > > // OpenSSL HMAC-SHA256 Context Initialization > > // > > memset(HmacSha256Context, 0, HMAC_SHA256_CTX_SIZE); > > if (HMAC_CTX_reset ((HMAC_CTX *)HmacSha256Context) != 1) { > > return FALSE; > > } > > if (HMAC_Init_ex ((HMAC_CTX *)HmacSha256Context, Key, (UINT32) KeySize, > EVP_sha256(), NULL) != 1) { > > return FALSE; > > } > > > > return TRUE; > > } > > As the leading comment says, "HmacSha256Context" is user-supplied > memory. If you remove the memset() call from the function, then > HMAC_CTX_reset() will be invoked on user-supplied memory that may not > have been cleared. Then HMAC_CTX_reset() will be called on garbage. > You're right, if the user can supply a chunk of memory with *appropriate* size as HmacContext. Since we deleted the macro HMAC_XXX_CTX_SIZE, it's impossible for user to do that now. HMAC_CTX is a forward declaration. MSVC refuses to give result of sizeof (HMAC_CTX). The user cannot know how many bytes needed by HMAC_CTX. Therefore there's no such use cases any longer. I think we could update the comments to enforce the use of HmacXxxNew() to get context. User supplied-memory is not acceptable. We can still keep the HMAC_CTX_reset line so that the user can still re-use the context got before by HmacXxxNew(). I think HMAC_CTX_reset works well with an empty Context or init-ed Context. > (2) The only way that I can see for fixing this problem is to remove the > Hmac(Md5|Sha1|Sha256)Init functions too. > > I think that is safe to do, because I can't see any callers in the edk2 > codebase. > > One tricky part is that the leading comments of the > Hmac(Md5|Sha1|Sha256)(Update|Final) functions refer to > Hmac(Md5|Sha1|Sha256)Init. In other words, we do not have code > references to Hmac(Md5|Sha1|Sha256)Init, but we have documentation > references. This means that those comments should be updated as well -- > they should refer to Hmac(Md5|Sha1|Sha256)New instead. > The Init interface is needed to supply user's key for HMAC. It seems the only way to do that. I suggest to keep it. > (3) In case we'd like to continue providing functions that accept "Key" > and "KeySize", for HMAC context initialization, then those functions > will have to call HMAC_CTX_new() internally. Meaning that they can no > longer take user-supplied memory; the context will have to be allocated > inside OpenSSL, and returned to the caller. Yes, the variable encryption feature I'm working on needs to supply user supplied key. I think it'd be better to keep it. Like I suggested above, we should not allow user-supplied context and it's almost impossible for use to supply correct size of context. Thanks for the comments. Regards, Jian > > Thanks > Laszlo > > > > > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > Signed-off-by: Jian J Wang <jian.j.wang@intel.com> > > --- > > CryptoPkg/Include/Library/BaseCryptLib.h | 51 ------------------- > > .../Library/BaseCryptLib/Hmac/CryptHmacMd5.c | 32 ------------ > > .../BaseCryptLib/Hmac/CryptHmacMd5Null.c | 20 -------- > > .../Library/BaseCryptLib/Hmac/CryptHmacSha1.c | 33 ------------ > > .../BaseCryptLib/Hmac/CryptHmacSha1Null.c | 20 -------- > > .../BaseCryptLib/Hmac/CryptHmacSha256.c | 32 ------------ > > .../BaseCryptLib/Hmac/CryptHmacSha256Null.c | 20 -------- > > .../BaseCryptLibNull/Hmac/CryptHmacMd5Null.c | 20 -------- > > .../BaseCryptLibNull/Hmac/CryptHmacSha1Null.c | 20 -------- > > .../Hmac/CryptHmacSha256Null.c | 20 -------- > > 10 files changed, 268 deletions(-) > > > > diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h > b/CryptoPkg/Include/Library/BaseCryptLib.h > > index 8fe303a0b3..ffe606fa3f 100644 > > --- a/CryptoPkg/Include/Library/BaseCryptLib.h > > +++ b/CryptoPkg/Include/Library/BaseCryptLib.h > > @@ -1025,23 +1025,6 @@ Sm3HashAll ( > > // MAC (Message Authentication Code) Primitive > > > //=============================================================== > ====================== > > > > -/** > > - 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.) > > - > > - If this interface is not supported, then return zero. > > - > > - @return The size, in bytes, of the context buffer required for HMAC-MD5 > operations. > > - @retval 0 This interface is not supported. > > - > > -**/ > > -UINTN > > -EFIAPI > > -HmacMd5GetContextSize ( > > - VOID > > - ); > > - > > /** > > Allocates and initializes one HMAC_CTX context for subsequent HMAC-MD5 > use. > > > > @@ -1175,23 +1158,6 @@ HmacMd5Final ( > > OUT UINT8 *HmacValue > > ); > > > > -/** > > - 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.) > > - > > - If this interface is not supported, then return zero. > > - > > - @return The size, in bytes, of the context buffer required for HMAC-SHA1 > operations. > > - @retval 0 This interface is not supported. > > - > > -**/ > > -UINTN > > -EFIAPI > > -HmacSha1GetContextSize ( > > - VOID > > - ); > > - > > /** > > Allocates and initializes one HMAC_CTX context for subsequent HMAC-SHA1 > use. > > > > @@ -1325,23 +1291,6 @@ HmacSha1Final ( > > OUT UINT8 *HmacValue > > ); > > > > -/** > > - 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.) > > - > > - If this interface is not supported, then return zero. > > - > > - @return The size, in bytes, of the context buffer required for HMAC-SHA256 > operations. > > - @retval 0 This interface is not supported. > > - > > -**/ > > -UINTN > > -EFIAPI > > -HmacSha256GetContextSize ( > > - VOID > > - ); > > - > > /** > > Allocates and initializes one HMAC_CTX context for subsequent HMAC- > SHA256 use. > > > > diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c > b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c > > index 19e9fbeae6..819842392b 100644 > > --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c > > +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c > > @@ -9,37 +9,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include "InternalCryptLib.h" > > #include <openssl/hmac.h> > > > > -// > > -// NOTE: OpenSSL redefines the size of HMAC_CTX at > crypto/hmac/hmac_lcl.h > > -// #define HMAC_MAX_MD_CBLOCK_SIZE 144 > > -// > > -#define HMAC_MD5_CTX_SIZE (sizeof(void *) * 4 + sizeof(unsigned int) + \ > > - sizeof(unsigned char) * 144) > > - > > -/** > > - 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; > > -} > > - > > /** > > Allocates and initializes one HMAC_CTX context for subsequent HMAC-MD5 > use. > > > > @@ -109,7 +78,6 @@ HmacMd5Init ( > > // > > // OpenSSL HMAC-MD5 Context Initialization > > // > > - memset(HmacMd5Context, 0, HMAC_MD5_CTX_SIZE); > > if (HMAC_CTX_reset ((HMAC_CTX *)HmacMd5Context) != 1) { > > return FALSE; > > } > > diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5Null.c > b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5Null.c > > index 3aafed874b..205dc9e474 100644 > > --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5Null.c > > +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5Null.c > > @@ -8,26 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > > > #include "InternalCryptLib.h" > > > > -/** > > - 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 zero to indicate this interface is not supported. > > - > > - @retval 0 This interface is not supported. > > - > > -**/ > > -UINTN > > -EFIAPI > > -HmacMd5GetContextSize ( > > - VOID > > - ) > > -{ > > - ASSERT (FALSE); > > - return 0; > > -} > > - > > /** > > Allocates and initializes one HMAC_CTX context for subsequent HMAC-MD5 > use. > > > > diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c > b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c > > index 7d7df9640e..f45ecebc6d 100644 > > --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c > > +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c > > @@ -9,38 +9,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include "InternalCryptLib.h" > > #include <openssl/hmac.h> > > > > -// > > -// NOTE: OpenSSL redefines the size of HMAC_CTX at > crypto/hmac/hmac_lcl.h > > -// #define HMAC_MAX_MD_CBLOCK_SIZE 144 > > -// > > -// > > -#define HMAC_SHA1_CTX_SIZE (sizeof(void *) * 4 + sizeof(unsigned int) + \ > > - sizeof(unsigned char) * 144) > > - > > -/** > > - 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; > > -} > > - > > /** > > Allocates and initializes one HMAC_CTX context for subsequent HMAC-SHA1 > use. > > > > @@ -110,7 +78,6 @@ HmacSha1Init ( > > // > > // OpenSSL HMAC-SHA1 Context Initialization > > // > > - memset(HmacSha1Context, 0, HMAC_SHA1_CTX_SIZE); > > if (HMAC_CTX_reset ((HMAC_CTX *)HmacSha1Context) != 1) { > > return FALSE; > > } > > diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1Null.c > b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1Null.c > > index 547aa484ea..542350f15a 100644 > > --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1Null.c > > +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1Null.c > > @@ -8,26 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > > > #include "InternalCryptLib.h" > > > > -/** > > - 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 zero to indicate this interface is not supported. > > - > > - @retval 0 This interface is not supported. > > - > > -**/ > > -UINTN > > -EFIAPI > > -HmacSha1GetContextSize ( > > - VOID > > - ) > > -{ > > - ASSERT (FALSE); > > - return 0; > > -} > > - > > /** > > Allocates and initializes one HMAC_CTX context for subsequent HMAC-SHA1 > use. > > > > diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c > b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c > > index f24443e745..446d629d74 100644 > > --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c > > +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c > > @@ -9,37 +9,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include "InternalCryptLib.h" > > #include <openssl/hmac.h> > > > > -// > > -// NOTE: OpenSSL redefines the size of HMAC_CTX at > crypto/hmac/hmac_lcl.h > > -// #define HMAC_MAX_MD_CBLOCK_SIZE 144 > > -// > > -#define HMAC_SHA256_CTX_SIZE (sizeof(void *) * 4 + sizeof(unsigned int) + > \ > > - sizeof(unsigned char) * 144) > > - > > -/** > > - 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; > > -} > > - > > /** > > Allocates and initializes one HMAC_CTX context for subsequent HMAC- > SHA256 use. > > > > @@ -109,7 +78,6 @@ HmacSha256Init ( > > // > > // OpenSSL HMAC-SHA256 Context Initialization > > // > > - memset(HmacSha256Context, 0, HMAC_SHA256_CTX_SIZE); > > if (HMAC_CTX_reset ((HMAC_CTX *)HmacSha256Context) != 1) { > > return FALSE; > > } > > diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256Null.c > b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256Null.c > > index f0a4420e27..f8074cc617 100644 > > --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256Null.c > > +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256Null.c > > @@ -8,26 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > > > #include "InternalCryptLib.h" > > > > -/** > > - 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 zero to indicate this interface is not supported. > > - > > - @retval 0 This interface is not supported. > > - > > -**/ > > -UINTN > > -EFIAPI > > -HmacSha256GetContextSize ( > > - VOID > > - ) > > -{ > > - ASSERT (FALSE); > > - return 0; > > -} > > - > > /** > > Allocates and initializes one HMAC_CTX context for subsequent HMAC- > SHA256 use. > > > > diff --git a/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacMd5Null.c > b/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacMd5Null.c > > index 3aafed874b..205dc9e474 100644 > > --- a/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacMd5Null.c > > +++ b/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacMd5Null.c > > @@ -8,26 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > > > #include "InternalCryptLib.h" > > > > -/** > > - 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 zero to indicate this interface is not supported. > > - > > - @retval 0 This interface is not supported. > > - > > -**/ > > -UINTN > > -EFIAPI > > -HmacMd5GetContextSize ( > > - VOID > > - ) > > -{ > > - ASSERT (FALSE); > > - return 0; > > -} > > - > > /** > > Allocates and initializes one HMAC_CTX context for subsequent HMAC-MD5 > use. > > > > diff --git a/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha1Null.c > b/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha1Null.c > > index 547aa484ea..542350f15a 100644 > > --- a/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha1Null.c > > +++ b/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha1Null.c > > @@ -8,26 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > > > #include "InternalCryptLib.h" > > > > -/** > > - 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 zero to indicate this interface is not supported. > > - > > - @retval 0 This interface is not supported. > > - > > -**/ > > -UINTN > > -EFIAPI > > -HmacSha1GetContextSize ( > > - VOID > > - ) > > -{ > > - ASSERT (FALSE); > > - return 0; > > -} > > - > > /** > > Allocates and initializes one HMAC_CTX context for subsequent HMAC-SHA1 > use. > > > > diff --git a/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha256Null.c > b/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha256Null.c > > index f0a4420e27..f8074cc617 100644 > > --- a/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha256Null.c > > +++ b/CryptoPkg/Library/BaseCryptLibNull/Hmac/CryptHmacSha256Null.c > > @@ -8,26 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > > > #include "InternalCryptLib.h" > > > > -/** > > - 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 zero to indicate this interface is not supported. > > - > > - @retval 0 This interface is not supported. > > - > > -**/ > > -UINTN > > -EFIAPI > > -HmacSha256GetContextSize ( > > - VOID > > - ) > > -{ > > - ASSERT (FALSE); > > - return 0; > > -} > > - > > /** > > Allocates and initializes one HMAC_CTX context for subsequent HMAC- > SHA256 use. > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] CryptoPkg/BaseCryptLib: deprecate HmacXxxGetContextSize interface 2020-01-09 2:40 ` Wang, Jian J @ 2020-01-09 14:19 ` Laszlo Ersek 2020-01-14 5:50 ` [edk2-devel] " Wang, Jian J 0 siblings, 1 reply; 5+ messages in thread From: Laszlo Ersek @ 2020-01-09 14:19 UTC (permalink / raw) To: Wang, Jian J, devel@edk2.groups.io; +Cc: Lu, XiaoyuX On 01/09/20 03:40, Wang, Jian J wrote: >> -----Original Message----- >> From: Laszlo Ersek <lersek@redhat.com> >> Sent: Wednesday, January 08, 2020 6:24 PM >> To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io >> Cc: Lu, XiaoyuX <xiaoyux.lu@intel.com> >> Subject: Re: [PATCH] CryptoPkg/BaseCryptLib: deprecate >> HmacXxxGetContextSize interface >> >> On 01/08/20 08:26, Jian J Wang wrote: >>> /** >>> Initializes user-supplied memory pointed by HmacSha256Context as HMAC- >> SHA256 context for >>> subsequent use. >>> >>> If HmacSha256Context is NULL, then return FALSE. >>> >>> @param[out] HmacSha256Context Pointer to HMAC-SHA256 context being >> initialized. >>> @param[in] Key Pointer to the user-supplied key. >>> @param[in] KeySize Key size in bytes. >>> >>> @retval TRUE HMAC-SHA256 context initialization succeeded. >>> @retval FALSE HMAC-SHA256 context initialization failed. >>> >>> **/ >>> BOOLEAN >>> EFIAPI >>> HmacSha256Init ( >>> OUT VOID *HmacSha256Context, >>> IN CONST UINT8 *Key, >>> IN UINTN KeySize >>> ) >>> { >>> // >>> // Check input parameters. >>> // >>> if (HmacSha256Context == NULL || KeySize > INT_MAX) { >>> return FALSE; >>> } >>> >>> // >>> // OpenSSL HMAC-SHA256 Context Initialization >>> // >>> memset(HmacSha256Context, 0, HMAC_SHA256_CTX_SIZE); >>> if (HMAC_CTX_reset ((HMAC_CTX *)HmacSha256Context) != 1) { >>> return FALSE; >>> } >>> if (HMAC_Init_ex ((HMAC_CTX *)HmacSha256Context, Key, (UINT32) KeySize, >> EVP_sha256(), NULL) != 1) { >>> return FALSE; >>> } >>> >>> return TRUE; >>> } >> >> As the leading comment says, "HmacSha256Context" is user-supplied >> memory. If you remove the memset() call from the function, then >> HMAC_CTX_reset() will be invoked on user-supplied memory that may not >> have been cleared. Then HMAC_CTX_reset() will be called on garbage. >> > > You're right, if the user can supply a chunk of memory with *appropriate* > size as HmacContext. Since we deleted the macro HMAC_XXX_CTX_SIZE, > it's impossible for user to do that now. HMAC_CTX is a forward declaration. > MSVC refuses to give result of sizeof (HMAC_CTX). The user cannot know > how many bytes needed by HMAC_CTX. Therefore there's no such use cases > any longer. I think we could update the comments to enforce the use of > HmacXxxNew() to get context. User supplied-memory is not acceptable. > > We can still keep the HMAC_CTX_reset line so that the user can still re-use > the context got before by HmacXxxNew(). I think HMAC_CTX_reset works > well with an empty Context or init-ed Context. > >> (2) The only way that I can see for fixing this problem is to remove the >> Hmac(Md5|Sha1|Sha256)Init functions too. >> >> I think that is safe to do, because I can't see any callers in the edk2 >> codebase. >> >> One tricky part is that the leading comments of the >> Hmac(Md5|Sha1|Sha256)(Update|Final) functions refer to >> Hmac(Md5|Sha1|Sha256)Init. In other words, we do not have code >> references to Hmac(Md5|Sha1|Sha256)Init, but we have documentation >> references. This means that those comments should be updated as well -- >> they should refer to Hmac(Md5|Sha1|Sha256)New instead. >> > > The Init interface is needed to supply user's key for HMAC. It seems the only > way to do that. I suggest to keep it. > >> (3) In case we'd like to continue providing functions that accept "Key" >> and "KeySize", for HMAC context initialization, then those functions >> will have to call HMAC_CTX_new() internally. Meaning that they can no >> longer take user-supplied memory; the context will have to be allocated >> inside OpenSSL, and returned to the caller. > > Yes, the variable encryption feature I'm working on needs to supply user > supplied key. I think it'd be better to keep it. Like I suggested above, we > should not allow user-supplied context and it's almost impossible for use > to supply correct size of context. Assuming I understand your response correctly, I would suggest: (1) Renaming "Init" to "SetKey", (2) deleting both the memset() and the HMAC_CTX_reset() calls from "SetKey" (3) updating the comment on "SetKey" so that it does not refer to "user supplied memory"; instead, it should say that "SetKey" can only be called on context returned by the appropriate "New" call, and only immediately after the "New" call (no intervening operations permitted on the context). The goal of (1) is to clearly distinguish the key setting action from allocation/initialization. The goal of (2) and (3) together is to have a pristine (zalloc, reset, Init_ex) triplet, with no repeated actions, when getting a new context, and setting a key in it (that is, when the caller invokes New and then SetKey). Thanks, Laszlo ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCH] CryptoPkg/BaseCryptLib: deprecate HmacXxxGetContextSize interface 2020-01-09 14:19 ` Laszlo Ersek @ 2020-01-14 5:50 ` Wang, Jian J 0 siblings, 0 replies; 5+ messages in thread From: Wang, Jian J @ 2020-01-14 5:50 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com; +Cc: Lu, XiaoyuX Laszlo, Good suggestions. I'll send v2 patch soon. Regards, Jian > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek > Sent: Thursday, January 09, 2020 10:20 PM > To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io > Cc: Lu, XiaoyuX <xiaoyux.lu@intel.com> > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/BaseCryptLib: deprecate > HmacXxxGetContextSize interface > > On 01/09/20 03:40, Wang, Jian J wrote: > >> -----Original Message----- > >> From: Laszlo Ersek <lersek@redhat.com> > >> Sent: Wednesday, January 08, 2020 6:24 PM > >> To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io > >> Cc: Lu, XiaoyuX <xiaoyux.lu@intel.com> > >> Subject: Re: [PATCH] CryptoPkg/BaseCryptLib: deprecate > >> HmacXxxGetContextSize interface > >> > >> On 01/08/20 08:26, Jian J Wang wrote: > > >>> /** > >>> Initializes user-supplied memory pointed by HmacSha256Context as HMAC- > >> SHA256 context for > >>> subsequent use. > >>> > >>> If HmacSha256Context is NULL, then return FALSE. > >>> > >>> @param[out] HmacSha256Context Pointer to HMAC-SHA256 context > being > >> initialized. > >>> @param[in] Key Pointer to the user-supplied key. > >>> @param[in] KeySize Key size in bytes. > >>> > >>> @retval TRUE HMAC-SHA256 context initialization succeeded. > >>> @retval FALSE HMAC-SHA256 context initialization failed. > >>> > >>> **/ > >>> BOOLEAN > >>> EFIAPI > >>> HmacSha256Init ( > >>> OUT VOID *HmacSha256Context, > >>> IN CONST UINT8 *Key, > >>> IN UINTN KeySize > >>> ) > >>> { > >>> // > >>> // Check input parameters. > >>> // > >>> if (HmacSha256Context == NULL || KeySize > INT_MAX) { > >>> return FALSE; > >>> } > >>> > >>> // > >>> // OpenSSL HMAC-SHA256 Context Initialization > >>> // > >>> memset(HmacSha256Context, 0, HMAC_SHA256_CTX_SIZE); > >>> if (HMAC_CTX_reset ((HMAC_CTX *)HmacSha256Context) != 1) { > >>> return FALSE; > >>> } > >>> if (HMAC_Init_ex ((HMAC_CTX *)HmacSha256Context, Key, (UINT32) > KeySize, > >> EVP_sha256(), NULL) != 1) { > >>> return FALSE; > >>> } > >>> > >>> return TRUE; > >>> } > >> > >> As the leading comment says, "HmacSha256Context" is user-supplied > >> memory. If you remove the memset() call from the function, then > >> HMAC_CTX_reset() will be invoked on user-supplied memory that may not > >> have been cleared. Then HMAC_CTX_reset() will be called on garbage. > >> > > > > You're right, if the user can supply a chunk of memory with *appropriate* > > size as HmacContext. Since we deleted the macro HMAC_XXX_CTX_SIZE, > > it's impossible for user to do that now. HMAC_CTX is a forward declaration. > > MSVC refuses to give result of sizeof (HMAC_CTX). The user cannot know > > how many bytes needed by HMAC_CTX. Therefore there's no such use cases > > any longer. I think we could update the comments to enforce the use of > > HmacXxxNew() to get context. User supplied-memory is not acceptable. > > > > We can still keep the HMAC_CTX_reset line so that the user can still re-use > > the context got before by HmacXxxNew(). I think HMAC_CTX_reset works > > well with an empty Context or init-ed Context. > > > >> (2) The only way that I can see for fixing this problem is to remove the > >> Hmac(Md5|Sha1|Sha256)Init functions too. > >> > >> I think that is safe to do, because I can't see any callers in the edk2 > >> codebase. > >> > >> One tricky part is that the leading comments of the > >> Hmac(Md5|Sha1|Sha256)(Update|Final) functions refer to > >> Hmac(Md5|Sha1|Sha256)Init. In other words, we do not have code > >> references to Hmac(Md5|Sha1|Sha256)Init, but we have documentation > >> references. This means that those comments should be updated as well -- > >> they should refer to Hmac(Md5|Sha1|Sha256)New instead. > >> > > > > The Init interface is needed to supply user's key for HMAC. It seems the only > > way to do that. I suggest to keep it. > > > >> (3) In case we'd like to continue providing functions that accept "Key" > >> and "KeySize", for HMAC context initialization, then those functions > >> will have to call HMAC_CTX_new() internally. Meaning that they can no > >> longer take user-supplied memory; the context will have to be allocated > >> inside OpenSSL, and returned to the caller. > > > > Yes, the variable encryption feature I'm working on needs to supply user > > supplied key. I think it'd be better to keep it. Like I suggested above, we > > should not allow user-supplied context and it's almost impossible for use > > to supply correct size of context. > > Assuming I understand your response correctly, I would suggest: > > (1) Renaming "Init" to "SetKey", > > (2) deleting both the memset() and the HMAC_CTX_reset() calls from "SetKey" > > (3) updating the comment on "SetKey" so that it does not refer to "user > supplied memory"; instead, it should say that "SetKey" can only be > called on context returned by the appropriate "New" call, and only > immediately after the "New" call (no intervening operations permitted on > the context). > > The goal of (1) is to clearly distinguish the key setting action from > allocation/initialization. > > The goal of (2) and (3) together is to have a pristine (zalloc, reset, > Init_ex) triplet, with no repeated actions, when getting a new context, > and setting a key in it (that is, when the caller invokes New and then > SetKey). > > Thanks, > Laszlo > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-01-14 5:50 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-01-08 7:26 [PATCH] CryptoPkg/BaseCryptLib: deprecate HmacXxxGetContextSize interface Wang, Jian J 2020-01-08 10:24 ` Laszlo Ersek 2020-01-09 2:40 ` Wang, Jian J 2020-01-09 14:19 ` Laszlo Ersek 2020-01-14 5:50 ` [edk2-devel] " Wang, Jian J
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox