Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Use CryptographicallySecureRandomBytes for Guid generation#42770

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
jkotas merged 2 commits intodotnet:masterfromjkotas:random
Sep 28, 2020

Conversation

@jkotas
Copy link
Member

Fixes#42752

@ghost
Copy link

Tagging subscribers to this area:@bartonjs,@vcsjones,@krwq,@jeffhandley
See info in area-owners.md if you want to be subscribed.

@stephentoub
Copy link
Member

stephentoub commentedSep 26, 2020
edited
Loading

What is the perf impact of this, on Linux and on macOS? I get that there's a desire for the Unix implementation to maintain undocumented behaviors that Windows happens to have, but if memory serves, the perf impact here especially on mac was substantial. I worry we're going to keep just going back and forth based on the latest issue filed. The latest issue cites a potential security pit of failure, but previous issues have cited essentially the same but for perf.

danmoseley reacted with thumbs up emoji

@jkotas
Copy link
MemberAuthor

This change has no perf impact on Linux. This change is only changing failure mode on Linux. Before this change, we would fallback to weak random number generator when something went wrong. After this change, exception will be thrown in this case. It should never happen in practice (famous last words).

Yes, I expect that this be slower on macOS. I will try to get some numbers.

stephentoub reacted with thumbs up emoji

@jkotas
Copy link
MemberAuthor

The NewGuid performance is roughly the same on macOS and Linux with the current version of the change.

On OSX 1015, this change makesGuid.NewGuid about 2.2 slower.
On OSX 1014, this change makesGuid.NewGuid about 4.6 slower.

This is due to underlying OS implementation changes. arc4random seems to have gotten much slower in OSX 1015. arc4random in OSX is actually implemented as wrapper over OSX core crypto APIs (see ifdef at the top ofhttps://opensource.apple.com/source/Libc/Libc-1353.100.2/gen/FreeBSD/arc4random.c.auto.html), so it picks up performance impacts of core crypto hardening.

@stephentoub
Copy link
Member

On OSX 1015, this change makes Guid.NewGuid about 2.2 slower

That's much less than I was expecting. Thanks.

Copy link
Member

@stephentoubstephentoub left a comment

Choose a reason for hiding this comment

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

Thanks

@vcsjones
Copy link
Member

Just one question to help me understand...

arc4random seems to have gotten much slower in OSX 1015

But the numbers you posted show an improvement from 10.14 to 10.15.

@jkotas
Copy link
MemberAuthor

Here are numbers that I have collected in#42777:

OSX 1014:
GetCryptographicallySecureRandomBytes((byte*)&g, sizeof(Guid)): 1.07us per call
GetNonCryptographicallySecureRandomBytes((byte*)&g, sizeof(Guid)): 0.23us per call

OSX 1015:
GetCryptographicallySecureRandomBytes((byte*)&g, sizeof(Guid)): 0.93us per call
GetNonCryptographicallySecureRandomBytes((byte*)&g, sizeof(Guid)): 0.43us per call

(Caveat: These numbers are for release build of the runtime + debug build of the libraries. They may be slightly smaller if everything is release build.)

@jkotas
Copy link
MemberAuthor

If we wanted to make this better, we may look at:

@vcsjones
Copy link
Member

vcsjones commentedSep 26, 2020
edited
Loading

As another data point, here are some numbers from my Mac on10.16 11 Beta 8 built in release / release. It's a beta OS, so perhaps not valid.

Secure: 6384NonSecure: 688Secure: 6248NonSecure: 689Secure: 6318NonSecure: 677Secure: 6553NonSecure: 704Secure: 6318NonSecure: 684

Or about 640ns for secure and 69ns for insecure.

@jkotas
Copy link
MemberAuthor

Or about 640ns for secure and 69ns for insecure.

Hmm, I am wondering how the implementation of arc4random changed in the new macOS.

@am11
Copy link
Member

@vcsjones, just curious, are those numbers from Big Sur x64 or arm64?

@vcsjones
Copy link
Member

@am11 x64. The ARM64 DTK agreement pretty strongly tells you to not post any benchmarks of any kind. (Or really even discuss it)😊

am11 reacted with laugh emoji

@jkotasjkotasforce-pushed therandom branch 2 times, most recently from5f9b82c to5955cb5CompareSeptember 27, 2020 17:05
@jkotasjkotas merged commitae1b1be intodotnet:masterSep 28, 2020
@jkotasjkotas deleted the random branchSeptember 28, 2020 01:52
@ghostghost locked asresolvedand limited conversation to collaboratorsDec 7, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@stephentoubstephentoubstephentoub approved these changes

@GrabYourPitchforksGrabYourPitchforksAwaiting requested review from GrabYourPitchforks

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Guid.NewGuid should guarantee a full 122 bits of entropy on non-Windows platforms

5 participants

@jkotas@stephentoub@vcsjones@am11@Dotnet-GitSync-Bot

[8]ページ先頭

©2009-2025 Movatter.jp