windows: improve random source

[vszakats]

• Use the Windows API to seed the fallback random generator.

  This ensures to always have a random seed, even when libcurl is built
  with a vtls backend lacking a random generator API, such as rustls
  (experimental), GSKit and certain mbedTLS builds, or, when libcurl is
  built without a TLS backend. We reuse the Windows-specific random
  function from the Schannel backend.

• Implement support for BCryptGenRandom() [1] on Windows, as a
  replacement for the deprecated CryptGenRandom() [2] function.

  It is used as the secure random generator for Schannel, and also to
  provide entropy for libcurl's fallback random generator. The new
  function is supported on Vista and newer via its bcrypt.dll. It is
  used automatically when building for supported versions. It also works
  in UWP apps (the old function did not).

• Clear entropy buffer before calling the Windows random generator.

  This avoids using arbitrary application memory as entropy (with
  CryptGenRandom()) and makes sure to return in a predictable state
  when an API call fails.

[1] https://docs.microsoft.com/windows/win32/api/bcrypt/nf-bcrypt-bcryptgenrandom
[2] https://docs.microsoft.com/windows/win32/api/wincrypt/nf-wincrypt-cryptgenrandom

Closes #xxxx

[jay]

I'm almost sure we intentionally didn't clear the memory passed to CryptGenRandom because we wanted the arbitrary data to be used as a seed. "The CSP then uses this data to further randomize its internal seed." In other words
int x;
curl_win32_random(&x, sizeof x); // whatever is in x is used as an auxillary seed

[vszakats]

Looking at OpenSSL's code, they do clear the buffer (either with secure zero or just zmalloc, depending on config). I replicated that here. Passing an arbitrary uninitialized buffer to an external API may pass internal app or curl data, including secrets with it, which may not be the best.

UPDATE: Source code link added.

[jay]

Yes whatever is in memory is used to further randomize the seed so in my view it should be better not worse to do it that way. For example xor of a and b if a is random then the result cannot be less random regardless of b. b can only help. I think we could use some other opinions on this.

[bagder]

I'm reading the docs for CryptGenRandom which says

If an application has access to a good random source, it can fill the pbBuffer buffer with some random data before calling CryptGenRandom

Leaving just random data on the stack in there is quite far from a good random source but instead rather risk to be the same in every call.

But their own example then proceeds and uses the uninitialized buffer as input for the function, maybe implicating that they think that's the way to do it?

[vszakats]

I don't think their example is an indication of good practice here. CryptGenRandom() is now a deprecated function. The replacement function is BCryptGenRandom() ¹. The new function does not use the buffer for entropy by default. According to the documentation, it uses a "random number" instead. Caller has to explicitly pass the flag BCRYPT_RNG_USE_ENTROPY_IN_BUFFER to enable old behavior. But: Windows 8 and newer ignore this flag, which implies that this feature is abandoned on modern Windows and the buffer is never used for entropy, no matter what arguments are passed.

To me this tells that Microsoft also recognized this to be a bad idea, and the example code is a leftover from the past.

Footnotes

1. https://docs.microsoft.com/windows/win32/api/bcrypt/nf-bcrypt-bcryptgenrandom ↩

[bagder]

I'm totally fine with clearing it before use. I don't think leaving stack content there helps.

[vszakats]

In the meantime I'm preparing a patch adding support for BCryptGenRandom() in rand.c. This depends on a new DLL, bcrypt. It's generally harmless and exists on XP too. It's already a dependency of libssh2 when built with Schannel. Yet, this will be a "new-feature-window" candidate, due to necessary adjustments in some build systems.

Pre-clearing the buffer also applies to this case, also to ensure no garbage there when the API fails.

I've also enabled BCryptGenRandom() in OpenSSL for the next curl-for-win build.

UPDATE: added link to BCryptGenRandom() patch.

[jay]

