Skip to content

Shorten widths of BoxedUints #506

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

Merged
merged 5 commits into from
Apr 28, 2025
Merged

Shorten widths of BoxedUints #506

merged 5 commits into from
Apr 28, 2025

Conversation

fjarri
Copy link
Contributor

@fjarri fjarri commented Apr 23, 2025

Fixes #490

  • Bump crypto-bigint dependency to the current trunk.
  • Use the width of a single prime instead of the full modulus where possible. Brings bench_rsa_2048_pkcsv1_decrypt to pre-crypto-bigint values.
  • Add a check to RsaPrivateKey::from_components() to ensure consistency between the primes and the modulus.
  • Remove zero padding limbs from the primes and the modulus in RsaPrivateKey::from_components().
  • Add a check to rsa_decrypt() to ensure the bit precision of the ciphertext is the same as that of the modulus.

Notes:

  • bench_rsa_2048_pkcsv1_sign_blinded can be restored to the original performance by using variable-time inversion in algorithms::rsa::blind() (as it was during num-bigint times), but it seems to me that the blinding factor must be kept secret, so we have to use the constant-time inversion. This leads to about 5x slowdown compared to pre-crypto-bigint performance.
  • The changes in test vectors are due to Make random_bits_core platform independent crypto-bigint#781

Possible further improvements

@fjarri fjarri force-pushed the widths branch 3 times, most recently from cbc5789 to 7305628 Compare April 24, 2025 17:07
@fjarri fjarri marked this pull request as ready for review April 24, 2025 17:16
@tarcieri
Copy link
Member

The r^(-1) mod n was variable-time before crypto-bigint; is that secure?

Everything was variable-time before crypto-bigint, and to my knowledge it's insecure. It would probably be best to make it constant time despite the performance hit.

Hopefully we'll get better performance after a migration to bingcd

@tarcieri
Copy link
Member

  • The new try_set_precision() may be a method in BoxedUint
  • shorten/widen may need some fallible equivalents

So if I understand the goal of these, it's to prevent truncating the contents of a value, correct?

If so, adding them upstream seems fine. Perhaps they could be made constant-time?

@fjarri
Copy link
Contributor Author

fjarri commented Apr 25, 2025

Everything was variable-time before crypto-bigint, and to my knowledge it's insecure. It would probably be best to make it constant time despite the performance hit.

That's unfortunate, it's like a 5x slowdown.

So if I understand the goal of these, it's to prevent truncating the contents of a value, correct?

Yep.

@tarcieri
Copy link
Member

tarcieri commented Apr 25, 2025

@fjarri do you want to try to upstream try_set_precision and e.g. try_shorten first, or merge this as-is and follow up on that later?

@fjarri
Copy link
Contributor Author

fjarri commented Apr 25, 2025

Let me try it out, I'll see if I can distill some required functionality into general-use methods.

As for the inversion, I need to check if preparing the inverter in advance makes things any better.

@tarcieri
Copy link
Member

It’s unlikely to impact performance aside from optimizing away a single allocation

@fjarri
Copy link
Contributor Author

fjarri commented Apr 25, 2025

Converting to draft until we merge RustCrypto/crypto-bigint#809 in some form

tarcieri pushed a commit to RustCrypto/crypto-bigint that referenced this pull request Apr 27, 2025
This PR attempts to simplify some scenarios that were encountered during
the work on RustCrypto/RSA#506.

- Instead of having `BoxedUint::shorten()`/`widen()` methods, introduce
the `Resize` trait with more convenience methods. Implemented for
`BoxedUint`, `NonZero<BoxedUint>`, `Odd<BoxedUint>`, and their
references.
- `BoxedUint::shorten()` and `widen()` are marked as deprecated.

Note that `try_resize` fails if `self.bits() > at_least_bits_precision`,
unlike the more relaxed `shorten()` behavior.
@tarcieri tarcieri merged commit 96e059c into RustCrypto:master Apr 28, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slowdown after switch to crypto-bigint
2 participants