CA5384 Asymmetric encryption algorithm DSA is weak. Switch to an RSA … by elachlan · Pull Request #7234 · dotnet/msbuild

elachlan · 2022-01-08T06:49:34Z

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

…with at least 2048 key size, ECDH or ECDSA algorithm instead.

rainersigwald · 2022-01-10T18:36:03Z

src/Tasks/ManifestUtil/CngLightup.cs

            return s_getRsaPrivateKey(cert);
        }

-        internal static DSA GetDSAPublicKey(X509Certificate2 cert)


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";

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 · 2022-01-12T18:43:44Z

Sounds like this one should be closed, right?

elachlan · 2022-01-12T21:17:10Z

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 · 2022-01-18T04:37:56Z

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

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

1fea1d3

…with at least 2048 key size, ECDH or ECDSA algorithm instead.

elachlan mentioned this pull request Jan 8, 2022

Change CodeAnalysis rules from 'Info' to 'Warning' after fixing all instances of the violations #7174

Open

rainersigwald requested changes Jan 10, 2022

View reviewed changes

Forgind closed this Jan 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

CA5384 Asymmetric encryption algorithm DSA is weak. Switch to an RSA …#7234
elachlan wants to merge 1 commit intodotnet:mainfrom
elachlan:CA5384

elachlan commented Jan 8, 2022

Uh oh!

rainersigwald Jan 10, 2022

Uh oh!

elachlan Jan 10, 2022

Uh oh!

Forgind commented Jan 12, 2022

Uh oh!

elachlan commented Jan 12, 2022

Uh oh!

Forgind commented Jan 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	// 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";

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

	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 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

Conversation

elachlan commented Jan 8, 2022

Uh oh!

rainersigwald Jan 10, 2022

Choose a reason for hiding this comment

Uh oh!

elachlan Jan 10, 2022

Choose a reason for hiding this comment

Uh oh!

Forgind commented Jan 12, 2022

Uh oh!

elachlan commented Jan 12, 2022

Uh oh!

Forgind commented Jan 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants