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);
            }
        }

[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 😀.)

Any idea of the schedule for the fix? Week(s)? Month(s)?

Thanks

[ericstj]

Since this is servicing, at the very earliest it would be November. We'll need to discuss it per the bar, as mentioned above.

[fabricefou]

Since this is servicing, at the very earliest it would be November. We'll need to discuss it per the bar, as mentioned above.

Thanks

[ericstj]

Current thinking is to propose this for release/3.1 with an AppContext switch to go back to non-randomized in case someone is broken by the non-stable hash ordering (this has cause compat issues in the past).

[fabricefou]

Current thinking is to propose this for release/3.1 with an AppContext switch to go back to non-randomized in case someone is broken by the non-stable hash ordering (this has cause compat issues in the past).

If I understand well, default value for switch will be random seed activated ?

[joperezr]

If I understand well, default value for switch will be random seed activated ?

@ericstj please correct me if I'm wrong but I think the idea would be to keep the existing behavior in .NETStandard as we have it today ( so as to not break people depending on this today) but have an appcontext switch that allows you to get the .NET Core behavior if you want to. That would be an opt-in fix so that we don't break existing consumers.

[ericstj]

The opposite actually, we need to make the behavior consistent. We have the AppContext switch to go back to the broken behavior if someone happened to depend on it.

[fabricefou]

Thanks for the fix 👍 .
Any idea when the fix will be available as nuget package?

[joperezr]

Yes, as you can see we set the milestone to be 3.1.11 which is our next servicing for 3.1 for which we don't have exact dates but should be in NuGet within the next two months.

[fabricefou]

Yes, as you can see we set the milestone to be 3.1.11 which is our next servicing for 3.1 for which we don't have exact dates but should be in NuGet within the next two months.

Thanks

[fabricefou]

Hello any news for a NuGet release date?
Thanks

[KalleOlaviNiemitalo]

@fabricefou, Microsoft.Bcl.HashCode 1.1.1 was released last week. version.txt in that package refers to commit dotnet/corefx@59d2f36, which is on the release/3.1 branch and includes the fix dotnet/corefx#43004.

I don't know why this issue is still open.

[fabricefou]

@fabricefou, Microsoft.Bcl.HashCode 1.1.1 was released last week. version.txt in that package refers to commit dotnet/corefx@59d2f36, which is on the release/3.1 branch and includes the fix dotnet/corefx#43004.

I don't know why this issue is still open.

Thanks 👍

[joperezr]

I don't know why this issue is still open.

Seems like we just forgot to close it 😆. Closing now.