Dispose all resources used in CmsSignature #115666
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the handling of signing keys in CmsSignature to ensure all disposable resources are consistently disposed. The changes include:
- Introducing a new helper method GetSigningKey to centralize key retrieval and disposal.
- Updating the implementations for RSA, DSA, ECDsa, and SlhDsa to use the new helper method.
- Minor renaming adjustments for clarity in key-related parameters.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| CmsSignature.cs | Added GetSigningKey helper to encapsulate key retrieval and disposal logic. |
| CmsSignature.SlhDsa.cs | Updated signing logic to use GetSigningKey for consistent disposal. |
| CmsSignature.RSA.cs | Refactored signing and verification for RSA to leverage GetSigningKey and ensure proper resource disposal. |
| CmsSignature.ECDsa.cs | Replaced direct disposal of keys with GetSigningKey and adjusted parameter naming. |
| CmsSignature.DSA.cs | Updated DSA signing to use GetSigningKey for improved resource management. |
.../System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/CmsSignature.RSA.cs
Show resolved
Hide resolved
...ystem.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/CmsSignature.ECDsa.cs
Show resolved
Hide resolved
|
|
||
| if (key != null) | ||
| { | ||
| using (DSA certKey = certificate.GetDSAPublicKey()!) |
There was a problem hiding this comment.
Multiple calls to certificate.GetDSAPublicKey() for signature verification could be optimized by storing the key in a temporary variable.
| using (DSA certKey = certificate.GetDSAPublicKey()!) | |
| using (DSA certKey = dsaPublicKey!) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
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 (key != null) | ||
| { | ||
| using (ECDsa certKey = certificate.GetECDsaPublicKey()!) |
There was a problem hiding this comment.
using declaration would be nice here since scope is obvious. Just curious, what is the reason we generally prefer using blocks?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| certPublicKey; | ||
|
|
||
| if (privateKey is null) | ||
| using (GetSigningKey(key, certificate, silent, certificate.GetRSAPublicKey, out RSA? privateKey)) |
There was a problem hiding this comment.
Same here, using declaration would avoid all the indent diffs. This initialized at the top of the function and its lifetime should extend until the method returns.
| @@ -100,26 +100,17 @@ protected override bool Sign( | |||
| // not enforce them here. It is up to the caller to choose an appropriate hash. | |||
|
|
|||
| signatureParameters = null; | |||
| signatureAlgorithm = _signatureAlgorithm; | |||
There was a problem hiding this comment.
You've moved an assignment of this out to earlier than it used to be. Do any of the callers ever use this out in a shared memory location (i.e. a static field)?
It's generally best to only ever have one write to an out, just in case they become "side effect visible"; and this change makes signature failure go from non-null back to null.
There was a problem hiding this comment.
Oops, I'll remove the other null assignment below that I missed. The other algos also follow this pattern of assignment.
Dispose keys in CmsSignature more consistently.
Contributes to #115578