dsa: migrate to `crypto-bigint` #906

baloo · 2025-02-16T08:50:18Z

This is a rebase of #784

There are two commits on top that I think should fix tests. There is a bit of cleanup to do still.

baloo · 2025-02-16T08:50:41Z

aumetra · 2025-02-16T16:52:08Z

Apologies, I was a bit absent lately. I'll have a look when I get the time (which should be in like two hours at max)

baloo · 2025-02-16T20:45:13Z

No need to apologize! Please take care.
That was only to keep you updated on the progress and let you chime in as needed.

If there are specific things you wanted to be taken care of, please let me know I'd be happy to.

baloo · 2025-02-17T06:58:01Z

out of the last 3 tests that still fail:

sign_message: that one needs the vectors to be adjusted, we're switching the backend and by doing so the primes we get from the RNG are different because their generation is now different.
signer_verifier_signature: that one is a precision that needs to be adjusted.
verify_signature: I'm not quite sure yet.

Overall having to adjust precisions seems to be ... inefficent? error-prone? I guess there is a way better method for doing that. I need to have a look at RSA how they did it there.

aumetra · 2025-02-19T11:03:12Z

Overall having to adjust precisions seems to be ... inefficent? error-prone? I guess there is a way better method for doing that. I need to have a look at RSA how they did it there.

Seems like even rsa did the same thing. Just via the .widen method which is, I guess, a newer method that didn't exist when I started this PR a year ago

aumetra · 2025-02-19T11:04:08Z

Scratch that, it existed, I just didn't see it at that time

tarcieri · 2025-02-19T14:07:07Z

@baloo the precision aspect is definitely a bit tricky. num-bigint does it automatically, however this leads to variable-time operation depending on the value’s precision which is unsuitable for private key operations

baloo · 2025-02-19T17:43:34Z

Oh I understand why it's done! I'm just not sure I know what I'm doing [insert here the picture of a dog].

baloo · 2025-03-02T07:04:57Z

okay, it's definitely on the ugly side, but tests do pass \o/

I think I got the hang of it, I think I can clean things up from there, but ... tomorrow.

aumetra · 2025-03-02T09:54:36Z

Nice! I mean I have a few hours to kill today, too, so I can go ahead and try to clean up some things, too. I'll PR it into your fork when I get to it

baloo

Those two NonZero I'm not sure about.

baloo · 2025-03-03T03:51:28Z

dsa/src/signing_key.rs

+        // TODO(baloo): is there any way R could be zero here? Could it be any reason for K to be
+        //              one?
+        let r_short = NonZero::new(r_short).unwrap();
+        let r = NonZero::new(r).unwrap();


I'm not sure about this unwrap, isn't there a way for r = g.modpow(k, p) % q to be zero?

IIRC r and s should neither be ever zero, and if they are it's an invalid signature. So panicking here is fine since AFAIK there would have had to be a cosmic ray or something hitting the CPU register/memory chip for that to exist.

Oh right, that ties to the k value computed in the secret_number and secret_number_rfc6979!

That would then be a bug and a panic might be in order.

baloo · 2025-03-03T03:52:41Z

dsa/src/signing_key.rs

+        // TODO(baloo): is there any way S could be zero here?
+        let s = NonZero::new(s).unwrap();


Similarly here, s could be zero?

baloo · 2025-03-03T05:09:23Z

