Skip to content
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

c-split vulnerability fixes #34

Merged
merged 5 commits into from
Oct 19, 2023
Merged

Conversation

davidsemakula
Copy link
Contributor

@davidsemakula davidsemakula commented Oct 16, 2023

Summary of changes
Fixes c-split vulnerability

Changes introduced in this pull request:

See this comment (and preceding thread) for more details.

NOTE: This is the same proof that ring-Pedersen parameters parameter ($\tilde N$, $s$, $t$) are generated correctly in CGGMP20. However, I'm not confident the existing similar proof in the FS-DKR crate isn't using ambiguous encoding for the commitment data, so I opted to implement this instead using merlin for the Fiat-Shamir transformation.

Reference issue to close (if applicable)

Closes #23

@davidsemakula
Copy link
Contributor Author

cc @tmpfs @ivokub

@ivokub
Copy link
Contributor

ivokub commented Oct 16, 2023

What do you think about if for development mode instead of using we would use safe primes but shorter so they are quicker to sample? So instead of 2048 bits we would sample 512 etc bits.

If we use safe primes, then it is also simpler to ensure that h1 (and correspondingly h2) are quadratic residues by taking square of randomly sampled value mod p*q. Then, we can also ensure that ord(h1) = (p-1)/2 * (q-1)/2.

If it seems good than can implement the changes.

@davidsemakula
Copy link
Contributor Author

What do you think about if for development mode instead of using we would use safe primes but shorter so they are quicker to sample? So instead of 2048 bits we would sample 512 etc bits.

@ivokub I think it's worth trying.

If we use safe primes, then it is also simpler to ensure that h1 (and correspondingly h2) are quadratic residues by taking square of randomly sampled value mod p*q. Then, we can also ensure that ord(h1) = (p-1)/2 * (q-1)/2.

Yeah, this is what CGGMP20 does when generating the ring-Pedersen parameters ($\tilde N$, $s$, $t$), see section 6.4.1.

However, for c-split, the prover in the composite dlog proof is the dishonest party (they change to being the verifier during MtA which is the point at which they mount the attack), so we'd also need to generate a proof for $ord(h1) = (p-1)/2 * (q-1)/2$ for the other party (I'm assuming this is about the possibility of initializing Pointcheval's proof securely, correct me if I'm wrong 🙂).

@ivokub
Copy link
Contributor

ivokub commented Oct 16, 2023

What do you think about if for development mode instead of using we would use safe primes but shorter so they are quicker to sample? So instead of 2048 bits we would sample 512 etc bits.

@ivokub I think it's worth trying.

I started doing it, but its a bit more involved than I anticipated. There are a few parameters and verifiers in the protocols also check that the values are in some expected ranges. Will continue on it.

If we use safe primes, then it is also simpler to ensure that h1 (and correspondingly h2) are quadratic residues by taking square of randomly sampled value mod p*q. Then, we can also ensure that ord(h1) = (p-1)/2 * (q-1)/2.

Yeah, this is what CGGMP20 does when generating the ring-Pedersen parameters (N~, s, t), see section 6.4.1.

However, for c-split, the prover in the composite dlog proof is the dishonest party (they change to being the verifier during MtA which is the point at which they mount the attack), so we'd also need to generate a proof for ord(h1)=(p−1)/2∗(q−1)/2 for the other party (I'm assuming this is about the possibility of initializing Pointcheval's proof securely, correct me if I'm wrong 🙂).

Yes, I think the prover also has to give the proof for the order of h1. But imo if we add Paillier-Blum proof then it should be sufficient to only check that Jacobi symbol of h1 is 1 and h1 is not trivial (1 or -1)?

@ivokub
Copy link
Contributor

ivokub commented Oct 16, 2023

Yeah, this is what CGGMP20 does when generating the ring-Pedersen parameters (N~, s, t), see section 6.4.1.
However, for c-split, the prover in the composite dlog proof is the dishonest party (they change to being the verifier during MtA which is the point at which they mount the attack), so we'd also need to generate a proof for ord(h1)=(p−1)/2∗(q−1)/2 for the other party (I'm assuming this is about the possibility of initializing Pointcheval's proof securely, correct me if I'm wrong 🙂).

Yes, I think the prover also has to give the proof for the order of h1. But imo if we add Paillier-Blum proof then it should be sufficient to only check that Jacobi symbol of h1 is 1 and h1 is not trivial (1 or -1)?

On a second thought - I'm not sure we need to show ord(h1) = (p-1)/2 * (q-1)/2. All the verifier checks in PI-prm hold modulo N. If the prover choses h1 as a quadratic non-residue, then it leaks a a bit or two about h1, but doesn't imo affect soundness.

@ivokub
Copy link
Contributor

ivokub commented Oct 16, 2023

Suggested edit:

