Clean up Windows RNG #1079
Clean up Windows RNG #1079
Conversation
|
nmake test passes |
|
@mattcaswell is this PR okay? |
crypto/rand/rand_win.c
Outdated
There was a problem hiding this comment.
I'm wondering whether we should retain RAND_event(), and just have it call RAND_poll() (ignoring all params), and return RAND_status()? i.e. for backwards compat.
There was a problem hiding this comment.
I think this is a good time to make a clean break. But keeping them as deprecated would be a reasonable option.
There was a problem hiding this comment.
Yes, I think we keep them but deprecated (using the OPENSSL_API_COMPAT macro as @richsalz suggests), and all they do is call RAND_poll/RAND_status.
There was a problem hiding this comment.
So
if OPENSSL_API_COMPAT < 0x10100000L
?
There was a problem hiding this comment.
Yes. And wrap the header file declarations in DEPRECATEDIN_1_1_0()
There was a problem hiding this comment.
Oh...which also means the documentation needs to change too :-)
There was a problem hiding this comment.
Oh...which also means the documentation needs to change too :-)
I noted that too here:
Line 167 in 1dc1ea1
This change means gdi32.dll is no longer needed?
|
Yes, this PR looks a lot better. I just have a few minor comments. |
|
For the sake of maintaining the history, this PR replaces #512. |
|
Ok, functions added back as deprecated in header/source/docs. nmake test still passes. |
doc/crypto/RAND_add.pod
Outdated
There was a problem hiding this comment.
No, it returns RAND_status().
|
Just done a test build and everything looks good. One final thing I spotted. libcrypto.num needs updating to mark RAND_event() and RAND_screen() as DEPRECATEDIN_1_1_0 |
|
Done @mattcaswell |
|
+1 |
|
Needs a second team review now. |
… calling conventions
|
For reference, the same issue has been raised in Go: Firefox has asked Microsoft for clarification a while ago: If the argument is not to use "deprecated" functions, at least consider using BCryptGenRandom (with BCRYPT_USE_SYSTEM_PREFERRED_RNG) on Windows 7 and higher as CryptoAPI itself is a legacy API (superseded by NCrypt/BCrypt). Also for reference, the following projects have decided against the CryptoAPI:
It's clear that CryptGenRandom has an unnecessary overhead (and dependency) which can be avoided by using either BCryptGenRandom or RtlGenRandom. |
|
Another part of this PR was removing all the #ifdef code for various windows platforms. I don't think adding another for Win7 and greater is a good fit for this change. CryptoAPI may be legacy, but it's not deprecated, so I think it's the appropriate choice here. We're only grabbing 32 bytes from it anyway. If this was being called all the time then tiny changes in performance would be worth increasing code complexity. But I just don't see the tradeoff here. |
|
Agree with @xoloki. It's part of the official API, this cleans up things a great deal, and is portable across all Windows platforms. Perhaps in the next release, when we drop XP and other old platforms, we can look at BCryptoGenRandom. Also, doesn't this address a couple of open RT tickets? +1 |
|
+1 in its current form ... |
|
So, want me to merge? |
… calling conventions Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tim Hudson <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #1079)
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tim Hudson <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #1079)
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tim Hudson <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #1079)
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tim Hudson <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #1079)
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tim Hudson <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #1079)
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tim Hudson <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #1079)
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tim Hudson <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #1079)
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tim Hudson <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #1079)
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tim Hudson <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #1079)
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tim Hudson <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #1079)
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tim Hudson <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #1079)
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tim Hudson <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #1079)
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tim Hudson <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #1079)
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tim Hudson <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #1079)
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tim Hudson <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #1079)
|
thanks for your efforts! merged!! |
|
Thanks so much @mattcaswell, @levitte, and @richsalz! |
|
any chance to have this causing problems on linux? |
|
This is all in code that doesn't even get compiled on linux, so there should be no impacts. |
GlobalMemoryStatus is deprecated and may return incorrect data (https://msdn.microsoft.com/en-us/library/windows/desktop/aa366586(v=vs.85).aspx). Since we already use `BCryptGenRandom` or `BCryptGenRandom`, I guess we can remove memory status and process ID as sources of randomness? openssl#1079 removed a lot of code that manually gathers entropy. I think based on the same logic, we can remove these two as well.
11 participants
This PR removes all of the dangerous Windows entropy gathering routines in favor of standard CryptGenRandom calls, as was discussed in the "Improving OpenSSL default RNG" thread on openssl-dev. This fixes common, repeatable crashes that happen when running openssl under the VS debugger.