public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Wang, Jian J" <jian.j.wang@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
	"lersek@redhat.com" <lersek@redhat.com>,
	"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
Date: Tue, 30 Apr 2019 10:47:19 +0000	[thread overview]
Message-ID: <D827630B58408649ACB04F44C5100036258F1018@SHSMSX107.ccr.corp.intel.com> (raw)
In-Reply-To: <68d920c5-f8d0-b00a-39a3-9a58e9ede494@redhat.com>

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
> 
> 


      reply	other threads:[~2019-04-30 10:47 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-29  8:15 [PATCH 0/3] CryptoPkg: Upgrade OpenSSL to 1_1_1b xiaoyux.lu
2019-04-29  8:15 ` [PATCH 1/3] CryptoPkg/IntrinsicLib: add ftol2 function Xiaoyu lu
2019-04-29  8:15 ` [PATCH 2/3] CryptoPkg: Upgrade openssl to 1.1.1b Xiaoyu lu
2019-04-29 18:01   ` [edk2-devel] " Laszlo Ersek
2019-04-30 10:00     ` Xiaoyu lu
2019-04-30 15:31       ` Laszlo Ersek
2019-05-14  5:21         ` Xiaoyu lu
2019-04-29  8:15 ` [PATCH 3/3] CryptoPkg/BaseCryptLib: updata HMAC_ctx size Xiaoyu lu
2019-04-29 14:28   ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-29 18:31   ` Laszlo Ersek
2019-04-30  1:16     ` Wang, Jian J
2019-04-30 10:26       ` Laszlo Ersek
2019-04-30 10:47         ` Wang, Jian J [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=D827630B58408649ACB04F44C5100036258F1018@SHSMSX107.ccr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox