Hi Matthew Carlson Do you have any thought on the feedback below? Do you make any update in your patch V6?
> -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao, Jiewen > Sent: Saturday, August 1, 2020 8:26 AM > To: matthewfcarl...@gmail.com; devel@edk2.groups.io > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, XiaoyuX <xiaoyux...@intel.com> > Subject: Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg: OpensslLib: Use RngLib to > generate entropy in rand_pool > > Hi > I have read https://bugzilla.tianocore.org/show_bug.cgi?id=1871 > I would like to give R-B, because the code matches what described in Bugzilla. > > Before that, I would like double confirm on the randomness requirement. > According to > https://software.intel.com/content/www/us/en/develop/blogs/the-difference- > between-rdrand-and-rdseed.html, the RDSEED is a "Non-deterministic random > bit generator", while RDRAND is a "Cryptographically secure pseudorandom > number generator" > > Before this patch: > rand_pool_acquire_entropy()-> RandGetSeed128()- > >MicroSecondDelay()+RandGetBytes()->GetRandomNoise64()- > >AsmReadTsc()+MicroSecondDelay(). > rand_pool_add_nonce_data()->GetPerformanceCounter()+RandGetBytes() > It seems return TSC and TimerCounter. > > After this patch: > rand_pool_acquire_entropy()->RandGetBytes()->GetRandomNumber64()- > >AsmRdRand64(). > rand_pool_add_nonce_data()->RandGetBytes() > It becomes pseudorandom. > > So the meaning of the function seems changed. > I have not checked the randomness requirement for those two functions yet. > But could anyone confirm that a pseudorandom value returned is OK? > > Or should we use RDSEED for non-deterministic value? > > Thank you > Yao Jiewen > > > > -----Original Message----- > > From: matthewfcarl...@gmail.com <matthewfcarl...@gmail.com> > > Sent: Saturday, August 1, 2020 4:27 AM > > To: devel@edk2.groups.io > > Cc: Yao, Jiewen <jiewen....@intel.com>; Wang, Jian J > <jian.j.w...@intel.com>; > > Lu, XiaoyuX <xiaoyux...@intel.com>; Matthew Carlson > > <matthewfcarl...@gmail.com> > > Subject: [PATCH v3 1/3] CryptoPkg: OpensslLib: Use RngLib to generate > entropy > > in rand_pool > > > > From: Matthew Carlson <mac...@microsoft.com> > > > > Changes OpenSSL to no longer depend on TimerLib and instead use RngLib. > > This allows platforms to decide for themsevles what sort of entropy source > > they provide to OpenSSL and TlsLib. > > > > Cc: Jiewen Yao <jiewen....@intel.com> > > Cc: Jian J Wang <jian.j.w...@intel.com> > > Cc: Xiaoyu Lu <xiaoyux...@intel.com> > > Signed-off-by: Matthew Carlson <matthewfcarl...@gmail.com> > > --- > > CryptoPkg/Library/OpensslLib/rand_pool.c | 203 > > ++------------------ > > CryptoPkg/Library/OpensslLib/rand_pool_noise.c | 29 --- > > CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c | 43 ----- > > CryptoPkg/CryptoPkg.dsc | 1 + > > CryptoPkg/Library/OpensslLib/OpensslLib.inf | 15 +- > > CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf | 15 +- > > CryptoPkg/Library/OpensslLib/rand_pool_noise.h | 29 --- > > 7 files changed, 22 insertions(+), 313 deletions(-) > > > > diff --git a/CryptoPkg/Library/OpensslLib/rand_pool.c > > b/CryptoPkg/Library/OpensslLib/rand_pool.c > > index 9e0179b03490..b3ff03b2aa13 100644 > > --- a/CryptoPkg/Library/OpensslLib/rand_pool.c > > +++ b/CryptoPkg/Library/OpensslLib/rand_pool.c > > @@ -11,53 +11,18 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include <openssl/aes.h> > > > > > > > > #include <Uefi.h> > > > > -#include <Library/TimerLib.h> > > > > - > > > > -#include "rand_pool_noise.h" > > > > - > > > > -/** > > > > - Get some randomness from low-order bits of GetPerformanceCounter > results. > > > > - And combine them to the 64-bit value > > > > - > > > > - @param[out] Rand Buffer pointer to store the 64-bit random value. > > > > - > > > > - @retval TRUE Random number generated successfully. > > > > - @retval FALSE Failed to generate. > > > > -**/ > > > > -STATIC > > > > -BOOLEAN > > > > -EFIAPI > > > > -GetRandNoise64FromPerformanceCounter( > > > > - OUT UINT64 *Rand > > > > - ) > > > > -{ > > > > - UINT32 Index; > > > > - UINT32 *RandPtr; > > > > - > > > > - if (NULL == Rand) { > > > > - return FALSE; > > > > - } > > > > - > > > > - RandPtr = (UINT32 *) Rand; > > > > - > > > > - for (Index = 0; Index < 2; Index ++) { > > > > - *RandPtr = (UINT32) (GetPerformanceCounter () & 0xFF); > > > > - MicroSecondDelay (10); > > > > - RandPtr++; > > > > - } > > > > - > > > > - return TRUE; > > > > -} > > > > +#include <Library/RngLib.h> > > > > > > > > /** > > > > Calls RandomNumber64 to fill > > > > a buffer of arbitrary size with random bytes. > > > > + This is a shim layer to RngLib. > > > > > > > > @param[in] Length Size of the buffer, in bytes, to fill with. > > > > @param[out] RandBuffer Pointer to the buffer to store the random > > result. > > > > > > > > - @retval EFI_SUCCESS Random bytes generation succeeded. > > > > - @retval EFI_NOT_READY Failed to request random bytes. > > > > + @retval True Random bytes generation succeeded. > > > > + @retval False Failed to request random bytes. > > > > > > > > **/ > > > > STATIC > > > > @@ -73,17 +38,17 @@ RandGetBytes ( > > > > > > Ret = FALSE; > > > > > > > > + if (RandBuffer == NULL) { > > > > + DEBUG((DEBUG_ERROR, "[OPENSSL_RAND_POOL] NULL RandBuffer. No > > random numbers are generated and your system is not secure\n")); > > > > + ASSERT(FALSE); // Since we can't generate random numbers, we should > > assert. Otherwise we will just blow up later. > > > > + return Ret; > > > > + } > > > > + > > > > + > > > > while (Length > 0) { > > > > - // > > > > - // Get random noise from platform. > > > > - // If it failed, fallback to PerformanceCounter > > > > - // If you really care about security, you must override > > > > - // GetRandomNoise64FromPlatform. > > > > - // > > > > - Ret = GetRandomNoise64 (&TempRand); > > > > - if (Ret == FALSE) { > > > > - Ret = GetRandNoise64FromPerformanceCounter (&TempRand); > > > > - } > > > > + // Use RngLib to get random number > > > > + Ret = GetRandomNumber64(&TempRand); > > > > + > > > > if (!Ret) { > > > > return Ret; > > > > } > > > > @@ -100,125 +65,6 @@ RandGetBytes ( > > return Ret; > > > > } > > > > > > > > -/** > > > > - Creates a 128bit random value that is fully forward and backward > > prediction > > resistant, > > > > - suitable for seeding a NIST SP800-90 Compliant. > > > > - This function takes multiple random numbers from PerformanceCounter to > > ensure reseeding > > > > - and performs AES-CBC-MAC over the data to compute the seed value. > > > > - > > > > - @param[out] SeedBuffer Pointer to a 128bit buffer to store the random > > seed. > > > > - > > > > - @retval TRUE Random seed generation succeeded. > > > > - @retval FALSE Failed to request random bytes. > > > > - > > > > -**/ > > > > -STATIC > > > > -BOOLEAN > > > > -EFIAPI > > > > -RandGetSeed128 ( > > > > - OUT UINT8 *SeedBuffer > > > > - ) > > > > -{ > > > > - BOOLEAN Ret; > > > > - UINT8 RandByte[16]; > > > > - UINT8 Key[16]; > > > > - UINT8 Ffv[16]; > > > > - UINT8 Xored[16]; > > > > - UINT32 Index; > > > > - UINT32 Index2; > > > > - AES_KEY AESKey; > > > > - > > > > - // > > > > - // Chose an arbitrary key and zero the feed_forward_value (FFV) > > > > - // > > > > - for (Index = 0; Index < 16; Index++) { > > > > - Key[Index] = (UINT8) Index; > > > > - Ffv[Index] = 0; > > > > - } > > > > - > > > > - AES_set_encrypt_key (Key, 16 * 8, &AESKey); > > > > - > > > > - // > > > > - // Perform CBC_MAC over 32 * 128 bit values, with 10us gaps between 128 > bit > > value > > > > - // The 10us gaps will ensure multiple reseeds within the system time > > with a > > large > > > > - // design margin. > > > > - // > > > > - for (Index = 0; Index < 32; Index++) { > > > > - MicroSecondDelay (10); > > > > - Ret = RandGetBytes (16, RandByte); > > > > - if (!Ret) { > > > > - return Ret; > > > > - } > > > > - > > > > - // > > > > - // Perform XOR operations on two 128-bit value. > > > > - // > > > > - for (Index2 = 0; Index2 < 16; Index2++) { > > > > - Xored[Index2] = RandByte[Index2] ^ Ffv[Index2]; > > > > - } > > > > - > > > > - AES_encrypt (Xored, Ffv, &AESKey); > > > > - } > > > > - > > > > - for (Index = 0; Index < 16; Index++) { > > > > - SeedBuffer[Index] = Ffv[Index]; > > > > - } > > > > - > > > > - return Ret; > > > > -} > > > > - > > > > -/** > > > > - Generate high-quality entropy source. > > > > - > > > > - @param[in] Length Size of the buffer, in bytes, to fill with. > > > > - @param[out] Entropy Pointer to the buffer to store the entropy > > data. > > > > - > > > > - @retval EFI_SUCCESS Entropy generation succeeded. > > > > - @retval EFI_NOT_READY Failed to request random data. > > > > - > > > > -**/ > > > > -STATIC > > > > -BOOLEAN > > > > -EFIAPI > > > > -RandGenerateEntropy ( > > > > - IN UINTN Length, > > > > - OUT UINT8 *Entropy > > > > - ) > > > > -{ > > > > - BOOLEAN Ret; > > > > - UINTN BlockCount; > > > > - UINT8 Seed[16]; > > > > - UINT8 *Ptr; > > > > - > > > > - BlockCount = Length / 16; > > > > - Ptr = (UINT8 *) Entropy; > > > > - > > > > - // > > > > - // Generate high-quality seed for DRBG Entropy > > > > - // > > > > - while (BlockCount > 0) { > > > > - Ret = RandGetSeed128 (Seed); > > > > - if (!Ret) { > > > > - return Ret; > > > > - } > > > > - CopyMem (Ptr, Seed, 16); > > > > - > > > > - BlockCount--; > > > > - Ptr = Ptr + 16; > > > > - } > > > > - > > > > - // > > > > - // Populate the remained data as request. > > > > - // > > > > - Ret = RandGetSeed128 (Seed); > > > > - if (!Ret) { > > > > - return Ret; > > > > - } > > > > - CopyMem (Ptr, Seed, (Length % 16)); > > > > - > > > > - return Ret; > > > > -} > > > > - > > > > /* > > > > * Add random bytes to the pool to acquire requested amount of entropy > > > > * > > > > @@ -238,7 +84,7 @@ size_t rand_pool_acquire_entropy(RAND_POOL *pool) > > buffer = rand_pool_add_begin(pool, bytes_needed); > > > > > > > > if (buffer != NULL) { > > > > - Ret = RandGenerateEntropy(bytes_needed, buffer); > > > > + Ret = RandGetBytes(bytes_needed, buffer); > > > > if (FALSE == Ret) { > > > > rand_pool_add_end(pool, 0, 0); > > > > } else { > > > > @@ -257,13 +103,8 @@ size_t rand_pool_acquire_entropy(RAND_POOL > *pool) > > */ > > > > int rand_pool_add_nonce_data(RAND_POOL *pool) > > > > { > > > > - struct { > > > > - UINT64 Rand; > > > > - UINT64 TimerValue; > > > > - } data = { 0 }; > > > > - > > > > - RandGetBytes(8, (UINT8 *)&(data.Rand)); > > > > - data.TimerValue = GetPerformanceCounter(); > > > > + UINT8 data[16]; > > > > + RandGetBytes(sizeof(data), data); > > > > > > > > return rand_pool_add(pool, (unsigned char*)&data, sizeof(data), 0); > > > > } > > > > @@ -275,13 +116,8 @@ int rand_pool_add_nonce_data(RAND_POOL *pool) > > */ > > > > int rand_pool_add_additional_data(RAND_POOL *pool) > > > > { > > > > - struct { > > > > - UINT64 Rand; > > > > - UINT64 TimerValue; > > > > - } data = { 0 }; > > > > - > > > > - RandGetBytes(8, (UINT8 *)&(data.Rand)); > > > > - data.TimerValue = GetPerformanceCounter(); > > > > + UINT8 data[16]; > > > > + RandGetBytes(sizeof(data), data); > > > > > > > > return rand_pool_add(pool, (unsigned char*)&data, sizeof(data), 0); > > > > } > > > > @@ -313,4 +149,3 @@ void rand_pool_cleanup(void) > > void rand_pool_keep_random_devices_open(int keep) > > > > { > > > > } > > > > - > > > > diff --git a/CryptoPkg/Library/OpensslLib/rand_pool_noise.c > > b/CryptoPkg/Library/OpensslLib/rand_pool_noise.c > > deleted file mode 100644 > > index 212834e27acc..000000000000 > > --- a/CryptoPkg/Library/OpensslLib/rand_pool_noise.c > > +++ /dev/null > > @@ -1,29 +0,0 @@ > > -/** @file > > > > - Provide rand noise source. > > > > - > > > > -Copyright (c) 2019, Intel Corporation. All rights reserved.<BR> > > > > -SPDX-License-Identifier: BSD-2-Clause-Patent > > > > - > > > > -**/ > > > > - > > > > -#include <Library/BaseLib.h> > > > > - > > > > -/** > > > > - Get 64-bit noise source > > > > - > > > > - @param[out] Rand Buffer pointer to store 64-bit noise source > > > > - > > > > - @retval FALSE Failed to generate > > > > -**/ > > > > -BOOLEAN > > > > -EFIAPI > > > > -GetRandomNoise64 ( > > > > - OUT UINT64 *Rand > > > > - ) > > > > -{ > > > > - // > > > > - // Return FALSE will fallback to use PerformanceCounter to > > > > - // generate noise. > > > > - // > > > > - return FALSE; > > > > -} > > > > diff --git a/CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c > > b/CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c > > deleted file mode 100644 > > index 4158106231fd..000000000000 > > --- a/CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c > > +++ /dev/null > > @@ -1,43 +0,0 @@ > > -/** @file > > > > - Provide rand noise source. > > > > - > > > > -Copyright (c) 2019, Intel Corporation. All rights reserved.<BR> > > > > -SPDX-License-Identifier: BSD-2-Clause-Patent > > > > - > > > > -**/ > > > > - > > > > -#include <Library/BaseLib.h> > > > > -#include <Library/DebugLib.h> > > > > -#include <Library/TimerLib.h> > > > > - > > > > -/** > > > > - Get 64-bit noise source > > > > - > > > > - @param[out] Rand Buffer pointer to store 64-bit noise source > > > > - > > > > - @retval TRUE Get randomness successfully. > > > > - @retval FALSE Failed to generate > > > > -**/ > > > > -BOOLEAN > > > > -EFIAPI > > > > -GetRandomNoise64 ( > > > > - OUT UINT64 *Rand > > > > - ) > > > > -{ > > > > - UINT32 Index; > > > > - UINT32 *RandPtr; > > > > - > > > > - if (NULL == Rand) { > > > > - return FALSE; > > > > - } > > > > - > > > > - RandPtr = (UINT32 *)Rand; > > > > - > > > > - for (Index = 0; Index < 2; Index ++) { > > > > - *RandPtr = (UINT32) ((AsmReadTsc ()) & 0xFF); > > > > - RandPtr++; > > > > - MicroSecondDelay (10); > > > > - } > > > > - > > > > - return TRUE; > > > > -} > > > > diff --git a/CryptoPkg/CryptoPkg.dsc b/CryptoPkg/CryptoPkg.dsc > > index 1af78468a19c..0490eeb7e22f 100644 > > --- a/CryptoPkg/CryptoPkg.dsc > > +++ b/CryptoPkg/CryptoPkg.dsc > > @@ -60,6 +60,7 @@ > > BaseCryptLib|CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf > > > > TlsLib|CryptoPkg/Library/TlsLibNull/TlsLibNull.inf > > > > HashApiLib|CryptoPkg/Library/BaseHashApiLib/BaseHashApiLib.inf > > > > + RngLib|MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf > > > > > > > > [LibraryClasses.ARM, LibraryClasses.AARCH64] > > > > # > > > > diff --git a/CryptoPkg/Library/OpensslLib/OpensslLib.inf > > b/CryptoPkg/Library/OpensslLib/OpensslLib.inf > > index dbbe5386a10c..4baad565564c 100644 > > --- a/CryptoPkg/Library/OpensslLib/OpensslLib.inf > > +++ b/CryptoPkg/Library/OpensslLib/OpensslLib.inf > > @@ -571,22 +571,9 @@ > > $(OPENSSL_PATH)/ssl/statem/statem_local.h > > > > # Autogenerated files list ends here > > > > buildinf.h > > > > - rand_pool_noise.h > > > > ossl_store.c > > > > rand_pool.c > > > > > > > > -[Sources.Ia32] > > > > - rand_pool_noise_tsc.c > > > > - > > > > -[Sources.X64] > > > > - rand_pool_noise_tsc.c > > > > - > > > > -[Sources.ARM] > > > > - rand_pool_noise.c > > > > - > > > > -[Sources.AARCH64] > > > > - rand_pool_noise.c > > > > - > > > > [Packages] > > > > MdePkg/MdePkg.dec > > > > CryptoPkg/CryptoPkg.dec > > > > @@ -594,7 +581,7 @@ > > [LibraryClasses] > > > > BaseLib > > > > DebugLib > > > > - TimerLib > > > > + RngLib > > > > PrintLib > > > > > > > > [LibraryClasses.ARM] > > > > diff --git a/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf > > b/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf > > index 616ccd9f62d1..3557711bd85a 100644 > > --- a/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf > > +++ b/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf > > @@ -520,22 +520,9 @@ > > $(OPENSSL_PATH)/crypto/x509v3/v3_admis.h > > > > # Autogenerated files list ends here > > > > buildinf.h > > > > - rand_pool_noise.h > > > > ossl_store.c > > > > rand_pool.c > > > > > > > > -[Sources.Ia32] > > > > - rand_pool_noise_tsc.c > > > > - > > > > -[Sources.X64] > > > > - rand_pool_noise_tsc.c > > > > - > > > > -[Sources.ARM] > > > > - rand_pool_noise.c > > > > - > > > > -[Sources.AARCH64] > > > > - rand_pool_noise.c > > > > - > > > > [Packages] > > > > MdePkg/MdePkg.dec > > > > CryptoPkg/CryptoPkg.dec > > > > @@ -543,7 +530,7 @@ > > [LibraryClasses] > > > > BaseLib > > > > DebugLib > > > > - TimerLib > > > > + RngLib > > > > PrintLib > > > > > > > > [LibraryClasses.ARM] > > > > diff --git a/CryptoPkg/Library/OpensslLib/rand_pool_noise.h > > b/CryptoPkg/Library/OpensslLib/rand_pool_noise.h > > deleted file mode 100644 > > index 75acc686a9f1..000000000000 > > --- a/CryptoPkg/Library/OpensslLib/rand_pool_noise.h > > +++ /dev/null > > @@ -1,29 +0,0 @@ > > -/** @file > > > > - Provide rand noise source. > > > > - > > > > -Copyright (c) 2019, Intel Corporation. All rights reserved.<BR> > > > > -SPDX-License-Identifier: BSD-2-Clause-Patent > > > > - > > > > -**/ > > > > - > > > > -#ifndef __RAND_POOL_NOISE_H__ > > > > -#define __RAND_POOL_NOISE_H__ > > > > - > > > > -#include <Uefi/UefiBaseType.h> > > > > - > > > > -/** > > > > - Get 64-bit noise source. > > > > - > > > > - @param[out] Rand Buffer pointer to store 64-bit noise source > > > > - > > > > - @retval TRUE Get randomness successfully. > > > > - @retval FALSE Failed to generate > > > > -**/ > > > > -BOOLEAN > > > > -EFIAPI > > > > -GetRandomNoise64 ( > > > > - OUT UINT64 *Rand > > > > - ); > > > > - > > > > - > > > > -#endif // __RAND_POOL_NOISE_H__ > > > > -- > > 2.27.0.windows.1 > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64223): https://edk2.groups.io/g/devel/message/64223 Mute This Topic: https://groups.io/mt/75915446/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
