feat: switch from num-bigint-dig to crypto-bigint #394

dignifiedquire · 2023-11-29T13:23:38Z

~~Very, very WIP~~

Not anymore, this is ready for review.
Replaces all usage of num-bigint-dig based BigInt usage with the new crypto-bigint crate, using BoxedUint

Current known issue is that we do have a performance regression, which will be able to get rid of over time:

# crypto-bigint

# macbook m1
test bench_rsa_2048_pkcsv1_decrypt      ... bench:   7,184,387.50 ns/iter (+/- 425,598.69)
test bench_rsa_2048_pkcsv1_sign_blinded ... bench:  13,453,579.10 ns/iter (+/- 686,276.31)

# AMD
test bench_rsa_2048_pkcsv1_decrypt      ... bench:   9,260,832.80 ns/iter (+/- 30,013.38)
test bench_rsa_2048_pkcsv1_sign_blinded ... bench:  16,610,079.40 ns/iter (+/- 251,292.53)

# master

# macbook m1
test bench_rsa_2048_pkcsv1_decrypt      ... bench:   1,117,479.15 ns/iter (+/- 31,334.30)
test bench_rsa_2048_pkcsv1_sign_blinded ... bench:   1,337,437.55 ns/iter (+/- 88,624.39)

# AMD
test bench_rsa_2048_pkcsv1_decrypt      ... bench:   1,414,348.80 ns/iter (+/- 12,585.71)
test bench_rsa_2048_pkcsv1_sign_blinded ... bench:   1,685,650.00 ns/iter (+/- 11,105.71)

TODOs

src/algorithms/rsa.rs

dignifiedquire · 2024-12-01T21:33:10Z

CI is finally green again, time for lots of review and cleanup

dignifiedquire · 2024-12-02T09:15:03Z

@zeerooth nostd builds should be working again

dignifiedquire · 2024-12-02T09:19:57Z

src/algorithms/generate.rs

        let mut pi = prime_limit / (prime_limit.ln() - 1f64);
+        #[cfg(not(feature = "std"))]
+        let mut pi = prime_limit / (libm::logf(prime_limit as f32) as f64 - 1f64);


this is not entirely correct or optimal, but I am not sure what other way there is

@fjarri is there anything in crypto-primes for this?

No, nothing like that currently

@dignifiedquire so if I understand correctly, the purpose of this calculation is ultimately for a check that keygen terminates in a reasonable amount of time?

tarcieri · 2024-12-19T16:52:37Z

@dignifiedquire seems like you can remove "Very, very WIP" now

dignifiedquire · 2024-12-19T17:40:35Z

@dignifiedquire seems like you can remove "Very, very WIP" now

🤣 yeah, finally

Snowiiii · 2025-01-02T14:38:38Z

@dignifiedquire how it's going ?

tarcieri · 2025-01-02T14:39:23Z

He's waiting on review, which I'll try to do soon

tarcieri · 2025-01-04T19:29:50Z

Cargo.toml

 digest = { version = "=0.11.0-pre.9", default-features = false, features = ["alloc", "oid"] }
 pkcs1 = { version = "0.8.0-rc.0", default-features = false, features = ["alloc", "pkcs8"] }
 pkcs8 = { version = "0.11.0-rc.0", default-features = false, features = ["alloc"] }
 signature = { version = "=2.3.0-pre.4", default-features = false, features = ["alloc", "digest", "rand_core"] }
 spki = { version = "0.8.0-rc.1", default-features = false, features = ["alloc"] }
 zeroize = { version = "1.5", features = ["alloc"] }
+crypto-bigint = { version = "0.6.0-rc.6", default-features = false, features = ["zeroize", "alloc"] }
+crypto-primes = { version = "0.6.0-pre.2", default-features = false }
+libm = "0.2"


IMO it would be nice to avoid this, especially if its only used for no_std prime counting

If all you need from there is logf(x), you can probably just approximate it with x.bits() / log2(e), pre-calculating log2(e).

Or even take log2(e) == 1 if the approximation allows it :)

bits() seems to not be around either..

tarcieri · 2025-01-04T19:30:36Z

README.md