Any build system that targets xp or later will need crypt32 lib included in the lib list. i've updated the premade project templates to handle that (see suggested commit i made in your branch) , you will need to investigate if other build systems need it. if cryptgenrandom is xp minimum then there may need to be some accommodation for older build systems, unfortunately pre xp doc is sparse.

Regarding the memset issue in previous review I still don't think it was a bad idea but I won't insist. When we switch to BCrypt* functions it also seems unnecessary but for a different reason, if the function returns success then it was successful, so there's no reason to clear the memory beforehand.

[jay]

Suggested change

	CURLcode curl_win32_random(unsigned char *entropy, size_t length);
	CURLcode Curl_win32_random(unsigned char *entropy, size_t length);

and move to rand.h then include rand.h in schannel.c

[vszakats]

Did the rename, thanks. I tried to investigate this beforehand, but the link from docs/CODE_STYLE.md, https://curl.se/dev/internals.html#symbols, misses most of the information since d324ac8, incuding the symbols sections and refers to Everything Curl. But could not find "naming things" info in the book.

As for copying the declaration from a common header to each place of use. What is the reason we do this?

[vszakats]

@jay: As for crypt32 advapi32. Its lib and CryptGenRandom() was available way back to Win95, and also WinCE. With one gotcha: the Unicode version CryptAcquireContextW() was not present in Win95, only CryptAcquireContextA() was. So if Win95-compatibility is important, we can force using the "ANSI" version, unconditionally even, as none of the exchanged values are Unicode strings:

#if !defined(UNDER_CE) && !defined(__CEGCC__) && !defined(__MINGW32CE__)  /* if WinCE-compatibility is a goal also */
  #undef CryptAcquireContext
  #define CryptAcquireContext CryptAcquireContextA
#endif

To make MSVC support simpler and make it automatically apply to all external MSVC build environments, pulling in crypt32 advapi32 can be solved by adding this to rand.c:

#ifdef WIN32
#include <wincrypt.h>
#ifdef _MSC_VER
#  pragma comment(lib, "advapi32.lib")
#endif
#endif

What do you think?

[bagder]

As for copying the declaration from a common header to each place of use. What is the reason we do this?

That should only be done if we for some reason can't just include the main header file with the declaration.

[jay]

It should be documented that we use Curl_ for internally shared functions and curl_ publicly but I can't find it. @bagder?

Generally speaking each unit should have its own header (eg rand.c has rand.h) and that header should contain its Curl_ functions. (There are some exceptions to this but generally it's true). If we put everything in a big header it would be messy, harder to manage and slow things down to include everything when you don't need everything. This way only units that need Curl_ functions from rand.c include rand.h.

[bagder]

I mistakenly removed that section in a previous cleanup, coming back in #9037

[vszakats]

In effect this update makes USE_WIN32_CRYPTO mandatory on Windows. Which seems safe to me, but it triggers a series of updates. I'm wondering if tearing off this change from this patch would be a better way, and committing it in next feature window, together with the bcrypt patch, which touches quite the same areas.

I've created a new PR #9038 for the /dev/urandom parts (which are tiny), so we can continue with this into the next feature window.

Unless there is an objection, I'll merge the bcrypt patch to this PR.

[vszakats]

Dunno, Curl_win32_random() will probably not be used anywhere else than rand.c and schannel.c in the future, so the extra maintenance burden of maintaining copies of the declaration is small, but I still feel uncomfortable duplicating declarations, and we also have a common header that's included by both.

So what is our common vote on this? (I'm neutral)

[vszakats]

I subtracted the /dev/urandom hunks and merged separately, rebased on that and added the BCryptGenRandom() patch to this PR. Also added curl_ → Curl_ rename, Makefile.m32 and CMake lib updates, and the #pragma to pull crypt32.lib for MSVC.

Pending question/TODO items:

• Vote to duplicate-or-not Curl_win32_random() declarations. @bagder @jay [left as-is until further input] [moved to rand.h.
• Is the #pragma method enough (for bcrypt.lib and advapi32.lib), or do we need the updates for each MSVC build file? @jay [CI tests are passing, suggesting it works?]
• Decide if we want to clear entropy buffer on API failure to be extra sure the APIs didn't put garbage there. [my vote: most likely not necessary.]
• Update autotools to add bcrypt unconditionally on native Windows.
• Decide if we want Win95 compatibility with CryptAcquireContext(). [my vote: NO]
• Decide if we want WinCE compatibility when resolving the above. [my vote: NO, curl doesn't show signs of WinCE support] [UPDATE: actually it does, but unless we also want Win95 compatibilty, this patch should not break existing WinCE support.]
• Existing USE_WIN32_CRYPTO option is now required to build libcurl, so related detection/conditions can now be deleted. Also note that crypt32 is practically already a requirement for curl, being a dependency of at least Schannel and OpenSSL, and possibly other TLS backends, and also of libgsasl (which uses CryptGenRandom() a little bit wrong by missing the CRYPT_SILENT flag, and also leaking arbitrary memory via the passed buffer /cc @jas4711). bcrypt is already a dependency of LibreSSL. [my vote: leave it there for now and replace with WIN32 checks later.] [UPDATE: It should stay. It is actively used to differenciate Windows App (aka UWP) env from Win32.]

[jay]

move to rand.h

[jay]

If someone sets their build target to minimum XP we want curl to still build so as long as that's possible it should be fine.

is there a realistic userbase of old mingw? It seems to be causing a disproportionally large amount of effort to keep up.
mingw-w64 is a free upgrade.

I don't know and that isn't something asked in the surveys but maybe it should be. I had some builds that used it but I stopped them after I wasn't able to build OpenSSL 1.1.1.

[bagder]

is there a realistic userbase of old mingw

I don't think it would be unreasonable for us to drop support for it.

[mback2k]

So bcrypt would become an unconditional requirement to build curl? Wouldn't that mean dropping Windows XP support?

[mback2k]

So bcrypt would become an unconditional requirement to build libcurl? Wouldn't that mean dropping Windows XP support?

[vszakats]

It only requires bcrypt at build-time. So the toolchain/SDK should be Vista-compatible. But if you build with _WIN32_WINNT=0x0501 (=XP), it will continue to run on XP, because no bcrypt exports will be referenced (and thus the lib not linked) in that case.

[vszakats]

Just did an x86 test for XP and now I'm not sure. Investigating...

[vszakats]

So, I re-did the test above accurately, and it confirms that even though bcrypt is present in the lib list, it WILL NOT be linked to the final binary, when building for XP (ie. -D_WIN32_WINNT=0x0501). The default target with modern toolchains may not be XP anymore, in such case XP should be targeted manually.

log output (with no dependencies, Makefile.m32, x86, -D_WIN32_WINNT=0x0501, LLVM/clang + mingw-w64, GNU ld):

[...]
clang -I. -I../include -O3 --target=i686-w64-mingw32 --sysroot=/usr/local/opt/mingw-w64/toolchain-i686 -fno-ident -DCURL_STATICLIB -DHAVE_ATOMIC -DHAVE_SIGNAL -DNDEBUG -DHAVE_STRTOK_R -DUSE_HEADERS_API -DHAVE_BOOL_T -DHAVE_STDBOOL_H -DHAVE_STRING_H -DHAVE_SETJMP_H -DHAVE_FTRUNCATE -D_FILE_OFFSET_BITS=64 -DSIZEOF_OFF_T=8 -DHAVE_LIBGEN_H -DHAVE_BASENAME -fno-asynchronous-unwind-tables -D_WIN32_WINNT=0x0501 -DCURL_DISABLE_ALTSVC=1 -DCURL_DISABLE_CRYPTO_AUTH=1 -DCURL_DISABLE_DICT=1 -DCURL_DISABLE_FILE=1 -DCURL_DISABLE_GOPHER=1 -DCURL_DISABLE_MQTT=1 -DCURL_DISABLE_RTSP=1 -DCURL_DISABLE_SMB=1 -DCURL_DISABLE_TELNET=1 -DCURL_DISABLE_TFTP=1 -DCURL_DISABLE_FTP=1 -DCURL_DISABLE_IMAP=1 -DCURL_DISABLE_POP3=1 -DCURL_DISABLE_SMTP=1 -DCURL_DISABLE_LDAP=1 -DCURL_DISABLE_LDAPS=1 -DHAS_ALPN -W -Wall -m32 -DBUILDING_LIBCURL -DUSE_SCHANNEL -DUSE_WINDOWS_SSPI -DENABLE_IPV6 -c rand.c -o rand.o
[...]
clang --target=i686-w64-mingw32 --sysroot=/usr/local/opt/mingw-w64/toolchain-i686 -static-libgcc -Wl,--nxcompat -Wl,--dynamicbase  -Wl,-Map,libcurl.map ../libcurl.def -m32 -shared -o libcurl.dll \
	  -Wl,--output-def,libcurl.def,--out-implib,libcurl.dll.a \
	  altsvc.o amigaos.o asyn-ares.o asyn-thread.o base64.o bufref.o c-hyper.o conncache.o connect.o content_encoding.o cookie.o curl_addrinfo.o curl_ctype.o curl_des.o curl_endian.o curl_fnmatch.o curl_get_line.o curl_gethostname.o curl_gssapi.o curl_memrchr.o curl_multibyte.o curl_ntlm_core.o curl_ntlm_wb.o curl_path.o curl_range.o curl_rtmp.o curl_sasl.o curl_sspi.o curl_threads.o dict.o doh.o dotdot.o dynbuf.o easy.o easygetopt.o easyoptions.o escape.o file.o fileinfo.o formdata.o ftp.o ftplistparser.o getenv.o getinfo.o gopher.o h2h3.o hash.o headers.o hmac.o hostasyn.o hostip.o hostip4.o hostip6.o hostsyn.o hsts.o http.o http2.o http_chunks.o http_digest.o http_negotiate.o http_ntlm.o http_proxy.o http_aws_sigv4.o idn_win32.o if2ip.o imap.o inet_ntop.o inet_pton.o krb5.o ldap.o llist.o md4.o md5.o memdebug.o mime.o mprintf.o mqtt.o multi.o netrc.o nonblock.o openldap.o parsedate.o pingpong.o pop3.o progress.o psl.o rand.o rename.o rtsp.o select.o sendf.o setopt.o sha256.o share.o slist.o smb.o smtp.o socketpair.o socks.o socks_gssapi.o socks_sspi.o speedcheck.o splay.o strcase.o strdup.o strerror.o strtok.o strtoofft.o system_win32.o telnet.o tftp.o timediff.o timeval.o transfer.o url.o urlapi.o version.o version_win32.o warnless.o wildcard.o vauth/cleartext.o vauth/cram.o vauth/digest.o vauth/digest_sspi.o vauth/gsasl.o vauth/krb5_gssapi.o vauth/krb5_sspi.o vauth/ntlm.o vauth/ntlm_sspi.o vauth/oauth2.o vauth/spnego_gssapi.o vauth/spnego_sspi.o vauth/vauth.o vtls/bearssl.o vtls/gskit.o vtls/gtls.o vtls/hostcheck.o vtls/keylog.o vtls/mbedtls.o vtls/mbedtls_threadlock.o vtls/nss.o vtls/openssl.o vtls/rustls.o vtls/schannel.o vtls/schannel_verify.o vtls/sectransp.o vtls/vtls.o vtls/wolfssl.o vtls/x509asn1.o vquic/msh3.o vquic/ngtcp2.o vquic/quiche.o vquic/vquic.o vssh/libssh.o vssh/libssh2.o vssh/wolfssh.o libcurl.res -lcrypt32 -lwldap32 -lws2_32 -lbcrypt
[...]
clang --target=i686-w64-mingw32 --sysroot=/usr/local/opt/mingw-w64/toolchain-i686 -static-libgcc -Wl,--nxcompat -Wl,--dynamicbase -Wl,--pic-executable,-e,_mainCRTStartup -Wl,-Map,curl.map -m32 -static -o curl.exe curl.res slist_wc.o tool_binmode.o tool_bname.o tool_cb_dbg.o tool_cb_hdr.o tool_cb_prg.o tool_cb_rea.o tool_cb_see.o tool_cb_wrt.o tool_cfgable.o tool_dirhie.o tool_doswin.o tool_easysrc.o tool_filetime.o tool_findfile.o tool_formparse.o tool_getparam.o tool_getpass.o tool_help.o tool_helpers.o tool_hugehelp.o tool_libinfo.o tool_listhelp.o tool_main.o tool_msgs.o tool_operate.o tool_operhlp.o tool_panykey.o tool_paramhlp.o tool_parsecfg.o tool_progress.o tool_strdup.o tool_setopt.o tool_sleep.o tool_urlglob.o tool_util.o tool_vms.o tool_writeout.o tool_writeout_json.o tool_xattr.o strtoofft.o timediff.o nonblock.o warnless.o curl_ctype.o curl_multibyte.o version_win32.o dynbuf.o -L../lib -lcurl -lcrypt32 -lwldap32 -lws2_32 -lbcrypt
[...]
+ grep -a -E -i '(file format|dll name)'
+ i686-w64-mingw32-objdump --all-headers ./src/curl.exe
./src/curl.exe:     file format pei-i386
	DLL Name: ADVAPI32.dll
	DLL Name: CRYPT32.dll
	DLL Name: KERNEL32.dll
	DLL Name: msvcrt.dll
	DLL Name: WS2_32.dll
+ grep -a -E -i '(file format|dll name)'
+ i686-w64-mingw32-objdump --all-headers ./lib/libcurl.dll
./lib/libcurl.dll:     file format pei-i386
	DLL Name: ADVAPI32.dll
	DLL Name: CRYPT32.dll
	DLL Name: KERNEL32.dll
	DLL Name: msvcrt.dll
	DLL Name: WS2_32.dll

[bagder]

The default target with modern toolchains may not be XP anymore, in such case XP should be targeted manually.

That seems totally reasonable to me. The amount of people building curl for XP in this day and age is bound to be minuscule.

[vszakats]

As for the requirement for a Vista-capable toolchain/SDK, according to this list, curl will not build anymore with Visual Studio .NET 2003, and now requires Visual C++ 2005. The oldest version supported via native build files in curl is Visual Studio 2010. Compilers compatible with an official post-Vista SDK should also work (Intel C++ maybe?).

For mingw-w64, I'm not sure of the exact date, but upgrading those to a newer version is usually not a big problem and free. Original MinGW still works with autotools (thanks to the dedicated detection in this patch).

As for other toolchains (POCC? former Borland C?), I haven't followed them for a while, but I'm guessing anything that is regularly updated should have received Vista support by now.

LibreSSL and OpenSSL also use bcrypt, and even libssh2 when built with the Schannel (aka WinCNG) backend.

[jay]

As far as winbuild and pregenerated Visual Studio project files go because bcrypt is only included by pragma when targeting vista or later I don't think it should be a problem.

[vszakats]

Looked-up in mingw-w64 and it turns out that v2.0.0 was the first to support bcrypt.h, released on 2011-10-10. (v3.0.0 from 2013-09-20 introduced BCRYPT_USE_SYSTEM_PREFERRED_RNG, but the patch works without this macro.)

Ref: https://sourceforge.net/projects/mingw-w64/files/mingw-w64/mingw-w64-release/

[mback2k]

Same as my other 2 comments regarding bcrypt becoming an unconditional requirement to build curl.