1. Patchew
  2. EDK2
  3. [edk2-devel] [PATCH 0/3] CryptoPkg: Upgrade OpenSSL to 1_1_1b
  4. View patch

[edk2-devel] [PATCH 3/3] CryptoPkg/BaseCryptLib: updata HMAC_ctx size

Xiaoyu lu posted 3 patches 6 years, 9 months ago
[edk2-devel] [PATCH 3/3] CryptoPkg/BaseCryptLib: updata HMAC_ctx size
Posted by Xiaoyu lu 6 years, 9 months ago 
From: Xiaoyu Lu <xiaoyux.lu@intel.com>

Openssl internally redefines the size of HMAC_CTX,
but there is no external definition.
So add an additional nubmer.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Ting Ye <ting.ye@intel.com>
Signed-off-by: Xiaoyu Lu <xiaoyux.lu@intel.com>
---
 CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c    | 11 ++++++++++-
 CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c   | 12 ++++++++++--
 CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c | 12 ++++++++++--
 3 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
index 3134806..3ffb8e2 100644
--- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
+++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
@@ -9,8 +9,17 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include "InternalCryptLib.h"
 #include <openssl/hmac.h>
 
+//
+// NOTE: HMAC_MAX_MD_CBLOCK is deprecated.
+//       #define HMAC_MAX_MD_CBLOCK 128
+//       Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h
+//       #define HMAC_MAX_MD_CBLOCK_SIZE     144
+//       But we need to compatible with previous API.
+//       So fix it with correct size 144-128 = 16.
+//
 #define HMAC_MD5_CTX_SIZE    sizeof(void *) * 4 + sizeof(unsigned int) + \
