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

Feat: Add Schnorr signature scheme #313

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Feat: Add Schnorr signature scheme #313

wants to merge 7 commits into from

Conversation

yelhousni
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@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.

There is a key leakage/forgeability problem imo.

var P bls12377.G1Affine
P.ScalarMultiplicationBase(k)

if hFunc != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I think this is incorrect. In Schnorr we use the hash for Fiat-Shamir challenge. If hFunc==nil, then we do not hash signer randomness into challenge and get a weak instance. Depending on the attack, we either leak secret key or allow forging signatures.

In description, we should rather think as the challenge as H1(P || H2(m)), where hFunc refers to H2 not H1. H1 should be fixed per-curve to fill the whole scalar element.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought about it. I think the secret key is safe, but still allows forgeability.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think there is no point into hashing the message (H2) before hashing its concatenation with P (H1), as we consider arbitrary long messages. My initial thought was that if the user uses nil as hFunc it would mean he did H(P || m) prior to signing, and we just convert it to and integer. But I agree that if it opens room to ambiguity we should clear this up. So either:

  • We remove the hFunc argument from the interface and enforce a hash function inside Sign() and Verify() for all signature schemes --> but we wouldn't be able to replace with a SNARK-friendly hash in gnark if needed, or
  • if hFunc == nil then we use an encoded hash inside Sign() and Verify(), otherwise we use the hFunc provided by the caller --> but we would always compute a hash and can't delegate this (e.g. out-circuit in SNARKs), or
  • hFunc would mean H1 as in your example and we always do H2 --> same we can't delegate H2 but the interface and implementation would be similar across signature schemes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

One option is if there is some nice SNARK-friendly instantiation of the Fiat-Shamir challenger for Schnorr signatures. https://eprint.iacr.org/2020/915.pdf says there is (Theorem 2.4 and Construction 2.5), but as I understand, imo requires a (one-time?) trusted setup. But if it is true, then we can fix H1 and hFunc gives H2.

var _P bls12377.G1Affine
_P.FromJacobian(&P)

if hFunc != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

See comment about Fiat-Shamir in Sign.

@ivokub
Copy link
Collaborator

ivokub commented Jan 30, 2023

@yelhousni - actually, maybe we could change interfaces such that the hash functions are a property of the key, not runtime parameter to Sign and Verify. So, when we have schnorr.GenerateKey then we feed all the properties we want (hash function H1, H2 etc.) and then later just call pk.Sign(msg) and vk.Verify(sig, msg). We could also embed the information in the marshalled keys and so when unmarshalling all the properties are also applied.

I think this is also a bit more future-proof as for example RSA has a lot of parameters you can tweak (not that we want to get into RSA-land, but just in case) and it really doesn't make sense to pass them manually around everywhere. And if we do not pass any options when generating the keys, then we choose defaults (e.g. SHA256 for ECDSA etc.). Does it makes sense? If so, then I would create a new issue to design it further.

@yelhousni
Copy link
Collaborator Author

Does it makes sense? If so, then I would create a new issue to design it further.

Yup that makes sense.

@yelhousni yelhousni self-assigned this Apr 5, 2023
@yelhousni yelhousni added this to the v0.10.0 milestone Apr 5, 2023
@gbotrel gbotrel changed the base branch from develop to master August 22, 2023 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants