-
-
Notifications
You must be signed in to change notification settings - Fork 9.9k
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
signature: Clamp PSS salt len to MD len #19724
signature: Clamp PSS salt len to MD len #19724
Conversation
The CI failure is relevant. |
I'm not sure how to solve the failure with the old provider. The issue here is that the provider computes the salt length independently from the routines that write the CMS, so while the signature generated by the old provider has a 222 bytes salt, the CMS will say it has a 32 byte salt. Any pointers on how to not compute the salt length twice (and thus incorrectly), and instead obtain it from the signature or the provider? |
I am afraid this implies we cannot change this. |
IMO the solution would be to use RSA_PSS_SALTLEN_DIGEST as the default or to invent another default value. |
That will not work, because for some combinations of key length and digest (e.g. SHA-256 and RSA-512, or SHA-512 and RSA-1024) , RSA_PSS_SALTLEN_DIGEST is too long. Using a different value will also fail with the older providers. However, I think it might be possible to obtain the correct value from the provider using |
The problem is third party applications might depend on the value the same way as our CMS implementation did. |
823238c
to
f7fd2c2
Compare
It would fix the problem with the failing CI probably and yeah, it should have been done like that. However it won't solve the possible compatibility problem with 3rd party apps. |
@t8m as it's a FIPS requirement, we definitely need smth in this direction |
The different default value would be a different default value inside the provider context - IMO that should work for new provider with new libcrypto. It would not work for combination of old libcrypto with new provider though because the value would be unknown to new libcrypto. |
That something could be a sentence in the security policy. |
We're being told this is no longer enough. We can't wiggle our way out of this in the security policy, it either needs to be an explicit indicator (which we're going to ship downstream along with this change), or an implicit indicator (i.e., it must fail). Breaking the few applications that were relying on the |
Putting an OTC hold on it to discuss it on regular meeting |
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.
Pretty sure we discussed this within OTC already when I added this. It was done this way in order to not break things.
f7fd2c2
to
280cd94
Compare
I've changed this to introduce a new option RSA_PSS_SALTLEN_AUTO_DIGEST_MAX and switch the CMS routines to determine the salt length from the provider's implementation. This preserves compatibility with older providers, while still supporting the new value. |
crypto/rsa/rsa_ameth.c
Outdated
|
||
if (EVP_PKEY_CTX_get_signature_md(pkctx, &sigmd) <= 0) | ||
return NULL; | ||
if (EVP_PKEY_CTX_get_rsa_mgf1_md(pkctx, &mgf1md) <= 0) | ||
return NULL; | ||
if (EVP_PKEY_CTX_get_rsa_pss_saltlen(pkctx, &saltlen) <= 0) | ||
return NULL; | ||
if (saltlen == -1) { | ||
if (saltlen == RSA_PSS_SALTLEN_DIGEST || saltlen == RSA_PSS_SALTLEN_AUTO) { |
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.
Why is SALTLEN_AUTO here now and not equivalent to SALTLEN_MAX?
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.
You're right, that's an oversight because I didn't review the diff against master again. I'll fix that.
@@ -188,22 +188,36 @@ static void *rsa_newctx(void *provctx, const char *propq) | |||
prsactx->libctx = PROV_LIBCTX_OF(provctx); | |||
prsactx->flag_allow_md = 1; | |||
prsactx->propq = propq_copy; | |||
/* Maximum for sign, auto for verify */ | |||
prsactx->saltlen = RSA_PSS_SALTLEN_AUTO; | |||
/* Maximum until digest lnegth for sign, auto for verify */ |
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.
up to digest length
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.
[x] done
@@ -407,8 +421,8 @@ static int rsa_signverify_init(void *vprsactx, void *vrsa, | |||
|
|||
prsactx->operation = operation; | |||
|
|||
/* Maximum for sign, auto for verify */ | |||
prsactx->saltlen = RSA_PSS_SALTLEN_AUTO; | |||
/* Maximize until digest length for sign, auto for verify */ |
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.
up to
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.
[x] done
I think this will still break the Provider versions compat CI that is run on push as the libcrypto from 3.0.7 won't be able to cope with the changed default in the fips provider from master. |
280cd94
to
6eda3de
Compare
I think that can be fixed by cherry-picking |
I'd be OK with this solution. Still, it's better if we discuss it in OTC first. |
For your information, I think the reason why NIST has this salt length limitation is this paragraph in PKCS #1 v2.1 on page 34 (emphasis mine):
|
OTC: CMS code bug fix should be fixed in 3.0 and above, the default for salt length can be changed in 3.1 and above. |
FIPS 186-4 section 5 "The RSA Digital Signature Algorithm", subsection 5.5 "PKCS openssl#1" says: "For RSASSA-PSS […] the length (in bytes) of the salt (sLen) shall satisfy 0 <= sLen <= hLen, where hLen is the length of the hash function output block (in bytes)." Introduce a new option RSA_PSS_SALTLEN_AUTO_DIGEST_MAX and make it the default. The new value will behave like RSA_PSS_SALTLEN_AUTO, but will not use more than the digest length when signing, so that FIPS 186-4 is not violated. This value has two advantages when compared with RSA_PSS_SALTLEN_DIGEST: (1) It will continue to do auto-detection when verifying signatures for maximum compatibility, where RSA_PSS_SALTLEN_DIGEST would fail for other digest sizes. (2) It will work for combinations where the maximum salt length is smaller than the digest size, which typically happens with large digest sizes (e.g., SHA-512) and small RSA keys. J.-S. Coron shows in "Optimal Security Proofs for PSS and Other Signature Schemes. Advances in Cryptology – Eurocrypt 2002, volume 2332 of Lecture Notes in Computer Science, pp. 272 – 287. Springer Verlag, 2002." that longer salts than the output size of modern hash functions do not increase security: "For example,for an application in which at most one billion signatures will be generated, k0 = 30 bits of random salt are actually sufficient to guarantee the same level of security as RSA, and taking a larger salt does not increase the security level." Signed-off-by: Clemens Lang <cllang@redhat.com>
a73ebe2
to
89bfb0c
Compare
CI failure in CIFuzz/Fuzzing seems unrelated to me. |
@beldmit please reconfirm. |
Reconfirm. |
This pull request is ready to merge |
Rather than computing the PSS salt length again in core using ossl_rsa_ctx_to_pss_string, which calls rsa_ctx_to_pss and computes the salt length, obtain it from the provider using the OSSL_SIGNATURE_PARAM_ALGORITHM_ID param to handle the case where the interpretation of the magic constants in the provider differs from that of OpenSSL core. Add tests that verify that the rsa_pss_saltlen:max, rsa_pss_saltlen:<integer> and rsa_pss_saltlen:digest options work and put the computed digest length into the CMS_ContentInfo struct when using CMS. Do not add a test for the salt length generated by a provider when no specific rsa_pss_saltlen option is defined, since that number could change between providers and provider versions, and we want to preserve compatibility with older providers. Signed-off-by: Clemens Lang <cllang@redhat.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19724)
FIPS 186-4 section 5 "The RSA Digital Signature Algorithm", subsection 5.5 "PKCS #1" says: "For RSASSA-PSS […] the length (in bytes) of the salt (sLen) shall satisfy 0 <= sLen <= hLen, where hLen is the length of the hash function output block (in bytes)." Introduce a new option RSA_PSS_SALTLEN_AUTO_DIGEST_MAX and make it the default. The new value will behave like RSA_PSS_SALTLEN_AUTO, but will not use more than the digest length when signing, so that FIPS 186-4 is not violated. This value has two advantages when compared with RSA_PSS_SALTLEN_DIGEST: (1) It will continue to do auto-detection when verifying signatures for maximum compatibility, where RSA_PSS_SALTLEN_DIGEST would fail for other digest sizes. (2) It will work for combinations where the maximum salt length is smaller than the digest size, which typically happens with large digest sizes (e.g., SHA-512) and small RSA keys. J.-S. Coron shows in "Optimal Security Proofs for PSS and Other Signature Schemes. Advances in Cryptology – Eurocrypt 2002, volume 2332 of Lecture Notes in Computer Science, pp. 272 – 287. Springer Verlag, 2002." that longer salts than the output size of modern hash functions do not increase security: "For example,for an application in which at most one billion signatures will be generated, k0 = 30 bits of random salt are actually sufficient to guarantee the same level of security as RSA, and taking a larger salt does not increase the security level." Signed-off-by: Clemens Lang <cllang@redhat.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19724)
Merged to master branch. Unfortunately this does not cherry-pick cleanly to 3.1. @neverpanic Could you please create pull requests against 3.1 and 3.0 (CMS fix only) branches? |
Rather than computing the PSS salt length again in core using ossl_rsa_ctx_to_pss_string, which calls rsa_ctx_to_pss and computes the salt length, obtain it from the provider using the OSSL_SIGNATURE_PARAM_ALGORITHM_ID param to handle the case where the interpretation of the magic constants in the provider differs from that of OpenSSL core. Add tests that verify that the rsa_pss_saltlen:max, rsa_pss_saltlen:<integer> and rsa_pss_saltlen:digest options work and put the computed digest length into the CMS_ContentInfo struct when using CMS. Do not add a test for the salt length generated by a provider when no specific rsa_pss_saltlen option is defined, since that number could change between providers and provider versions, and we want to preserve compatibility with older providers. Signed-off-by: Clemens Lang <cllang@redhat.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19724) (cherry picked from commit 5a3bbe1)
FIPS 186-4 section 5 "The RSA Digital Signature Algorithm", subsection 5.5 "PKCS openssl#1" says: "For RSASSA-PSS […] the length (in bytes) of the salt (sLen) shall satisfy 0 <= sLen <= hLen, where hLen is the length of the hash function output block (in bytes)." Introduce a new option RSA_PSS_SALTLEN_AUTO_DIGEST_MAX and make it the default. The new value will behave like RSA_PSS_SALTLEN_AUTO, but will not use more than the digest length when signing, so that FIPS 186-4 is not violated. This value has two advantages when compared with RSA_PSS_SALTLEN_DIGEST: (1) It will continue to do auto-detection when verifying signatures for maximum compatibility, where RSA_PSS_SALTLEN_DIGEST would fail for other digest sizes. (2) It will work for combinations where the maximum salt length is smaller than the digest size, which typically happens with large digest sizes (e.g., SHA-512) and small RSA keys. J.-S. Coron shows in "Optimal Security Proofs for PSS and Other Signature Schemes. Advances in Cryptology – Eurocrypt 2002, volume 2332 of Lecture Notes in Computer Science, pp. 272 – 287. Springer Verlag, 2002." that longer salts than the output size of modern hash functions do not increase security: "For example,for an application in which at most one billion signatures will be generated, k0 = 30 bits of random salt are actually sufficient to guarantee the same level of security as RSA, and taking a larger salt does not increase the security level." Signed-off-by: Clemens Lang <cllang@redhat.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19724) (cherry picked from commit 6c73ca4)
Rather than computing the PSS salt length again in core using ossl_rsa_ctx_to_pss_string, which calls rsa_ctx_to_pss and computes the salt length, obtain it from the provider using the OSSL_SIGNATURE_PARAM_ALGORITHM_ID param to handle the case where the interpretation of the magic constants in the provider differs from that of OpenSSL core. Add tests that verify that the rsa_pss_saltlen:max, rsa_pss_saltlen:<integer> and rsa_pss_saltlen:digest options work and put the computed digest length into the CMS_ContentInfo struct when using CMS. Do not add a test for the salt length generated by a provider when no specific rsa_pss_saltlen option is defined, since that number could change between providers and provider versions, and we want to preserve compatibility with older providers. Signed-off-by: Clemens Lang <cllang@redhat.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19724) (cherry picked from commit 5a3bbe1)
Rather than computing the PSS salt length again in core using ossl_rsa_ctx_to_pss_string, which calls rsa_ctx_to_pss and computes the salt length, obtain it from the provider using the OSSL_SIGNATURE_PARAM_ALGORITHM_ID param to handle the case where the interpretation of the magic constants in the provider differs from that of OpenSSL core. Add tests that verify that the rsa_pss_saltlen:max, rsa_pss_saltlen:<integer> and rsa_pss_saltlen:digest options work and put the computed digest length into the CMS_ContentInfo struct when using CMS. Do not add a test for the salt length generated by a provider when no specific rsa_pss_saltlen option is defined, since that number could change between providers and provider versions, and we want to preserve compatibility with older providers. Signed-off-by: Clemens Lang <cllang@redhat.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19724)
FIPS 186-4 section 5 "The RSA Digital Signature Algorithm", subsection 5.5 "PKCS #1" says: "For RSASSA-PSS […] the length (in bytes) of the salt (sLen) shall satisfy 0 <= sLen <= hLen, where hLen is the length of the hash function output block (in bytes)." Introduce a new option RSA_PSS_SALTLEN_AUTO_DIGEST_MAX and make it the default. The new value will behave like RSA_PSS_SALTLEN_AUTO, but will not use more than the digest length when signing, so that FIPS 186-4 is not violated. This value has two advantages when compared with RSA_PSS_SALTLEN_DIGEST: (1) It will continue to do auto-detection when verifying signatures for maximum compatibility, where RSA_PSS_SALTLEN_DIGEST would fail for other digest sizes. (2) It will work for combinations where the maximum salt length is smaller than the digest size, which typically happens with large digest sizes (e.g., SHA-512) and small RSA keys. J.-S. Coron shows in "Optimal Security Proofs for PSS and Other Signature Schemes. Advances in Cryptology – Eurocrypt 2002, volume 2332 of Lecture Notes in Computer Science, pp. 272 – 287. Springer Verlag, 2002." that longer salts than the output size of modern hash functions do not increase security: "For example,for an application in which at most one billion signatures will be generated, k0 = 30 bits of random salt are actually sufficient to guarantee the same level of security as RSA, and taking a larger salt does not increase the security level." Signed-off-by: Clemens Lang <cllang@redhat.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19724)
Rather than computing the PSS salt length again in core using ossl_rsa_ctx_to_pss_string, which calls rsa_ctx_to_pss and computes the salt length, obtain it from the provider using the OSSL_SIGNATURE_PARAM_ALGORITHM_ID param to handle the case where the interpretation of the magic constants in the provider differs from that of OpenSSL core. Add tests that verify that the rsa_pss_saltlen:max, rsa_pss_saltlen:<integer> and rsa_pss_saltlen:digest options work and put the computed digest length into the CMS_ContentInfo struct when using CMS. Do not add a test for the salt length generated by a provider when no specific rsa_pss_saltlen option is defined, since that number could change between providers and provider versions, and we want to preserve compatibility with older providers. Signed-off-by: Clemens Lang <cllang@redhat.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19724)
FIPS 186-4 section 5 "The RSA Digital Signature Algorithm", subsection 5.5 "PKCS openssl#1" says: "For RSASSA-PSS […] the length (in bytes) of the salt (sLen) shall satisfy 0 <= sLen <= hLen, where hLen is the length of the hash function output block (in bytes)." Introduce a new option RSA_PSS_SALTLEN_AUTO_DIGEST_MAX and make it the default. The new value will behave like RSA_PSS_SALTLEN_AUTO, but will not use more than the digest length when signing, so that FIPS 186-4 is not violated. This value has two advantages when compared with RSA_PSS_SALTLEN_DIGEST: (1) It will continue to do auto-detection when verifying signatures for maximum compatibility, where RSA_PSS_SALTLEN_DIGEST would fail for other digest sizes. (2) It will work for combinations where the maximum salt length is smaller than the digest size, which typically happens with large digest sizes (e.g., SHA-512) and small RSA keys. J.-S. Coron shows in "Optimal Security Proofs for PSS and Other Signature Schemes. Advances in Cryptology – Eurocrypt 2002, volume 2332 of Lecture Notes in Computer Science, pp. 272 – 287. Springer Verlag, 2002." that longer salts than the output size of modern hash functions do not increase security: "For example,for an application in which at most one billion signatures will be generated, k0 = 30 bits of random salt are actually sufficient to guarantee the same level of security as RSA, and taking a larger salt does not increase the security level." Signed-off-by: Clemens Lang <cllang@redhat.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19724)
FIPS 186-4 section 5 "The RSA Digital Signature Algorithm", subsection 5.5 "PKCS #1" says: "For RSASSA-PSS […] the length (in bytes) of the salt (sLen) shall satisfy 0 <= sLen <= hLen, where hLen is the length of the hash function output block (in bytes)."
Switch the meaning of RSA_PSS_SALTLEN_AUTO to use at most the digest length, so that the default does not violate FIPS 186-4. Shorter values are still possible when long hashes are used with short keys.
Longer values can be created by using a specific value, or RSA_PSS_SALTLEN_MAX.
Checklist