Skip to content

Dispose all resources used in CmsSignature #115666

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Dispose all resources used in CmsSignature #115666

Show file tree
Hide file tree
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
143 changes: 75 additions & 68 deletions ...stem.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/CmsSignature.DSA.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,31 +71,34 @@ internal override bool VerifySignature(
return false;
}

DSAParameters dsaParameters = dsa.ExportParameters(false);
int bufSize = 2 * dsaParameters.Q!.Length;
using (dsa)
{
DSAParameters dsaParameters = dsa.ExportParameters(false);
int bufSize = 2 * dsaParameters.Q!.Length;

#if NET || NETSTANDARD2_1
byte[] rented = CryptoPool.Rent(bufSize);
Span<byte> ieee = new Span<byte>(rented, 0, bufSize);
byte[] rented = CryptoPool.Rent(bufSize);
Span<byte> ieee = new Span<byte>(rented, 0, bufSize);

try
{
try
{
#else
byte[] ieee = new byte[bufSize];
byte[] ieee = new byte[bufSize];
#endif
if (!DsaDerToIeee(signature, ieee))
{
return false;
}
if (!DsaDerToIeee(signature, ieee))
{
return false;
}

return dsa.VerifySignature(valueHash, ieee);
return dsa.VerifySignature(valueHash, ieee);
#if NET || NETSTANDARD2_1
}
finally
{
CryptoPool.Return(rented, bufSize);
}
}
finally
{
CryptoPool.Return(rented, bufSize);
}
#endif
}
}

protected override bool Sign(
Expand All @@ -115,71 +118,75 @@ protected override bool Sign(
Debug.Assert(Helpers.IsDSASupported);
signatureParameters = null;

// If there's no private key, fall back to the public key for a "no private key" exception.
DSA? dsa = key as DSA ??
PkcsPal.Instance.GetPrivateKeyForSigning<DSA>(certificate, silent) ??
certificate.GetDSAPublicKey();

if (dsa == null)
using (GetSigningKey(key, certificate, silent, certificate.GetDSAPublicKey, out DSA? dsa))
{
signatureAlgorithm = null;
signatureValue = null;
return false;
}
if (dsa == null)
{
signatureAlgorithm = null;
signatureValue = null;
return false;
}

string? oidValue =
hashAlgorithmOid switch
string? oidValue =
hashAlgorithmOid switch
{
Oids.Sha1 => Oids.DsaWithSha1,
Oids.Sha256 => Oids.DsaWithSha256,
Oids.Sha384 => Oids.DsaWithSha384,
Oids.Sha512 => Oids.DsaWithSha512,
_ => null
};

if (oidValue == null)
{
Oids.Sha1 => Oids.DsaWithSha1,
Oids.Sha256 => Oids.DsaWithSha256,
Oids.Sha384 => Oids.DsaWithSha384,
Oids.Sha512 => Oids.DsaWithSha512,
_ => null
};

if (oidValue == null)
{
signatureAlgorithm = null;
signatureValue = null;
return false;
}
signatureAlgorithm = null;
signatureValue = null;
return false;
}

signatureAlgorithm = oidValue;
signatureAlgorithm = oidValue;

#if NET || NETSTANDARD2_1
// The Q size cannot be bigger than the KeySize.
byte[] rented = CryptoPool.Rent(dsa.KeySize / 8);
int bytesWritten = 0;
// The Q size cannot be bigger than the KeySize.
byte[] rented = CryptoPool.Rent(dsa.KeySize / 8);
int bytesWritten = 0;

try
{
if (dsa.TryCreateSignature(dataHash, rented, out bytesWritten))
try
{
var signature = new ReadOnlySpan<byte>(rented, 0, bytesWritten);

if (key != null && !certificate.GetDSAPublicKey()!.VerifySignature(dataHash, signature))
if (dsa.TryCreateSignature(dataHash, rented, out bytesWritten))
{
// key did not match certificate
signatureValue = null;
return false;
var signature = new ReadOnlySpan<byte>(rented, 0, bytesWritten);

if (key != null)
{
using (DSA certKey = certificate.GetDSAPublicKey()!)
Copy link
Preview

Copilot AI May 16, 2025

Choose a reason for hiding this comment

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

Multiple calls to certificate.GetDSAPublicKey() for signature verification could be optimized by storing the key in a temporary variable.

Suggested change
using (DSA certKey = certificate.GetDSAPublicKey()!)
using (DSA certKey = dsaPublicKey!)

Copilot uses AI. Check for mistakes.

Copy link
Member Author

Choose a reason for hiding this comment

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

If a private key isn't passed in as an argument but the cert has an associated private key, then GetDSAPublicKey will never be called, so we should only call it when it is needed.

Also GetDSAPublicKey is never called multiple times. If GetSigningKey calls it, it's to fail the Sign call with an appropriate "no private key" error and there won't be subsequent calls to GetSigningKey.

{
if (!certKey.VerifySignature(dataHash, signature))
{
// key did not match certificate
signatureValue = null;
return false;
}
}
}

signatureValue = DsaIeeeToDer(signature);
return true;
}

signatureValue = DsaIeeeToDer(signature);
return true;
}
}
finally
{
CryptoPool.Return(rented, bytesWritten);
}
finally
{
CryptoPool.Return(rented, bytesWritten);
}

signatureValue = null;
return false;
signatureValue = null;
return false;
#else
byte[] signature = dsa.CreateSignature(dataHash);
signatureValue = DsaIeeeToDer(new ReadOnlySpan<byte>(signature));
return true;
byte[] signature = dsa.CreateSignature(dataHash);
signatureValue = DsaIeeeToDer(new ReadOnlySpan<byte>(signature));
return true;
#endif
}
}
}
}
Expand Down
171 changes: 89 additions & 82 deletions ...em.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/CmsSignature.ECDsa.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,36 +71,39 @@ internal override bool VerifySignature(
return false;
}

