Analyzer suggestion: flag calls to RandomNumberGenerator.GetNonZeroBytes

[GrabYourPitchforks]

The method RandomNumberGenerator.GetNonZeroBytes is very rarely the correct method to call to generate random data. The only scenarios for calling this method are for protocols which cannot handle embedded nulls within a random buffer. (And with few exceptions, most protocols that behave in such a manner aren't following hygienic practice.)

To generate random data, the application should instead call RandomNumberGenerator.GetBytes. This helps ensure no loss of entropy in the buffer returned to the caller.

When to suppress this rule: only when implementing a protocol that specifies that buffers populated with random data should exclude 0x00 bytes.

[ghost]

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

[GrabYourPitchforks]

See also: dotnet/wcf#4378 and dotnet/wcf#4377.

Here are some other [potentially incorrect] usages of this across MSFT-owned code bases:

• https://github.com/Azure/azure-iot-sdk-csharp/blob/master/iothub/service/src/Common/Security/CryptoKeyGenerator.cs
• https://github.com/OfficeDev/ews-managed-api/blob/master/Core/ExchangeServiceBase.cs

[bartonjs]

The only legitimate case for it that I know if in .NET Framework was the surprising managed implementation of RSA PKCS#1 v1.5 encryption padding (since it's a random non-zero buffer with a 0x00 terminator)... which is probably why the method was created in the first place.

In Core we only had it because of API Compatibility.

But warning "this is probably not the method you want" seems good to me.

[stephentoub]

Before going ahead with something here, I'd like to have a better sense for whether the few found use cases should actually be replaced (e.g. the cited https://github.com/Azure/azure-iot-sdk-csharp/blob/master/iothub/service/src/Common/Security/CryptoKeyGenerator.cs example is on RNGCryptoServiceProvider rather than RandomNumberGenerator). If we expect there are legitimate use cases for it, and we only find a few uses of it, it could very well be that they're valid uses, in which case such an analyzer would just introduce noise. Or if we expect that there are basically no valid uses of it and all found uses should be replaced, then shouldn't we just [Obsolete(...)] it? Doing so has the same user impact as an analyzer that always warns on the use of a particular method, can similarly have its own diagnostic ID, etc.

[GrabYourPitchforks]

@stephentoub The call site you mentioned should be replaced with simply GetBytes. If the Azure service which this communicates with cannot handle symmetric keys with null bytes, that should be reported to the Azure security team, as it represents a possible implementation flaw in their service API. (I can't tell what service this might be for since I don't see any calls to this API outside of unit tests.) Regardless, having a compile-time notification for these call sites is waving a huge red flag that says "something is off here and you need to double-check your code."

I'm still not generally sold on the idea of using [Obsolete] as the mechanism for these. To me, [Obsolete] means "We intend on removing this API at some point. If you keep using this API you're almost certainly going to be broken in a future version." I don't see a world where we remove the GetNonZeroBytes API. It still has a legitimate use case.

[stephentoub]

The call site you mentioned should be replaced with simply GetBytes.

It already is GetBytes, that's my point. The call sites you cited in that file are on RNGCryptoServiceProvider, not RandomNumberGenerator, and this issue is about RandomNumberGenerator.
https://github.com/Azure/azure-iot-sdk-csharp/blob/1513e4275afc67a47f8c423bff128dc02fa3ceab/iothub/service/src/Common/Security/CryptoKeyGenerator.cs#L47-L49
https://github.com/Azure/azure-iot-sdk-csharp/blob/1513e4275afc67a47f8c423bff128dc02fa3ceab/iothub/service/src/Common/Security/CryptoKeyGenerator.cs#L69-L71

I'm still not generally sold on the idea of using [Obsolete] as the mechanism for these.

And if we raise a warning indiscriminately every time a particular method is used, I don't understand how that's not saying "this should not be used".

[terrajobst]

Video

• A custom analyzer would effectively do the same as obsoleting it.
• We should just obsolete it, asssuming we can add turn it off by default by putting the diagnostic ID in the SDK .editorconfig root file. We should tie to the security rules
• @GrabYourPitchforks, please follow up to figure out why people call this API. Some usage seems high and it's hard to imagine people are generating secrets.

namespace System.Security.Cryptography
{
    public abstract class RandomNumberGenerator
    {
        public abstract void GetNonZeroBytes(byte[] data);
        public abstract void GetNonZeroBytes(Span<byte> data);
    }
}

[GSPP]

Is the plan to remove these APIs in a future release? Or are they just marked with a label saying "caution, this is likely not what you want but it might be OK"?

[Joe4evr]

@GSPP The latter.

[GrabYourPitchforks]

I've started discussions with some folks knowledgeable about CodeQL to see if that's a better place to put this analyzer. Will loop back here as discussions progress.