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

["Yao, Jiewen" <jiewen.yao@intel.com>]

Thanks to ask this question Christopher.

When we create the CryptoPkg, our goal is to pick openssl as implementation choice. We don’t want to mandate any EDKII consumer use openssl.

The EDKII consumer shall have freedom to choose whatever crypto library they want to use. Even Intel IPP and mebdTLS are options. A vendor may choose a GPL wolfssl, or vendor proprietary close source implementation. We choose openssl just because it is the BSD license and it is used widely when we make decision.

That is why we spend effort to design BaseCryptoLib API to abstract the crypto action, instead of calling openssl function directly in our code.
Whenever we add new API, we need evaluate if it is applicable for any other crypto implantation and have negative impact.

That assumption that EDKII must link openssl is NOT our original intent.
Feel free to drop the patch or reject the Bugzilla, if you want.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Zurcher, Christopher J <christopher.j.zurcher@intel.com>
> Sent: Thursday, September 17, 2020 9:21 AM
> To: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>
> Subject: RE: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP
> (Envelope) Digest interface
> 
> Jiewen,
> 
> This patch set assumes that EDK2 is linked with OpenSSL for the foreseeable
> future. The end goal would be to move all applicable Crypto access to the EVP
> interface and remove the individual modules we maintain for each algorithm.
> The primary benefit here is reducing complexity and code duplication. Without
> this end goal, this patch set will not be particularly useful.
> 
> If the design goal of BaseCryptLib is to allow OpenSSL to be replaced by other
> crypto providers, I should abandon this patch set, and we can save the EVP
> transition for when moving to OpenSSL 3 becomes mandatory. At that time, the
> CryptX.c family can be modified to call EVP functions.
> (This could even be done transparently, by returning UINTN from GetContextSize
> and using the user-supplied "context" to store the OpenSSL-supplied context
> pointer.)
> Delaying EVP adoption in this case would also allow more time for platforms to
> adopt the Crypto Services model, which should help offset the size impact.
> 
> Please let me know which path should be taken.
> 
> Thanks,
> Christopher Zurcher
> 
> > -----Original Message-----
> > From: Laszlo Ersek <lersek@redhat.com>
> > Sent: Wednesday, September 16, 2020 09:05
> > To: Yao, Jiewen <jiewen.yao@intel.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 v3 1/3] CryptoPkg/BaseCryptLib: Add EVP
> (Envelope) Digest interface
> >
> > On 09/16/20 16:17, Yao, Jiewen wrote:
> > > Thank you Laszlo.
> > > You forced me to read the code again and more carefully. :-)
> > >
> > > I believe I understand why you use NULL to free the context now - to support
> error path.
> > >
> > > First, I did have some new thought for EVP API. Let’s consider 3 cases, 1)
> Hash all data one time, 2) Hash update
> > data multiple times, 3) Error during update.
> > >
> > > A. In current design, the API sequence is:
> > > 1) EvpMdHashAll (Digest)
> > > 2) EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest)
> > > 3) EvpMdInit, EvpMdUpdate->ERROR, EvpMdFinal (NULL)
> > >
> > > B. We can auto free context in EvpMdUpdate in error path  - the API
> sequence is:
> > > 1) EvpMdHashAll (Digest)
> > > 2) EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest)
> > > 3) EvpMdInit, EvpMdUpdate->ERROR
> >
> > I don't like "B" because in the loop body where you call EvpMdUpdate(),
> > you may encounter an error from a different source than EvpMdUpdate()
> > itself. For example, you may be reading data from the network or a disk
> > file. If fetching the next chunk fails, we'd still want to clean up the
> > EVP MD context. Therefore, if EvpMdUpdate() would itself invalidate the
> > context, if it failed, then that would *complicate* the error path. (=
> > Clean up the context after the loop body *only* if something different
> > from EvpMdUpdate() failed.)
> >
> > >
> > > C: We can use New/Free style - similar to HMAC
> > > 1) EvpMdHashAll (Digest)
> > > 2) EvpMdNew, EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal
> (Digest), EvpMdFree
> > > 3) EvpMdNew, EvpMdInit, EvpMdUpdate->ERROR, EvpMdFree
> >
> > Yes, this was the first pattern, the one that caused me to say "please
> > don't do this". In this pattern, the context may exist between "New" and
> > "Init", and also between "Final" and "Free". Those two states are
> > useless and only good for causing confusion.
> >
> > For example, are you allowed to Duplicate a context in those states?
> > It's best to prevent even the *asking* of that question.
> >
> > >
> > > I checked below APIs:
> > > 1) openssl (EVP_MD_CTX_new, EVP_DigestInit_ex, EVP_DigestUpdate,
> EVP_DigestFinal_ex, EVP_MD_CTX_free)
> > > 2) mbedtls (mbedtls_sha256_init, mbedtls_sha256_starts_ret,
> mbedtls_sha256_update_ret, mbedtls_sha256_finish_ret,
> > mbedtls_sha256_free)
> > > 3) BaseCryptoLib HMAC (HmacSha256New, HmacSha256SetKey,
> HmacSha256Update, HmacSha256Final, HmacSha256Free)
> > >
> > > All APIs include free function to free the context, I don’t think it is a bad idea
> to have EvpMdFree() API.
> > > I would like to propose option - C.
> >
> > - I cannot comment on mbedtls (2).
> >
> > - I think the current BaseCryptLib HMAC APIs (3) are not great, and we
> > should use this opportunity to improve upon them.
> >
> > - Regarding openssl (1), as I understand it, the situation is different
> > from edk2. In OpenSSL, EVP_MD_CTX is *not* an opaque type (expressed in
> > C language terms, it is not an "incomplete" type, but a "complete"
> > type). Therefore, an expression such as sizeof(EVP_MD_CTX) works.
> >
> > The consequence is that OpenSSL code itself can use the following style:
> >
> > void func(void)
> > {
> >   EVP_MD_CTX ctx;
> >
> >   EVP_DigestInit_ex(&ctx, ...);
> >   ...
> >   EVP_DigestFinal_ex(&ctx, ...)
> > }
> >
> > In other words, OpenSSL-native code is permitted to know the internals
> > of EVP_MD_CTX, therefore the OpenSSL-internal code may allocate
> > EVP_MD_CTX with means that are *different* from EVP_MD_CTX_new() and
> > EVP_MD_CTX_free().
> >
> > The caller of (Init, Final) may use (New, Free) for memory management,
> > or may use something completely different (for example, local variables).
> >
> > Therefore, offering the (Init, Final) APIs separately from (New, Free)
> > is meaningful.
> >
> > But the situation in edk2 -- or any other OpenSSL *consumer* -- is
> > different. In edk2, EVP_MD_CTX is an opaque type -- in C language terms:
> > an "incomplete" type. OpenSSL deliberately hides the internals of
> > EVP_MD_CTX.
> >
> > See "CryptoPkg/Library/OpensslLib/openssl/include/openssl/ossl_typ.h":
> >
> > > typedef struct evp_md_ctx_st EVP_MD_CTX;
> >
> > and the "evp_md_ctx_st" structure type is only defined in
> > "CryptoPkg/Library/OpensslLib/openssl/crypto/evp/evp_local.h", whose
> > contents -- I think! -- OpenSSL client code cannot, or *should not*,
> > refer to.
> >
> > This means that OpenSSL consumer code can *only* rely on
> > EVP_MD_CTX_new() and EVP_MD_CTX_free(), for allocating and releasing the
> > context, respectively. "sizeof(EVP_MD_CTX)" is not permitted, and
> > defining a local or global variable of type EVP_MD_CTX is also not valid.
> >
> > This means that the only reason for separating (Init, Final) from (New,
> > Free) falls away, in OpenSSL consumer code. In OpenSSL consumer code,
> > there is no reason to keep *both* pairs of APIs.
> >
> >
> > Please note that, this (very prudent) information hiding / encapsulation
> > in OpenSSL used to be violated by edk2 in the past, intentionally.
> > That's what the HmacXxxGetContextSize() APIs were about -- those APIs
> > forcefully leaked the context sizes to client code, so that client code
> > in edk2 could perform its own allocation.
> >
> > But in <https://bugzilla.tianocore.org/show_bug.cgi?id=1792>, we finally
> > eliminated HmacXxxGetContextSize(). As a part of that, we also
> > simplified the HMAC API -- note that Init was replaced by SetKey. And
> > because we were reworking an existent API, I didn't propose very
> > intrusive changes -- I didn't propose fusing Final and Free, for example.
> >
> > Now, the EVP MD APIs are just being introduced to edk2. So we have a
> > chance at getting them right, and making them minimal.
> >
> > > Second, I believe EVP is just something in openssl. It does not mean that we
> *have to* design API to follow
> > openssl.
> > > After rethink the size impact, I do have concern to merge all Hash
> implementation together in one function.
> > > It might means nothing if the crypto library is based upon openssl.
> > > But if the cryptolib implementation is based upon other crypto, such as Intel
> IPP
> >
> (https://github.com/slimbootloader/slimbootloader/tree/master/BootloaderCo
> mmonPkg/Library/IppCryptoLib) or mbedTls
> > (https://github.com/jyao1/edk2/tree/DeviceSecurity/CryptoMbedTlsPkg), then
> we can NOT get size benefit by separating
> > the hash algorithm.
> > >
> > >
> > > I would like to propose separate EvpMdxxx.
> > > EvpMdNew -> Sha256New, Sha384New
> > > EvpMdInit -> Sha256Init, Sha384Init
> > > EvpMdUpdate -> Sha256Update, Sha384Update
> > > EvpMdFinal -> Sha256Final, Sha384Final
> > > EvpMdFree -> Sha256Free, Sha384Free
> >
> > I have no comment on this.
> >
> > > We can do similar thing with Hmac, to deprecate Sha256GetContextSize()
> API,
> >
> > Yes, good idea.
> >
> > > and replace caller with Sha256New() / Sha384Free()
> >
> > Indeed.
> >
> > But then -- there's no reason left for keeping (New, Free) separate from
> > (Init, Final).
> >
> > Thanks
> > Laszlo