Re: [edk2-devel] [PATCH 1/1] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface

["Yao, Jiewen" <jiewen.yao@intel.com>]

1) Agree with Laszlo.
We need extend internal protocol/ppi for this new API.

2) Do you have any data on the size difference between old SHA implementation or new MD implementation?

Thank you
Yao Jiewen

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, August 13, 2020 5:47 PM
> To: devel@edk2.groups.io; Zurcher, Christopher J
> <christopher.j.zurcher@intel.com>
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Lu, XiaoyuX <xiaoyux.lu@intel.com>
> Subject: Re: [edk2-devel] [PATCH 1/1] CryptoPkg/BaseCryptLib: Add EVP
> (Envelope) Digest interface
> 
> Hi Christopher,
> 
> (+Mike,
> 
> On 08/13/20 03:20, Zurcher, Christopher J wrote:
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2545
> >
> > The EVP interface should be used in place of discrete digest function
> > calls.
> >
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> > Signed-off-by: Christopher J Zurcher <christopher.j.zurcher@intel.com>
> > ---
> >  CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf    |   1 +
> >  CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf     |   1 +
> >  CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf |   1 +
> >  CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf     |   1 +
> >  CryptoPkg/Include/Library/BaseCryptLib.h           | 122 +++++++++++
> >  CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c    | 228
> ++++++++++++++++++++
> >  6 files changed, 354 insertions(+)
> 
> (1) This patch extends the library class header, but updates only one
> *set* of the three library instance *sets*. The other two instance
> *sets* are:
> 
> - BaseCryptLibNull (just one instance), for which it should not be hard
> to provide Null implementations of the new functions;
> 
> - BaseCryptLibOnProtocolPpi (three instances -- Pei, Dxe, Smm).
> 
> 
> BaseCryptLibOnProtocolPpi is a tough nut, because it seems to require
> extending:
> 
> - the crypto service driver at CryptoPkg/Driver/,
> 
> - the interface to that driver (CryptoPkg/Private/Protocol/Crypto.h --
> reused by both "CryptoPkg/Private/Ppi/Crypto.h" and
> "CryptoPkg/Private/Protocol/SmmCrypto.h"),
> 
> - the PCD_CRYPTO_SERVICE_FAMILY_ENABLE structure at
> "CryptoPkg/Include/Pcd/PcdCryptoServiceFamilyEnable.h", for configuring
> the driver,
> 
> - the various PcdCryptoServiceFamilyEnable settings / build profiles in
> CryptoPkg/CryptoPkg.dsc.
> 
> 
> >
> > diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> > index 4aae2aba95..3968f29412 100644
> > --- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> > +++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> > @@ -50,6 +50,7 @@
> >    Pk/CryptAuthenticode.c
> >    Pk/CryptTs.c
> >    Pem/CryptPem.c
> > +  Evp/CryptEvpMd.c
> >
> >    SysCall/CrtWrapper.c
> >    SysCall/TimerWrapper.c
> > diff --git a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> > index dc28e3a11d..d0b91716d0 100644
> > --- a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> > +++ b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> > @@ -57,6 +57,7 @@
> >    Pk/CryptTsNull.c
> >    Pem/CryptPemNull.c
> >    Rand/CryptRandNull.c
> > +  Evp/CryptEvpMd.c
> >
> >    SysCall/CrtWrapper.c
> >    SysCall/ConstantTimeClock.c
> > diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> > index 5005beed02..9f3accd35b 100644
> > --- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> > +++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> > @@ -56,6 +56,7 @@
> >    Pk/CryptAuthenticodeNull.c
> >    Pk/CryptTsNull.c
> >    Pem/CryptPem.c
> > +  Evp/CryptEvpMd.c
> >
> >    SysCall/CrtWrapper.c
> >    SysCall/TimerWrapper.c
> > diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> > index 91ec3e03bf..420623cdc6 100644
> > --- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> > +++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> > @@ -54,6 +54,7 @@
> >    Pk/CryptAuthenticodeNull.c
> >    Pk/CryptTsNull.c
> >    Pem/CryptPem.c
> > +  Evp/CryptEvpMd.c
> >
> >    SysCall/CrtWrapper.c
> >    SysCall/ConstantTimeClock.c
> > diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h
> b/CryptoPkg/Include/Library/BaseCryptLib.h
> > index ae9bde9e37..f3bf8aac0c 100644
> > --- a/CryptoPkg/Include/Library/BaseCryptLib.h
> > +++ b/CryptoPkg/Include/Library/BaseCryptLib.h
> > @@ -1012,6 +1012,128 @@ HmacSha256Final (
> >    OUT     UINT8  *HmacValue
> >    );
> >
> >
> +//==============================================================
> =======================
> > +//    EVP (Envelope) Primitive
> >
> +//==============================================================
> =======================
> > +
> > +/**
> > +  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD
> use.
> > +
> > +  @return  Pointer to the EVP_MD_CTX context that has been initialized.
> > +           If the allocations fails, EvpMdNew() returns NULL.
> > +
> > +**/
> > +VOID *
> > +EFIAPI
> > +EvpMdNew (
> > +  VOID
> > +  );
> > +
> > +/**
> > +  Release the specified EVP_MD_CTX context.
> > +
> > +  @param[in]  EvpMdContext  Pointer to the EVP_MD_CTX context to be
> released.
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +EvpMdFree (
> > +  IN  VOID  *EvpMdContext
> > +  );
> > +
> > +/**
> > +  Initializes user-supplied memory pointed to by EvpMdContext as
> EVP_MD_CTX
> > +  context for subsequent use.
> > +
> > +  If EvpMdContext is NULL, then return FALSE.
> > +  If DigestName is NULL, then return FALSE.
> > +
> > +  @param[out]   EvpMdContext  Pointer to EVP_MD_CTX context being
> initialized.
> > +  @param[in]    DigestName    Pointer to the digest name.
> > +
> > +  @retval TRUE   EVP_MD_CTX context initialization succeeded.
> > +  @retval FALSE  EVP_MD_CTX context initialization failed.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdInit (
> > +  OUT VOID          *EvpMdContext,
> > +  IN  CONST CHAR8   *DigestName
> > +  );
> 
> (2) I suggest merging the New() and Init() functions into one -- we
> should only have a New() function:
> 
> VOID *
> EFIAPI
> EvpMdNew (
>   IN  CONST CHAR8 *DigestName
>   );
> 
> And this function would collect the actions from both the current New()
> and Init() functions.
> 
> I'm proposing this because:
> 
> (2a) Init() is not really useful. In theory we might be able to call
> Init() right after Final(), but that seems like a very obscure use case.
> 
> (2b) New() is useless without Init(); it makes no sense for a context to
> exist, somewhere in the edk2 codebase, *between* New() and Init().
> 
> (2c) Init() used to encourage a usage pattern (at least with HMACs)
> where the caller would allocate its own context storage first. This
> usage was fraught with a memory leak
> <https://bugzilla.tianocore.org/show_bug.cgi?id=2095#c7>. This isn't a
> practical problem in this case, because we're -- correctly -- not
> offering a GetContextSize() in this patch, but Init() is still a vestige
> of a bad pattern.
> 
> Basically a caller should go through New -> Update[n] -> Final -> Free,
> possibly calling Duplicate before or after Update.
> 
> ... I could even entertain merging "Final" into "Free", as there seems
> to be no point for a context to exist, in edk2, between Final and Free.
> That would make for New -> Update[n] -> Free.
> 
> I understand that this would diverge from the current model of the
> "digest function family", but I see it as an improvement.
> 
> 
> > +
> > +/**
> > +  Makes a copy of an existing EVP_MD context.
> > +
> > +  If EvpMdContext is NULL, then return FALSE.
> > +  If NewEvpMdContext is NULL, then return FALSE.
> > +
> > +  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
> > +  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
> > +
> > +  @retval TRUE   EVP_MD context copy succeeded.
> > +  @retval FALSE  EVP_MD context copy failed.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdDuplicate (
> > +  IN  CONST VOID    *EvpMdContext,
> > +  OUT VOID          *NewEvpMdContext
> > +  );
> > +
> > +/**
> > +  Digests the input data and updates EVP_MD context.
> > +
> > +  This function performs EVP digest on a data buffer of the specified size.
> > +  It can be called multiple times to compute the digest of long or
> discontinuous data streams.
> > +  EVP_MD context should be already correctly initialized by EvpMdInit(), and
> should not
> > +  be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
> > +
> > +  If EvpMdContext is NULL, then return FALSE.
> > +
> > +  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
> > +  @param[in]       Data               Pointer to the buffer containing the data to be
> digested.
> > +  @param[in]       DataSize           Size of Data buffer in bytes.
> > +
> > +  @retval TRUE   EVP data digest succeeded.
> > +  @retval FALSE  EVP data digest failed.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdUpdate (
> > +  IN OUT  VOID        *EvpMdContext,
> > +  IN      CONST VOID  *Data,
> > +  IN      UINTN       DataSize
> > +  );
> > +
> > +/**
> > +  Completes computation of the EVP digest value.
> > +
> > +  This function completes EVP hash computation and retrieves the digest
> value into
> > +  the specified memory. After this function has been called, the EVP context
> cannot
> > +  be used again.
> > +  EVP context should be already correctly initialized by EvpMdInit(), and
> should
> > +  not be finalized by EvpMdFinal(). Behavior with invalid EVP context is
> undefined.
> > +
> > +  If EvpMdContext is NULL, then return FALSE.
> > +  If Digest is NULL, then return FALSE.
> > +
> > +  @param[in, out]  EvpMdContext   Pointer to the EVP context.
> > +  @param[out]      Digest         Pointer to a buffer that receives the EVP digest
> value.
> > +
> > +  @retval TRUE   EVP digest computation succeeded.
> > +  @retval FALSE  EVP digest computation failed.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdFinal (
> > +  IN OUT  VOID   *EvpMdContext,
> > +  OUT     UINT8  *DigestValue
> > +  );
> > +
> >
> //===============================================================
> ======================
> >  //    Symmetric Cryptography Primitive
> >
> //===============================================================
> ======================
> > diff --git a/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
> b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
> > new file mode 100644
> > index 0000000000..a38c300eb8
> > --- /dev/null
> > +++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
> > @@ -0,0 +1,228 @@
> > +/** @file
> > +  EVP MD Wrapper Implementation for OpenSSL.
> > +
> > +Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
> > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#include "InternalCryptLib.h"
> > +#include <openssl/evp.h>
> > +
> > +/**
> > +  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD
> use.
> > +
> > +  @return  Pointer to the EVP_MD_CTX context that has been initialized.
> > +           If the allocations fails, EvpMdNew() returns NULL.
> > +
> > +**/
> > +VOID *
> > +EFIAPI
> > +EvpMdNew (
> > +  VOID
> > +  )
> > +{
> > +  //
> > +  // Allocates & Initializes EVP_MD_CTX Context by OpenSSL
> EVP_MD_CTX_new()
> > +  //
> > +  return (VOID *) EVP_MD_CTX_new ();
> > +}
> 
> (3) This (VOID*) cast is not needed, and makes the code harder to read,
> in my opinion.
> 
> 
> > +
> > +/**
> > +  Release the specified EVP_MD_CTX context.
> > +
> > +  @param[in]  EvpMdContext  Pointer to the EVP_MD_CTX context to be
> released.
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +EvpMdFree (
> > +  IN  VOID  *EvpMdContext
> > +  )
> > +{
> > +  //
> > +  // Free OpenSSL EVP_MD_CTX Context
> > +  //
> > +  EVP_MD_CTX_free ((EVP_MD_CTX *)EvpMdContext);
> > +}
> 
> (4) Same as (3), just in the reverse direction -- I suggest dropping the
> (EVP_MD_CTX *) cast.
> 
> (Points (3) and (4) apply to some more locations in this patch; if you
> agree with my comments, please review the rest for these superfluous casts.)
> 
> 
> (5) If possible, I'd suggest appending at least one other patch to this
> series, for putting the new interfaces to use at once. We should pick a
> library or driver in edk2 that currently consumes Sha256Init() and its
> friends, and convert those calls to EvpMd*().
> 
> Thanks,
> Laszlo
> 
> > +
> > +/**
> > +  Initializes user-supplied memory pointed to by EvpMdContext as
> EVP_MD_CTX
> > +  context for subsequent use.
> > +
> > +  If EvpMdContext is NULL, then return FALSE.
> > +  If DigestName is NULL, then return FALSE.
> > +
> > +  @param[out]   EvpMdContext  Pointer to EVP_MD_CTX context being
> initialized.
> > +  @param[in]    DigestName    Pointer to the digest name.
> > +
> > +  @retval TRUE   EVP_MD_CTX context initialization succeeded.
> > +  @retval FALSE  EVP_MD_CTX context initialization failed.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdInit (
> > +  OUT VOID          *EvpMdContext,
> > +  IN  CONST CHAR8   *DigestName
> > +  )
> > +{
> > +  CONST EVP_MD    *Digest;
> > +
> > +  //
> > +  // Check input parameters.
> > +  //
> > +  if (EvpMdContext == NULL || DigestName == NULL) {
> > +    return FALSE;
> > +  }
> > +
> > +  //
> > +  // OpenSSL EVP_MD Context Initialization
> > +  //
> > +  if (EVP_MD_CTX_reset ((EVP_MD_CTX *)EvpMdContext) != 1) {
> > +    return FALSE;
> > +  }
> > +
> > +  Digest = EVP_get_digestbyname (DigestName);
> > +  if (Digest == NULL) {
> > +    return FALSE;
> > +  }
> > +
> > +  if (EVP_DigestInit_ex ((EVP_MD_CTX *)EvpMdContext, Digest, NULL) != 1) {
> > +    return FALSE;
> > +  }
> > +
> > +  return TRUE;
> > +}
> > +
> > +/**
> > +  Makes a copy of an existing EVP_MD context.
> > +
> > +  If EvpMdContext is NULL, then return FALSE.
> > +  If NewEvpMdContext is NULL, then return FALSE.
> > +
> > +  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
> > +  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
> > +
> > +  @retval TRUE   EVP_MD context copy succeeded.
> > +  @retval FALSE  EVP_MD context copy failed.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdDuplicate (
> > +  IN  CONST VOID    *EvpMdContext,
> > +  OUT VOID          *NewEvpMdContext
> > +  )
> > +{
> > +  //
> > +  // Check input parameters.
> > +  //
> > +  if (EvpMdContext == NULL || NewEvpMdContext == NULL) {
> > +    return FALSE;
> > +  }
> > +
> > +  if (EVP_MD_CTX_copy ((EVP_MD_CTX *)NewEvpMdContext, (EVP_MD_CTX
> *)EvpMdContext) != 1) {
> > +    return FALSE;
> > +  }
> > +
> > +  return TRUE;
> > +}
> > +
> > +/**
> > +  Digests the input data and updates EVP_MD context.
> > +
> > +  This function performs EVP digest on a data buffer of the specified size.
> > +  It can be called multiple times to compute the digest of long or
> discontinuous data streams.
> > +  EVP_MD context should be already correctly initialized by EvpMdInit(), and
> should not
> > +  be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
> > +
> > +  If EvpMdContext is NULL, then return FALSE.
> > +
> > +  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
> > +  @param[in]       Data               Pointer to the buffer containing the data to be
> digested.
> > +  @param[in]       DataSize           Size of Data buffer in bytes.
> > +
> > +  @retval TRUE   EVP data digest succeeded.
> > +  @retval FALSE  EVP data digest failed.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdUpdate (
> > +  IN OUT  VOID        *EvpMdContext,
> > +  IN      CONST VOID  *Data,
> > +  IN      UINTN       DataSize
> > +  )
> > +{
> > +  //
> > +  // Check input parameters.
> > +  //
> > +  if (EvpMdContext == NULL) {
> > +    return FALSE;
> > +  }
> > +
> > +  //
> > +  // Check invalid parameters, in case only DataLength was checked in
> OpenSSL
> > +  //
> > +  if (Data == NULL && DataSize != 0) {
> > +    return FALSE;
> > +  }
> > +
> > +  //
> > +  // OpenSSL EVP digest update
> > +  //
> > +  if (EVP_DigestUpdate ((EVP_MD_CTX *)EvpMdContext, Data, DataSize) != 1)
> {
> > +    return FALSE;
> > +  }
> > +
> > +  return TRUE;
> > +}
> > +
> > +/**
> > +  Completes computation of the EVP digest value.
> > +
> > +  This function completes EVP hash computation and retrieves the digest
> value into
> > +  the specified memory. After this function has been called, the EVP context
> cannot
> > +  be used again.
> > +  EVP context should be already correctly initialized by EvpMdInit(), and
> should
> > +  not be finalized by EvpMdFinal(). Behavior with invalid EVP context is
> undefined.
> > +
> > +  If EvpMdContext is NULL, then return FALSE.
> > +  If Digest is NULL, then return FALSE.
> > +
> > +  @param[in, out]  EvpMdContext   Pointer to the EVP context.
> > +  @param[out]      Digest         Pointer to a buffer that receives the EVP digest
> value.
> > +
> > +  @retval TRUE   EVP digest computation succeeded.
> > +  @retval FALSE  EVP digest computation failed.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdFinal (
> > +  IN OUT  VOID   *EvpMdContext,
> > +  OUT     UINT8  *DigestValue
> > +  )
> > +{
> > +  UINT32  Length;
> > +
> > +  //
> > +  // Check input parameters.
> > +  //
> > +  if (EvpMdContext == NULL || DigestValue == NULL) {
> > +    return FALSE;
> > +  }
> > +
> > +  //
> > +  // OpenSSL EVP digest finalization
> > +  //
> > +  if (EVP_DigestFinal_ex ((EVP_MD_CTX *)EvpMdContext, DigestValue,
> &Length) != 1) {
> > +    return FALSE;
> > +  }
> > +  if (EVP_MD_CTX_reset ((EVP_MD_CTX *)EvpMdContext) != 1) {
> > +    return FALSE;
> > +  }
> > +
> > +  return TRUE;
> > +}
> >