Laszlo,
> -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Tuesday, May 14, 2019 12:15 AM > To: devel@edk2.groups.io; Wang, Jian J <jian.j.w...@intel.com>; Lu, XiaoyuX > <xiaoyux...@intel.com> > Cc: Ye, Ting <ting...@intel.com> > Subject: Re: [edk2-devel] [PATCH v2 5/6] CryptoPkg: Upgrade OpenSSL to 1.1.1b > > 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. > I think you're right. Current porting in tree simply returns 0. > > > 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. > I have to admit I forgot the SMM part. You're right it's complex to do. Let's forget this option at present. > 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. > Sorry about that Xiaoyu sent out update patch series before we get consensus. He misunderstood some details in the review process. (Apparently I didn't coach him well.) Actually we wanted to use hardware seed/rand generator at first. I thought it might not be acceptable due to the fact it's processor dependent. So we hesitated. My understanding to above comment is that you agree to use rdrand/rdseed if available and fall back to TimerLib otherwise, right? > 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. > Sorry again. I understand the situation here. Thanks for every effort you put in the review and test. Really appreciate it. As to the use cases, I checked all code in edk2 and found following code which call RandSeed/RandBytes interface. RandSeed is called with (NULL, 0). My understanding is this will let openssl to generate the seed. - CryptoPkg\Library\TlsLib\TlsInit.c - SecurityPkg\HddPassword\HddPasswordDxe.c For openssl itself, I found components asn1, bn, evp, ocsp, pem, pkcs7, pkcs12, rsa, ssl (in addition to cms, dsa, srp, which are disabled in edk2) will call rand_* interface. Thanks, Jian > 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...@intel.com> > >> Cc: Wang, Jian J <jian.j.w...@intel.com>; Ye, Ting <ting...@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 > >> > >> > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#40578): https://edk2.groups.io/g/devel/message/40578 Mute This Topic: https://groups.io/mt/31552212/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
