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

Make unpredictableSeed use BCryptGenRandom (CNG) on Windows #10624

Merged
merged 1 commit into from
Jan 26, 2025

Conversation

0xEAB
Copy link
Member

@0xEAB 0xEAB commented Jan 19, 2025

This patch changes unpredictableSeed to call CryptGenRandom on Windows.
This patch changes unpredictableSeed to call BCryptGenRandom on Windows.
Analogous to #10623 (Linux).

On the one hand, the legacy CryptoAPI might be deprecated, on the other hand using it does not introduce a new DLL dependency. advapi32 is already required by std.registry and Phobos is linked against as seen in the current Makefile.
Using the more modern CNG (BCryptGenRandom) would not only introduce a new dependency, but also require us to add the corresponding bindings to druntime. I figured that wouldn’t be worth the effort for now.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @0xEAB!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#10624"

@0xEAB 0xEAB added Severity:Enhancement OS:Windows Issues Specific to Windows labels Jan 19, 2025
@0xEAB 0xEAB force-pushed the unpredictable-seed-loss32 branch 5 times, most recently from a3b6342 to 499bfa0 Compare January 19, 2025 04:16
@0xEAB 0xEAB force-pushed the unpredictable-seed-loss32 branch 4 times, most recently from 20c2a61 to 345d936 Compare January 19, 2025 04:50
std/random.d Outdated
Comment on lines 1782 to 1789
import core.sys.windows.wincrypt :
CryptAcquireContext,
CryptGenRandom,
CryptReleaseContext,
CRYPT_VERIFYCONTEXT,
MS_STRONG_PROV,
PROV_RSA_FULL;
Copy link
Member Author

Choose a reason for hiding this comment

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

Blame D-Scanner for this monstrosity :)

@0xEAB 0xEAB force-pushed the unpredictable-seed-loss32 branch from 345d936 to 9a3cdea Compare January 19, 2025 04:55
@ichordev
Copy link
Contributor

On the one hand, the legacy CryptoAPI might be deprecated, on the other hand using it does not introduce a new DLL dependency. advapi32 is already required by std.registry and Phobos is linked against as seen in the current Makefile. Using the more modern CNG (BCryptGenRandom) would not only introduce a new dependency, but also require us to add the corresponding bindings to druntime. I figured that wouldn’t be worth the effort for now.

I was pretty surprised that DRuntime didn't already have BCrypt bindings. It shouldn't be much work for me to add them. What do you think?

@thewilsonator
Copy link
Contributor

What do you think?

Yes please.

@ichordev
Copy link
Contributor

ichordev commented Jan 19, 2025

Yes please.

Will do.
Does anyone know where Microsoft are hiding their header files? All I managed to find is a bunch of SDK downloads which don't contain any headers upon extraction. In fact all they seem to contain is obfuscated nonsense.

EDIT: found it

@0xEAB
Copy link
Member Author

0xEAB commented Jan 25, 2025

dlang/dmd#20740 has been merged – great!
Also: Thanks, @ichordev for adding the bindings :)

@0xEAB 0xEAB changed the title Make unpredictableSeed use CryptGenRandom (CryptoAPI) on Windows Make unpredictableSeed use BCryptGenRandom (CNG) on Windows Jan 25, 2025
@0xEAB 0xEAB force-pushed the unpredictable-seed-loss32 branch from 6b6bd3c to ad93bd7 Compare January 25, 2025 18:11
@0xEAB
Copy link
Member Author

0xEAB commented Jan 25, 2025

I suppose this needs dlang/dmd#20780 to have a chance of working.

@ichordev
Copy link
Contributor

I suppose this needs dlang/dmd#20780 to have a chance of working.

Whoops, sorry! 😣

@0xEAB
Copy link
Member Author

0xEAB commented Jan 25, 2025

Attribute soup got a bit too salty.
dlang/dmd#20781

@thewilsonator
Copy link
Contributor

Merged dependant PR

@0xEAB 0xEAB force-pushed the unpredictable-seed-loss32 branch from 19914df to 5a80f7d Compare January 25, 2025 22:12
@0xEAB 0xEAB marked this pull request as ready for review January 25, 2025 22:38
@0xEAB 0xEAB requested a review from LightBender January 25, 2025 22:38
@0xEAB 0xEAB force-pushed the unpredictable-seed-loss32 branch from 5a80f7d to 5294b6a Compare January 25, 2025 22:41
@dlang-bot dlang-bot merged commit 99602a5 into dlang:master Jan 26, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants