This change only partially resolves #32391 and is a follow-up to #14089.
The old randomness API has been deprecated and will be removed at some point.1
For reference on BCryptGenRandom, see: https://learn.microsoft.com/en-us/windows/win32/api/bcrypt/nf-bcrypt-bcryptgenrandom.
STATUS_SUCCESS gets defined here since including ntstatus.h is more trouble than it’s worth.
The old randomness API has been deprecated and may be removed soon.[^1]
For reference on `BCryptGenRandom`, see: https://learn.microsoft.com/en-us/windows/win32/api/bcrypt/nf-bcrypt-bcryptgenrandom.
`STATUS_SUCCESS`[^2] gets defined here since including `ntstatus.h` is
more trouble than it's worth. [^3]
[^1]: https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptacquirecontextw & https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptgenrandom
[^2]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-erref/596a1078-e883-4972-9bbc-49e60bebca55
[^3]: See https://github.com/bitcoin-core/secp256k1/blob/70f149b9a1bf4ed3266f97774d0ae9577534bf40/examples/examples_util.h#L19-L28
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32400.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
292- RandFailure();
293- }
294- ret = CryptGenRandom(hProvider, NUM_OS_RANDOM_BYTES, ent32);
295- if (!ret) {
296+ constexpr uint32_t STATUS_SUCCESS{0x00000000};
297+ NTSTATUS status = BCryptGenRandom(NULL, ent32, NUM_OS_RANDOM_BYTES, BCRYPT_USE_SYSTEM_PREFERRED_RNG);
Usage looks correct according to documentation:
hAlgorithm=NULL: required (see below)pbBuffer=ent32: “The address of a buffer that receives the random number. "cbBuffer=NUM_OS_RANDOM_BYTES: “The size, in bytes, of the pbBuffer buffer.”dwFlags=BCRYPT_USE_SYSTEM_PREFERRED_RNG: “Use the system-preferred random number generator algorithm. The hAlgorithm parameter must be NULL.”Nit: Might want to use named arguments here.
317@@ -318,6 +318,8 @@ if(WIN32)
318 else()
319 try_append_cxx_flags("-Wa,-mbig-obj" TARGET core_interface_debug SKIP_LINK)
320 endif()
321+
322+ target_link_libraries(core_interface INTERFACE bcrypt)
as expected:
0unning symbol and dynamic library checks...
1bcrypt.dll is not in ALLOWED_LIBRARIES!
bitcoin_util and bitcoinkernel targets?
in the guix-built bitcoind.exe i can still see the Crypt* functions used (objdump -p bitcoind.exe):
0 DLL Name: ADVAPI32.dll
1 vma: Hint/Ord Member-Name Bound-To
2 dcf730 1194 CryptAcquireContextA
3 dcf748 1211 CryptGenRandom
4 dcf75a 1221 CryptReleaseContext
5 dcf770 1400 InitializeSecurityDescriptor
6 dcf790 1757 SetSecurityDescriptorDacl
So some other dependency is still using them.
Edit: the culprit seems to be libevent_core.a. The current version of libevent (2.1.12-stable) uses Crypt* functions in arc4random.c. The good news is that this was replaced by bcrypt upstream in 2020 in https://github.com/libevent/libevent/commit/eb7bed03c4ba38838b48064fa870334d1bfd517c (but of course given libevent’s slow release proces…).
Edit2: i can’t actually narrow down the use of CryptReleaseContext yet. The other two functions are from libevent at least, but that function is not used there, as it uses a static provider that is never released. OK this may be a red herring as it concerns header-only libraries but boost has some references to Crypt functions too:
0./include/boost/winapi/crypt.hpp:BOOST_FORCEINLINE BOOL_ CryptReleaseContext(HCRYPTPROV_ hProv, DWORD_ dwFlags)
1./include/boost/winapi/crypt.hpp: return ::CryptReleaseContext(hProv, dwFlags);
2./include/boost/uuid/detail/random_provider_wincrypt.ipp: boost::ignore_unused(boost::winapi::CryptReleaseContext(hProv_, 0));
Edit3: yes-boost was a red herring. We actually have another use. You won’t believe it: in the __guard_setup code in mingw’s internals, used for -fstack-protector:
00000000140ab5b60 <__guard_setup>:
1 140ab5b60: 48 83 ec 48 sub $0x48,%rsp
2 140ab5b64: 48 83 3d 54 66 31 00 cmpq $0x0,0x316654(%rip) # 140dcc1c0 <__stack_chk_guard>
3 140ab5b6b: 00
4 140ab5b6c: 74 05 je 140ab5b73 <__guard_setup+0x13>
5 140ab5b6e: 48 83 c4 48 add $0x48,%rsp
6 140ab5b72: c3 ret
7 140ab5b73: 31 c0 xor %eax,%eax
8 140ab5b75: 45 31 c0 xor %r8d,%r8d
9 140ab5b78: 31 d2 xor %edx,%edx
10 140ab5b7a: c7 44 24 20 40 00 00 movl $0xf0000040,0x20(%rsp)
11 140ab5b81: f0
12 140ab5b82: 48 89 44 24 38 mov %rax,0x38(%rsp)
13 140ab5b87: 48 8d 4c 24 38 lea 0x38(%rsp),%rcx
14 140ab5b8c: 41 b9 01 00 00 00 mov $0x1,%r9d
15 140ab5b92: ff 15 50 90 31 00 call *0x319050(%rip) # 140dcebe8 <__IAT_start__>
16 140ab5b98: 85 c0 test %eax,%eax
17 140ab5b9a: 75 12 jne 140ab5bae <__guard_setup+0x4e>
18 140ab5b9c: 66 c7 05 21 66 31 00 movw $0xff0a,0x316621(%rip) # 140dcc1c6 <__stack_chk_guard+0x6>
19 140ab5ba3: 0a ff
20 140ab5ba5: c6 05 14 66 31 00 00 movb $0x0,0x316614(%rip) # 140dcc1c0 <__stack_chk_guard>
21 140ab5bac: eb c0 jmp 140ab5b6e <__guard_setup+0xe>
22 140ab5bae: 48 8b 4c 24 38 mov 0x38(%rsp),%rcx
23 140ab5bb3: ba 08 00 00 00 mov $0x8,%edx
24 140ab5bb8: 4c 8d 05 01 66 31 00 lea 0x316601(%rip),%r8 # 140dcc1c0 <__stack_chk_guard>
25 140ab5bbf: ff 15 2b 90 31 00 call *0x31902b(%rip) # 140dcebf0 <__imp_CryptGenRandom>
26 140ab5bc5: 48 8b 4c 24 38 mov 0x38(%rsp),%rcx
27 140ab5bca: 31 d2 xor %edx,%edx
28 140ab5bcc: 85 c0 test %eax,%eax
29 140ab5bce: 74 0a je 140ab5bda <__guard_setup+0x7a>
30 140ab5bd0: 48 83 3d e8 65 31 00 cmpq $0x0,0x3165e8(%rip) # 140dcc1c0 <__stack_chk_guard>
31 140ab5bd7: 00
32 140ab5bd8: 75 08 jne 140ab5be2 <__guard_setup+0x82>
33 140ab5bda: ff 15 18 90 31 00 call *0x319018(%rip) # 140dcebf8 <__imp_CryptReleaseContext>
34 140ab5be0: eb ba jmp 140ab5b9c <__guard_setup+0x3c>
35 140ab5be2: ff 15 10 90 31 00 call *0x319010(%rip) # 140dcebf8 <__imp_CryptReleaseContext>
36 140ab5be8: eb 84 jmp 140ab5b6e <__guard_setup+0xe>
There is a patch for this (from 2020): https://gcc.gnu.org/pipermail/gcc-patches/2020-June/547018.html . i don’t know if it made it into any version. Edit4: no, it’s even now in upstream gcc https://github.com/gcc-mirror/gcc/blob/master/libssp/ssp.c#L82
To be clear i’m 100% for making this change, but getting rid of use of this API completely in the release will take more follow-up work.
| File | commit 5b8046a6e893b7fad5a93631e6d1e70db31878af(master) | commit 800a5c4358d9cb0ebea09ed7a6f48901b7763ca8(pull/32400/merge) |
|---|---|---|
| *-aarch64-linux-gnu-debug.tar.gz | e794b56c3d74f5b6... |
|
| *-aarch64-linux-gnu.tar.gz | 76076b6c7ddb7a48... |
|
| *-arm-linux-gnueabihf-debug.tar.gz | e0ef0f11877b5d77... |
|
| *-arm-linux-gnueabihf.tar.gz | 1e604fd0016570ea... |
|
| *-arm64-apple-darwin-codesigning.tar.gz | 4764cf7dfe7dd172... |
|
| *-arm64-apple-darwin-unsigned.tar.gz | a60c4a7be73f72d7... |
|
| *-arm64-apple-darwin-unsigned.zip | 5c704e1ebc3bf38c... |
|
| *-powerpc64-linux-gnu-debug.tar.gz | c8eba72a13144dca... |
|
| *-powerpc64-linux-gnu.tar.gz | 26a572ca44096d04... |
|
| *-riscv64-linux-gnu-debug.tar.gz | 5d0dc5fe0ac443de... |
|
| *-riscv64-linux-gnu.tar.gz | 9946f9c4a47dd63a... |
|
| *-x86_64-apple-darwin-codesigning.tar.gz | e3aa61fb8bc8b310... |
|
| *-x86_64-apple-darwin-unsigned.tar.gz | 44414b6826f7109f... |
|
| *-x86_64-apple-darwin-unsigned.zip | e2a01ca9afd93451... |
|
| *-x86_64-linux-gnu-debug.tar.gz | 5628fc8fb34ecb0e... |
|
| *-x86_64-linux-gnu.tar.gz | 139fb6a520d84d10... |
|
| *.tar.gz | e17b7116ff138a28... |
|
| SHA256SUMS.part | ed4decabf0b96cea... |
|
| guix_build.log | ed53eebc3789c2ec... |
db1ec2a2c2f52aba... |
| guix_build.log.diff | 295331433bedc417... |