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

Need NonblockingRng based on BCryptGenRandom for Windows #164

Closed
noloader opened this issue Apr 26, 2016 · 18 comments
Closed

Need NonblockingRng based on BCryptGenRandom for Windows #164

noloader opened this issue Apr 26, 2016 · 18 comments

Comments

@noloader
Copy link
Collaborator

noloader commented Apr 26, 2016

BCryptGenRandom is needed to provide a generator for Windows Store Apps (its also available for Windows Vista and above on the desktop). Docs are available at BCryptGenRandom function.

This is part of the port to the new managed Windows platforms. Also see Issue 143: Support for Universal Windows Platform (UWP).

Also see Native code for Windows Phone 8 on MSDN, Supported Win32 APIs for Windows Phone 8 on MSDN, Conditional compilation with preprocessor directives on MSDN, Platform defined macros for windows store app on Microsoft Social and Dual-use Coding Techniques for Games.

@noloader noloader changed the title Need BlockingRNG based on BCryptGenRandom for Windows Need NonblockingRng based on BCryptGenRandom for Windows Apr 26, 2016
@DevJPM
Copy link
Contributor

DevJPM commented Apr 26, 2016

The fix should be a simple conditional include of <bcrypt.h> in osrng.cpp paired with a conditional call to BCryptGenRandom(NULL,output,(ULONG)size,BCRYPT_USE_SYSTEM_PREFERRED_RNG) in void NonblockingRng::GenerateBlock(byte *output, size_t size) that throws on error (e.g. != 0) just like our CryptGenRandom() call.

With "conditional" I mean "only for store / UWP / phone" apps to avoid breaking XP because those APIs are Vista+ exclusive.

@zabulus
Copy link
Contributor

zabulus commented Apr 28, 2016

Could comeone provide a patch for the Random Number generator only at Issue 164: Need NonblockingRng based on BCryptGenRandom for Windows?

I could. But I not quite understand relation with CRYPTOPP_WINRT_AVAILABLE define. Do you want patch with #ifdef CRYPTOPP_WINRT_AVAILABLE guard but without CRYPTOPP_WINRT_AVAILABLE define itself in config? Am I understood correct?

@noloader
Copy link
Collaborator Author

noloader commented Apr 28, 2016

Could comeone provide a patch for the Random Number generator only at Issue 164: Need NonblockingRng based on BCryptGenRandom for Windows?

I could. But I not quite understand relation with CRYPTOPP_WINRT_AVAILABLE define. Do you want patch with #ifdef CRYPTOPP_WINRT_AVAILABLE guard but without CRYPTOPP_WINRT_AVAILABLE define itself in config? Am I understood correct?

According to Microsoft documents on file with NIST, the BCrypt interface is part of Cryptography Next generation (CNG) and applies to Windows 8, Windows RT, Windows Server 2012, Windows Storage Server 2012, and Windows Phone 8. Also, according to the MSDN docs, BCryptGenRandom is Vista and above. However, there are some things missing in Vista that are available in Vista SP1. Lets target Windows 7.0/Server 2008 and make it a clean cut-over.

Here's what the tentative change to config.h looks like for this particular component. We give the user an option, and then we make a choice if the user did not select one:

// Define this to use features provided by Microsoft's CryptoAPI.
// Currently the only feature used is random number generation.
// This macro will be ignored if NO_OS_DEPENDENCE is defined.
// #define USE_MS_CRYPTOAPI

// Define this to use features provided by Microsoft's CryptoNG API.
// Currently the only feature used is random number generation.
// This macro will be ignored if NO_OS_DEPENDENCE is defined.
// #define USE_MS_CNGAPI

// If the user did not make a choice, then select CryptoNG if either
// Visual Studio 2010 is available, or Windows 7 or above is available.
#if !defined(USE_MS_CRYPTOAPI) && !defined(USE_MS_CNGAPI)
# if (_MSC_VER >= 1600) || (defined(_WIN32_WINNT_WIN7) && ((WINVER >= _WIN32_WINNT_WIN7) || (_WIN32_WINNT >= _WIN32_WINNT_WIN7)))
#  define USE_MS_CNGAPI
# else
#  define USE_MS_CRYPTOAPI
# endif
#endif

