Use CryptographicallySecureRandomBytes for Guid generation

[jkotas]

Fixes #42752

[ghost]

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

[stephentoub]

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.

[jkotas]

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.

[jkotas]

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

On OSX 1015, this change makes Guid.NewGuid about 2.2 slower.
On OSX 1014, this change makes Guid.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 of https://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]

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

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

[stephentoub]

Thanks

[vcsjones]

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]

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]

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

• Using OS APIs with less fixed overhead than reading from /dev/urandom. It may be measurable faster to call getrandom syscall on Linux (started in Interop.GetRandomBytes(): Use getrandom() if supported  #1433, but never finished) or SecRandomCopyBytes on OSX.
• If it is not good enough, we may consider generating the random numbers in batches as suggested in Guid.NewGuid() is 10x slower on Linux compared to Windows #13628 (comment), but such strategy comes with tradeoffs.

[vcsjones]

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

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

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

[jkotas]

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

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

[am11]

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

[vcsjones]

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