-                             sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
+                             sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16)
+
 
 /**
   Retrieves the size, in bytes, of the context buffer required for HMAC-MD5 operations.
diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
index bbe3df4..e59602e 100644
--- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
+++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
@@ -9,8 +9,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include "InternalCryptLib.h"
 #include <openssl/hmac.h>
 
-#define HMAC_SHA1_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int) + \
-                             sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
+//
+// NOTE: HMAC_MAX_MD_CBLOCK is deprecated.
+//       #define HMAC_MAX_MD_CBLOCK 128
+//       Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h
+//       #define HMAC_MAX_MD_CBLOCK_SIZE     144
+//       But we need to compatible with previous API.
+//       So fix it with correct size 144-128 = 16.
+//
+#define  HMAC_SHA1_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int) + \
+                             sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16)
 
 /**
   Retrieves the size, in bytes, of the context buffer required for HMAC-SHA1 operations.
diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
index ac9084f..8d0570b 100644
--- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
+++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
@@ -9,8 +9,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include "InternalCryptLib.h"
 #include <openssl/hmac.h>
 
-#define HMAC_SHA256_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int) + \
-                               sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
+//
+// NOTE: HMAC_MAX_MD_CBLOCK is deprecated.
+//       #define HMAC_MAX_MD_CBLOCK 128
+//       Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h
+//       #define HMAC_MAX_MD_CBLOCK_SIZE     144
+//       But we need to compatible with previous API.
+//       So fix it with correct size 144-128 = 16.
+//
+#define HMAC_SHA256_CTX_SIZE    sizeof(void *) * 4 + sizeof(unsigned int) + \
+                             sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16)
 
 /**
   Retrieves the size, in bytes, of the context buffer required for HMAC-SHA256 operations.
-- 
2.7.4


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

View/Reply Online (#39753): https://edk2.groups.io/g/devel/message/39753
Mute This Topic: https://groups.io/mt/31381055/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 3/3] CryptoPkg/BaseCryptLib: updata HMAC_ctx size
Posted by Philippe Mathieu-Daudé 6 years, 9 months ago 
Hi Xiaoyu,

Small typos: "update" in subject.
Maybe better described as "Make HMAC_CTX_SIZE backward compatible"?


On 4/29/19 10:15 AM, Xiaoyu lu wrote:
> From: Xiaoyu Lu <xiaoyux.lu@intel.com>
> 
> Openssl internally redefines the size of HMAC_CTX,

"OpenSSL"?

> but there is no external definition.
> So add an additional nubmer.

"number"

> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Ting Ye <ting.ye@intel.com>

Can you add the reference?

"Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1089"

> Signed-off-by: Xiaoyu Lu <xiaoyux.lu@intel.com>
> ---
>  CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c    | 11 ++++++++++-
>  CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c   | 12 ++++++++++--
>  CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c | 12 ++++++++++--
>  3 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
> index 3134806..3ffb8e2 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
> @@ -9,8 +9,17 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include "InternalCryptLib.h"
>  #include <openssl/hmac.h>
>  
> +//
> +// NOTE: HMAC_MAX_MD_CBLOCK is deprecated.
> +//       #define HMAC_MAX_MD_CBLOCK 128
> +//       Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h

OpenSSL

> +//       #define HMAC_MAX_MD_CBLOCK_SIZE     144
> +//       But we need to compatible with previous API.
> +//       So fix it with correct size 144-128 = 16.
> +//
>  #define HMAC_MD5_CTX_SIZE    sizeof(void *) * 4 + sizeof(unsigned int) + \
> -                             sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
> +                             sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16)

Can you put this expression between parenthesis? (and the other ones).

> +
>  
>  /**
>    Retrieves the size, in bytes, of the context buffer required for HMAC-MD5 operations.
> diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
> index bbe3df4..e59602e 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
> @@ -9,8 +9,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include "InternalCryptLib.h"
>  #include <openssl/hmac.h>
>  
> -#define HMAC_SHA1_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int) + \
> -                             sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
> +//
> +// NOTE: HMAC_MAX_MD_CBLOCK is deprecated.
> +//       #define HMAC_MAX_MD_CBLOCK 128
> +//       Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h

Ditto,

> +//       #define HMAC_MAX_MD_CBLOCK_SIZE     144
> +//       But we need to compatible with previous API.
> +//       So fix it with correct size 144-128 = 16.
> +//
> +#define  HMAC_SHA1_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int) + \
> +                             sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16)
>  
>  /**
>    Retrieves the size, in bytes, of the context buffer required for HMAC-SHA1 operations.
> diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
> index ac9084f..8d0570b 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
> @@ -9,8 +9,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include "InternalCryptLib.h"
>  #include <openssl/hmac.h>
>  
> -#define HMAC_SHA256_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int) + \
> -                               sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
> +//
> +// NOTE: HMAC_MAX_MD_CBLOCK is deprecated.
> +//       #define HMAC_MAX_MD_CBLOCK 128
> +//       Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h

Ditto.

Thanks!

> +//       #define HMAC_MAX_MD_CBLOCK_SIZE     144
> +//       But we need to compatible with previous API.
> +//       So fix it with correct size 144-128 = 16.
> +//
> +#define HMAC_SHA256_CTX_SIZE    sizeof(void *) * 4 + sizeof(unsigned int) + \
> +                             sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16)
>  
>  /**
>    Retrieves the size, in bytes, of the context buffer required for HMAC-SHA256 operations.
> 

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

View/Reply Online (#39778): https://edk2.groups.io/g/devel/message/39778
Mute This Topic: https://groups.io/mt/31381055/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 3/3] CryptoPkg/BaseCryptLib: updata HMAC_ctx size
Posted by Laszlo Ersek 6 years, 9 months ago 
On 04/29/19 10:15, Xiaoyu lu wrote:
> From: Xiaoyu Lu <xiaoyux.lu@intel.com>
>
> Openssl internally redefines the size of HMAC_CTX,
> but there is no external definition.
> So add an additional nubmer.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Ting Ye <ting.ye@intel.com>
> Signed-off-by: Xiaoyu Lu <xiaoyux.lu@intel.com>
> ---
>  CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c    | 11 ++++++++++-
>  CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c   | 12 ++++++++++--
>  CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c | 12 ++++++++++--
>  3 files changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
> index 3134806..3ffb8e2 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
> @@ -9,8 +9,17 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include "InternalCryptLib.h"
>  #include <openssl/hmac.h>
>
> +//
> +// NOTE: HMAC_MAX_MD_CBLOCK is deprecated.
> +//       #define HMAC_MAX_MD_CBLOCK 128
> +//       Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h
> +//       #define HMAC_MAX_MD_CBLOCK_SIZE     144
> +//       But we need to compatible with previous API.
> +//       So fix it with correct size 144-128 = 16.
> +//
>  #define HMAC_MD5_CTX_SIZE    sizeof(void *) * 4 + sizeof(unsigned int) + \
> -                             sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
> +                             sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16)
> +
>
>  /**
>    Retrieves the size, in bytes, of the context buffer required for HMAC-MD5 operations.
> diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
> index bbe3df4..e59602e 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
> @@ -9,8 +9,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include "InternalCryptLib.h"
>  #include <openssl/hmac.h>
>
> -#define HMAC_SHA1_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int) + \
> -                             sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
> +//
> +// NOTE: HMAC_MAX_MD_CBLOCK is deprecated.
> +//       #define HMAC_MAX_MD_CBLOCK 128
> +//       Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h
> +//       #define HMAC_MAX_MD_CBLOCK_SIZE     144
> +//       But we need to compatible with previous API.
> +//       So fix it with correct size 144-128 = 16.
> +//
> +#define  HMAC_SHA1_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int) + \
> +                             sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16)
>
>  /**
>    Retrieves the size, in bytes, of the context buffer required for HMAC-SHA1 operations.
> diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
> index ac9084f..8d0570b 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
> @@ -9,8 +9,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include "InternalCryptLib.h"
>  #include <openssl/hmac.h>
>
> -#define HMAC_SHA256_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int) + \
> -                               sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
> +//
> +// NOTE: HMAC_MAX_MD_CBLOCK is deprecated.
> +//       #define HMAC_MAX_MD_CBLOCK 128
> +//       Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h
> +//       #define HMAC_MAX_MD_CBLOCK_SIZE     144
> +//       But we need to compatible with previous API.
> +//       So fix it with correct size 144-128 = 16.
> +//
> +#define HMAC_SHA256_CTX_SIZE    sizeof(void *) * 4 + sizeof(unsigned int) + \
> +                             sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16)
>
>  /**
>    Retrieves the size, in bytes, of the context buffer required for HMAC-SHA256 operations.
>

I believe I disagree with this patch.

The issue is well described here:

  https://github.com/openssl/openssl/pull/4338

And the real solution, for edk2, is apparently described in edk2
already:

[CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c]:

> /**
>   Retrieves the size, in bytes, of the context buffer required for HMAC-MD5 operations.
>   (NOTE: This API is deprecated.
>          Use HmacMd5New() / HmacMd5Free() for HMAC-MD5 Context operations.)
>
>   @return  The size, in bytes, of the context buffer required for HMAC-MD5 operations.
>
> **/
> UINTN
> EFIAPI
> HmacMd5GetContextSize (
>   VOID
>   )
> {
>   //
>   // Retrieves the OpenSSL HMAC-MD5 Context Size
>   // NOTE: HMAC_CTX object was made opaque in openssl-1.1.x, here we just use the
>   //       fixed size as a workaround to make this API work for compatibility.
>   //       We should retire HmacMd5GetContextSize() in future, and use HmacMd5New()
>   //       and HmacMd5Free() for context allocation and release.
>   //
>   return (UINTN) HMAC_MD5_CTX_SIZE;
> }

[CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c]:

> /**
>   Retrieves the size, in bytes, of the context buffer required for HMAC-SHA1 operations.
>   (NOTE: This API is deprecated.
>          Use HmacSha1New() / HmacSha1Free() for HMAC-SHA1 Context operations.)
>
>   @return  The size, in bytes, of the context buffer required for HMAC-SHA1 operations.
>
> **/
> UINTN
> EFIAPI
> HmacSha1GetContextSize (
>   VOID
>   )
> {
>   //
>   // Retrieves the OpenSSL HMAC-SHA1 Context Size
>   // NOTE: HMAC_CTX object was made opaque in openssl-1.1.x, here we just use the
>   //       fixed size as a workaround to make this API work for compatibility.
>   //       We should retire HmacSha15GetContextSize() in future, and use HmacSha1New()
>   //       and HmacSha1Free() for context allocation and release.
>   //
>   return (UINTN) HMAC_SHA1_CTX_SIZE;
> }

[CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c]:

> /**
>   Retrieves the size, in bytes, of the context buffer required for HMAC-SHA256 operations.
>   (NOTE: This API is deprecated.
>          Use HmacSha256New() / HmacSha256Free() for HMAC-SHA256 Context operations.)
>
>   @return  The size, in bytes, of the context buffer required for HMAC-SHA256 operations.
>
> **/
> UINTN
> EFIAPI
> HmacSha256GetContextSize (
>   VOID
>   )
> {
>   //
>   // Retrieves the OpenSSL HMAC-SHA256 Context Size
>   // NOTE: HMAC_CTX object was made opaque in openssl-1.1.x, here we just use the
>   //       fixed size as a workaround to make this API work for compatibility.
>   //       We should retire HmacSha256GetContextSize() in future, and use HmacSha256New()
>   //       and HmacSha256Free() for context allocation and release.
>   //
>   return (UINTN)HMAC_SHA256_CTX_SIZE;
> }

Is there any reason why we can't clean up this code first, in a separate
patch series, before moving to OpenSSL 1.1.1?

Because, the "NOTE"s are more than two years old now... They come from
commit 4c270243995a ("CryptoPkg: Update HMAC Wrapper with opaque
HMAC_CTX object.", 2017-03-29).

This has been our technical debt ever since, violating the OpenSSL
effort to hide implementation details. I think we should eliminate this
debt, rather than make it worse.

(Obviously this is just my opinion, and I'm not a CryptoPkg
co-maintainer.)

Thanks
Laszlo

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

View/Reply Online (#39797): https://edk2.groups.io/g/devel/message/39797
Mute This Topic: https://groups.io/mt/31381055/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 3/3] CryptoPkg/BaseCryptLib: updata HMAC_ctx size
Posted by Wang, Jian J 6 years, 9 months ago 
Laszlo,


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, April 30, 2019 2:31 AM
> To: devel@edk2.groups.io; Lu, XiaoyuX <xiaoyux.lu@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Ye, Ting <ting.ye@intel.com>
> Subject: Re: [edk2-devel] [PATCH 3/3] CryptoPkg/BaseCryptLib: updata
> HMAC_ctx size
> 
> On 04/29/19 10:15, Xiaoyu lu wrote:
> > From: Xiaoyu Lu <xiaoyux.lu@intel.com>
> >
> > Openssl internally redefines the size of HMAC_CTX,
> > but there is no external definition.
> > So add an additional nubmer.
> >
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Ting Ye <ting.ye@intel.com>
> > Signed-off-by: Xiaoyu Lu <xiaoyux.lu@intel.com>
> > ---
> >  CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c    | 11 ++++++++++-
> >  CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c   | 12 ++++++++++--
> >  CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c | 12 ++++++++++-
> -
> >  3 files changed, 30 insertions(+), 5 deletions(-)
> >
> > diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
> b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
> > index 3134806..3ffb8e2 100644
> > --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
> > +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
> > @@ -9,8 +9,17 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >  #include "InternalCryptLib.h"
> >  #include <openssl/hmac.h>
> >
> > +//
> > +// NOTE: HMAC_MAX_MD_CBLOCK is deprecated.
> > +//       #define HMAC_MAX_MD_CBLOCK 128
> > +//       Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h
> > +//       #define HMAC_MAX_MD_CBLOCK_SIZE     144
> > +//       But we need to compatible with previous API.
> > +//       So fix it with correct size 144-128 = 16.
> > +//
> >  #define HMAC_MD5_CTX_SIZE    sizeof(void *) * 4 + sizeof(unsigned int) + \
> > -                             sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
> > +                             sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16)
> > +
> >
> >  /**
> >    Retrieves the size, in bytes, of the context buffer required for HMAC-MD5
> operations.
> > diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
> b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
> > index bbe3df4..e59602e 100644
> > --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
> > +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
> > @@ -9,8 +9,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >  #include "InternalCryptLib.h"
> >  #include <openssl/hmac.h>
> >
> > -#define HMAC_SHA1_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int) + \
> > -                             sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
> > +//
> > +// NOTE: HMAC_MAX_MD_CBLOCK is deprecated.
> > +//       #define HMAC_MAX_MD_CBLOCK 128
> > +//       Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h
> > +//       #define HMAC_MAX_MD_CBLOCK_SIZE     144
> > +//       But we need to compatible with previous API.
> > +//       So fix it with correct size 144-128 = 16.
> > +//
> > +#define  HMAC_SHA1_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int) + \
> > +                             sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16)
> >
> >  /**
> >    Retrieves the size, in bytes, of the context buffer required for HMAC-SHA1
> operations.
> > diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
> b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
> > index ac9084f..8d0570b 100644
> > --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
> > +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
> > @@ -9,8 +9,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >  #include "InternalCryptLib.h"
> >  #include <openssl/hmac.h>
> >
> > -#define HMAC_SHA256_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int) + \
> > -                               sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
> > +//
> > +// NOTE: HMAC_MAX_MD_CBLOCK is deprecated.
> > +//       #define HMAC_MAX_MD_CBLOCK 128
> > +//       Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h
> > +//       #define HMAC_MAX_MD_CBLOCK_SIZE     144
> > +//       But we need to compatible with previous API.
> > +//       So fix it with correct size 144-128 = 16.
> > +//
> > +#define HMAC_SHA256_CTX_SIZE    sizeof(void *) * 4 + sizeof(unsigned int) +
> \
> > +                             sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16)
> >
> >  /**
> >    Retrieves the size, in bytes, of the context buffer required for HMAC-SHA256
> operations.
> >
> 
> I believe I disagree with this patch.
> 
> The issue is well described here:
> 
>   https://github.com/openssl/openssl/pull/4338
> 
> And the real solution, for edk2, is apparently described in edk2
> already:
> 
> [CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c]:
> 
> > /**
> >   Retrieves the size, in bytes, of the context buffer required for HMAC-MD5
> operations.
> >   (NOTE: This API is deprecated.
> >          Use HmacMd5New() / HmacMd5Free() for HMAC-MD5 Context
> operations.)
> >
> >   @return  The size, in bytes, of the context buffer required for HMAC-MD5
> operations.
> >
> > **/
> > UINTN
> > EFIAPI
> > HmacMd5GetContextSize (
> >   VOID
> >   )
> > {
> >   //
> >   // Retrieves the OpenSSL HMAC-MD5 Context Size
> >   // NOTE: HMAC_CTX object was made opaque in openssl-1.1.x, here we just
> use the
> >   //       fixed size as a workaround to make this API work for compatibility.
> >   //       We should retire HmacMd5GetContextSize() in future, and use
> HmacMd5New()
> >   //       and HmacMd5Free() for context allocation and release.
> >   //
> >   return (UINTN) HMAC_MD5_CTX_SIZE;
> > }
> 
> [CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c]:
> 
> > /**
> >   Retrieves the size, in bytes, of the context buffer required for HMAC-SHA1
> operations.
> >   (NOTE: This API is deprecated.
> >          Use HmacSha1New() / HmacSha1Free() for HMAC-SHA1 Context
> operations.)
> >
> >   @return  The size, in bytes, of the context buffer required for HMAC-SHA1
> operations.
> >
> > **/
> > UINTN
> > EFIAPI
> > HmacSha1GetContextSize (
> >   VOID
> >   )
> > {
> >   //
> >   // Retrieves the OpenSSL HMAC-SHA1 Context Size
> >   // NOTE: HMAC_CTX object was made opaque in openssl-1.1.x, here we just
> use the
> >   //       fixed size as a workaround to make this API work for compatibility.
> >   //       We should retire HmacSha15GetContextSize() in future, and use
> HmacSha1New()
> >   //       and HmacSha1Free() for context allocation and release.
> >   //
> >   return (UINTN) HMAC_SHA1_CTX_SIZE;
> > }
> 
> [CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c]:
> 
> > /**
> >   Retrieves the size, in bytes, of the context buffer required for HMAC-SHA256
> operations.
> >   (NOTE: This API is deprecated.
> >          Use HmacSha256New() / HmacSha256Free() for HMAC-SHA256 Context
> operations.)
> >
> >   @return  The size, in bytes, of the context buffer required for HMAC-SHA256
> operations.
> >
> > **/
> > UINTN
> > EFIAPI
> > HmacSha256GetContextSize (
> >   VOID
> >   )
> > {
> >   //
> >   // Retrieves the OpenSSL HMAC-SHA256 Context Size
> >   // NOTE: HMAC_CTX object was made opaque in openssl-1.1.x, here we just
> use the
> >   //       fixed size as a workaround to make this API work for compatibility.
> >   //       We should retire HmacSha256GetContextSize() in future, and use
> HmacSha256New()
> >   //       and HmacSha256Free() for context allocation and release.
> >   //
> >   return (UINTN)HMAC_SHA256_CTX_SIZE;
> > }
> 
> Is there any reason why we can't clean up this code first, in a separate
> patch series, before moving to OpenSSL 1.1.1?
> 
> Because, the "NOTE"s are more than two years old now... They come from
> commit 4c270243995a ("CryptoPkg: Update HMAC Wrapper with opaque
> HMAC_CTX object.", 2017-03-29).
> 
> This has been our technical debt ever since, violating the OpenSSL
> effort to hide implementation details. I think we should eliminate this
> debt, rather than make it worse.
> 
> (Obviously this is just my opinion, and I'm not a CryptoPkg
> co-maintainer.)
> 

I agree that we need to clean up the deprecated macros and api. The problem
is that XxxGetContextSize() are public APIs and there're dozens of modules which
still call them. This can't be done in short time. What about we file a BZ to track
this issue and target to next stable tag?

Regards,
Jian

> Thanks
> Laszlo

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

View/Reply Online (#39803): https://edk2.groups.io/g/devel/message/39803
Mute This Topic: https://groups.io/mt/31381055/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 3/3] CryptoPkg/BaseCryptLib: updata HMAC_ctx size
Posted by Laszlo Ersek 6 years, 9 months ago 
On 04/30/19 03:16, Wang, Jian J wrote:
> Laszlo,
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Tuesday, April 30, 2019 2:31 AM
>> To: devel@edk2.groups.io; Lu, XiaoyuX <xiaoyux.lu@intel.com>
>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Ye, Ting <ting.ye@intel.com>
>> Subject: Re: [edk2-devel] [PATCH 3/3] CryptoPkg/BaseCryptLib: updata
>> HMAC_ctx size
>>
>> On 04/29/19 10:15, Xiaoyu lu wrote:
>>> From: Xiaoyu Lu <xiaoyux.lu@intel.com>
>>>
>>> Openssl internally redefines the size of HMAC_CTX,
>>> but there is no external definition.
>>> So add an additional nubmer.
>>>
>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>> Cc: Ting Ye <ting.ye@intel.com>
>>> Signed-off-by: Xiaoyu Lu <xiaoyux.lu@intel.com>
>>> ---
>>>  CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c    | 11 ++++++++++-
>>>  CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c   | 12 ++++++++++--
>>>  CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c | 12 ++++++++++-
>> -
>>>  3 files changed, 30 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
>> b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
>>> index 3134806..3ffb8e2 100644
>>> --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
>>> +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
>>> @@ -9,8 +9,17 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>>>  #include "InternalCryptLib.h"
>>>  #include <openssl/hmac.h>
>>>
>>> +//
>>> +// NOTE: HMAC_MAX_MD_CBLOCK is deprecated.
>>> +//       #define HMAC_MAX_MD_CBLOCK 128
>>> +//       Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h
>>> +//       #define HMAC_MAX_MD_CBLOCK_SIZE     144
>>> +//       But we need to compatible with previous API.
>>> +//       So fix it with correct size 144-128 = 16.
>>> +//
>>>  #define HMAC_MD5_CTX_SIZE    sizeof(void *) * 4 + sizeof(unsigned int) + \
>>> -                             sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
>>> +                             sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16)
>>> +
>>>
>>>  /**
>>>    Retrieves the size, in bytes, of the context buffer required for HMAC-MD5
>> operations.
>>> diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
>> b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
>>> index bbe3df4..e59602e 100644
>>> --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
>>> +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
>>> @@ -9,8 +9,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>>>  #include "InternalCryptLib.h"
>>>  #include <openssl/hmac.h>
>>>
>>> -#define HMAC_SHA1_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int) + \
>>> -                             sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
>>> +//
>>> +// NOTE: HMAC_MAX_MD_CBLOCK is deprecated.
>>> +//       #define HMAC_MAX_MD_CBLOCK 128
>>> +//       Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h
>>> +//       #define HMAC_MAX_MD_CBLOCK_SIZE     144
>>> +//       But we need to compatible with previous API.
>>> +//       So fix it with correct size 144-128 = 16.
>>> +//
>>> +#define  HMAC_SHA1_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int) + \
>>> +                             sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16)
>>>
>>>  /**
>>>    Retrieves the size, in bytes, of the context buffer required for HMAC-SHA1
>> operations.
>>> diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
>> b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
>>> index ac9084f..8d0570b 100644
>>> --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
>>> +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
>>> @@ -9,8 +9,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>>>  #include "InternalCryptLib.h"
>>>  #include <openssl/hmac.h>
>>>
>>> -#define HMAC_SHA256_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int) + \
>>> -                               sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
>>> +//
>>> +// NOTE: HMAC_MAX_MD_CBLOCK is deprecated.
>>> +//       #define HMAC_MAX_MD_CBLOCK 128
>>> +//       Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h
>>> +//       #define HMAC_MAX_MD_CBLOCK_SIZE     144
>>> +//       But we need to compatible with previous API.
>>> +//       So fix it with correct size 144-128 = 16.
>>> +//
>>> +#define HMAC_SHA256_CTX_SIZE    sizeof(void *) * 4 + sizeof(unsigned int) +
>> \
>>> +                             sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16)
>>>
>>>  /**
>>>    Retrieves the size, in bytes, of the context buffer required for HMAC-SHA256
>> operations.
>>>
>>
>> I believe I disagree with this patch.
>>
>> The issue is well described here:
>>
>>   https://github.com/openssl/openssl/pull/4338
>>
>> And the real solution, for edk2, is apparently described in edk2
>> already:
>>
>> [CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c]:
>>
>>> /**
>>>   Retrieves the size, in bytes, of the context buffer required for HMAC-MD5
>> operations.
>>>   (NOTE: This API is deprecated.
>>>          Use HmacMd5New() / HmacMd5Free() for HMAC-MD5 Context
>> operations.)
>>>
>>>   @return  The size, in bytes, of the context buffer required for HMAC-MD5
>> operations.
>>>
>>> **/
>>> UINTN
>>> EFIAPI
>>> HmacMd5GetContextSize (
>>>   VOID
>>>   )
>>> {
>>>   //
>>>   // Retrieves the OpenSSL HMAC-MD5 Context Size
>>>   // NOTE: HMAC_CTX object was made opaque in openssl-1.1.x, here we just
>> use the
>>>   //       fixed size as a workaround to make this API work for compatibility.
>>>   //       We should retire HmacMd5GetContextSize() in future, and use
>> HmacMd5New()
>>>   //       and HmacMd5Free() for context allocation and release.
>>>   //
>>>   return (UINTN) HMAC_MD5_CTX_SIZE;
>>> }
>>
>> [CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c]:
>>
>>> /**
>>>   Retrieves the size, in bytes, of the context buffer required for HMAC-SHA1
>> operations.
>>>   (NOTE: This API is deprecated.
>>>          Use HmacSha1New() / HmacSha1Free() for HMAC-SHA1 Context
>> operations.)
>>>
>>>   @return  The size, in bytes, of the context buffer required for HMAC-SHA1
>> operations.
>>>
>>> **/
>>> UINTN
>>> EFIAPI
>>> HmacSha1GetContextSize (
>>>   VOID
>>>   )
>>> {
>>>   //
>>>   // Retrieves the OpenSSL HMAC-SHA1 Context Size
>>>   // NOTE: HMAC_CTX object was made opaque in openssl-1.1.x, here we just
>> use the
>>>   //       fixed size as a workaround to make this API work for compatibility.
>>>   //       We should retire HmacSha15GetContextSize() in future, and use
>> HmacSha1New()
>>>   //       and HmacSha1Free() for context allocation and release.
>>>   //
>>>   return (UINTN) HMAC_SHA1_CTX_SIZE;
>>> }
>>
>> [CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c]:
>>
>>> /**
>>>   Retrieves the size, in bytes, of the context buffer required for HMAC-SHA256
>> operations.
>>>   (NOTE: This API is deprecated.
>>>          Use HmacSha256New() / HmacSha256Free() for HMAC-SHA256 Context
>> operations.)
>>>
>>>   @return  The size, in bytes, of the context buffer required for HMAC-SHA256
>> operations.
>>>
>>> **/
>>> UINTN
>>> EFIAPI
>>> HmacSha256GetContextSize (
>>>   VOID
>>>   )
>>> {
>>>   //
>>>   // Retrieves the OpenSSL HMAC-SHA256 Context Size
>>>   // NOTE: HMAC_CTX object was made opaque in openssl-1.1.x, here we just
>> use the
>>>   //       fixed size as a workaround to make this API work for compatibility.
>>>   //       We should retire HmacSha256GetContextSize() in future, and use
>> HmacSha256New()
>>>   //       and HmacSha256Free() for context allocation and release.
>>>   //
>>>   return (UINTN)HMAC_SHA256_CTX_SIZE;
>>> }
>>
>> Is there any reason why we can't clean up this code first, in a separate
>> patch series, before moving to OpenSSL 1.1.1?
>>
>> Because, the "NOTE"s are more than two years old now... They come from
>> commit 4c270243995a ("CryptoPkg: Update HMAC Wrapper with opaque
>> HMAC_CTX object.", 2017-03-29).
>>
>> This has been our technical debt ever since, violating the OpenSSL
>> effort to hide implementation details. I think we should eliminate this
>> debt, rather than make it worse.
>>
>> (Obviously this is just my opinion, and I'm not a CryptoPkg
>> co-maintainer.)
>>
> 
> I agree that we need to clean up the deprecated macros and api. The problem
> is that XxxGetContextSize() are public APIs and there're dozens of modules which
> still call them. This can't be done in short time. What about we file a BZ to track
> this issue and target to next stable tag?

I don't see any calls to either of HmacMd5GetContextSize,
HmacSha1GetContextSize, and HmacSha256GetContextSize in the open source
edk2 tree.

Do you mean those dozens of calls are in the edk2-platforms project, or
in proprietary platfoms?

I do see that the change affects the <BaseCryptLib.h> lib class header,
so it seems justified to postpone the real fix to the next stable tag.
Please file a BZ for this.

Regarding this patch:

(1) The commit message should explain the situation a lot more clearly.
It should reference both

- https://github.com/openssl/openssl/pull/4338

and

- OpenSSL commit e0810e3502bb ("Fix HMAC SHA3-224 and HMAC SHA3-256.",
2018-09-04)

(2) The commit message should also reference the new TianoCore BZ (about
removing these functions from the lib class).

(3) The code comments are confusing, and should be deleted, in my
opinion. A good commit message will suffice.

In particular, the following part of the comment:

  "we need to compatible with previous API"

is very confusing, as it first made me think that we should stick with
the *previous* value, i.e. 128, for compatibility with callers of these
functions.

(4) In the patch (= the new code), HMAC_MAX_MD_CBLOCK should be
eliminated completely, and the sum (HMAC_MAX_MD_CBLOCK + 16) should be
replaced with the constant 144.

The fact that the new context size is 16 bytes larger than what it used
to be (due to the introduction of SHA3-224) is circumstantial. The old
value (128) is irrelevant, so we shouldn't express the new value
relative to the old value. The new size is 144 bytes because the new
size is 144 bytes.

Thanks
Laszlo

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

View/Reply Online (#39837): https://edk2.groups.io/g/devel/message/39837
Mute This Topic: https://groups.io/mt/31381055/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 3/3] CryptoPkg/BaseCryptLib: updata HMAC_ctx size
Posted by Wang, Jian J 6 years, 9 months ago 
Laszlo,

Yes, we have internal platforms using them. I'll file a BZ to remove those APIs in next
stable tag.

Regards,
Jian


> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Tuesday, April 30, 2019 6:27 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io; Lu, XiaoyuX
> <xiaoyux.lu@intel.com>
> Cc: Ye, Ting <ting.ye@intel.com>
> Subject: Re: [edk2-devel] [PATCH 3/3] CryptoPkg/BaseCryptLib: updata
> HMAC_ctx size
> 
> On 04/30/19 03:16, Wang, Jian J wrote:
> > Laszlo,
> >
> >
> >> -----Original Message-----
> >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >> Sent: Tuesday, April 30, 2019 2:31 AM
> >> To: devel@edk2.groups.io; Lu, XiaoyuX <xiaoyux.lu@intel.com>
> >> Cc: Wang, Jian J <jian.j.wang@intel.com>; Ye, Ting <ting.ye@intel.com>
> >> Subject: Re: [edk2-devel] [PATCH 3/3] CryptoPkg/BaseCryptLib: updata
> >> HMAC_ctx size
> >>
> >> On 04/29/19 10:15, Xiaoyu lu wrote:
> >>> From: Xiaoyu Lu <xiaoyux.lu@intel.com>
> >>>
> >>> Openssl internally redefines the size of HMAC_CTX,
> >>> but there is no external definition.
> >>> So add an additional nubmer.
> >>>
> >>> Cc: Jian J Wang <jian.j.wang@intel.com>
> >>> Cc: Ting Ye <ting.ye@intel.com>
> >>> Signed-off-by: Xiaoyu Lu <xiaoyux.lu@intel.com>
> >>> ---
> >>>  CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c    | 11
> ++++++++++-
> >>>  CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c   | 12
> ++++++++++--
> >>>  CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c | 12
> ++++++++++-
> >> -
> >>>  3 files changed, 30 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
> >> b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
> >>> index 3134806..3ffb8e2 100644
> >>> --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
> >>> +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
> >>> @@ -9,8 +9,17 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >>>  #include "InternalCryptLib.h"
> >>>  #include <openssl/hmac.h>
> >>>
> >>> +//
> >>> +// NOTE: HMAC_MAX_MD_CBLOCK is deprecated.
> >>> +//       #define HMAC_MAX_MD_CBLOCK 128
> >>> +//       Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h
> >>> +//       #define HMAC_MAX_MD_CBLOCK_SIZE     144
> >>> +//       But we need to compatible with previous API.
> >>> +//       So fix it with correct size 144-128 = 16.
> >>> +//
> >>>  #define HMAC_MD5_CTX_SIZE    sizeof(void *) * 4 + sizeof(unsigned int) + \
> >>> -                             sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
> >>> +                             sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16)
> >>> +
> >>>
> >>>  /**
> >>>    Retrieves the size, in bytes, of the context buffer required for HMAC-MD5
> >> operations.
> >>> diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
> >> b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
> >>> index bbe3df4..e59602e 100644
> >>> --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
> >>> +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
> >>> @@ -9,8 +9,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >>>  #include "InternalCryptLib.h"
> >>>  #include <openssl/hmac.h>
> >>>
> >>> -#define HMAC_SHA1_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int) + \
> >>> -                             sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
> >>> +//
> >>> +// NOTE: HMAC_MAX_MD_CBLOCK is deprecated.
> >>> +//       #define HMAC_MAX_MD_CBLOCK 128
> >>> +//       Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h
> >>> +//       #define HMAC_MAX_MD_CBLOCK_SIZE     144
> >>> +//       But we need to compatible with previous API.
> >>> +//       So fix it with correct size 144-128 = 16.
> >>> +//
> >>> +#define  HMAC_SHA1_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int) +
> \
> >>> +                             sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16)
> >>>
> >>>  /**
> >>>    Retrieves the size, in bytes, of the context buffer required for HMAC-SHA1
> >> operations.
> >>> diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
> >> b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
> >>> index ac9084f..8d0570b 100644
> >>> --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
> >>> +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
> >>> @@ -9,8 +9,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >>>  #include "InternalCryptLib.h"
> >>>  #include <openssl/hmac.h>
> >>>
> >>> -#define HMAC_SHA256_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int)
> + \
> >>> -                               sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
> >>> +//
> >>> +// NOTE: HMAC_MAX_MD_CBLOCK is deprecated.
> >>> +//       #define HMAC_MAX_MD_CBLOCK 128
> >>> +//       Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h
> >>> +//       #define HMAC_MAX_MD_CBLOCK_SIZE     144
> >>> +//       But we need to compatible with previous API.
> >>> +//       So fix it with correct size 144-128 = 16.
> >>> +//
> >>> +#define HMAC_SHA256_CTX_SIZE    sizeof(void *) * 4 + sizeof(unsigned int)
> +
> >> \
> >>> +                             sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + 16)
> >>>
> >>>  /**
> >>>    Retrieves the size, in bytes, of the context buffer required for HMAC-
> SHA256
> >> operations.
> >>>
> >>
> >> I believe I disagree with this patch.
> >>
> >> The issue is well described here:
> >>
> >>   https://github.com/openssl/openssl/pull/4338
> >>
> >> And the real solution, for edk2, is apparently described in edk2
> >> already:
> >>
> >> [CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c]:
> >>
> >>> /**
> >>>   Retrieves the size, in bytes, of the context buffer required for HMAC-MD5
> >> operations.
> >>>   (NOTE: This API is deprecated.
> >>>          Use HmacMd5New() / HmacMd5Free() for HMAC-MD5 Context
> >> operations.)
> >>>
> >>>   @return  The size, in bytes, of the context buffer required for HMAC-MD5
> >> operations.
> >>>
> >>> **/
> >>> UINTN
> >>> EFIAPI
> >>> HmacMd5GetContextSize (
> >>>   VOID
> >>>   )
> >>> {
> >>>   //
> >>>   // Retrieves the OpenSSL HMAC-MD5 Context Size
> >>>   // NOTE: HMAC_CTX object was made opaque in openssl-1.1.x, here we
> just
> >> use the
> >>>   //       fixed size as a workaround to make this API work for compatibility.
> >>>   //       We should retire HmacMd5GetContextSize() in future, and use
> >> HmacMd5New()
> >>>   //       and HmacMd5Free() for context allocation and release.
> >>>   //
> >>>   return (UINTN) HMAC_MD5_CTX_SIZE;
> >>> }
> >>
> >> [CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c]:
> >>
> >>> /**
> >>>   Retrieves the size, in bytes, of the context buffer required for HMAC-SHA1
> >> operations.
> >>>   (NOTE: This API is deprecated.
> >>>          Use HmacSha1New() / HmacSha1Free() for HMAC-SHA1 Context
> >> operations.)
> >>>
> >>>   @return  The size, in bytes, of the context buffer required for HMAC-SHA1
> >> operations.
> >>>
> >>> **/
> >>> UINTN
> >>> EFIAPI
> >>> HmacSha1GetContextSize (
> >>>   VOID
> >>>   )
> >>> {
> >>>   //
> >>>   // Retrieves the OpenSSL HMAC-SHA1 Context Size
> >>>   // NOTE: HMAC_CTX object was made opaque in openssl-1.1.x, here we
> just
> >> use the
> >>>   //       fixed size as a workaround to make this API work for compatibility.
> >>>   //       We should retire HmacSha15GetContextSize() in future, and use
> >> HmacSha1New()
> >>>   //       and HmacSha1Free() for context allocation and release.
> >>>   //
> >>>   return (UINTN) HMAC_SHA1_CTX_SIZE;
> >>> }
> >>
> >> [CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c]:
> >>
> >>> /**
> >>>   Retrieves the size, in bytes, of the context buffer required for HMAC-
> SHA256
> >> operations.
> >>>   (NOTE: This API is deprecated.
> >>>          Use HmacSha256New() / HmacSha256Free() for HMAC-SHA256
> Context
> >> operations.)
> >>>
> >>>   @return  The size, in bytes, of the context buffer required for HMAC-
> SHA256
> >> operations.
> >>>
> >>> **/
> >>> UINTN
> >>> EFIAPI
> >>> HmacSha256GetContextSize (
> >>>   VOID
> >>>   )
> >>> {
> >>>   //
> >>>   // Retrieves the OpenSSL HMAC-SHA256 Context Size
> >>>   // NOTE: HMAC_CTX object was made opaque in openssl-1.1.x, here we
> just
> >> use the
> >>>   //       fixed size as a workaround to make this API work for compatibility.
> >>>   //       We should retire HmacSha256GetContextSize() in future, and use
> >> HmacSha256New()
> >>>   //       and HmacSha256Free() for context allocation and release.
> >>>   //
> >>>   return (UINTN)HMAC_SHA256_CTX_SIZE;
> >>> }
> >>
> >> Is there any reason why we can't clean up this code first, in a separate
> >> patch series, before moving to OpenSSL 1.1.1?
> >>
> >> Because, the "NOTE"s are more than two years old now... They come from
> >> commit 4c270243995a ("CryptoPkg: Update HMAC Wrapper with opaque
> >> HMAC_CTX object.", 2017-03-29).
> >>
> >> This has been our technical debt ever since, violating the OpenSSL
> >> effort to hide implementation details. I think we should eliminate this
> >> debt, rather than make it worse.
> >>
> >> (Obviously this is just my opinion, and I'm not a CryptoPkg
> >> co-maintainer.)
> >>
> >
> > I agree that we need to clean up the deprecated macros and api. The problem
> > is that XxxGetContextSize() are public APIs and there're dozens of modules
> which
> > still call them. This can't be done in short time. What about we file a BZ to
> track
> > this issue and target to next stable tag?
> 
> I don't see any calls to either of HmacMd5GetContextSize,
> HmacSha1GetContextSize, and HmacSha256GetContextSize in the open source
> edk2 tree.
> 
> Do you mean those dozens of calls are in the edk2-platforms project, or
> in proprietary platfoms?
> 
> I do see that the change affects the <BaseCryptLib.h> lib class header,
> so it seems justified to postpone the real fix to the next stable tag.
> Please file a BZ for this.
> 
> Regarding this patch:
> 
> (1) The commit message should explain the situation a lot more clearly.
> It should reference both
> 
> - https://github.com/openssl/openssl/pull/4338
> 
> and
> 
> - OpenSSL commit e0810e3502bb ("Fix HMAC SHA3-224 and HMAC SHA3-256.",
> 2018-09-04)
> 
> (2) The commit message should also reference the new TianoCore BZ (about
> removing these functions from the lib class).
> 
> (3) The code comments are confusing, and should be deleted, in my
> opinion. A good commit message will suffice.
> 
> In particular, the following part of the comment:
> 
>   "we need to compatible with previous API"
> 
> is very confusing, as it first made me think that we should stick with
> the *previous* value, i.e. 128, for compatibility with callers of these
> functions.
> 
> (4) In the patch (= the new code), HMAC_MAX_MD_CBLOCK should be
> eliminated completely, and the sum (HMAC_MAX_MD_CBLOCK + 16) should be
> replaced with the constant 144.
> 
> The fact that the new context size is 16 bytes larger than what it used
> to be (due to the introduction of SHA3-224) is circumstantial. The old
> value (128) is irrelevant, so we shouldn't express the new value
> relative to the old value. The new size is 144 bytes because the new
> size is 144 bytes.
> 
> Thanks
> Laszlo
> 
> 


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

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.