int bufSize;
checked
using (key)
{
// fieldSize = ceil(KeySizeBits / 8);
int fieldSize = (key.KeySize + 7) / 8;
bufSize = 2 * fieldSize;
}
int bufSize;
checked
{
// fieldSize = ceil(KeySizeBits / 8);
int fieldSize = (key.KeySize + 7) / 8;
bufSize = 2 * fieldSize;
}

#if NET || NETSTANDARD2_1
byte[] rented = CryptoPool.Rent(bufSize);
Span<byte> ieee = new Span<byte>(rented, 0, bufSize);
byte[] rented = CryptoPool.Rent(bufSize);
Span<byte> ieee = new Span<byte>(rented, 0, bufSize);

try
{
try
{
#else
byte[] ieee = new byte[bufSize];
byte[] ieee = new byte[bufSize];
#endif
if (!DsaDerToIeee(signature, ieee))
{
return false;
}
if (!DsaDerToIeee(signature, ieee))
{
return false;
}

return key.VerifyHash(valueHash, ieee);
return key.VerifyHash(valueHash, ieee);
#if NET || NETSTANDARD2_1
}
finally
{
CryptoPool.Return(rented, bufSize);
}
}
finally
{
CryptoPool.Return(rented, bufSize);
}
#endif
}
}

