OK, abandoning this patch set.
The BZ can stay open as we will still have to migrate the existing files to EVP
eventually.
--
Christopher Zurcher
> -----Original Message-----
> From: Yao, Jiewen <jiewen....@intel.com>
> Sent: Wednesday, September 16, 2020 18:34
> To: Zurcher, Christopher J <christopher.j.zurc...@intel.com>; Laszlo Ersek
> <ler...@redhat.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, XiaoyuX <xiaoyux...@intel.com>
> Subject: RE: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP
> (Envelope) Digest interface
>
> 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.zurc...@intel.com>
> > Sent: Thursday, September 17, 2020 9:21 AM
> > To: Laszlo Ersek <ler...@redhat.com>; Yao, Jiewen <jiewen....@intel.com>;
> > devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, XiaoyuX <xiaoyux...@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 <ler...@redhat.com>
> > > Sent: Wednesday, September 16, 2020 09:05
> > > To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io; Zurcher,
> > Christopher J
> > > <christopher.j.zurc...@intel.com>
> > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, XiaoyuX
> > > <xiaoyux...@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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#65347): https://edk2.groups.io/g/devel/message/65347
Mute This Topic: https://groups.io/mt/76878643/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
