CryptGenRandom is deprecated #14089

1. 
   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
2. fanquake added the label Windows on Aug 28, 2018
3. 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)
4. 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.
5. 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
6. fingera force-pushed on Aug 29, 2018
7. fingera force-pushed on Aug 29, 2018
8. fingera force-pushed on Aug 29, 2018
9. 
   fingera commented at 5:39 am on August 29, 2018: contributor

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

    Concept ACK
11. 
    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

12. 
    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.
13. 
    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

14. 
    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?

15. 
    luke-jr commented at 1:10 pm on August 30, 2018: member

    Could do both?
16. 
    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 :)

17. 
    sipa commented at 3:06 am on August 31, 2018: member

    We will in the near future probably remove OpenSSL as a randomness source.
18. 
    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?
19. 
    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.

20. 
    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
21. fingera force-pushed on Aug 31, 2018
22. add randomness source 1e8f80d76f
23. fingera force-pushed on Aug 31, 2018
24. 
    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.
25. laanwj closed this on Aug 31, 2018

    ---

26. 
    fingera commented at 9:02 am on August 31, 2018: contributor

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

Contributors
fingera laanwj ken2812221 NicolasDorier luke-jr sipa

Labels
Windows