Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rand_win.c may use BCryptGenRandom with BCRYPT_USE_SYSTEM_PREFERRED_RNG #2168

Closed
noloader opened this issue Jan 3, 2017 · 2 comments
Closed

Comments

@noloader
Copy link
Contributor

noloader commented Jan 3, 2017

I believe there's a few issues with rand_win.c.

First, BCryptGenRandom is called with BCRYPT_USE_SYSTEM_PREFERRED_RNG. On some version of Windows, I believe that's the infamous Dual EC generator. Apparently Microsoft has been infiltrated by the NSA, too (as if NSA_KEY was not evidence enough).

Second, BCryptGenRandom is used without a call to BCryptOpenAlgorithmProvider. I'm fairly certain using BCryptOpenAlgorithmProvider like below avoids Dual EC. Its not as convenient as using BCRYPT_USE_SYSTEM_PREFERRED_RNG (gee, what a coincidence):

#ifdef RAND_WINDOWS_USE_BCRYPT
# include <bcrypt.h>
# ifndef BCRYPT_SUCCESS
#  define BCRYPT_SUCCESS(Status) (((NTSTATUS)(Status)) >= 0)
# endif
#else
# include <wincrypt.h>
#endif
...

#if RAND_WINDOWS_USE_BCRYPT
NTSTATUS ret = ::BCryptOpenAlgorithmProvider(&m_handle,
                                             BCRYPT_RNG_ALGORITHM,
                                             MS_PRIMITIVE_PROVIDER, 0);
ASSERT(BCRYPT_SUCCESS(ret));
...
#endif

Third, the Bcrypt library defines success as (NTSTATUS)ret >= 0, and not (NTSTATUS)ret == STATUS_SUCCESS.

@Mari2411
Copy link

Mari2411 commented Jan 3, 2017

(NTSTATUS)ret >= 0, (NTSTATUS)ret

== STATUS_SUCCESS

@richsalz
Copy link
Contributor

Fixed with the recent RAND system updates.

mspncp referenced this issue Apr 2, 2019
BCryptGenRandom() is available for Windows Vista and newer versions, see
https://docs.microsoft.com/en-us/windows/desktop/api/bcrypt/nf-bcrypt-bcryptgenrandom

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #8639)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants