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...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Signed-off-by: Jian J Wang <jian.j.w...@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.
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53009): https://edk2.groups.io/g/devel/message/53009
Mute This Topic: https://groups.io/mt/69523367/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
