Re: [edk2-devel] [PATCH v4 0/7] CryptoPkg: Upgrade OpenSSL to 1.1.1b

["Wang, Jian J" <jian.j.wang@intel.com>]

Ard,


> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ard
> Biesheuvel
> Sent: Tuesday, May 21, 2019 9:39 PM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io; Lu, XiaoyuX
> <xiaoyux.lu@intel.com>; Ye, Ting <ting.ye@intel.com>; Leif Lindholm
> <leif.lindholm@linaro.org>; Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2-devel] [PATCH v4 0/7] CryptoPkg: Upgrade OpenSSL to 1.1.1b
> 
> On Tue, 21 May 2019 at 13:23, Laszlo Ersek <lersek@redhat.com> wrote:
> >
> > Hi,
> >
> > On 05/21/19 11:09, Wang, Jian J wrote:
> > > Ard,
> > >
> > >> -----Original Message-----
> > >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Ard
> > >> Biesheuvel
> > >> Sent: Tuesday, May 21, 2019 5:02 PM
> > >> To: Wang, Jian J <jian.j.wang@intel.com>
> > >> Cc: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>; Lu, XiaoyuX
> > >> <xiaoyux.lu@intel.com>; Ye, Ting <ting.ye@intel.com>; Leif Lindholm
> > >> <leif.lindholm@linaro.org>; Gao, Liming <liming.gao@intel.com>
> > >> Subject: Re: [edk2-devel] [PATCH v4 0/7] CryptoPkg: Upgrade OpenSSL to
> 1.1.1b
> > >>
> > >> On Tue, 21 May 2019 at 09:43, Wang, Jian J <jian.j.wang@intel.com> wrote:
> > >>>
> > >>> Hi Ard,
> > >>>
> > >>> Any comments?
> > >>>
> > >>> Regards,
> > >>> Jian
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf
> Of
> > >> Wang,
> > >>>> Jian J
> > >>>> Sent: Monday, May 20, 2019 9:41 AM
> > >>>> To: devel@edk2.groups.io; ard.biesheuvel@linaro.org; Laszlo Ersek
> > >>>> <lersek@redhat.com>
> > >>>> Cc: Lu, XiaoyuX <xiaoyux.lu@intel.com>; Ye, Ting <ting.ye@intel.com>;
> Leif
> > >>>> Lindholm <leif.lindholm@linaro.org>; Gao, Liming
> <liming.gao@intel.com>
> > >>>> Subject: Re: [edk2-devel] [PATCH v4 0/7] CryptoPkg: Upgrade OpenSSL to
> > >> 1.1.1b
> > >>>>
> > >>>> Ard,
> > >>>>
> > >>>>
> > >>>>> -----Original Message-----
> > >>>>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf
> Of
> > >> Ard
> > >>>>> Biesheuvel
> > >>>>> Sent: Friday, May 17, 2019 11:06 PM
> > >>>>> To: Laszlo Ersek <lersek@redhat.com>
> > >>>>> Cc: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io; Lu,
> > >> XiaoyuX
> > >>>>> <xiaoyux.lu@intel.com>; Ye, Ting <ting.ye@intel.com>; Leif Lindholm
> > >>>>> <leif.lindholm@linaro.org>; Gao, Liming <liming.gao@intel.com>
> > >>>>> Subject: Re: [edk2-devel] [PATCH v4 0/7] CryptoPkg: Upgrade OpenSSL
> to
> > >>>> 1.1.1b
> > >>>>>
> > >>>>> On Fri, 17 May 2019 at 15:17, Laszlo Ersek <lersek@redhat.com> wrote:
> > >>>>>>
> > >>>>>> On 05/17/19 15:04, Laszlo Ersek wrote:
> > >>>>>>> On 05/17/19 07:11, Wang, Jian J wrote:
> > >>>>>>>> Hi Laszlo,
> > >>>>>>>>
> > >>>>>>>> There's already a float library used in OpensslLib.inf.
> > >>>>>>>>
> > >>>>>>>> [LibraryClasses.ARM]
> > >>>>>>>>   ArmSoftFloatLib
> > >>>>>>>>
> > >>>>>>>> The problem is that the below instance doesn't implement
> > >> __aeabi_ui2d
> > >>>>>>>> and __aeabi_d2uiz (I encountered this one as well)
> > >>>>>>>>
> > >>>>>>>>   ArmPkg\Library\ArmSoftFloatLib\ArmSoftFloatLib.inf
> > >>>>>>>>
> > >>>>>>>> I think we can update this library support those two APIs. So what
> > >> about
> > >>>>>>>> we still push the patch and file a BZ to fix this issue?
> > >>>>>>>
> > >>>>>>> I'm OK with that, but it will break ARM and AARCH64 platforms that
> > >>>>>>> consume OpensslLib (directly or through BaseCryptLib), so this
> question
> > >>>>>>> is up to Leif and Ard to decide.
> > >>>>>>
> > >>>>>> Correction: break ARM platforms only, not AARCH64.
> > >>>>>>
> > >>>>>
> > >>>>> We obviously need to fix this before we can upgrade to a new OpenSSL
> > >> version.
> > >>>>>
> > >>>>> Do we really have a need for the random functions? These seem the
> only
> > >>>>> ones that use floating point, which the UEFI spec does not permit, so
> > >>>>> it would be better if we could fix this by removing the dependency on
> > >>>>> FP in the first place (and get rid of ArmSoftFloatLib entirely)
> > >>>>>
> > >>>>
> > >>>> BaseCryptLib provides RandSeed/RandBytes interface which wrap
> openssl
> > >> rand
> > >>>> functionalities. These interfaces are used by following components in
> edk2
> > >>>>
> > >>>>   - CryptoPkg\Library\TlsLib\TlsInit.c
> > >>>>   - SecurityPkg\HddPassword\HddPasswordDxe.c
> > >>>>
> > >>>> Openssl components, like asn1, bn, evp, ocsp, pem, pkcs7, pkcs12, rsa,
> ssl (in
> > >>>> addition
> > >>>> to cms, dsa, srp, which are disabled in edk2) will call rand_* interface as
> well.
> > >>>>
> > >>
> > >> If we have both internal (to Openssl) and external users of the RNG
> > >> api, then I guess there is no way to work around this. It is
> > >> unfortunate, since the RNG code in OpenSSL doesn't actually use double
> > >> types except for keeping an entropy count, which could just as easily
> > >> be kept in an integer variable.
> >
> > (1) I think I agree... However, it seems that the first function (or one
> > of the first functions) in OpenSSL to take an "entropy" parameter, of
> > type "double", was RAND_add(). And the RAND_add() manual states,
> >
> >        RAND_add() mixes the num bytes at buf into the PRNG state.
> >        Thus, if the data at buf are unpredictable to an adversary,
> >        this increases the uncertainty about the state and makes the
> >        PRNG output less predictable. Suitable input comes from user
> >        interaction (random key presses, mouse movements) and certain
> >        hardware events. The entropy argument is (the lower bound of)
> >        an estimate of how much randomness is contained in buf,
> >        measured in bytes. Details about sources of randomness and how
> >        to estimate their entropy can be found in the literature, e.g.
> >        RFC 1750.
> >
> > I've now looked up RFC 1750, and it contains copious amounts of math on
> > irrational numbers. Hence the use of floating point in OpenSSL, I'd guess.
> >
> >   https://www.ietf.org/rfc/rfc1750.txt
> >
> > ... After digging a bit in the OpenSSL git history, I've found the
> > following commit (from 19 years ago):
> >
> > commit 853f757ecea74a271a7c5cdee3f3b5fe0d3ae863
> > Author: Bodo Möller <bodo@openssl.org>
> > Date:   Sat Feb 19 15:22:53 2000 +0000
> >
> >     Allow for higher granularity of entropy estimates by using 'double'
> >     instead of 'unsigned' counters.
> >     Seed PRNG in MacOS/GetHTTPS.src/GetHTTPS.cpp.
> >
> >     Partially submitted by Yoram Meroz <yoram@mail.idrive.com>.
> >
> > It was the commit with
> >
> > -void RAND_add(const void *buf,int num,int entropy);
> > +void RAND_add(const void *buf,int num,double entropy);
> >
> > FWIW, the "PRNG" reference at the end of the commit message seems
> > meaningless. Check for yourself:
> >
> > $ git show 853f757ecea7:MacOS/GetHTTPS.src/GetHTTPS.cpp
> >
> > The fact that "entropy" is now of type "double" does not seem to be put
> > to use, anywhere in that file.
> >
> > I'll send a query to the openssl-users mailing list, just so we
> > understand better.
> >
> 
> Thanks for doing the paleontological research here.
> 
> However, the outcome of this query is not going to affect our short
> term issue with this code.
> 
> I will try to come back to this issue as soon as I can, but I am a bit
> swamped at the moment.
> 
> 

