330 zeroize #511

hridambasu · 2023-12-07T16:20:13Z

Closes #330

src/presign/participant.rs

gatoWololo · 2023-12-14T00:28:58Z

src/presign/participant.rs

-            k,
-            rho,
+            k: k.into(),
+            rho: rho.into(),


I am not sure if this is the right place to convert from a Nonce to a SecretNonce. For example, rho is generated above in the EncryptionKey::encrypt function, this function create a regular Nonce. The encrypt function uses the nonce like so let c = self.encrypt_with_nonce(x, &MaskedNonce(nonce.clone()))?;. Here, we are cloning the nonce. Since this is not a SecretNonce it will go out of scope without getting zeroized. So we have a copy of rho lingering in memory.

I believe we need to handle these cases as well? Otherwise, what is the point of zeroizing here but not there?

src/presign/participant.rs

gatoWololo · 2023-12-14T00:35:15Z

src/presign/participant.rs

@@ -1037,7 +1037,8 @@ impl PresignKeyShareAndInfo {
            rng,
        )?;
        let mut transcript = Transcript::new(b"PiLogProof");
-        let secret = ProverSecret::new(&sender_r1_priv.gamma, &sender_r1_priv.nu);
+        let nonce_clone = &sender_r1_priv.nu.get_nonce().clone();


Here you are taking a SecretNonce, accessing the underlying Nonce and cloning it. So now we have a copy of nu that won't get zeroized. This seems like a security issue? We should either:

Fix this to not create a non-Secret copy. The best way to fix this, is probably to have ProverSecret use a SecretNonce instead of a Nonce.

If we really do want to convert a SecretNonce -> Nonce we should have a comment justifying why it is okay.

src/presign/participant.rs

src/presign/round_one.rs

src/presign/round_three.rs

src/presign/participant.rs

src/presign/round_three.rs

gatoWololo

I just went through and reviewed again. There are still some issues we should fix before merging this. I left new comments for those. Additionally:

There is some commented out code on this PR. Please delete those.
We should move the SecretBigNumber, SecretNonce, and SecreteScalar to a more appropriate location. I recommend the src/utils.rs file.

gatoWololo · 2023-12-21T00:31:01Z

I "marked as resolved" the original comments that have been fixed. I left some open that we are still no handling.

We need to decide how to handle those comments even if we don't plan on fixing it as part of this PR? We could move some of this comments as comments in the code? @hridambasu @cryptograph thoughts?

gatoWololo · 2024-01-02T18:49:02Z

Cargo.toml

@@ -21,6 +21,7 @@ displaydoc = { version = "0.2", default-features = false }
 flame = { version = "0.2", optional = true }
 flamer = { version = "0.3", optional = true }
 generic-array = "0.14"
+#gmp-mpfr-sys = { version = "1.4", features = ["use-system-libs"] }


Please remove commented out dependency.

gatoWololo · 2024-01-02T18:49:16Z

src/paillier.rs

@@ -9,6 +9,8 @@ use crate::{
    parameters::PRIME_BITS,
    utils::{modpow, random_bn_in_z_star, CRYPTOGRAPHIC_RETRY_MAX},
 };
+//use crate::presign::round_three::SecretBigNumber;


Remove commented out imports.

gatoWololo · 2024-01-02T18:49:29Z

src/paillier.rs

@@ -134,6 +136,7 @@ impl EncryptionKey {
        // `encrypt_with_nonce`.
        let nonce =
            random_bn_in_z_star(rng, self.modulus()).map_err(|_| PaillierError::RetryFailed)?;
+        //let secretnonce = SecretBigNumber::from_number(nonce);


Remove commented out code.

gatoWololo · 2024-01-02T18:55:42Z

src/presign/mod.rs

@@ -19,8 +19,8 @@
 mod input;
 mod participant;
 mod record;
-mod round_one;
-mod round_three;
+pub(crate) mod round_one;


These two modules were made pub(crate) since the Secret* types are defined in them. As we talked about, I think this is the wrong place for these types and we should move them. Once they are moved, this change can be reverted (will make a separate comment for that).

gatoWololo · 2024-01-02T19:00:13Z

src/presign/participant.rs

@@ -1141,7 +1157,10 @@ impl PresignKeyShareAndInfo {
                    self.aux_info_public.pk(),
                    &Gamma,
                ),
-                ProverSecret::new(&sender_r1_priv.k, &sender_r1_priv.rho),
+                ProverSecret::new(
+                    &sender_r1_priv.k.get_bignumber_secret().clone(),


We are copying the underlying secret to a non-secret for k and rho. We should comment flagging this fact so we can fix it later.

gatoWololo · 2024-01-02T19:00:45Z

src/presign/round_one.rs

@@ -22,16 +22,32 @@ use serde::{Deserialize, Serialize};
 use std::fmt::Debug;
 use zeroize::ZeroizeOnDrop;

+use super::round_three::SecretBigNumber;


Per my previous comment: We should move the SecretBigNumber, SecretNonce, and SecreteScalar to a more appropriate location. I recommend the src/utils.rs file.

gatoWololo · 2024-01-02T19:01:03Z

src/presign/round_one.rs

-    pub nu: Nonce,
-    #[zeroize(skip)]
+    pub nu: SecretNonce,
+    //#[zeroize(skip)]


Remove commented out code.

gatoWololo · 2024-01-02T19:01:13Z

src/presign/round_one.rs

    pub G: Ciphertext, // Technically can be public but is only one per party
-    #[zeroize(skip)]
+    //#[zeroize(skip)]


Remove commented out code.

gatoWololo · 2024-01-02T19:01:48Z

src/presign/round_three.rs

    pub Delta: CurvePoint,
 }
+#[derive(Clone, ZeroizeOnDrop)]


Move SecretBigNumber to more appropriate file.

gatoWololo · 2024-01-02T19:02:04Z

src/presign/round_two.rs

@@ -20,16 +20,18 @@ use crate::{
        Proof, ProofContext,
    },
 };
-use libpaillier::unknown_order::BigNumber;
+//use libpaillier::unknown_order::BigNumber;


Remove commented out code.

Hridam Basu and others added 5 commits December 1, 2023 20:56

secretbignumber added

74d3c14

secret scalar added

d8751ee

delta error

f7eb332

round one file changes made

241ba08

round_two changes made

2283c30

hridambasu requested a review from gatoWololo December 7, 2023 16:20

hridambasu added 3 commits December 11, 2023 23:59

invert error fixed

2eeb5e2

max errors fixed

c8f828c

all errors fixed

055410d

hridambasu requested a review from cryptograph December 13, 2023 09:08

hridambasu added 2 commits December 13, 2023 15:29

gmp version fixed

fe3f415

gmp removed

8c850c8