| Message ID | 20240121194901.344206-1-git@jvdsn.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: rsa - restrict plaintext/ciphertext values more in FIPS mode | expand |
On Sun, Jan 21, 2024 at 01:49:00PM -0600, Joachim Vandersmissen wrote: > > static int _rsa_enc(const struct rsa_mpi_key *key, MPI c, MPI m) > { > + /* For FIPS, SP 800-56Br2, Section 7.1.1 requires 1 < m < n - 1 */ > + if (fips_enabled && rsa_check_payload_fips(m, key->n)) > + return -EINVAL; > + > /* (1) Validate 0 <= m < n */ > if (mpi_cmp_ui(m, 0) < 0 || mpi_cmp(m, key->n) >= 0) > return -EINVAL; I think this check makes sense in general, so why not simply replace the second check above with the new check? Thanks,
Hi Herbert, On 1/25/24 23:58, Herbert Xu wrote: > On Sun, Jan 21, 2024 at 01:49:00PM -0600, Joachim Vandersmissen wrote: >> static int _rsa_enc(const struct rsa_mpi_key *key, MPI c, MPI m) >> { >> + /* For FIPS, SP 800-56Br2, Section 7.1.1 requires 1 < m < n - 1 */ >> + if (fips_enabled && rsa_check_payload_fips(m, key->n)) >> + return -EINVAL; >> + >> /* (1) Validate 0 <= m < n */ >> if (mpi_cmp_ui(m, 0) < 0 || mpi_cmp(m, key->n) >= 0) >> return -EINVAL; > I think this check makes sense in general, so why not simply > replace the second check above with the new check? Yes, mathematically speaking the values 1 and n - 1 aren't suitable for RSA (they will always be fixed points). I simply didn't want to introduce a breaking change. If you think a breaking change is acceptable, I can update the patch to replace the RFC3447 check with the stricter check. > > Thanks,
On Fri, Jan 26, 2024 at 12:13:00AM -0600, Joachim Vandersmissen wrote: > > Yes, mathematically speaking the values 1 and n - 1 aren't suitable for RSA > (they will always be fixed points). I simply didn't want to introduce a > breaking change. If you think a breaking change is acceptable, I can update > the patch to replace the RFC3447 check with the stricter check. Please do. We can always change it later if someone complains. Thanks,
diff --git a/crypto/rsa.c b/crypto/rsa.c index b9cd11fb7d36..b5c67e6f8774 100644 --- a/crypto/rsa.c +++ b/crypto/rsa.c @@ -24,12 +24,36 @@ struct rsa_mpi_key { MPI qinv; }; +static int rsa_check_payload_fips(MPI x, MPI n) +{ + MPI n1; + + if (mpi_cmp_ui(x, 1) <= 0) + return -EINVAL; + + n1 = mpi_alloc(0); + if (!n1) + return -ENOMEM; + + if (mpi_sub_ui(n1, n, 1) || mpi_cmp(x, n1) >= 0) { + mpi_free(n1); + return -EINVAL; + } + + mpi_free(n1); + return 0; +} + /* * RSAEP function [RFC3447 sec 5.1.1] * c = m^e mod n; */ static int _rsa_enc(const struct rsa_mpi_key *key, MPI c, MPI m) { + /* For FIPS, SP 800-56Br2, Section 7.1.1 requires 1 < m < n - 1 */ + if (fips_enabled && rsa_check_payload_fips(m, key->n)) + return -EINVAL; + /* (1) Validate 0 <= m < n */ if (mpi_cmp_ui(m, 0) < 0 || mpi_cmp(m, key->n) >= 0) return -EINVAL; @@ -50,6 +74,11 @@ static int _rsa_dec_crt(const struct rsa_mpi_key *key, MPI m_or_m1_or_h, MPI c) MPI m2, m12_or_qh; int ret = -ENOMEM; + /* For FIPS, SP 800-56Br2, Section 7.1.2 requires 1 < c < n - 1 */ + if (fips_enabled && rsa_check_payload_fips(c, key->n)) + return -EINVAL; + + /* (1) Validate 0 <= c < n */ if (mpi_cmp_ui(c, 0) < 0 || mpi_cmp(c, key->n) >= 0) return -EINVAL;
SP 800-56Br2, Section 7.1.1 [1] specifies that: 1. If m does not satisfy 1 < m < (n – 1), output an indication that m is out of range, and exit without further processing. Similarly, Section 7.1.2 of the same standard specifies that: 1. If the ciphertext c does not satisfy 1 < c < (n – 1), output an indication that the ciphertext is out of range, and exit without further processing. [1] https://doi.org/10.6028/NIST.SP.800-56Br2 Signed-off-by: Joachim Vandersmissen <git@jvdsn.com> --- crypto/rsa.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)