Fix seed initialization in Microsoft.Bcl.Hashcode

[fabricefou]

Description

Private field s_seed is always set to 0 due to a bug in method GetRandomBytes.
In this method the parameter byte* buffer is always overwritten, so random value is lost.

We can easily reproduce the problem with this code.

var seed = typeof(HashCode).GetField("s_seed", BindingFlags.NonPublic | BindingFlags.Statics);

This problem don't existe in .NET Core implementation.

Configuration

.NET Framework

Regression?

It's not really a regression but not the expected behavior.
People using this package want the same behavior than in .NET core.

And if it's not fixed, I think this package should be mark as deprecated.

[stephentoub]

Specifically this code is problematic:
https://github.com/dotnet/corefx/blob/3009a1fc20d9cb47d117399040d31c23f7b23e02/src/Microsoft.Bcl.HashCode/src/Interop.GetRandomBytes.cs#L10

cc: @joperezr

[fabricefou]

Thanks to be so reactive !
Any idea if this bug will be fix and when ?
(I can of course copy paste implementation and fix the random seed problem, but if you release the nuget package with the fix it's better 😀.)

[stephentoub]

cc: @ericstj

[danmoseley]

It's interesting, given our recent conversations about Guid, that this code is generating a Guid purely as a source of random bytes. Isn't there a more canonical way to fill a buffer with cryptographically random bytes? Or perhaps it isn't available in .NET Standard?

[ericstj]

IIRC @bartonjs usually recommends to only use crypto when you need it. If all you need is random, then other methods are more efficient. Not sure if crytpo is needed here, I would imagine no since in net5.0 this uses

runtime/src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetRandomBytes.cs

Line 20 in 855e296

Sys.GetNonCryptographicallySecureRandomBytes(buffer, length);

As far as fixing this, it would need to be done in 3.1 servicing. @danmosemsft what's your feeling for this meeting servicing bar? Related discussion: #39895 (comment)

[bartonjs]

Because this is used for things like avoiding dictionary collision attacks, it's important to use good random in the static seed generation (dotnet/corefx#25013 (comment) was the original discussion).

If this is OOB@NS2.0, then (if we're keeping the signature on the GetRandomBytes call for code sharing reasons):

    internal static unsafe void GetRandomBytes(byte* buffer, int length)
    {
        byte[] tmp = new byte[length];

        using (RandomNumberGenerator rng = RandomNumberGenerator.Create())
        {
            rng.GetBytes(tmp);
        }

        Marshal.Copy(tmp, 0, (IntPtr)buffer, length);
    }

Since it's only ever used once, it shouldn't be too big of a deal.

It's probably cleaner to make GenerateGlobalSeed() just have a different implementation in the package built variant

        private static unsafe uint GenerateGlobalSeed()
        {
            byte[] tmp = new byte[sizeof(uint));
             
            using (RandomNumberGenerator rng = RandomNumberGenerator.Create())
            {
                rng.GetBytes(tmp);
            }

            fixed (byte* pTmp = tmp)
            {
                return *((uint*)pTmp);
            }
        }