Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 0 additions & 28 deletions src/Tasks/ManifestUtil/CngLightup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ namespace System.Security.Cryptography
{
internal static partial class CngLightup
{
private const string DsaOid = "1.2.840.10040.4.1";
private const string RsaOid = "1.2.840.113549.1.1.1";

private const string HashAlgorithmNameTypeName = "System.Security.Cryptography.HashAlgorithmName";
Expand Down Expand Up @@ -60,9 +59,6 @@ internal static partial class CngLightup

private static readonly Lazy<bool> s_preferRsaCng = new Lazy<bool>(DetectRsaCngSupport);

private static volatile Func<X509Certificate2, DSA> s_getDsaPublicKey;
private static volatile Func<X509Certificate2, DSA> s_getDsaPrivateKey;

private static volatile Func<X509Certificate2, RSA> s_getRsaPublicKey;
private static volatile Func<X509Certificate2, RSA> s_getRsaPrivateKey;
private static volatile Func<RSA, byte[], string, byte[]> s_rsaPkcs1SignMethod;
Expand Down Expand Up @@ -115,30 +111,6 @@ internal static RSA GetRSAPrivateKey(X509Certificate2 cert)
return s_getRsaPrivateKey(cert);
}

internal static DSA GetDSAPublicKey(X509Certificate2 cert)
Copy link
Member

@rainersigwald rainersigwald Jan 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@elachlan elachlan Jan 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

#if !CNG_LIGHTUP_NO_SYSTEM_CORE
internal static ECDsa GetECDsaPublicKey(X509Certificate2 cert)
{
Expand Down