CNGAPI was selected because that's what Microsoft calls it at Cryptography API: Next Generation.

I tested it at Commit 500c312a5872: Add missing header for Windows 8, Windows Server 2012, and Windows Phone 8, so I'm fairly certain the macro is in good working order.

@noloader
Copy link
Collaborator Author

noloader commented Apr 28, 2016

Here's where I am with the patch. Its testing OK under Desktop and Store Apps. However, I can't get that damn Windows Phone to compile despite Microsoft's statements that its available on Windows 8, Windows RT, Windows Server 2012, Windows Storage Server 2012, and Windows Phone 8.

windows-store branch.

@noloader
Copy link
Collaborator Author

@smlu, @vukovinski, @DevJPM, @zabulus,

Testing was going OK until I hit Vista and VS2008. According to the BCryptGenRandom function, the function is supposed to fail with BCRYPT_USE_SYSTEM_PREFERRED_RNG. However, it does not fail and it appears to produce random values???


Stepping back to 10,000 feet at the security engineering level, I don't like this:

+   NTSTATUS ret = BCryptGenRandom(NULL, output, (ULONG)size, BCRYPT_USE_SYSTEM_PREFERRED_RNG);

That's because BCryptGenRandom may be using BCRYPT_RNG_DUAL_EC_ALGORITHM. The security policy on file with NIST says either BCRYPT_RNG_ALGORITHM or BCRYPT_RNG_DUAL_EC_ALGORITHM can be used when fulfilling a call to BCryptGenRandom. But which is used and under what circumstances is not documented. See page 17 of 37 at Cryptographic Primitives Library (BCRYPTPRIMITIVES.DLL).

And according to CNG Algorithm Identifiers, BCRYPT_RNG_DUAL_EC_ALGORITHM was not removed until Windows 10. That's nearly a decade of potentially vulnerable applications. There's no doubt in my mind that Microsoft has abandoned the support and left the down level devices and clients vulnerable.

I tried to side step it by using the provider:

NTSTATUS ret = BCryptOpenAlgorithmProvider(&m_hProvider, BCRYPT_RNG_ALGORITHM, MS_PRIMITIVE_PROVIDER, 0);
...
NTSTATUS ret = BCryptGenRandom(m_hProvider, output, (ULONG)size, 0);
...

But it fails with an ERROR_INVALID_PARAMETER. The only code I've been able to get working is the NULL provider/dwFlags BCRYPT_USE_SYSTEM_PREFERRED_RNG.

@zabulus
Copy link
Contributor

zabulus commented Apr 28, 2016

BCRYPT_USE_SYSTEM_PREFERRED_RNG

https://blogs.msdn.microsoft.com/winsdk/2010/05/28/how-to-make-your-custom-rng-random-number-generator-implementation-the-default-rng-provider-for-the-system-using-cng-apis/

By setting your implementation to the top, the BCryptGenRandom function will automatically handle opening and closing RNG algorithm handles for you if you call BCryptGenRandom with a NULL algorithm handle and set the BCRYPT_USE_SYSTEM_PREFERRED_RNG flag.

I think BCRYPT_USE_SYSTEM_PREFERRED_RNG affects NULL value in hAlgorithm

@zabulus
Copy link
Contributor

zabulus commented Apr 28, 2016

Further googling shows that my assumption is right.
API call doesn't fails with this flag, because you explicitly specify hAlgorithm

@denisbider
Copy link
Collaborator

denisbider commented Apr 28, 2016

This has worked everywhere I tested:

BCryptOpenAlgorithmProvider(&hRng, BCRYPT_RNG_ALGORITHM, NULL, 0)

@noloader
Copy link
Collaborator Author

noloader commented May 1, 2016

@smlu, @vukovinski, @DevJPM, @zabulus,

This is bad... I was struggling with BCryptGenRandom function on ARM (Windows Phone and Windows Store apps). It was either missing headers (<bcrypt.h>) or missing link-time libraries (bcrypt.lib). I spent days trying to figure out what I was doing wrong.

It appears Microsoft has taken to lying about availability of APIs; see What header to include for an NTSTATUS when building for ARM platforms? I guess if they can't build a superior platform, the next best thing is to lie and manipulate to make it appear the platform is first class.

