CA5384 Asymmetric encryption algorithm DSA is weak. Switch to an RSA …

[elachlan]

Relates to #7174
https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/CA5384

[rainersigwald]

It looks like this is called via reflection, and I don't think it should be removed. I think we should not enable this rule, because I don't understand the compatibility implications of removing support for this style of signing at build time.

msbuild/src/Tasks/ManifestUtil/CngLightup.cs

Lines 520 to 530 in 1fea1d3

	// Now, find the api we want to call:
	//
	// (one of
	// GetRSAPublicKey(this X509Certificate2 c)
	// GetRSAPrivateKey(this X509Certificate2 c)
	// GetDSAPublicKey(this X509Certificate2 c)
	// GetDSAPrivateKey(this X509Certificate2 c)
	// GetECDsaPublicKey(this X509Certificate2 c)
	// GetECDsaPrivateKey(this X509Certificate2 c)
	// )
	string methodName = "Get" + algorithmName + (isPublic ? "Public" : "Private") + "Key";

[elachlan]

Ahh bummer, I thought we could get away with it because it wasn't referenced anywhere and it was not a public API.

The code you linked is a private function, it is called via GetDSAPublicKey/GetDSAPrivateKey:

msbuild/src/Tasks/ManifestUtil/CngLightup.cs

Lines 118 to 140 in 14efa06

	internal static DSA GetDSAPublicKey(X509Certificate2 cert)
	{
	if (s_getDsaPublicKey == null)
	{
	s_getDsaPublicKey =
	BindCoreDelegate<DSA>("DSA", isPublic: true) ??
	BindGetCapiPublicKey<DSA, DSACryptoServiceProvider>(DsaOid);
	}
	return s_getDsaPublicKey(cert);
	}
	internal static DSA GetDSAPrivateKey(X509Certificate2 cert)
	{
	if (s_getDsaPrivateKey == null)
	{
	s_getDsaPrivateKey =
	BindCoreDelegate<DSA>("DSA", isPublic: false) ??
	BindGetCapiPrivateKey<DSA>(DsaOid, csp => new DSACryptoServiceProvider(csp));
	}
	return s_getDsaPrivateKey(cert);
	}

The above methods call BindCoreDelegate is using reflection to call System.Security.Cryptography.X509Certificates.DSACertificateExtensions.GetDSAPublicKey/GetDSAPrivateKey

Additionally, CngLightup is only used when RUNTIME_TYPE_NETCORE=false.

The only public exposed functions I see that end up calling CNGLightup are in src/Tasks/ManifestUtil/SecurityUtil.cs

All calls end up in:

msbuild/src/Tasks/ManifestUtil/SecurityUtil.cs

Line 599 in 14efa06

private static void SignFileInternal(X509Certificate2 cert, Uri timestampUrl, string path, bool targetFrameworkSupportsSha256, System.Resources.ResourceManager resources)

If the file being signed is a portable executable it will use signtool.exe via SignPEFileInternal, if it isn't it will then call GetRSAPrivateKey and throw an OnlyRSACertsAreAllowed error if its null.

msbuild/src/Tasks/ManifestUtil/SecurityUtil.cs

Lines 625 to 637 in 14efa06

	else
	{
	#if RUNTIME_TYPE_NETCORE
	IntPtr hModule = IntPtr.Zero;
	using (RSA rsa = cert.GetRSAPrivateKey())
	#else
	using (RSA rsa = CngLightup.GetRSAPrivateKey(cert))
	#endif
	{
	if (rsa == null)
	throw new ApplicationException(resources.GetString("SecurityUtil.OnlyRSACertsAreAllowed"));
	try

There is then the file header comment, which I think might mean this file is pulled in externally somewhere?:

msbuild/src/Tasks/ManifestUtil/CngLightup.cs

Lines 14 to 17 in 518c041

	// There are cases where we have multiple assemblies that are going to import this file and
	// if they are going to also have InternalsVisibleTo between them, there will be a compiler warning
	// that the type is found both in the source and in a referenced assembly. The compiler will prefer
	// the version of the type defined in the source

[Forgind]

Sounds like this one should be closed, right?

[elachlan]

Sounds like this one should be closed, right?

I am not sure. Would the DSA functionality be called from outside of msbuild? If so maybe we need to add an suppression attribute for this analyzer?

[Forgind]

Not a bad idea, but personally, I'd prefer to not touch it until someone complains.