1. Patchew
  2. EDK2
  3. View series

[edk2-devel] [PATCH] CryptoPkg/BaseCryptLib: deprecate HmacXxxGetContextSize interface

Wang, Jian J posted 1 patch 4 years, 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/edk2 tags/patchew/20200108072650.1353-1-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(-)
[edk2-devel] [PATCH] CryptoPkg/BaseCryptLib: deprecate HmacXxxGetContextSize interface
Posted by Wang, Jian J 4 years, 3 months ago 
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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53004): https://edk2.groups.io/g/devel/message/53004
Mute This Topic: https://groups.io/mt/69523367/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] CryptoPkg/BaseCryptLib: deprecate HmacXxxGetContextSize interface
Posted by Laszlo Ersek 4 years, 3 months ago 
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.
>
>


-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] CryptoPkg/BaseCryptLib: deprecate HmacXxxGetContextSize interface
Posted by Wang, Jian J 4 years, 3 months ago 
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.
> >
> >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53043): https://edk2.groups.io/g/devel/message/53043
Mute This Topic: https://groups.io/mt/69523367/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] CryptoPkg/BaseCryptLib: deprecate HmacXxxGetContextSize interface
Posted by Laszlo Ersek 4 years, 3 months ago 
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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53083): https://edk2.groups.io/g/devel/message/53083
Mute This Topic: https://groups.io/mt/69523367/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] CryptoPkg/BaseCryptLib: deprecate HmacXxxGetContextSize interface
Posted by Wang, Jian J 4 years, 3 months ago 
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
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53206): https://edk2.groups.io/g/devel/message/53206
Mute This Topic: https://groups.io/mt/69523367/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.