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

["Laszlo Ersek" <lersek@redhat.com>]

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.


>> 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
> 
>> 
>