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

Add ecdsa_adaptor module #14

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

Add ecdsa_adaptor module #14

wants to merge 20 commits into from

Conversation

jonasnick
Copy link
Owner

@jonasnick jonasnick commented Apr 3, 2020

This module implements single signer ECDSA adaptor signatures following "One-Time Verifiably Encrypted Signatures A.K.A. Adaptor Signatures" by Lloyd Fournier https://github.com/LLFourn/one-time-VES/blob/master/main.pdf).

Note that at this module is currently a work in progress. It's not secure nor stable. Let me repeat: IT IS EXTREMELY DANGEROUS AND RECKLESS TO USE HIS MODULE IN PRODUCTION. DON'T!

@jonasnick jonasnick force-pushed the ecdsa-adaptor-sigs branch 2 times, most recently from a37083c to d6c271b Compare April 3, 2020 23:11
@jonasnick jonasnick force-pushed the ecdsa-adaptor-sigs branch from aa14da7 to d4f796b Compare April 4, 2020 23:19
@AdamISZ
Copy link

AdamISZ commented Apr 6, 2020

Thanks for putting this together, it's really cool if we can get this stuff in the lib.

A few more thoughts I thought it might be an idea to add here:

  • As already mentioned but don't want to forget: there's an argument that the second generator point (gen2) should be included in the deterministic nonce function (for the dleq proof). We agreed on mm that this is sensible.

.. on review I think you addressed all my other comments already, either in code or in TODO

Do you envisage having the dleq stuff separate at some point, given it may be useful outside of the ecdsa adaptor case?

@jonasnick
Copy link
Owner Author

there's an argument that the second generator point (gen2) should be included in the deterministic nonce function

Yeah, that's important. I fixed that right after you mentioned it, so unless I'm missing something, that fix should be included in the current version.

Do you envisage having the dleq stuff separate at some point, given it may be useful outside of the ecdsa adaptor case?

Perhaps the dleq stuff could be its own module and automatically turned on when you enable ecdsa-adaptors or whatever else.

@AdamISZ
Copy link

AdamISZ commented Apr 7, 2020

Yeah, that's important. I fixed that right after you mentioned it, so unless I'm missing something, that fix should be included in the current version.

Oh sorry, yes, I see it now. Looks good.

Perhaps the dleq stuff could be its own module and automatically turned on when you enable ecdsa-adaptors or whatever else.

Yes. Was thinking of coinjoin/dual funding type use cases ('scarcity token' things).

if (buf33[0] > 1) {
return 0;
}
secp256k1_ge_set_xo_var(p, &x, buf33[0]);

Choose a reason for hiding this comment

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

Absolutely, great catch. Pushed fix. There could be more issues like that because this PR doesn't include a complete test suite yet (as mentioned in the TODO list of secp256k1_ecdsa_adaptor.h.

return 1;
}

int secp256k1_ecdsa_adaptor_extract_secret(const secp256k1_context* ctx, unsigned char *adaptor_secret32, const secp256k1_ecdsa_signature *sig, const unsigned char *adaptor_sig65, const secp256k1_pubkey *adaptor) {
Copy link

Choose a reason for hiding this comment

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

What do you think about enforcing the logic that if the correct secret is extracted then this always implies the signature is correct (for both ECDSA and Schnorr). This is what I went for here: https://github.com/LLFourn/secp256kfun/blob/master/ecdsa_fun/src/adaptor/mod.rs#L256. I check that the R_x in the ciphertext is the same as the R_x in the final signature.

This is to protect against the weird but I think not unimaginable situation where an attacker sends you a sig that has a correct s value but a bogus R value off-chain. Since you successfully extract the secret the programmer may think that this implies the signature was valid too. If they try to use it later they may end up in a pickle.

This is not normally an issue from a protocol standpoint since it is usually the creator of the encrypted signature that is trying to extract the secret so the R value is not interesting. I'm not sure this is always the case though. I think there is no case where you want to extract the secret from an invalid signature.

It is also just faster to extract/verify at the same time rather than verify then extract.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good idea and very cheap to do. Added a fix that compares the R values. I haven't thought through all the edge cases to guarantee that the signature is correct.

Copy link

@LLFourn LLFourn Sep 7, 2020

Choose a reason for hiding this comment

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

Thanks! I will put this in the DLC specification then. I think as long as you are not enforcing low_s in verification then making sure that the R_x values are the same is sufficient as long as the encrypted signature is valid and you successfully recover the correct key.

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.

4 participants