We _really_ need this now: Preprocessor definitions for Universal Windows Platform?

@denisbider
Copy link
Collaborator

I do not see any lying. If I take the answer in the Stack Overflow thread at face value, you were targeting an old edition of the Windows Store platform (8.0 and 8.1) which is now considered obsolete.

@noloader
Copy link
Collaborator Author

noloader commented May 2, 2016

I do not see any lying. If I take the answer in the Stack Overflow thread at face value, you were targeting an old edition of the Windows Store platform (8.0 and 8.1) which is now considered obsolete.

Microsoft manages to get the desktop and server versions of APIs correct. For example, you can get details on API availability going back to Windows 2000 and Windows XP. Then they take to omitting details on the lack of availability on the platform they are trying to promote. If that API were truly available for Windows Store, then I would not be having these problems. That material omission is lying in my book. I don't care how Microsoft marketing chooses to spin it.

@noloader
Copy link
Collaborator Author

noloader commented May 2, 2016

Now open on Stack Overflow: Random numbers for Windows Phone 8 and Windows Store 8?

@noloader
Copy link
Collaborator Author

noloader commented May 2, 2016

Now open on the Crypto++ mailing list: Windows Phone, Windows Store and testing.

@denisbider
Copy link
Collaborator

Why would you want to target Windows Store 8? No one wants to target that. It is defunct.

Windows Store does not follow the model of earlier desktop/server Windows development. Earlier, you would install Windows Server 2003, and then keep using that for 15 years. When new features are added to Windows Server 2003 R2, Windows Server 2008, Windows Server 2008 R2, and so on, your existing Windows Server 2003 installation would NOT update to get those features. It would only update to get security updates.

The Windows Store model is different. Older versions are just plain not supported. Developers are expected to target the latest version of the platform, and users are expected to update to always have the latest version of the platform.

There IS no older version of this platform to target.

@denisbider
Copy link
Collaborator

For what it's worth - I'm not saying that I like the always auto-updating Windows Store model. I am deeply distrustful of this model of pushing software updates without any way for a user to opt out of them.

But, that is the model, and that is why Windows Store does not show compatibility for past versions.

@noloader
Copy link
Collaborator Author

noloader commented May 4, 2016

Why would you want to target Windows Store 8? No one wants to target that. It is defunct.

I have 4 devices. One is a Windows Surface RT tablet, one is a Windows Surface Pro, and two are Windows 8 Phones. I wont upgrade the Windows Surface Pro to Windows 10 because I don't like the interface. The Windows Surface RT tablet and phones don't get updates. That's the only platform available.

I imagine others are in the same situation. They have devices that are frozen for whatever reason.

We also have to be mindful of OEM partnerships. Medical equipment manufacturers license a platform, have it certified with FDA and then it does not change. I'm still doing work for some of them for Windows CE 5.0.

In fact, I've got two MacBooks that are down level due to FIPS operational requirements. Apple has abandoned them, too. (Its not just Microsoft).

I don't subscribe to the "Lets abandon our defective software" model used by browsers and companies like Apple and Microsoft. I don't think Crypto++ should either.

@denisbider
Copy link
Collaborator

On the one hand, I can't say I agree with the model.

On the other hand, for the past 15 years, we've only ever released updates to our SSH Server and Client by releasing new versions, and expecting users to upgrade to them.

It could be argued a platform is a platform, and a product is a product. You can upgrade an independent product more easily than a platform that has products that rely on it.

Especially when there is hardware bundled with the platform, and the company that provides the platform is large and successful, it seems negligent to just abandon it.

So yeah, I would tend to agree.

However, I'm not sure that it makes sense for Crypto++ to target Windows 8 Store when this version of is no longer supported by MS. I'm not sure that you can even release a product targeting that Store version; at least, probably not via the MS Store.

@noloader
Copy link
Collaborator Author

The windows-store branch was merged into master. This issue is now resolved.

There's one worrisome caveat with this bug report: Windows Phone 8 and Windows Store 8 _do not_ have a non-blocking generator available because Microsoft failed to provide access to either Crypto API or CryptoNG API.

Git does not provide a log entry for the merger (or I cannot find it), so I don't have a record to offer for the transaction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants