SignedCms: does not enable RSASSA-PSS signing

[dtivel]

When signing with an RSA public key certificate, SignedCms does not support generating an RSSASSA-PSS signature. In this case, SignedCms always generates an RSASSA-PKCS1-v1_5 signature.

API Proposal

namespace System.Security.Cryptography.Pkcs
{
    public partial class CmsSigner
    {
        // Existing ctors:
        // public CmsSigner();
        // public CmsSigner(SubjectIdentifierType signerIdentifierType);
        // public CmsSigner(X509Certificate2? certificate);
        // public CmsSigner(SubjectIdentifierType signerIdentifierType, X509Certificate2? certificate);
        //
        // public CmsSigner(
        //     SubjectIdentifierType signerIdentifierType,
        //     X509Certificate2? certificate,
        //     AsymmetricAlgorithm? privateKey);

        public CmsSigner(
            SubjectIdentifierType signerIdentifierType,
            X509Certificate2? certificate,
            RSA? privateKey,
            RSASignaturePadding? signaturePadding);

        public RSASignaturePadding? SignaturePadding { get; set; }
    }
}

[ghost]

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

---

When signing with an RSA public key certificate, SignedCms does not support generating an RSSASSA-PSS signature. In this case, SignedCms always generates an RSASSA-PKCS1-v1_5 signature.

Author:	dtivel
Assignees:	-
Labels:	area-System.Security, untriaged
Milestone:	-

[vcsjones]

Yeah. Looks like it will always use PKCS1 now.

I think we probably need a new API that accepts RSASignaturePadding on the CmsSigner. Maybe a proposal like:

namespace System.Security.Cryptography.Pkcs
{
    public sealed class CmsSigner
    {
        public CmsSigner(
            SubjectIdentifierType signerIdentifierType,
            X509Certificate2? certificate,
            AsymmetricAlgorithm? privateKey,
            RSASignaturePadding? signaturePadding);

        public RSASignaturePadding? SignaturePadding { get; set; }
    }
}

This would be similar to the changes in EnvelopedCms, except now we're doing it with SignedCms.

The constructor would throw if the cert is not an RSA cert, similar to how EnvelopedCms was done.

[bartonjs]

Assuming I'm reading things right, CmsSigner is a fully tweakable object (everything's get+set) whereas CmsRecipient is readonly. (I'm guessing they were both readonly at initial design, then since CmsSigner is mutable because of attributes it went full property panel.. but that's before my time.)

For local consistency, SignaturePadding would probably want to bet get/set (so you can use the default ctor + object initialization). The property set shouldn't validate (other than it's not an unknown value), but any accelerator ctors could. Since the property set can't validate it means that it'd be an exception that pops out of SignedCms.CreateSignature.

I'm not sure that I remember why we added the (X509Certificate2, RSAEncryptionPadding) ctor for CmsRecipient. I think I'm inclined to less-is-more and just having the 4 parameter one.

[vcsjones]

@bartonjs something like that then? (see edit). I also made the constructor parameter a nullable reference type which will mean "Pkcs1" when null.

[bartonjs]

Given how much is marked as nullable in that ctor, I think that there aren't early exceptions (aside from an ArgumentException that the padding is a gibberish value). "Your key object isn't RSA, but you specified RSA padding" would only come from the Sign/CounterSign methods. Oh well, that's the problem with property bag objects.

[dtivel]

Why use AsymmetricAlgorithm and not the more derived RSA type, similar to CertificateRequest(String, RSA, HashAlgorithmName, RSASignaturePadding)?

[vcsjones]

Why use AsymmetricAlgorithm and not the more derived RSA type, similar to CertificateRequest(String, RSA, HashAlgorithmName, RSASignaturePadding)?

Hm, well, we already expose the PrivateKey as a property as AsymmetricAlgorithm. CertificateRequest doesn't have a "what goes in is what comes out" property for the signing key.

runtime/src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/CmsSigner.cs

Line 22 in 51b4e3e

public AsymmetricAlgorithm? PrivateKey { get; set; }

So we could for this one constructor make it RSA which would get assigned to the PrivateKey, but on the other hand the existing constructor and property are AsymmetricAlgorithm.

Having the constructor argument be RSA doesn't mean we can avoid a runtime check because someone could assign the PrivateKey to something other than RSA after it has been constructed but before the signature has been made. Or they don't give a detached private key at all and expect us to grab it from the certificate instance.

On the other hand, typing the constructor arg as RSA? would communicate better that if you give us a detached private key, it needs to be RSA.

[bartonjs]

On the ctor it could certainly be (SubjectIdentifierType, X509Certificate2?, RSA?, RSASignaturePadding?) to give the RSA-relatedness.

Yeah, I might like that better.

[vcsjones]

@bartonjs I took a stab at implementing this over lunch and stumbled upon

runtime/src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/CmsSignature.RSA.cs

Line 282 in 3649506

Debug.Fail("RSA-PSS requires building parameters, which has no API.");

I don't know the context around this Debug.Assert but wanted to make sure there isn't additional API surface that we need to make this work. A quick eyeball of this makes me think... no. While we do need to make sure the SignerInfo gets the right SignatureParameters instead of just Null, I think we will just end up using defaults for everything (Mgf1, salt length == digest length, etc) since we don't support anything but that on the verify side.

[terrajobst]

Video

• Looks good as proposed

namespace System.Security.Cryptography.Pkcs
{
    public partial class CmsSigner
    {
        // Existing ctors:
        // public CmsSigner();
        // public CmsSigner(SubjectIdentifierType signerIdentifierType);
        // public CmsSigner(X509Certificate2? certificate);
        // public CmsSigner(SubjectIdentifierType signerIdentifierType, X509Certificate2? certificate);
        //
        // public CmsSigner(
        //     SubjectIdentifierType signerIdentifierType,
        //     X509Certificate2? certificate,
        //     AsymmetricAlgorithm? privateKey);

        public CmsSigner(
            SubjectIdentifierType signerIdentifierType,
            X509Certificate2? certificate,
            RSA? privateKey,
            RSASignaturePadding? signaturePadding);

        public RSASignaturePadding? SignaturePadding { get; set; }
    }
}

[vcsjones]

My comments were addressed in the API review, so, I'm mostly done with this then and will open a PR soon.