Re: [PATCH v2 0/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface


Yao, Jiewen
 

Comments below:

-----Original Message-----
From: Zurcher, Christopher J <christopher.j.zurcher@intel.com>
Sent: Tuesday, September 15, 2020 10:54 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
Cc: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>;
Lu, XiaoyuX <xiaoyux.lu@intel.com>
Subject: RE: [PATCH v2 0/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest
interface

Replies inline

-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Monday, September 14, 2020 18:22
To: Zurcher, Christopher J <christopher.j.zurcher@intel.com>;
devel@edk2.groups.io
Cc: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>;
Lu, XiaoyuX <xiaoyux.lu@intel.com>
Subject: RE: [PATCH v2 0/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope)
Digest
interface

Hi Zurcher:
Thanks for your work.
1) Please share with us what unit test you have done for all new APIs.
I unit tested both the native and Crypto Service implementations through the
modified Hash2DxeCrypto protocol.
I tested the Init/Update/Final flow as well as the HashAll function.


2) Please add comment on what is the valid DigestName in EvpMdInit().
Otherwise, people will have no idea on that.
I will add valid options in a comment.
I have to send another patch anyway to add a file in my commit (missed the
second copy of CryptEvpMdNull.c in the NullLib folder).


3) I assume the size will be unchanged if a module does not use the new
EVPMD
API, such as UEFI secure boot, TCG trusted boot. Please double confirm if
that is right understanding.
Yes, if a module does not call the EVPMD API, it should not grow in size.
The Crypto Service build output CryptoDxe.efi grew less than 1% after enabling
the EvpMd function family through PcdCryptoServiceFamilyEnable.
I suspect this is because the HmacSha256 Family was already enabled, and inside
OpenSSL the HMAC functions are wrappers for EVP functions.
So even with library-mode BaseCryptLib, any module that already calls the
HMAC functions should not see any size change by adding EVP.


Hi all:
I would like collect feedback on below:
-- "I replaced the MD5 and SHAx functions with EVP functions in
Hash2DxeCrypto, and it grew from ~26k to ~253k."

If there is negative size impact for the platform BIOS that is using
Hash2DxeCrypto, please share with the community.
The size change in Hash2DxeCrypto was seen while using the library-mode
BaseCryptLib implementation, not the Crypto Services driver.
We cannot move to OpenSSL 3 without replacing all low-level algorithm
functions with EVP calls, so platforms using Hash2DxeCrypto will have to eat the
size increase eventually.
For platforms using Hash2DxeCrypto, moving to the Crypto Services model
should help offset this increase.
[Jiewen] I think we need evaluate the size impact to decide if/when/how to move to OpenSSL 3 later.
We can cross the bridge when we come to it.



Thanks,
Christopher Zurcher


Thank you
Yao Jiewen

-----Original Message-----
From: Christopher J Zurcher <christopher.j.zurcher@intel.com>
Sent: Tuesday, September 15, 2020 8:58 AM
To: devel@edk2.groups.io
Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen
<jiewen.yao@intel.com>;
Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>
Subject: [PATCH v2 0/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest
interface

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2545

V2 changes:
Added NullLib implementation
Added Crypto Service implementation
Rebased Hash2DxeCrypto to use EVP interface instead of low-level functions
Removed unnecessary casts
Added "HashAll" utility function
Merged "New" and "Init" functions as well as "Final" and "Free" functions
Retained "Init/Update/Final" naming instead of "New/Update/Free" as this
conforms with common usage

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.

References:
https://www.openssl.org/docs/manmaster/man7/evp.html
https://www.openssl.org/docs/manmaster/man3/SHA256_Init.html

Cc: Laszlo Ersek <lersek@redhat.com>
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 (3):
CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
CryptoPkg: Add EVP to Crypto Service driver interface
SecurityPkg/Hash2DxeCrypto: Rebase Hash2DxeCrypto onto the EVP
interface

CryptoPkg/CryptoPkg.dsc | 3 +
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/Library/BaseCryptLibNull/BaseCryptLibNull.inf | 1 +
CryptoPkg/Include/Library/BaseCryptLib.h | 125 +++++++
CryptoPkg/Include/Pcd/PcdCryptoServiceFamilyEnable.h | 10 +
CryptoPkg/Private/Protocol/Crypto.h | 127 +++++++
SecurityPkg/Hash2DxeCrypto/Driver.h | 1 -
CryptoPkg/Driver/Crypto.c | 148 ++++++++-
CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c | 253
++++++++++++++
CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c | 124 +++++++
CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c | 140 ++++++++
SecurityPkg/Hash2DxeCrypto/Hash2DxeCrypto.c | 345 ++----------
--------
15 files changed, 965 insertions(+), 316 deletions(-)
create mode 100644 CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
create mode 100644
CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c

--
2.28.0.windows.1

Join devel@edk2.groups.io to automatically receive all group messages.