The community has decided to complete the upgrade for edk2-stable201905.
How long will you need to add those two APIs?

Regards,
Jian
> 
> 
> >
> > >> So we will need to fix ArmSoftFloatLib before we can merge this
> > >> OpenSSL update.
> >
> > (2) NB, I think we can no longer merge this feature for
> > edk2-stable201905. The soft feature freeze criterion is that all patches
> > be reviewed (approved) on-list before the SFF date / announcement, and
> > that was not fulfilled in this case.
> >
> >
> > >> I'm happy to help doing that, could you please
> > >> summarize what we are missing today?
> > >>
> > >
> > > Great. I think there're two intrinsic functions missing here
> > >
> > >   __aeabi_ui2d
> > >   __aeabi_d2uiz
> > >
> > > Laszlo, please double check if these two are enough.
> >
> > (3) I can only report the failure that trips up the build for me. I did
> > that here:
> >
> > http://mid.mail-archive.com/049e489c-b58f-0fc5-1c66-
> 8ad920d93979@redhat.com
> > https://edk2.groups.io/g/devel/message/40823
> >
> >
> > Thus, for me, the missing symbol was "__aeabi_ui2d".
> >
> > It's possible that the 32-bit ARM build will fail at a different (later)
> > stage as well, but I can't tell until I get past this one. (And I don't
> > think I can implement a "shim" function for the missing symbol, just to
> > let the build progress.)
> >
> > Thanks,
> > Laszlo
> >
> > > Thanks for doing this.
> > >
> > > Regards,
> > > Jian
> > >
> > >>
> > >
> >
> 
>