Fix minor random bias

[vcsjones]

This corrects two uses of RandomNumberGenerator that resulted in slight bias in their results. After some internal discussion it was determined that the bias did not weaken the randomness to the point where this needed to be treated as a vulnerability.

This also adds some tests just to make sure the random generation is not completely broken.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-directoryservices, @jay98014
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This corrects two uses of RandomNumberGenerator that resulted in slight bias in their results. After some internal discussion it was determined that the bias did not weaken the randomness to the point where this needed to be treated as a vulnerability.

This also adds some tests just to make sure the random generation is not completely broken.

Author:	vcsjones
Assignees:	-
Labels:	area-System.DirectoryServices
Milestone:	-

[stephentoub]

There's no way to achieve the desired goals here without implementing a custom non-biased GetInt32 method?

[vcsjones]

@stephentoub For .NET Standard 2.0, not to my knowledge. Based on an off-GitHub conversation with @bartonjs I think he was on the same page that we'd have to lift and adapt GetInt32.

[bartonjs]

Well, we could trim the alphabet, but that speeds up brute force (especially if we trim it down to 1 😄).

The alternative to dropping this copy here is something like

byte[] random = new byte[SomeNumber];
char[] output = new char[Whatever];
int randomPos = random.Length;

for (int writePos = 0; writePos < output.Length; writePos++)
{
    int randomVal;

    while (true)
    {
        if (randomPos >= random.Length)
        {
            RandomNumberGenerator.GetBytes(random);
            randomPos = 0;
        }

        randomVal = random[randomPos];
        randomPos++;
 
        Debug.Assert(alphabet.Length > 64 && alphabet.Length <= 128);
        randomVal &= 127;

        if (randomVal < alphabet.Length)
        {
            break;
        }
    }

    output[writePos] = alphabet[randomVal];
}

Which amounts to the same thing.

[joperezr]

@vcsjones are we still waiting on a signoff here?

[vcsjones]

@joperezr Not sure what's up with the enterprise-linux leg but it seems unrelated. I think this is okay to merge.

[bartonjs]

I haven't hit merge because I don't know what's in the enterprise-linux leg and I've been hoping that its DNS failures would get worked out. If you (@joperezr) are fine ignoring the leg, then I am, too. (I'll admit, I have difficulty understanding what sort of test could be failing due to this change)

[stephentoub]

I haven't hit merge because I don't know what's in the enterprise-linux leg and I've been hoping that its DNS failures would get worked out.

I believe it's fine to ignore. @wfurt can confirm.

[wfurt]

correct. The enterprise-linux failure is tracked by https://github.com/dotnet/core-eng/issues/15594 and it is environmental regression caused by infrastructure changes.