protected override bool Sign(
Expand All @@ -111,92 +114,96 @@ protected override bool Sign(
#endif
string? hashAlgorithmOid,
X509Certificate2 certificate,
object? certKey,
object? privateKey,
bool silent,
[NotNullWhen(true)] out string? signatureAlgorithm,
[NotNullWhen(true)] out byte[]? signatureValue,
out byte[]? signatureParameters)
{
signatureParameters = null;
// If there's no private key, fall back to the public key for a "no private key" exception.
ECDsa? key = certKey as ECDsa ??
PkcsPal.Instance.GetPrivateKeyForSigning<ECDsa>(certificate, silent) ??
certificate.GetECDsaPublicKey();

if (key == null)
using (GetSigningKey(privateKey, certificate, silent, certificate.GetECDsaPublicKey, out ECDsa? key))
{
signatureAlgorithm = null;
signatureValue = null;
return false;
}

string? oidValue =
hashAlgorithmOid switch
if (key == null)
{
Oids.Sha1 => Oids.ECDsaWithSha1,
Oids.Sha256 => Oids.ECDsaWithSha256,
Oids.Sha384 => Oids.ECDsaWithSha384,
Oids.Sha512 => Oids.ECDsaWithSha512,
signatureAlgorithm = null;
signatureValue = null;
return false;
}

string? oidValue =
hashAlgorithmOid switch
{
Oids.Sha1 => Oids.ECDsaWithSha1,
Oids.Sha256 => Oids.ECDsaWithSha256,
Oids.Sha384 => Oids.ECDsaWithSha384,
Oids.Sha512 => Oids.ECDsaWithSha512,
#if NET8_0_OR_GREATER
Oids.Sha3_256 => Oids.ECDsaWithSha3_256,
Oids.Sha3_384 => Oids.ECDsaWithSha3_384,
Oids.Sha3_512 => Oids.ECDsaWithSha3_512,
Oids.Sha3_256 => Oids.ECDsaWithSha3_256,
Oids.Sha3_384 => Oids.ECDsaWithSha3_384,
Oids.Sha3_512 => Oids.ECDsaWithSha3_512,
#endif
_ => null,
};
_ => null,
};

if (oidValue == null)
{
signatureAlgorithm = null;
signatureValue = null;
return false;
}
if (oidValue == null)
{
signatureAlgorithm = null;
signatureValue = null;
return false;
}

signatureAlgorithm = oidValue;
signatureAlgorithm = oidValue;

#if NET || NETSTANDARD2_1
int bufSize;
checked
{
// fieldSize = ceil(KeySizeBits / 8);
int fieldSize = (key.KeySize + 7) / 8;
bufSize = 2 * fieldSize;
}
int bufSize;
checked
{
// fieldSize = ceil(KeySizeBits / 8);
int fieldSize = (key.KeySize + 7) / 8;
bufSize = 2 * fieldSize;
}

byte[] rented = CryptoPool.Rent(bufSize);
int bytesWritten = 0;
byte[] rented = CryptoPool.Rent(bufSize);
int bytesWritten = 0;

try
{
if (key.TrySignHash(dataHash, rented, out bytesWritten))
try
{
var signedHash = new ReadOnlySpan<byte>(rented, 0, bytesWritten);

if (key != null && !certificate.GetECDsaPublicKey()!.VerifyHash(dataHash, signedHash))
if (key.TrySignHash(dataHash, rented, out bytesWritten))
{
// key did not match certificate
signatureValue = null;
return false;
var signedHash = new ReadOnlySpan<byte>(rented, 0, bytesWritten);

if (key != null)
{
using (ECDsa certKey = certificate.GetECDsaPublicKey()!)
Copy link
Member Author

Choose a reason for hiding this comment

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

using declaration would be nice here since scope is obvious. Just curious, what is the reason we generally prefer using blocks?

Copy link
Member

Choose a reason for hiding this comment

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

In my personal opinion: Braced using statements make it very clear where the end of the scope is.

if (needSomething)
{
    using (Something something = GetSomething())
    {
       ...
+      Console.WriteLine("Done.");
    }
}

You can see that that increased the lifetime of something. Maybe that's important, but in the "Done" only case here, it isn't. That should be outside the braces. When it looks like

if (needSomething)
{
    using Something something = GetSomething();
    ...
+   Console.WriteLine("Done");
}

Well, the brace that it went in front of is the "if". That matches the context it belonged to. But it extended the lifetime of something. Which may be irrelevant, or may be bad.

In security code, it is always important to know what the lifetime of things is. So braced usings.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if all of these "verify it against the cert public key" should be moved to other methods, so this becomes

if (key != null && !VerifyWithPublicKey(certificate, dataHash, signedHash))
{
   ...
}

which will keep the same indentation and logic levels.

It's also fine as inline code; since I don't know that it's going to extract a lot of code or help codify a pattern.

{
if (!certKey.VerifyHash(dataHash, signedHash))
{
// key did not match certificate
signatureValue = null;
return false;
}
}
}

signatureValue = DsaIeeeToDer(signedHash);
return true;
}

signatureValue = DsaIeeeToDer(signedHash);
return true;
}
}
finally
{
CryptoPool.Return(rented, bytesWritten);
}
finally
{
CryptoPool.Return(rented, bytesWritten);
}
#endif

signatureValue = DsaIeeeToDer(key.SignHash(
signatureValue = DsaIeeeToDer(key.SignHash(
#if NET || NETSTANDARD2_1
dataHash.ToArray()
dataHash.ToArray()
#else
dataHash
dataHash
#endif
));
return true;
));
return true;
}
}
}
}
Expand Down
Loading
Loading