@@ -81,7 +81,7 @@ You can follow our work on mitigating this issue in [#390].

 ## Minimum Supported Rust Version (MSRV)

-This crate supports Rust 1.72 or higher.
+This crate supports Rust 1.73 or higher.


This is crypto-bigint's latest MSRV:

Suggested change

This crate supports Rust 1.73 or higher.

This crate supports Rust 1.83 or higher.

fixed all around

tarcieri · 2025-01-04T19:31:46Z

src/lib.rs

@@ -222,7 +223,6 @@ extern crate alloc;
 #[cfg(feature = "std")]
 extern crate std;

-pub use num_bigint::BigUint;


Perhaps consider a BoxedUint re-export here, possibly using a shorter name:

pub use crypto_bigint::BoxedUint as Uint;

tarcieri · 2025-01-04T19:32:41Z

src/algorithms/generate.rs

+    // TODO: reenable, currently slow
+    // key_generation!(key_generation_multi_16_1024, 16, 1024);


src/algorithms/rsa.rs

tarcieri · 2025-01-04T19:37:44Z

src/algorithms/rsa.rs

+    let two = NonZero::new(BoxedUint::from(2u64))
+        .expect("2 is non zero")
+        .widen(bits);


Something which makes this pattern better would be nice. We have various NonZero::new_unwrap constructors which allow you to infallibly construct a NonZero.

278 | let two = NonZero::new_unwrap(2).widen(bits); | ^^^^^^^^^^ multiple `new_unwrap` found | = note: candidate #1 is defined in an impl for the type `crypto_bigint::NonZero<Limb>` = note: candidate #2 is defined in an impl for the type `crypto_bigint::NonZero<crypto_bigint::Uint<LIMBS>>`

seems to not exist for theBoxedUint version

@dignifiedquire aah, yeah the idea would be to const construct a NonZero<Uint> first and either convert it to a NonZero<BoxedUint> first or make it possible to use NonZero<Uint> as an operand.

fjarri · 2025-01-05T01:00:15Z

src/algorithms/generate.rs

@@ -93,14 +95,14 @@ pub(crate) fn generate_multi_prime_key_with_exp<R: CryptoRngCore + ?Sized>(

        let n = compute_modulus(&primes);

-        if n.bits() != bit_size {
+        if n.bits() as usize != bit_size {
            // This should never happen for nprimes == 2 because
            // gen_prime should set the top two bits in each prime.


The comment should be updated for the new name of the function.

Also I don't quite understand "gen_prime should set the top two bits in each prime" - what was that behavior for? generate_prime_with_rng() only sets one top bit, to ensure that the returned primes are of the requested bit size.

I was a bit curious about that myself. I saw @FiloSottile mention the same thing in this post:

https://words.filippo.io/dispatches/rsa-keygen-bench/

There is almost nothing special to selecting prime candidates. You draw an appropriately sized random number from a CSPRNG, and to avoid wasting time, you set the least significant bit and the two most significant bits: large even numbers are not prime, and setting the top two guarantees N won’t come out too small.

If you set only the top bit of a n/2 bits number, you get a value >= 2^(n/2-1). If you multiply two of them, you get a value >= 2^(n-2), which is at a minimum n-1 bits long, one too short for a n-bit modulus.

Ah I see, that makes sense. Do you think it should be the default behavior in crypto-primes?

If you mainly need the primes for RSA, yes, you are only wasting CPU rejecting small moduli after you found the primes. If you're using the primes for something else, it might depend on what they are for.

@fjarri are you planning on changing the behaviour?

I thought about it, and I don't think it should be the default - too specialized. It can be achieved with custom sieves, but I need to make a release first

that's fair, please do let me know if you have a release/how to make replicate the desired behavior with custom sieves

src/encoding.rs

tarcieri · 2025-01-21T18:08:37Z

FYI: release PR for crypto-bigint v0.6.0 is up: RustCrypto/crypto-bigint#750

YgorSouza · 2025-01-21T19:46:14Z

src/algorithms/generate.rs

        d: d_final,
        primes,
    })
 }

+/// Natural logrithm for `f64`.


Typo here: logrithm -> logarithm (and the same on line 123). Could add the typos GitHub action to the repo to avoid that.

dignifiedquire added 4 commits November 29, 2023 14:23

implement minimal decryption routine using BoxedUint

fcc3577

convert internals to use crypto-bigint

bc92994

store residue_params

21b7c81

implement blinding (tests failing currently)

98a86a2

dignifiedquire mentioned this pull request Nov 29, 2023

Migrating from num-bigint(-dig) to crypto-bigint #390

Open

dignifiedquire added 3 commits November 29, 2023 17:42

fix blinding and fold in new impl

f4131c5

implement basic widening strategy

2dafa35

fix some widening/padding

cb7d486

tarcieri reviewed Nov 29, 2023

View reviewed changes

src/algorithms/rsa.rs Outdated Show resolved Hide resolved

dignifiedquire added 2 commits November 29, 2023 20:39

cleanup rsa algorithm

fd19c88

docs and debugging

813fe77

tarcieri mentioned this pull request Nov 30, 2023

Splitting up crate to reduce advisory blast radius? #395

Closed

dignifiedquire added 5 commits November 30, 2023 22:07

fix crt value calculation in decryption

efc4460

cleanup

9fde5fb

convert core key algorithms

3f1751e

cache p and q params

b795c22

convert hazmt decrypt interface

6481533

tarcieri reviewed Dec 1, 2023

View reviewed changes

src/algorithms/rsa.rs Outdated Show resolved Hide resolved

dignifiedquire added 6 commits December 1, 2023 10:33

use crypto-bigint for encryption

97f8fa4

convert more internal use to crypto-bigint

cc3f03b

use crypto-bigint in signatuers

d02fbe7

convert from_components internally and pss signature

a5fc616

cleanup rsapublickey

7211bfa

store exponent as u64

c071ba1

sebadob mentioned this pull request Dec 1, 2023

Marvin Attack: potential key recovery through timing sidechannels sebadob/rauthy#197

Open

dignifiedquire added 5 commits December 2, 2023 13:16

fix: handle large m2

a811be0

cache boxedresiduie for qinv

102953d

use some assign operations

57d6f95

update to latest crypto-bigint

2cdd37c

update crypto-bigint

f7fa669

dignifiedquire marked this pull request as ready for review December 2, 2024 09:16

dignifiedquire changed the title ~~[WIP]: switch to crypto-bigint for decryption~~ [WIP]: feat: switch from num-bigint-dig to crypto-bigint Dec 2, 2024

dignifiedquire changed the title ~~[WIP]: feat: switch from num-bigint-dig to crypto-bigint~~ feat: switch from num-bigint-dig to crypto-bigint Dec 2, 2024

dignifiedquire commented Dec 2, 2024

View reviewed changes

dignifiedquire added 4 commits December 17, 2024 12:08

Merge remote-tracking branch 'origin/master' into const-crypto-biguint

d675b13

cleanup and disallow unwraps

cd99409

core error

c64c8ec

sigh

d755f51

benjaminbruun mentioned this pull request Dec 23, 2024

chore: bump rsa from 0.9.2 to 0.9.x onomondo/onomondo-softsim-cli#65

Draft

tarcieri mentioned this pull request Jan 2, 2025

RSA key generation comparatively slow #144

Open

tarcieri self-requested a review January 2, 2025 14:39

tarcieri reviewed Jan 4, 2025

View reviewed changes

src/algorithms/rsa.rs Outdated Show resolved Hide resolved

tarcieri reviewed Jan 4, 2025

View reviewed changes

fjarri reviewed Jan 5, 2025

View reviewed changes

tarcieri reviewed Jan 5, 2025

View reviewed changes

src/encoding.rs Outdated Show resolved Hide resolved

dignifiedquire added 2 commits January 18, 2025 14:07

apply CR

8064585

ci: update nightly

8cb0865

seqre mentioned this pull request Jan 21, 2025

RUSTSEC-2023-0071: Marvin Attack: potential key recovery through timing sidechannels cot-rs/cot#72

Open

YgorSouza reviewed Jan 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: switch from num-bigint-dig to crypto-bigint #394

feat: switch from num-bigint-dig to crypto-bigint #394

dignifiedquire commented Nov 29, 2023 •

edited

Loading

dignifiedquire commented Dec 1, 2024

dignifiedquire commented Dec 2, 2024

dignifiedquire Dec 2, 2024

tarcieri Dec 2, 2024

fjarri Dec 3, 2024

tarcieri Jan 4, 2025

tarcieri commented Dec 19, 2024

dignifiedquire commented Dec 19, 2024

Snowiiii commented Jan 2, 2025

tarcieri commented Jan 2, 2025

tarcieri Jan 4, 2025

fjarri Jan 5, 2025

fjarri Jan 5, 2025

dignifiedquire Jan 18, 2025

tarcieri Jan 4, 2025

dignifiedquire Jan 18, 2025

tarcieri Jan 4, 2025

dignifiedquire Jan 18, 2025

tarcieri Jan 4, 2025

tarcieri Jan 4, 2025

dignifiedquire Jan 18, 2025

tarcieri Jan 21, 2025

fjarri Jan 5, 2025

tarcieri Jan 5, 2025

FiloSottile Jan 5, 2025

fjarri Jan 5, 2025

FiloSottile Jan 5, 2025

dignifiedquire Jan 18, 2025

fjarri Jan 18, 2025

dignifiedquire Jan 20, 2025

tarcieri commented Jan 21, 2025

YgorSouza Jan 21, 2025

	This crate supports Rust 1.73 or higher.
	This crate supports Rust 1.83 or higher.

		// TODO: reenable, currently slow
		// key_generation!(key_generation_multi_16_1024, 16, 1024);

feat: switch from num-bigint-dig to crypto-bigint #394

Are you sure you want to change the base?

feat: switch from num-bigint-dig to crypto-bigint #394

Conversation

dignifiedquire commented Nov 29, 2023 • edited Loading

TODOs

dignifiedquire commented Dec 1, 2024

dignifiedquire commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarcieri commented Dec 19, 2024

dignifiedquire commented Dec 19, 2024

Snowiiii commented Jan 2, 2025

tarcieri commented Jan 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarcieri commented Jan 21, 2025

Choose a reason for hiding this comment

dignifiedquire commented Nov 29, 2023 •

edited

Loading