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

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

Laszlo,


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, May 14, 2019 12:15 AM
> To: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
> <xiaoyux.lu@intel.com>
> Cc: Ye, Ting <ting.ye@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.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
> >>
> >>
> >
> >
> > 
> >