From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web11.6783.1597312003744459116 for ; Thu, 13 Aug 2020 02:46:44 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=fnzEOpJr; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1597312002; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=s8nIR3l9WJn3EK8BkjMIkQeCBx4A+rBnCrzqzo0+KiQ=; b=fnzEOpJrVIAzEmNcKQgsqR4deUYP1/UTPK0TLstPiA0r7kXK1zxZY9cixIXBh8Cpa8ycYP iNZDfwKv//trElVMIJxRjUIkUvRlE+yj+zbKAz3bkEybn2mjb9w+AiBEXyKVTB21Cw2aoZ 1VEfnHMml1Cnxx+yuN9xg5OfZsgSqc0= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-516-BIk0E9zuMfePFkhZBH2_kg-1; Thu, 13 Aug 2020 05:46:35 -0400 X-MC-Unique: BIk0E9zuMfePFkhZBH2_kg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B4743107ACCA; Thu, 13 Aug 2020 09:46:34 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-59.ams2.redhat.com [10.36.114.59]) by smtp.corp.redhat.com (Postfix) with ESMTP id 25D505B696; Thu, 13 Aug 2020 09:46:32 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 1/1] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface To: devel@edk2.groups.io, christopher.j.zurcher@intel.com Cc: Jiewen Yao , Jian J Wang , Xiaoyu Lu References: <20200813012049.7402-1-christopher.j.zurcher@intel.com> <20200813012049.7402-2-christopher.j.zurcher@intel.com> From: "Laszlo Ersek" Message-ID: <15d6f379-9dd2-e6d7-5901-4b23e78c7794@redhat.com> Date: Thu, 13 Aug 2020 11:46:31 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200813012049.7402-2-christopher.j.zurcher@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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 > Cc: Jian J Wang > Cc: Xiaoyu Lu > Signed-off-by: Christopher J Zurcher > --- > 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 . 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.
> +SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#include "InternalCryptLib.h" > +#include > + > +/** > + 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; > +} >