From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web11.6790.1597312042562928451 for ; Thu, 13 Aug 2020 02:47:22 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ituoYLhy; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1597312041; 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=2e1QT98Xo0PUWMLEOuuBgvydG+OVX6FF1ISdJd9jQ9k=; b=ituoYLhyJIQQDZaWrbAQVf/nTwfSc9iPr2+KIRtTGgAr7z9HWHFkVJdkTel6B45/6bVv1U jvYy+Me4BTSGcP7cbhzqB2kge3EueA/HscTAKakZ6gpkSnvjGzf0TvoZQHuoaz73LgCKHW cOYXQ8sUoi/YcrsvZ+UOaN6sWZehv74= 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-215-a4hR_-VkMjmQ80RqkvoKZQ-1; Thu, 13 Aug 2020 05:47:19 -0400 X-MC-Unique: a4hR_-VkMjmQ80RqkvoKZQ-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 27DC780183C; Thu, 13 Aug 2020 09:47:18 +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 7C2BD709E4; Thu, 13 Aug 2020 09:47:16 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 1/1] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface From: "Laszlo Ersek" To: devel@edk2.groups.io, christopher.j.zurcher@intel.com Cc: Jiewen Yao , Jian J Wang , Xiaoyu Lu , Michael Kinney References: <20200813012049.7402-1-christopher.j.zurcher@intel.com> <20200813012049.7402-2-christopher.j.zurcher@intel.com> <15d6f379-9dd2-e6d7-5901-4b23e78c7794@redhat.com> Message-ID: Date: Thu, 13 Aug 2020 11:47:15 +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: <15d6f379-9dd2-e6d7-5901-4b23e78c7794@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0.002 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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 >> 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; >> +} >> >