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.

Sign up for GitHub

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

Jump to bottom

Add a transcript protocol function that checks for identity points. #248

Merged
merged 6 commits into from
May 7, 2019
Merged

Add a transcript protocol function that checks for identity points. #248

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0f2fed9
Add a transcript protocol function that checks for identity points.
hdevalence Jan 28, 2019
4af51a5
Update Merlin to 1.1
hdevalence Apr 30, 2019
40def3a
Merge branch 'main' into add-identity-check
hdevalence Apr 30, 2019
28b22bb
Allow zeros for value commitments.
hdevalence Apr 30, 2019
381bc74
Update calls in R1CS code.
hdevalence Apr 30, 2019
5dbbd2b
Merge branch 'main' into add-identity-check
hdevalence May 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ keywords = ["cryptography", "ristretto", "zero-knowledge", "bulletproofs"]
description = "A pure-Rust implementation of Bulletproofs using Ristretto"

[dependencies]
curve25519-dalek = { version = "1", features = ["serde"] }
curve25519-dalek = { version = "1.0.3", features = ["serde"] }
subtle = "2"
sha3 = "0.8"
digest = "0.8"
Expand Down
4 changes: 2 additions & 2 deletions src/inner_product_proof.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,8 @@ impl InnerProductProof {

let mut challenges = Vec::with_capacity(lg_n);
for (L, R) in self.L_vec.iter().zip(self.R_vec.iter()) {
transcript.commit_point(b"L", L);
transcript.commit_point(b"R", R);
transcript.validate_and_commit_point(b"L", L)?;
transcript.validate_and_commit_point(b"R", R)?;
challenges.push(transcript.challenge_scalar(b"u"));
}

Expand Down
11 changes: 6 additions & 5 deletions src/range_proof/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,18 +287,19 @@ impl RangeProof {
transcript.rangeproof_domain_sep(n as u64, m as u64);

for V in value_commitments.iter() {
transcript.commit_point(b"V", V);
transcript.validate_and_commit_point(b"V", V)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

For completeness sake we need to allow zero V because the proof should be sound even with zero blinding factors, and with zero secret value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On a properly-generated Pedersen commitment, the blinding factor will be zero with negligible probability, so a zero blinding factor almost certainly indicates an RNG failure and I don't think there's a good reason to accept them.

It might be the case that some higher-level protocol uses inputs that aren't secret, as you mentioned (private communication), and so someone might choose a zero blinding factor. I'm not sure this is a good design compared to forming the commitment properly and giving the opening (v, v_blinding); if the blinding factor is set to zero, the committer still has to supply v (since there's not an easy way to recover v from vB, even for low-entropy v). So this "optimization" doesn't seem to really simplify the structure, the committer still has to supply the opening, it's just that the opening is smaller.

That said, I'm not totally set on forcing the V commitments to be nonzero, I just don't see a good reason to allow it, and there are (in my opinion) good reasons not to allow it (see next comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could see two options for resolving this:

  1. Remove the V != 0 check to allow the zero-blinding-factors case, and leave other checks in place;
  2. Leave all of the checks in place, and later, if V != 0 turns out to block something, we can relax it in the future (since relaxing a check is easier to do later than adding one).

WDYT?

Copy link
Collaborator

@oleganza oleganza Feb 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Let's do (1) and merge this PR?

}
transcript.commit_point(b"A", &self.A);
transcript.commit_point(b"S", &self.S);

transcript.validate_and_commit_point(b"A", &self.A)?;
transcript.validate_and_commit_point(b"S", &self.S)?;

let y = transcript.challenge_scalar(b"y");
let z = transcript.challenge_scalar(b"z");
let zz = z * z;
let minus_z = -z;

transcript.commit_point(b"T_1", &self.T_1);
transcript.commit_point(b"T_2", &self.T_2);
transcript.validate_and_commit_point(b"T_1", &self.T_1)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

T1 and T2 commit to “leftover” cross-terms and separated from the rest of the statement using non-zero challenge powers. There should not be a valid reason for them to not be allowed to be zero. If there is such reason, it must be specifically explored and called out. Maybe with some choice of prior challenges and zero blinding factors, some cross terms could actually end up being zeroes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(I mean, maybe not in this RP protocol, but in R1CS, for instance)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There should not be a valid reason for them to not be allowed to be zero. If there is such reason, it must be specifically explored and called out.

I disagree somewhat with this reasoning.

My reasoning for not allowing them to be zero is: allowing prover-supplied points to be the identity point is a risk, because it allows a prover to kill arbitrary terms in the verification equation (by setting those points to the identity, so that their coefficients disappear). It seems to be the case for Bulletproofs that this doesn't cause problems, but there are certainly other protocols where it does. Checking that the prover-supplied points aren't the identity closes off this line of attack upfront, even if, for this protocol, that line may not lead anywhere.

So I view this as a defense-in-depth mechanism. Legitimate proofs won't have zero terms (or will have them with negligible probability), so I don't think that this check costs anything to the protocol, but it does add a hedge against a class of risk to the protocol.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a follow-up, here's a concrete example of the kind of thing I'm worried about: https://twitter.com/claudiorlandi/status/1090705045333659648

The description of the protocol implicitly assumed that some data was not the identity and nobody noticed, leaving a gap. Preemptively checking that points are not the identity (unless there's a reason to allow them) seems to me like a low-cost, sensible precaution.

transcript.validate_and_commit_point(b"T_2", &self.T_2)?;

let x = transcript.challenge_scalar(b"x");

Expand Down
30 changes: 30 additions & 0 deletions src/transcript.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,35 @@ use curve25519_dalek::ristretto::CompressedRistretto;
use curve25519_dalek::scalar::Scalar;
use merlin::Transcript;

use errors::ProofError;

pub trait TranscriptProtocol {
/// Commit a domain separator for an `n`-bit, `m`-party range proof.
fn rangeproof_domain_sep(&mut self, n: u64, m: u64);

/// Commit a domain separator for a length-`n` inner product proof.
fn innerproduct_domain_sep(&mut self, n: u64);

/// Commit a domain separator for a constraint system.
fn r1cs_domain_sep(&mut self);

/// Commit a 64-bit integer.
fn commit_u64(&mut self, label: &'static [u8], n: u64);

/// Commit a `scalar` with the given `label`.
fn commit_scalar(&mut self, label: &'static [u8], scalar: &Scalar);

/// Commit a `point` with the given `label`.
fn commit_point(&mut self, label: &'static [u8], point: &CompressedRistretto);

/// Check that a point is not the identity, then commit it to the
/// transcript. Otherwise, return an error.
fn validate_and_commit_point(
&mut self,
label: &'static [u8],
point: &CompressedRistretto,
) -> Result<(), ProofError>;

/// Compute a `label`ed challenge variable.
fn challenge_scalar(&mut self, label: &'static [u8]) -> Scalar;
}
Expand Down Expand Up @@ -56,6 +72,20 @@ impl TranscriptProtocol for Transcript {
self.commit_bytes(label, point.as_bytes());
}

fn validate_and_commit_point(
&mut self,
label: &'static [u8],
point: &CompressedRistretto,
) -> Result<(), ProofError> {
use curve25519_dalek::traits::IsIdentity;

if point.is_identity() {
Err(ProofError::VerificationError)
} else {
Ok(self.commit_bytes(label, point.as_bytes()))
}
}

fn challenge_scalar(&mut self, label: &'static [u8]) -> Scalar {
let mut buf = [0u8; 64];
self.challenge_bytes(label, &mut buf);
Expand Down