dsa/tests/signature.rs

 const MESSAGE_SIGNATURE_CRATE_ASN1: &[u8] = &[
-    0x30, 0x2C, 0x02, 0x14, 0x45, 0x1D, 0xE5, 0x76, 0x21, 0xD8, 0xFD, 0x76, 0xC1, 0x6F, 0x45, 0x4E,
-    0xDE, 0x5F, 0x09, 0x79, 0x76, 0x52, 0xF3, 0xA5, 0x02, 0x14, 0x53, 0x60, 0xE6, 0xB7, 0xF0, 0xCF,
-    0xAE, 0x49, 0xB1, 0x58, 0x5C, 0xCF, 0x5F, 0x3F, 0x94, 0x49, 0x21, 0xA0, 0xBF, 0xD2,
+    0x30, 0x2d, 0x2, 0x14, 0x4, 0x23, 0xc3, 0xf1, 0xa5, 0x15, 0x65, 0xe5, 0xcc, 0x30, 0xdd, 0x55,
+    0x96, 0x27, 0x30, 0xfb, 0x42, 0xf9, 0x2a, 0x6b, 0x2, 0x15, 0x0, 0x95, 0xd9, 0x42, 0xc2, 0xab,
+    0xe7, 0x62, 0x49, 0x80, 0x4f, 0x53, 0xf1, 0x37, 0x57, 0x81, 0x7b, 0xea, 0x71, 0xc9, 0x2f,
 ];

 /// Message signed by OpenSSL using the keys generated by this CSPRNG
 ///
 /// This signature was generated using the SHA-256 digest
-const MESSAGE_SIGNATURE_OPENSSL_ASN1: &[u8] = &[
-    0x30, 0x2C, 0x02, 0x14, 0x6D, 0xB3, 0x8E, 0xAF, 0x97, 0x13, 0x7E, 0x07, 0xFF, 0x24, 0xB8, 0x66,
-    0x97, 0x18, 0xE1, 0x6F, 0xD7, 0x9A, 0x28, 0x2D, 0x02, 0x14, 0x47, 0x8C, 0x0B, 0x96, 0x51, 0x08,
-    0x08, 0xC8, 0x34, 0x9D, 0x0D, 0x41, 0xC7, 0x73, 0x0F, 0xB5, 0x9C, 0xBB, 0x00, 0x34,
-];
+const MESSAGE_SIGNATURE_OPENSSL_ASN1: &[u8] = &hex!(
+    "302e 0215 0092 4523 9462 f59b 5936 d355
+     8b16 8dbb 0915 7ba1 5302 1500 9d8d 7aa7
+     02a6 2dde 1975 e9f2 a20c 73b4 7dd3 0912"
+);


The behavior of Components::generate changed because of the switch to crypto-bigint (and crypto-primes). This makes the "deterministic" private key used for test change. And as a result, makes the signatures change.

I don't believe we offered any guarantee towards the stability of the Component::generate?

No, we didn't make any guarantees. The only guarantee we provided was "it outputs something that works".

If some code relies on this staying the same it's really fragile anyway, since they'd have to use the same algorithms to generate primes and factors, which can differ between implementations. So yeah, this should be fine.

tarcieri · 2025-03-09T18:51:54Z

@baloo this one could actually use a rebase

required because of crypto-bigint

baloo · 2025-03-09T20:53:40Z

This probably should include tests stretching the bit precision of a signature to test against panic.
Maybe proptests would be a good fit?

tarcieri · 2025-03-09T20:57:42Z

Yes proptests would be good.

I need to pull this down and benchmark it but getting the precisions right has been a challenge in rsa. Did performance regress significantly?

baloo · 2025-03-09T22:25:31Z

It was pretty bad at some point (tests would take over a minute) but I did not dig why. It works fine now.

I did not run a benchmark.

baloo marked this pull request as draft February 16, 2025 08:50

baloo force-pushed the baloo/dsa/crypto-bigint branch 2 times, most recently from a5b6804 to fde178e Compare February 18, 2025 05:58

baloo mentioned this pull request Feb 19, 2025

Update signing method signatures to match upstream traits #907

Closed

baloo force-pushed the baloo/dsa/crypto-bigint branch 3 times, most recently from 26e40ac to 03f4d7a Compare March 2, 2025 07:04

tarcieri mentioned this pull request Mar 2, 2025

dsa: impl ZeroizeOnDrop for SigningKey #917

Open

baloo force-pushed the baloo/dsa/crypto-bigint branch 4 times, most recently from a481afb to 71639a3 Compare March 3, 2025 02:54

baloo commented Mar 3, 2025

View reviewed changes

baloo marked this pull request as ready for review March 3, 2025 03:53

baloo force-pushed the baloo/dsa/crypto-bigint branch 3 times, most recently from c0a9f41 to 5fd1ebc Compare March 3, 2025 03:57

baloo commented Mar 3, 2025

View reviewed changes

