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

["Wang, Jian J" <jian.j.wang@intel.com>]

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