CryptGenRandom is deprecated #14089

pull fingera wants to merge 1 commits into bitcoin:master from fingera:2-crypto-api-deprecated changing 7 files +40 −20

fingera commented at 8:31 AM on August 28, 2018: contributor

CryptGenRandom is deprecated https://docs.microsoft.com/en-us/windows/desktop/api/wincrypt/nf-wincrypt-cryptgenrandom
fanquake added the label Windows on Aug 28, 2018

in src/random.cpp:12 in 16f6607058 outdated

   8 | @@ -9,8 +9,16 @@
   9 |  #include <support/cleanse.h>
  10 |  #ifdef WIN32
  11 |  #include <compat.h> // for Windows API
  12 | -#include <wincrypt.h>
  13 | +#if defined(_MSC_VER) && defined(_WIN32_WINNT) && _WIN32_WINNT>=0x0601
  14 | +#include <bcrypt.h>

laanwj commented at 8:37 AM on August 28, 2018:

so with this, it will still use the deprecated function with mingw? (which is used for the distributed binaries)

in src/random.cpp:14 in 16f6607058 outdated

   8 | @@ -9,8 +9,16 @@
   9 |  #include <support/cleanse.h>
  10 |  #ifdef WIN32
  11 |  #include <compat.h> // for Windows API
  12 | -#include <wincrypt.h>
  13 | +#if defined(_MSC_VER) && defined(_WIN32_WINNT) && _WIN32_WINNT>=0x0601
  14 | +#include <bcrypt.h>
  15 | +#pragma comment(lib, "bcrypt.lib")

ken2812221 commented at 9:00 AM on August 28, 2018:

You should add this into build_msvc/*/*.vcxproj and configure.ac instead of adding random library into source code. Once you update configure.ac, the #if defined(_MSC_VER)..#endif thing could be removed.

in src/random.cpp:12 in 16f6607058 outdated

   8 | @@ -9,8 +9,16 @@
   9 |  #include <support/cleanse.h>
  10 |  #ifdef WIN32
  11 |  #include <compat.h> // for Windows API
  12 | -#include <wincrypt.h>
  13 | +#if defined(_MSC_VER) && defined(_WIN32_WINNT) && _WIN32_WINNT>=0x0601

ken2812221 commented at 9:05 AM on August 28, 2018:

checking _WIN32_WINNT is not necessary. The min version required to run Bitcoin Core is Vista which is the same as the required min version of BCryptGenRandom

fingera force-pushed on Aug 29, 2018
fingera force-pushed on Aug 29, 2018
fingera force-pushed on Aug 29, 2018
fingera commented at 5:39 AM on August 29, 2018: contributor

thankyou, now the minimum system is windows vista
ken2812221 commented at 7:19 AM on August 29, 2018: contributor

Concept ACK
laanwj commented at 12:42 PM on August 29, 2018: member

utACK on superficial code changes

however, as random generation is critical to wallet security, someone with actual knowledge of windows random APIs should review this
NicolasDorier commented at 4:36 PM on August 29, 2018: contributor

I would be inclined to NACK, new version of .NET still depend on advapi32.dll CryptGenRandom for secure RNG. I would be more confident to use when microsoft eat their dog food.
NicolasDorier commented at 4:39 PM on August 29, 2018: contributor

Extract from .NET 4.7.2 released in march 2018, still using CryptGenRandom

https://referencesource.microsoft.com/#mscorlib/system/security/cryptography/rngcryptoserviceprovider.cs

https://referencesource.microsoft.com/#mscorlib/system/security/cryptography/capinative.cs,1b2672c29bebdd55
fingera commented at 6:20 AM on August 30, 2018: contributor

Windows Vista: The random number generator is based on the hash-based random number generator specified in the FIPS 186-2 standard. Windows 8: Beginning with Windows 8, the RNG algorithm supports FIPS 186-3. Keys less than or equal to 1024 bits adhere to FIPS 186-2 and keys greater than 1024 to FIPS 186-3. https://docs.microsoft.com/zh-cn/windows/desktop/SecCNG/cng-algorithm-identifiers#BCRYPT_RNG_ALGORITHM

ok, maybe switch to bcrypt after many years?
luke-jr commented at 1:10 PM on August 30, 2018: member

Could do both?
fingera commented at 1:49 AM on August 31, 2018: contributor

bitcoin random source: // First source: OpenSSL's RNG // Second source: OS RNG // Third source: HW RNG, if available.

OpenSSL's RNG already append CryptGenRandom(Compile Target < Win7) OS RNG improve it, so OS RNG use new BCryptGenRandom :)
sipa commented at 3:06 AM on August 31, 2018: member

We will in the near future probably remove OpenSSL as a randomness source.
fingera commented at 4:51 AM on August 31, 2018: contributor

Remove randomness source must be very carefull. Can I do it? What to do now?
sipa commented at 4:56 AM on August 31, 2018: member

No, we're still relying on some of its features that need to be implemented differently first.

I'm just pointing out that OpenSSL using some API already is not a reason to not also use it ourselves.
fingera commented at 4:59 AM on August 31, 2018: contributor

Add both os source now? Or wait for Microsoft's new API to accept more tests
fingera force-pushed on Aug 31, 2018
add randomness source 1e8f80d76f
fingera force-pushed on Aug 31, 2018
laanwj commented at 8:42 AM on August 31, 2018: member

I'm closing this for now. We can do this at some point in the future, but I agree with @NicolasDorier that it's not relevant now.
laanwj closed this on Aug 31, 2018
fingera commented at 9:02 AM on August 31, 2018: contributor

ok, please read 14116
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#32400 random: Use modern Windows randomness functions