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

["Laszlo Ersek" <lersek@redhat.com>]

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;
> +}
>