Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const uint8_t *EM, int sLen, int min_sLen) { | ||
| // We have to avoid the underlying SHA services updating the indicator | ||
| // state, so we lock the state here. | ||
| FIPS_service_indicator_lock_state(); |
There was a problem hiding this comment.
warning: call to undeclared function 'FIPS_service_indicator_lock_state'; ISO C99 and later do not support implicit function declarations [clang-diagnostic-implicit-function-declaration]
FIPS_service_indicator_lock_state();
^
|
🔒 Security Review — View Report Please review before merging. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3470 +/- ##
==========================================
- Coverage 78.36% 78.19% -0.17%
==========================================
Files 700 700
Lines 125744 125784 +40
Branches 17388 17388
==========================================
- Hits 98540 98360 -180
- Misses 26332 26554 +222
+ Partials 872 870 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RSA_PSS_SALTLEN_AUTO on an EVP_PKEY_RSA_PSS key recovered any salt on verify, bypassing the key's minimum. Check the recovered length against min_saltlen, and keep that restriction across EVP_PKEY_CTX_dup.
6887ca7 to
26a6bb1
Compare
| // A restricted |EVP_PKEY_RSA_PSS| key also sets a minimum salt length. | ||
| if (min_sLen >= 0 && recovered_sLen < (size_t)min_sLen) { |
There was a problem hiding this comment.
The floor here is correct, but it makes an invariant load-bearing that is currently spelled out three separate times.
AUTO on a restricted key must produce a salt >= min_saltlen when signing, or we would emit PSS signatures that this new check then refuses. That holds only because the "maximum PSS salt length" computation agrees everywhere it appears:
| Site | Formula | Decrement condition |
|---|---|---|
p_rsa.c:106-108 (pkey_pss_init) |
RSA_size(rsa) - EVP_MD_size(md) - 2 |
(RSA_bits(rsa) & 0x7) == 1 |
padding.c:304-325 (RSA_padding_add_PKCS1_PSS_mgf1, AUTO) |
emLen - hLen - 2, emLen = RSA_size(rsa) |
MSBits == 0, MSBits = (BN_num_bits(rsa->n) - 1) & 0x7 |
x509/rsa_pss.c:169-172 (x509_rsa_ctx_to_pss, AUTO) |
EVP_PKEY_size(pk) - EVP_MD_size(sigmd) - 2 |
((EVP_PKEY_bits(pk) - 1) & 0x7) == 0 |
All three are the same predicate -- bits = 1 (mod 8) -- in three different spellings, so they do agree today and "signing is unaffected" is accurate as written. Before this PR a divergence was a cosmetic inconsistency; now it would mean producing signatures we cannot verify.
Could we collapse them into one helper? Something like this in crypto/fipsmodule/rsa/internal.h, defined once in this file:
// rsa_pss_max_saltlen returns the maximum PSS salt length usable with |rsa| and
// a |hLen|-byte digest, or a negative value if |rsa| is too small. This is the
// value |RSA_PSS_SALTLEN_AUTO| resolves to when signing.
int rsa_pss_max_saltlen(const RSA *rsa, size_t hLen);pkey_pss_init and the AUTO branch here are both inside the module boundary, so sharing between them costs nothing. x509_rsa_ctx_to_pss is outside it and works from an EVP_PKEY *, so folding that one in is optional -- a TODO pointing at the helper would already be an improvement.
If you would rather keep this PR tight, the alternative is a round-trip case in the new test: EVP_PKEY_sign_init + EVP_PKEY_CTX_set_rsa_pss_saltlen(ctx, RSA_PSS_SALTLEN_AUTO) + sign + verify with AUTO on the same restricted key. That pins the behaviour even if the formulas stay duplicated. The new test is thorough but it only signs via RSA_sign_pss_mgf1 at the RSA layer -- the EVP sign path with AUTO, which is what the "unaffected" claim is about, is never exercised.
| return rsa_verify_pss_mgf1(rsa, tbs, tbslen, rctx->md, rctx->mgf1md, | ||
| rctx->saltlen, rctx->min_saltlen, sig, |
There was a problem hiding this comment.
Nit: this passes rctx->min_saltlen, whose "no restriction" sentinel is NO_PSS_SALT_LEN_RESTRICTION (-1, top of this file), into a parameter documented against a different macro, RSA_PSS_NO_SALTLEN_MINIMUM (-1, fipsmodule/rsa/internal.h).
It is correct today, and robust rather than accidental, since the callee's doc says "like any negative value, imposes no minimum". But two names for one sentinel across the evp/rsa boundary invites drift. Could we either use the rsa/internal.h macro here, or note the correspondence at this call site?
3 participants
Context and motivation
An
EVP_PKEY_RSA_PSSkey can restrict the minimum salt length. That minimum was enforced everywhere exceptRSA_PSS_SALTLEN_AUTO, where verify recovered the salt length from the signature and accepted any value, including zero.EVP_PKEY_CTX_dupalso droppedmin_saltlen, so a duplicated context lost the restriction entirely.Description of changes
RSA_PSS_SALTLEN_AUTOremains settable on a restricted key, but verify now checks the recovered salt length against the key's minimum, plumbed through new module-internal variants ofRSA_verify_pss_mgf1andRSA_verify_PKCS1_PSS_mgf1. The public functions pass "no minimum" and are unchanged.pkey_rsa_copynow propagatesmin_saltlen. The rest is readability: a publicRSA_PSS_SALTLEN_AUTO(-2) alongside the existingRSA_PSS_SALTLEN_DIGEST, replacing the bare-2literals and comments.Two non-obvious points:
EVP_PKEY_CTX_set_rsa_pss_saltlentime for a restricted verify; we accept it and enforce the floor on the recovered length instead, keeping AUTO's meaning intact. So we accept any salt at or above the minimum where OpenSSL requires exactly it, and the failure surfaces fromEVP_PKEY_verifyrather than the setter. Documented there.pkey_pss_initalready rejects a key whose minimum exceeds that maximum, so signatures we produce still verify.Testing
New
EVPExtraTest.RestrictedPssAutoSaltlenHonorsMinimumuseskExampleRSAPSSKeyPKCS8, which omitssaltLengthand so carries the RFC 4055 default minimum of 20. It signs at the RSA layer with salt lengths 0, 1, 19, 20 and 24, then verifies each throughEVP_PKEY_verifywith AUTO set, on both the original and a duplicated context: below the minimum must fail, at or above must pass.Each half of the fix was reverted in turn to confirm the test catches it -- disabling the floor check lets the short-salt signatures verify, dropping the
min_saltlencopy breaks the duplicated context, and turning the floor into an equality test breaks the salt-24 case.Review considerations
EVP_PKEY_RSA_PSSkeys verifying with AUTO, where a verify can now fail that previously succeeded. That is the intent, but it is a tightening.crypto/fipsmodule, and service indicator handling is unchanged.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.