-
Notifications
You must be signed in to change notification settings - Fork 5.2k
104080 custom rsapss salt length #119255
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.
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
base: main
Are you sure you want to change the base?
104080 custom rsapss salt length #119255
Conversation
…ation/generation with RSA keys. dotnet#104080
…lt customizations
…pported there) Updated reference code
Cose does not support custom salt lengths with PSS.
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request adds support for custom PSS salt length for RSA-based signature operations. The implementation extends the RSASignaturePadding
class with a new CreatePss(int saltLength)
method and updates related cryptographic components to utilize custom salt lengths.
Key changes include:
- Extended
RSASignaturePadding
to support custom PSS salt lengths with new constants and factory method - Updated all RSA implementation backends (OpenSSL, CNG, BCrypt, etc.) to handle custom salt lengths
- Added comprehensive test coverage for various salt length scenarios
- Updated high-level APIs like
CmsSigner
andCertificateRequest
to support the new functionality
Reviewed Changes
Copilot reviewed 40 out of 41 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey_rsa.h | Added pssSaltLength parameter to RSA sign/verify functions |
src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey_rsa.c | Implemented custom salt length support in OpenSSL backend |
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/RSASignaturePadding.cs | Added CreatePss method, PssSaltLength property, and related constants |
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/RSAPssX509SignatureGenerator.cs | Updated to use custom salt lengths from padding configuration |
src/libraries/Common/src/System/Security/Cryptography/RsaPaddingProcessor.cs | Modified PSS encoding/verification to accept custom salt lengths |
src/libraries/Common/src/System/Security/Cryptography/RsaPaddingProcessor.DigestInfo.cs | Added salt length calculation logic and moved digest info constants |
src/libraries/System.Security.Cryptography/tests/X509Certificates/CertificateCreation/RSAPssX509SignatureGeneratorTests.cs | Added comprehensive tests for custom PSS salt lengths |
src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/CmsSigner.cs | Updated validation to accept any PSS mode padding |
src/libraries/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseSigner.cs | Added validation to prevent custom salt lengths in COSE |
src/libraries/Common/src/System/Security/Cryptography/RSASecurityTransforms.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/Security/Cryptography/RsaPaddingProcessor.DigestInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography/ref/System.Security.Cryptography.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cng/tests/RSACngPkcs8TestsWithCustomPssSaltLength.cs
Outdated
Show resolved
Hide resolved
...rity.Cryptography/tests/X509Certificates/CertificateCreation/CertificateRequestChainTests.cs
Outdated
Show resolved
Hide resolved
…tes/CertificateCreation/CertificateRequestChainTests.cs Co-authored-by: Copilot <[email protected]>
….Cryptography.cs Co-authored-by: Copilot <[email protected]>
src/libraries/Common/src/Interop/Windows/BCrypt/Interop.BCryptSignHash.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/Interop/Windows/BCrypt/Interop.BCryptVerifySignature.cs
Outdated
Show resolved
Hide resolved
@@ -41,6 +41,9 @@ internal RSASignaturePadding GetSignaturePadding( | |||
HashAlgorithm.Algorithm)); | |||
} | |||
|
|||
#if NET10_0_OR_GREATER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is tricky. Right now, .NET 10 is "done". Technically this work will go in for .NET 11. However, the runtime and SDK are not "bumped" to .NET 11 yet so #if NET11_0_OR_GREATER
doesn't exist. We might need to wait for this work until #118583 is complete.
src/libraries/Common/src/System/Security/Cryptography/RSAAppleCrypto.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/Security/Cryptography/RSAAppleCrypto.cs
Outdated
Show resolved
Hide resolved
...braries/System.Security.Cryptography/src/System/Security/Cryptography/RSASignaturePadding.cs
Outdated
Show resolved
Hide resolved
...braries/System.Security.Cryptography/src/System/Security/Cryptography/RSASignaturePadding.cs
Outdated
Show resolved
Hide resolved
...braries/System.Security.Cryptography/src/System/Security/Cryptography/RSASignaturePadding.cs
Outdated
Show resolved
Hide resolved
...braries/System.Security.Cryptography/src/System/Security/Cryptography/RSASignaturePadding.cs
Outdated
Show resolved
Hide resolved
...rity.Cryptography/tests/X509Certificates/CertificateCreation/CertificateRequestUsageTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made it about halfway, but have to disappear for a bit, so posting what I've already seen.
src/libraries/Common/src/System/Security/Cryptography/RSAAndroid.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/Security/Cryptography/RSAOpenSsl.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/Security/Cryptography/RsaPaddingProcessor.DigestInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/Security/Cryptography/RsaPaddingProcessor.DigestInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/Security/Cryptography/RsaPaddingProcessor.DigestInfo.cs
Outdated
Show resolved
Hide resolved
...braries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/SignVerify.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cng/tests/RSACngPkcs8TestsPssSaltLength.cs
Outdated
Show resolved
Hide resolved
...raries/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseSigner.cs
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/tests/CoseSignerTests.cs
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Pkcs/tests/SignedCms/SignedCmsTests.netcoreapp.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Pkcs/tests/SignedCms/SignedCmsWholeDocumentTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Pkcs/tests/SignedCms/SignedCmsWholeDocumentTests.cs
Outdated
Show resolved
Hide resolved
...urity.Cryptography/tests/X509Certificates/CertificateCreation/CertificateRequestLoadTests.cs
Outdated
Show resolved
Hide resolved
...urity.Cryptography/tests/X509Certificates/CertificateCreation/CertificateRequestLoadTests.cs
Show resolved
Hide resolved
...Cryptography/tests/X509Certificates/CertificateCreation/RSAPssX509SignatureGeneratorTests.cs
Outdated
Show resolved
Hide resolved
...Cryptography/tests/X509Certificates/CertificateCreation/RSAPssX509SignatureGeneratorTests.cs
Outdated
Show resolved
Hide resolved
...Cryptography/tests/X509Certificates/CertificateCreation/RSAPssX509SignatureGeneratorTests.cs
Outdated
Show resolved
Hide resolved
...braries/System.Security.Cryptography/src/System/Security/Cryptography/RSASignaturePadding.cs
Show resolved
Hide resolved
...urity.Cryptography/tests/X509Certificates/CertificateCreation/CertificateRequestLoadTests.cs
Outdated
Show resolved
Hide resolved
...s/System.Security.Cryptography/tests/X509Certificates/CertificateCreation/CrlBuilderTests.cs
Outdated
Show resolved
Hide resolved
public static RSASignaturePadding CreatePss(int saltLength) | ||
{ | ||
switch (saltLength) | ||
{ | ||
case PssSaltLengthIsHashLength or PssSaltLengthMax: | ||
case > 0: | ||
case >= 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that a test that had been expecting zero to fail has been removed. But I don't see any tests showing
- That 0 is allowed here
- That 0 works when directly using the type
- That 0 works when used with CertificateRequest
- That 0 works when used with SignedCms
Please add those tests (or show me where I missed them)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Added test to RSASignaturePadding_Constructor_ValidParameters
- PssRoundTripParameters already includes 0.
- CreateSignature_RsaPss already includes a test for 0 (via CreateSignature_RsaPssTests)
- Added a test for 0 in SelfSign_RSA_PssPadding_CustomSaltLength
// Apple does not support custom salt length for the PSS padding | ||
if (padding.Mode == RSASignaturePaddingMode.Pss && | ||
(padding.PssSaltLength != RSASignaturePadding.PssSaltLengthIsHashLength && | ||
padding.PssSaltLength != RsaPaddingProcessor.HashLength(hashAlgorithm))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the tests for Apple say that customizing the salt size isn't supported, but this says that a specified salt size is allowed if it matches the PssSaltLengthIsHashLength calculated size.
That suggests that we're missing negative tests (e.g. we want to see the exception when calling Apple), and that we don't have a test for when it matches.
If I've just missed the tests, feel free to point them out to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Added a test PssUnsupportedPssSaltLengthOnApple in SignVerify
- I've updated the PssRoundTripParameters to return test values with the salt length matching the hash length. This should cover the positive test.
Added support for custom PSS salt length for RSA based signature operations. Resolves #104080.
With this PR, I have extended the RsaSignaturePadding class with a custom salt length as discussed in #104080. I've also updated the CmsSigner and the CertificateRequest class to support this.
CoseSigner did not need functionality updates, because the spec does not support custom salt length. I have added checks to prevent this to be configured on the CoseSigner.
Tests where either added or updated to test the new functionality.
All RSA implementations which support this (MAC doesn't seem to support this) were updated.