Re: [edk2-devel] [PATCH v2 5/6] CryptoPkg: Upgrade OpenSSL to 1.1.1b

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

On 05/10/19 12:26, Wang, Jian J wrote:
> Hi Laszlo,
> 
> rand_* is needed by openssl itself. BaseCryptLib also provide RandomSeed()
> and RandomBytes() interface to wrap openssl rand functionality. We can't
> just drop them. From platform independent perspective, using performance
> counter is the best choice we have. If we want to achieve the uttermost
> secure mechanism, only hardware seed/rand generator can meet it. Do you
> agree to add cpu specific instruction to do that? For those processors
> which don't support seed/rand generation, we have to fall back to performance
> counter.

I have not been aware of RandomSeed() / RandomBytes().

The RandomSeed() function lets the caller specify a seed, which is good
design, IMO. In case the caller does not pass in a seed, the function
implements its own seed.

For that default seeding, we have the following implementations:

(1) "CryptoPkg/Library/BaseCryptLib/Rand/CryptRand.c" -- uses a constant
string as seed. :(

(2) "CryptoPkg/Library/BaseCryptLib/Rand/CryptRandItc.c" -- calls
AsmReadItc() to calculate the seed. The function AsmReadItc() is not
defined (or even declared) anywhere in edk2, so I don't know what it does.

(3) "CryptoPkg/Library/BaseCryptLib/Rand/CryptRandNull.c" -- calls
ASSERT(FALSE), then returns FALSE.

(4) "CryptoPkg/Library/BaseCryptLib/Rand/CryptRandTsc.c" -- calls
AsmReadTsc() to calculate the seed.


PEIMs seem to use (3), which appears safe.

DXE drivers, runtime DXE drivers, and SMM drivers, use (4) on IA32/X64,
and (1) on ARM/AARCH64. All quite unfortunate.

Option (2) is never used in edk2.

Based on the above analysis (is it correct?), I must agree that the v1
and v2 approaches in the present patch set, namely "constant data" and
"TimerLib", may not be worse than what we already have.


> Another option is that we could make use of EFI_RNG_PROTOCOL. According
> to UEFI spec (see below), it can be used to get entropy.
> 
> "This protocol is used to provide random numbers for use in applications,
> or entropy for seeding other random number generators."
> 
> Again, we could use performance counter as a fall back if EFI_RNG_PROTOCOL
> is not provided by a platform. So if a platform really care about the security,
> it should provide a EFI_RNG_PROTOCOL. This can also help to hide the
> hardware/platform dependency.

Consuming EFI_RNG_PROTOCOL would be an improvement.

But it looks quite tricky. Namely, EFI_RNG_PROTOCOL may be provided by a
UEFI driver that follows the UEFI driver model, and so the protocol
could become available first in the BDS phase (when the underlying hwrng
device were connected). Until then, no driver that depends on entropy --
through BaseCryptLib or OpensslLib -- should be dispatched.

On the other hand, if the platform hardware does not include a hwrng
(and so EFI_RNG_PROTOCOL is never produced), then the entropy dependent
drivers should be dispatched as soon as this fact is determined
(dynamically, at every boot).

In other words, entropy-dependent drivers should wait until platform
code (likely in BDS) makes the determination whether EFI_RNG_PROTOCOL
can offer high-quality entropy, or the fallbacks have to be used.

To make it even more complex, SMM drivers that need entropy should
likely never use EFI_RNG_PROTOCOL, because a 3rd party UEFI driver
should not be able to feed entropy (sensitive crypto data) to a
privileged (SMM) driver.

Do you think this is possible to implement?... It looks extremely
complex to me.

Honestly the best I could suggest is, "use RDRAND if available, fall
back to TimerLib otherwise". :( But I would understand if you wanted to
postpone RDRAND until later.

Given this situation, I think I can't give A-b or R-b for this patch in
the series. I think I can only offer regression-testing (for the
upcoming v3). And. I don't intend to block this work based on the
entropy source alone, any more.

... Can we at least collect a detailed list of use cases for which we
must provide entropy? I think we should document that somewhere.

Thanks,
Laszlo

> 
> Regards,
> Jian
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>> Laszlo Ersek
>> Sent: Friday, May 10, 2019 1:30 AM
>> To: devel@edk2.groups.io; Lu, XiaoyuX <xiaoyux.lu@intel.com>
>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Ye, Ting <ting.ye@intel.com>
>> Subject: Re: [edk2-devel] [PATCH v2 5/6] CryptoPkg: Upgrade OpenSSL to 1.1.1b
>>
>> On 05/09/19 19:15, Laszlo Ersek wrote:
>>
>>> How about the following:
>>>
>>> - It seems like we cannot convince OpenSSL to *never* call these
>>>   functions, under UEFI.
>>>
>>> - We also cannot provide an implementation that is *guaranteed* to be
>>>   secure enough, IMO.
>>>
>>> - It seems like these functions *should* never be called in the edk2
>>>   build however, given that we're not trying to do anything "new" with
>>>   OpenSSL in edk2 -- we just want to use the new OpenSSL release for the
>>>   same old things.
>>>
>>> - So why not just ensure that these functions *never return*?
>>>
>>> (1) Basically implement all of the functions like this:
>>>
>>>   ASSERT (FALSE);
>>>   CpuDeadLoop ();
>>>   //
>>>   // if a return value is needed
>>>   //
>>>   return 0;
>>>
>>> What do you think about this approach?
>>
>> I notice that "rand" is another module in OpenSSL.
>>
>> Can we try adding "no-rand" to our Configure invocation? Perhaps the
>> need for all of the rand_* functions goes away then.
>>
>> Thanks
>> Laszlo
>>
>>
> 
> 
> 
>