tarcieri mentioned this pull request Mar 7, 2025

dsa: Eq is missing for Components, SigningKey and VerifyingKey #881

Open

baloo force-pushed the baloo/dsa/crypto-bigint branch from b715031 to e8a249b Compare March 9, 2025 05:39

aumetra and others added 21 commits March 9, 2025 12:38

progress

7b4c03e

fix hex decoding

a53ddcd

.to_montgomery() -> .retrieve

438cfd3

pin dependencies

bc28d13

fix rfc6979 generation

15397a4

align all the precisions (its ugly)

32e1eac

fixup signature

7258545

fixup secret derivation

a88bb0a

dsa: bump MSRV to 1.85; bump to edition 2024

b9ebf06

required because of crypto-bigint

fixup key generation 1/2

6f89a6d

Fixup verifying key

494b86e

fixup keypair gen

a9d55e2

dsa: fixup documentation

4fb3503

fixup last tests

f42e331

fixup ci job definition

cd5ccdb

fixup extraneous import

bffeaf7

fixup clippy

6dad547

cleanup unwraps

ff9e5c7

reduce_hash

bfa8703

cleanup

35fbcd8

explicit the unwrap on the s and r values

56c953b

baloo force-pushed the baloo/dsa/crypto-bigint branch from e8a249b to 56c953b Compare March 9, 2025 19:39

baloo added 2 commits March 9, 2025 20:35

check precision of a signature

c947951

dsa: check signature verification with proptest

09e7676

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dsa: migrate to `crypto-bigint` #906

dsa: migrate to `crypto-bigint` #906

baloo commented Feb 16, 2025

baloo commented Feb 16, 2025

aumetra commented Feb 16, 2025

baloo commented Feb 16, 2025 •

edited

Loading

baloo commented Feb 17, 2025 •

edited

Loading

aumetra commented Feb 19, 2025

aumetra commented Feb 19, 2025

tarcieri commented Feb 19, 2025

baloo commented Feb 19, 2025

baloo commented Mar 2, 2025

aumetra commented Mar 2, 2025

baloo left a comment

baloo Mar 3, 2025

aumetra Mar 3, 2025

baloo Mar 3, 2025

baloo Mar 3, 2025 •

edited

Loading

baloo Mar 3, 2025

aumetra Mar 3, 2025

tarcieri commented Mar 9, 2025

baloo commented Mar 9, 2025

tarcieri commented Mar 9, 2025

baloo commented Mar 9, 2025 •

edited

Loading

		// TODO(baloo): is there any way S could be zero here?
		let s = NonZero::new(s).unwrap();

dsa: migrate to crypto-bigint #906

Are you sure you want to change the base?

dsa: migrate to crypto-bigint #906

Conversation

baloo commented Feb 16, 2025

baloo commented Feb 16, 2025

aumetra commented Feb 16, 2025

baloo commented Feb 16, 2025 • edited Loading

baloo commented Feb 17, 2025 • edited Loading

aumetra commented Feb 19, 2025

aumetra commented Feb 19, 2025

tarcieri commented Feb 19, 2025

baloo commented Feb 19, 2025

baloo commented Mar 2, 2025

aumetra commented Mar 2, 2025

baloo left a comment

Choose a reason for hiding this comment

baloo Mar 3, 2025

Choose a reason for hiding this comment

aumetra Mar 3, 2025

Choose a reason for hiding this comment

baloo Mar 3, 2025

Choose a reason for hiding this comment

baloo Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

baloo Mar 3, 2025

Choose a reason for hiding this comment

aumetra Mar 3, 2025

Choose a reason for hiding this comment

tarcieri commented Mar 9, 2025

baloo commented Mar 9, 2025

tarcieri commented Mar 9, 2025

baloo commented Mar 9, 2025 • edited Loading

dsa: migrate to `crypto-bigint` #906

dsa: migrate to `crypto-bigint` #906

baloo commented Feb 16, 2025 •

edited

Loading

baloo commented Feb 17, 2025 •

edited

Loading

baloo Mar 3, 2025 •

edited

Loading

baloo commented Mar 9, 2025 •

edited

Loading