Skip to content

CryptGenRandom is deprecated #14089

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.

Sign up for GitHub

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

Jump to bottom
Closed
fingera wants to merge 1 commit into from
Closed

CryptGenRandom is deprecated #14089

fingera wants to merge 1 commit into from

Conversation

@fingera
Copy link
Contributor

@fingera fingera commented Aug 28, 2018

laanwj
laanwj reviewed Aug 28, 2018
View reviewed changes
src/random.cpp Outdated
Copy link
Member

@laanwj laanwj Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so with this, it will still use the deprecated function with mingw?
(which is used for the distributed binaries)

ken2812221
ken2812221 reviewed Aug 28, 2018
View reviewed changes
src/random.cpp Outdated
Copy link
Contributor

@ken2812221 ken2812221 Aug 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should add this into build_msvc/*/*.vcxproj and configure.ac instead of adding random library into source code. Once you update configure.ac, the #if defined(_MSC_VER)..#endif thing could be removed.

src/random.cpp Outdated
Copy link
Contributor

@ken2812221 ken2812221 Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checking _WIN32_WINNT is not necessary. The min version required to run Bitcoin Core is Vista which is the same as the required min version of BCryptGenRandom

@fingera fingera force-pushed the 2-crypto-api-deprecated branch 3 times, most recently from a235c05 to b3142a4 Compare August 29, 2018 03:52
@fingera
Copy link
Contributor Author

fingera commented Aug 29, 2018

thankyou, now the minimum system is windows vista

@ken2812221
Copy link
Contributor

ken2812221 commented Aug 29, 2018

Concept ACK

@laanwj
Copy link
Member

laanwj commented Aug 29, 2018

utACK on superficial code changes

however, as random generation is critical to wallet security, someone with actual knowledge of windows random APIs should review this

@NicolasDorier
Copy link
Contributor

NicolasDorier commented Aug 29, 2018

I would be inclined to NACK, new version of .NET still depend on advapi32.dll CryptGenRandom for secure RNG. I would be more confident to use when microsoft eat their dog food.

@NicolasDorier
Copy link
Contributor

NicolasDorier commented Aug 29, 2018 •
edited
Loading

Extract from .NET 4.7.2 released in march 2018, still using CryptGenRandom

https://referencesource.microsoft.com/#mscorlib/system/security/cryptography/rngcryptoserviceprovider.cs

https://referencesource.microsoft.com/#mscorlib/system/security/cryptography/capinative.cs,1b2672c29bebdd55

@fingera
Copy link
Contributor Author

fingera commented Aug 30, 2018

Windows Vista: The random number generator is based on the hash-based random number generator specified in the FIPS 186-2 standard.
Windows 8: Beginning with Windows 8, the RNG algorithm supports FIPS 186-3. Keys less than or equal to 1024 bits adhere to FIPS 186-2 and keys greater than 1024 to FIPS 186-3.
https://docs.microsoft.com/zh-cn/windows/desktop/SecCNG/cng-algorithm-identifiers#BCRYPT_RNG_ALGORITHM

ok, maybe switch to bcrypt after many years?

@luke-jr
Copy link
Member

luke-jr commented Aug 30, 2018

Could do both?

@fingera
Copy link
Contributor Author

fingera commented Aug 31, 2018

bitcoin random source:
// First source: OpenSSL's RNG
// Second source: OS RNG
// Third source: HW RNG, if available.

OpenSSL's RNG already append CryptGenRandom(Compile Target < Win7)
OS RNG improve it, so OS RNG use new BCryptGenRandom :)

@sipa
Copy link
Member

sipa commented Aug 31, 2018

We will in the near future probably remove OpenSSL as a randomness source.

@fingera
Copy link
Contributor Author

fingera commented Aug 31, 2018

Remove randomness source must be very carefull.
Can I do it?
What to do now?

@sipa
Copy link
Member

sipa commented Aug 31, 2018

No, we're still relying on some of its features that need to be implemented differently first.

I'm just pointing out that OpenSSL using some API already is not a reason to not also use it ourselves.

@fingera
Copy link
Contributor Author

fingera commented Aug 31, 2018 •
edited
Loading

Add both os source now? Or wait for Microsoft's new API to accept more tests

@fingera fingera force-pushed the 2-crypto-api-deprecated branch from 2a77bb5 to cb5d5d8 Compare August 31, 2018 05:42
@fingera fingera force-pushed the 2-crypto-api-deprecated branch from cb5d5d8 to 1e8f80d Compare August 31, 2018 05:50
@laanwj
Copy link
Member

laanwj commented Aug 31, 2018

I'm closing this for now.
We can do this at some point in the future, but I agree with @NicolasDorier that it's not relevant now.

@laanwj laanwj closed this Aug 31, 2018
@fingera
Copy link
Contributor Author

fingera commented Aug 31, 2018 •
edited
Loading

ok, please read 14116

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@laanwj laanwj laanwj left review comments

+1 more reviewer

@ken2812221 ken2812221 ken2812221 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Windows

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@fingera @ken2812221 @laanwj @NicolasDorier @luke-jr @sipa @fanquake