public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Laszlo Ersek" <lersek@redhat.com>
To: devel@edk2.groups.io, xiaoyux.lu@intel.com
Cc: Jian J Wang <jian.j.wang@intel.com>, Ting Ye <ting.ye@intel.com>
Subject: Re: [edk2-devel] [PATCH 3/3] CryptoPkg/BaseCryptLib: updata HMAC_ctx size
Date: Mon, 29 Apr 2019 20:31:18 +0200	[thread overview]
Message-ID: <be01c8df-4b3e-7cb6-bb3f-524436cd33db@redhat.com> (raw)
In-Reply-To: <1556525727-14875-4-git-send-email-xiaoyux.lu@intel.com>

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

  parent reply	other threads:[~2019-04-29 18:31 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 [this message]
2019-04-30  1:16     ` Wang, Jian J
2019-04-30 10:26       ` Laszlo Ersek
2019-04-30 10:47         ` Wang, Jian J

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=be01c8df-4b3e-7cb6-bb3f-524436cd33db@redhat.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