diff --git a/multi-party-ecdsa/src/utilities/zk_composite_dlog.rs b/multi-party-ecdsa/src/utilities/zk_composite_dlog.rs
index f69f39a..7a8e21a 100644
--- a/multi-party-ecdsa/src/utilities/zk_composite_dlog.rs
+++ b/multi-party-ecdsa/src/utilities/zk_composite_dlog.rs
@@ -94,13 +94,11 @@ fn compute_challenges(
     // Parses challenge bits.
     let mut challenge_bits = [ChallengeBit::ZERO; STAT_SECURITY];
     for (idx, byte) in challenge_bytes.iter().enumerate() {
-        // We're only looking for non-zero bits (i.e. 1)
-        // since the rest are already set to zero by default.
-        let bits = format!("{byte:08b}");
-        for (i, char) in bits.chars().enumerate() {
-            if char == '1' {
-                challenge_bits[idx * 8 + i] = ChallengeBit::ONE;
-            }
+        for i in 0..8 {
+            challenge_bits[idx * 8 + i] = match (byte & (1 << i)) > 0 {
+                false => ChallengeBit::ZERO,
+                true => ChallengeBit::ONE,
+            };
         }
     }
 

@davidsemakula
Copy link
Contributor Author

Suggested edit:

diff --git a/multi-party-ecdsa/src/utilities/zk_composite_dlog.rs b/multi-party-ecdsa/src/utilities/zk_composite_dlog.rs
index f69f39a..7a8e21a 100644
--- a/multi-party-ecdsa/src/utilities/zk_composite_dlog.rs
+++ b/multi-party-ecdsa/src/utilities/zk_composite_dlog.rs
@@ -94,13 +94,11 @@ fn compute_challenges(
     // Parses challenge bits.
     let mut challenge_bits = [ChallengeBit::ZERO; STAT_SECURITY];
     for (idx, byte) in challenge_bytes.iter().enumerate() {
-        // We're only looking for non-zero bits (i.e. 1)
-        // since the rest are already set to zero by default.
-        let bits = format!("{byte:08b}");
-        for (i, char) in bits.chars().enumerate() {
-            if char == '1' {
-                challenge_bits[idx * 8 + i] = ChallengeBit::ONE;
-            }
+        for i in 0..8 {
+            challenge_bits[idx * 8 + i] = match (byte & (1 << i)) > 0 {
+                false => ChallengeBit::ZERO,
+                true => ChallengeBit::ONE,
+            };
         }
     }
 

Thanks, incorporated but slightly modified to preserve the bit order and to skip leading and trailing zero bits

@davidsemakula
Copy link
Contributor Author

if we add Paillier-Blum proof then it should be sufficient to only check that Jacobi symbol of h1 is 1

@ivokub Shouldn't Jacobi symbol of $h_1$ be -1?
Other than that, makes sense ... I guess plus checking that $h_1 \in Z^* _N$

@davidsemakula
Copy link
Contributor Author

davidsemakula commented Oct 17, 2023

On a second thought - I'm not sure we need to show ord(h1) = (p-1)/2 * (q-1)/2. All the verifier checks in PI-prm hold modulo N. If the prover choses h1 as a quadratic non-residue, then it leaks a a bit or two about h1, but doesn't imo affect soundness.

@ivokub For PI-prm, sure ... $h_1$ being a quadratic residue only matters to the prover, so the verifier doesn't need to check it.
For pointcheval's proof, the verifier needs to check it (i.e Jacobi symbol is -1, plus proof that N is semiprime) for the proof to be sound AFAICT.

@ivokub
Copy link
Contributor

ivokub commented Oct 17, 2023

@ivokub For PI-prm, sure ... h1 being a quadratic residue only matters to the prover, so the verifier doesn't need to check it. For pointcheval's proof, the verifier needs to check it (i.e Jacobi symbol is -1, plus proof that N is semiprime) for the proof to be sound AFAICT.

I'm a bit lost - do we still need Pointcheval's proof anywhere after implementing DLNProof/PI-prm?

@davidsemakula
Copy link
Contributor Author

davidsemakula commented Oct 17, 2023

I'm a bit lost - do we still need Pointcheval's proof anywhere after implementing DLNProof/PI-prm?

Sorry, we don't 🙂

See here, I thought the motivation for checking quadratic residues was for the possibility of initializing Pointcheval's proof securely, which you seemed to confirm in the next comment 🙂

For PI-prm, I think we can simply follow section 6.4.1 when generating $h_1$ and $h_2$ (similar to your suggestion 🙂).

@ivokub
Copy link
Contributor

ivokub commented Oct 17, 2023

I'm a bit lost - do we still need Pointcheval's proof anywhere after implementing DLNProof/PI-prm?

Sorry, we don't 🙂

See here, I thought the motivation for checking quadratic residues was for the possibility of initializing Pointcheval's proof securely, which you seemed to confirm in the next comment 🙂

For PI-prm, I think we can simply follow section 6.4.1 when generating h1 and h2 (similar to your suggestion 🙂).

Oh yes, I guess it was just a confusion. Yup, we don't need Jacobi symbol and it is sufficient to follow 6.4.1 (i.e. sample random tau and take h1=tau^2).

In that case, I would say I'm good with the PR. I would still try to have different configurations for parameters depending if in test/dev mode (and using safe primes everywhere for compatibility), but I will do it in a separate PR. It would impact different places and makes reading this PR more difficult.

Copy link
Contributor

@ivokub ivokub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@davidsemakula
Copy link
Contributor Author

@drewstone This should be good enough to merge, other stuff (see Ivo's suggestions above) can be addressed in subsequent PRs.

@drewstone drewstone merged commit 01e978d into tangle-network:main Oct 19, 2023
4 of 5 checks passed
@drewstone
Copy link
Contributor

Merged @davidsemakula @ivokub awesome work!

luca992 added a commit to luca992/cggmp-threshold-ecdsa that referenced this pull request Nov 23, 2023
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.

Review c-split vulnerability from TSS Shock
3 participants