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

Possible error in SchnorrProof.make_schnorr_proof(), or spec 2.A changed? #278

Closed
JohnLCaron opened this issue Jan 15, 2021 · 11 comments
Closed
Assignees
Labels
code owner only Only for Code Owner wontfix This will not be worked on

Comments

@JohnLCaron
Copy link
Contributor

JohnLCaron commented Jan 15, 2021

In writing a validator for equation 2.A ("2. Guardian Public-Key Validation")

   (A) The challenge c i,j is correctly computed as c = H(Q, K, h ) mod q, where Q is the crypto_base_hash.

This test is failing in my validator. But if I remove Q, it works.

c = H(K, h ) mod q.

Looking at line 80 of schnorr.py:

c = hash_elems(k, h)

possible error? Or spec had to be changed because crypto_base_hash not available?

@JohnLCaron
Copy link
Contributor Author

So the question is for @benaloh or another crypto expert, as to whether the challenge should / must include Q in it, and to the python implementors if the omission was deliberate, and if so why. If its inadvertent, I can submit a PR.

@benaloh
Copy link

benaloh commented Jan 17, 2021

Yes. The base hash Q should be included in all of the subsequent hash computations used for challenges and otherwise. This binds all of the parameters together so that an attacker can't try to mix and match things in inappropriate ways.

@JohnLCaron
Copy link
Contributor Author

Fixing this proof will mean that validation will break for previously generated election records.

Perhaps the record serialization needs to be versioned, since its likely other breaking changes will be needed in the future? Perhaps add a version field to CiphertextElectionContext?

@keithrfung keithrfung added this to the 🔥 Trogdor v1.3.0 milestone Jan 20, 2021
@keithrfung keithrfung added the code owner only Only for Code Owner label Jan 20, 2021
@kiniry
Copy link

kiniry commented Jan 27, 2021

It looks like this computation was correctly implemented in the original C implementation and the Rust verifier. How did this change happen/evolve in the Python and new verifier(s)? That's a large change in protocol.

@danwallach
Copy link
Collaborator

Probably happened because I typed it in wrong from the spec.

@AddressXception AddressXception self-assigned this Feb 4, 2021
@AddressXception
Copy link
Collaborator

AddressXception commented Feb 5, 2021

IIRC, this was a decision made to allow guardians to exist (and be verifiable) externally to a specific election context. Is this a feature we want to keep, or do we want to instead follow the specification and require that new guardians are seeded for every election?

Some use cases have allowed for the same guardians to be used across multiple elections without having to run a key ceremony. I can see value in keeping this, but I can also see a strong case for the protocol forcing the regeneration of guardians for every election and thus requiring the application layer to handle the user experience as it sees fit.

We already bind a specific set of guardians to a specific ElectionDescription via the CiphertextElectionContext.elgamal_public_key and the crypto_extended_base_hash, but the reverse is not true. Is that enough?

Should we update the code, or should we update the spec?

@benaloh
Copy link

benaloh commented Feb 5, 2021

I'd argue against forcing new keys for each election. This seems to be more of a policy decision outside of EG.

If the guardian set changes, there should definitely be new keys. One can argue that for large, distinct elections, new keys are a good idea even if the same guardians are used. But there are also use cases where the same guardians are to be used for a sequence of separate elections that may occur in rapid-fire succession, and there is no reason to mandate burdensome re-keying for such cases.

@AddressXception
Copy link
Collaborator

I'd argue against forcing new keys for each election. This seems to be more of a policy decision outside of EG.

If the guardian set changes, there should definitely be new keys. One can argue that for large, distinct elections, new keys are a good idea even if the same guardians are used. But there are also use cases where the same guardians are to be used for a sequence of separate elections that may occur in rapid-fire succession, and there is no reason to mandate burdensome re-keying for such cases.

Ok sounds good, I'm reading that then as we should modify the spec to match the python repo instead of modifying the python repo to match the spec. Have we reached a quorum?

@benaloh
Copy link

benaloh commented Feb 5, 2021

The (extended) base hash should be included in (almost?) all subsequent hash computations in an election. This is what binds events to a particular election.

Can the commitments to key shares be an exception? Probably ... since this is mostly an internal negotiation between guardians.

Should the commitments to key shares be an exception? Probably not ... as the cost of the extra binding is minimal and the public may have some interest in resolution of disputes between guardians.

@AddressXception
Copy link
Collaborator

The (extended) base hash should be included in (almost?) all subsequent hash computations in an election. This is what binds events to a particular election.

Can the commitments to key shares be an exception? Probably ... since this is mostly an internal negotiation between guardians.

Should the commitments to key shares be an exception? Probably not ... as the cost of the extra binding is minimal and the public may have some interest in resolution of disputes between guardians.

ah, just saw this, see my comment on this other issue

@AddressXception
Copy link
Collaborator

cross posting from #279

ok great, so for clarity, we'll make the change in #279 to include the commitments as part of the calculation of the crypto_extended_base_hash. #278 will be closed as WONT_FIX and instead the specification will be adjusted so that schnorr proofs do not include the crypto_base_hash in their calculation and guardians are not explicitly coupled to an election (but an election is still explicitly coupled to a set of guardians.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code owner only Only for Code Owner wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

6 participants