* [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface @ 2020-08-13 1:20 Zurcher, Christopher J 2020-08-13 1:20 ` [PATCH 1/1] " Zurcher, Christopher J 0 siblings, 1 reply; 6+ messages in thread From: Zurcher, Christopher J @ 2020-08-13 1:20 UTC (permalink / raw) To: devel; +Cc: Jiewen Yao, Jian J Wang, Xiaoyu Lu BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2545 Low-level interfaces to message digest (hash) functions have been deprecated in OpenSSL 3. In order to upgrade to OpenSSL 3, all direct calls to low-level functions (such as SHA256_Init() in CryptSha256.c) will need to be replaced by EVP inteface calls. For reviewers: The added file CryptEvpMd.c is based on CryptHmacSha256.c and can be compared to that file to easily highlight changes. References: https://www.openssl.org/docs/manmaster/man7/evp.html https://www.openssl.org/docs/manmaster/man3/SHA256_Init.html Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Xiaoyu Lu <xiaoyux.lu@intel.com> Christopher J Zurcher (1): CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface 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(+) create mode 100644 CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c -- 2.28.0.windows.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface 2020-08-13 1:20 [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface Zurcher, Christopher J @ 2020-08-13 1:20 ` Zurcher, Christopher J 2020-08-13 9:46 ` [edk2-devel] " Laszlo Ersek 0 siblings, 1 reply; 6+ messages in thread From: Zurcher, Christopher J @ 2020-08-13 1:20 UTC (permalink / raw) To: devel; +Cc: Jiewen Yao, Jian J Wang, Xiaoyu Lu 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(+) 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 + ); + +/** + 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 (); +} + +/** + 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); +} + +/** + 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; +} -- 2.28.0.windows.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface 2020-08-13 1:20 ` [PATCH 1/1] " Zurcher, Christopher J @ 2020-08-13 9:46 ` Laszlo Ersek 2020-08-13 9:47 ` Laszlo Ersek 2020-08-13 14:38 ` Yao, Jiewen 0 siblings, 2 replies; 6+ messages in thread From: Laszlo Ersek @ 2020-08-13 9:46 UTC (permalink / raw) To: devel, christopher.j.zurcher; +Cc: Jiewen Yao, Jian J Wang, Xiaoyu Lu 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; > +} > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface 2020-08-13 9:46 ` [edk2-devel] " Laszlo Ersek @ 2020-08-13 9:47 ` Laszlo Ersek 2020-08-13 14:38 ` Yao, Jiewen 1 sibling, 0 replies; 6+ messages in thread From: Laszlo Ersek @ 2020-08-13 9:47 UTC (permalink / raw) To: devel, christopher.j.zurcher Cc: Jiewen Yao, Jian J Wang, Xiaoyu Lu, Michael Kinney On 08/13/20 11:46, Laszlo Ersek wrote: > Hi Christopher, > > (+Mike, bah I forgot to actually CC Mike. Doing that now. Sorry! Laszlo > > 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; >> +} >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface 2020-08-13 9:46 ` [edk2-devel] " Laszlo Ersek 2020-08-13 9:47 ` Laszlo Ersek @ 2020-08-13 14:38 ` Yao, Jiewen 2020-08-14 21:33 ` Zurcher, Christopher J 1 sibling, 1 reply; 6+ messages in thread From: Yao, Jiewen @ 2020-08-13 14:38 UTC (permalink / raw) To: Laszlo Ersek, devel@edk2.groups.io, Zurcher, Christopher J Cc: Wang, Jian J, Lu, XiaoyuX 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; > > +} > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface 2020-08-13 14:38 ` Yao, Jiewen @ 2020-08-14 21:33 ` Zurcher, Christopher J 0 siblings, 0 replies; 6+ messages in thread From: Zurcher, Christopher J @ 2020-08-14 21:33 UTC (permalink / raw) To: Yao, Jiewen, Laszlo Ersek, devel@edk2.groups.io; +Cc: Wang, Jian J, Lu, XiaoyuX I replaced the MD5 and SHAx functions with EVP functions in Hash2DxeCrypto, and it grew from ~26k to ~253k. -- Christopher Zurcher > -----Original Message----- > From: Yao, Jiewen <jiewen.yao@intel.com> > Sent: Thursday, August 13, 2020 07:38 > To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Zurcher, > Christopher J <christopher.j.zurcher@intel.com> > Cc: 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 > > 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; > > > +} > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-14 21:33 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-08-13 1:20 [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface Zurcher, Christopher J 2020-08-13 1:20 ` [PATCH 1/1] " Zurcher, Christopher J 2020-08-13 9:46 ` [edk2-devel] " Laszlo Ersek 2020-08-13 9:47 ` Laszlo Ersek 2020-08-13 14:38 ` Yao, Jiewen 2020-08-14 21:33 ` Zurcher, Christopher J
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox