ecdsa sign-to-contract module, with anti nonce covert chan util functions #669

benma · 2019-10-08T16:33:14Z

Alternative to #637, where the main s2c signing function lives in secp256k1.c. See this thread for some discussion about this: #637 (comment)

src/secp256k1.c

benma · 2019-10-10T09:16:16Z

I added a commit (recovery: re-use secp256k1_ecdsa_sign_helper) to show that the recovery module can also re-use the sign-helper as it was basically a copy/paste of secp256k1_ecdsa_sign().

Seeing this, after fixing up some cosmetics, I would prefer this alternative.

bitcoin-core/secp256k1#669

jonasnick

The approach in this alternative looks good. I didn't find any obvious critical vulnerabilities. It passes valgrind and coverage looks good. Being able to subsume recovery into the signing helper is a nice bonus.

Some of the things in this PR don't make much sense anymore because we're using a different public key type for schnorrsigs (xonly_pubkey) and therefore there's no reason anymore to have a unified s2c_opening for both ecdsa and schnorr. Also, the ec_commit functions don't apply to xonly_pubkey. I will rip this out of my PR and see if there's still some code that can be shared.

I'm wondering if a stable s2c api should allow the callers to provide their own hash function through a callback for example to create tagged hashes.

As an aside, this PR taken on its own lacks a detailed explanation for how it works. There is a more detailed writeup in my PR and I don't think we need both the protocol steps in this PR + the more detailed writeup.

There are some api artefacts in this PR (shared with mine) which are not ideal. For example secp256k1_pubkey *client_commit which isn't a pubkey (but a point).

include/secp256k1_ecdsa_sign_to_contract.h

src/secp256k1.c

src/modules/ecdsa_sign_to_contract/main_impl.h

include/secp256k1_ecdsa_sign_to_contract.h

jonasnick · 2019-11-13T21:18:59Z

src/modules/ecdsa_sign_to_contract/main_impl.h

+        return 0;
+    }
+
+    /* Check that sigr (x coordinate of R) matches the x coordinate of the commitment. */


nit: stress somehow that sigr is a scalar and the x coordinate of the commitment is a field element. I overlooked it at first. Also that sig[0:32] could be greater than the group order and signature_load won't complain. Perhaps `Check that sigr (x coordinate of R represented by a scalar) matches the x coordinate of the commitment (field element). Note that sig[0:32] can be greater than group order and therefore sigr will be reduced. But in that case ecdsa_verify doesn't pass"

More importantly, there needs to be a sentence explaining why comparing the x coordinate is sufficient. For example, because there's only two y coordinates per x coordinate and given an opening for a point, it's as difficult finding an opening to the negation of the point as to any other point. However, I do think there's some reduction in security because it's more likely to find a collision with a point and it's negation.

nit: stress somehow that sigr is a scalar and the x coordinate of the commitment is a field element.

Will do, though I thought that was clear as it is the same way with regular ecdsa verification.

Note that sig[0:32] can be greater than group order and therefore sigr will be reduced. But in that case ecdsa_verify doesn't pass

That is not true, is it? ecdsa_verify can still pass in this case, as the condition is sig_r == x (mod n) (the field element x is also reduced modulo n).

In the current code a few lines down, there actually might be a "mistake": we memory-compare the byte representation of the scalar sigr and the field element x, without first reducing x modulo n, which might fail for actually valid commitments with very low (negligible) probability.

Shouldn't this be implemented the same way as the actual secp256k1_ecdsa_verify()? Basically convert the field element x to a scalar and use secp256k1_scalar_eq() instead of memcmp, like here:

secp256k1/src/ecdsa_impl.h

Lines 236 to 238 in 074ab58

secp256k1_fe_get_b32(c, &pr_ge.x);

secp256k1_scalar_set_b32(&computed_r, c, NULL);

return secp256k1_scalar_eq(sigr, &computed_r);

edit: seems like secp256k1_scalar_set_b32() reduces modulo n

~~Generally a bit confused, I don't see "modulo n" reduction happening in signing nor verification despite it being part of the algorithm as described here:~~

~~https://en.wikipedia.org/wiki/Elliptic_Curve_Digital_Signature_Algorithm~~

Added one commit to extend the comment, and one to change to secp256k1_scalar_eq() to properly compare the values modulo n. The second comment about the reduced security will follow soon.

More importantly, there needs to be a sentence explaining why comparing the x coordinate is sufficient. For example, because there's only two y coordinates per x coordinate and given an opening for a point, it's as difficult finding an opening to the negation of the point as to any other point. However, I do think there's some reduction in security because it's more likely to find a collision with a point and it's negation.

Done.

src/modules/ecdsa_sign_to_contract/tests_impl.h

include/secp256k1.h

benma · 2019-12-29T19:57:27Z

Rebased and addressed some comments; I will take care of the others soon.

benma · 2020-01-17T12:40:30Z

Some of the things in this PR don't make much sense anymore because we're using a different public key type for schnorrsigs (xonly_pubkey) and therefore there's no reason anymore to have a unified s2c_opening for both ecdsa and schnorr. Also, the ec_commit functions don't apply to xonly_pubkey. I will rip this out of my PR and see if there's still some code that can be shared.

Let me know what to do - I'm okay to move the opening to the module, or keep it as it is and let you figure out what can be reused in your PR (if this PR is merged first).

I'm wondering if a stable s2c api should allow the callers to provide their own hash function through a callback for example to create tagged hashes.

My feeling is that we should keep it simple for now and move towards merging. If someone comes up with the need for this, I'd be happy to extend it then. It wouldn't be an invasive API change, so clients could update easily.

As an aside, this PR taken on its own lacks a detailed explanation for how it works. There is a more detailed writeup in my PR and I don't think we need both the protocol steps in this PR + the more detailed writeup.

Maybe once both PRs are merged, we can move the description to docs/anti_nonce_covert_channel_protocol.md and link to it from both modules? For now I copy/pasted the extended details of your docs to mine, I hope that's ok. The docs seem sufficient to me for a start, and of course can always be improved.

…eaks of public keys. The functionality is not exposed.

…tions

Sign a message while comitting to some data at the same time by by adding `hash(k1*G, data)` to the rfc6979 deterministic nonce. Includes verification function to check that the signature commits to the data.

The body of the recovery sign function is 99% the same secp256k1_ecdsa_sign. Now that this has moved to the sign helper, it can be re-used not only by the sign-to-contract module, but also by the recovery module.

benma · 2020-04-21T12:53:27Z

Rebased / fixed conflicts.

@jonasnick any chance of getting this merged soon? :)

apoelstra · 2020-11-27T21:42:46Z

I think we can merge this into -zkp soon. I have BlockstreamResearch/secp256k1-zkp#111. If you do your own rebase it might be easier to start from that since I did the work of combing the new unified ecdsa/recovery code with this PR's implementation of the same thing.

benma · 2020-12-03T15:53:55Z

I think we can merge this into -zkp soon. I have ElementsProject/secp256k1-zkp#111. If you do your own rebase it might be easier to start from that since I did the work of combing the new unified ecdsa/recovery code with this PR's implementation of the same thing.

😍 😍 😍

I will make time for this as soon as I can. Been eager to get this working in production for a long time.

Edit: I did not realize that your PR is already there and rebased. Great.

benma · 2020-12-16T22:23:48Z

This PR was rebased and reopened at BlockstreamResearch/secp256k1-zkp#111, where review and improvements continue.

47efb5e ecdsa-s2c: add ctime tests (Andrew Poelstra) 396b558 ecdsa-s2c: add anti-klepto protocol (Andrew Poelstra) 290dee5 ecdsa-s2c: add actual sign-to-contract functionality (Andrew Poelstra) 8e46cac ecdsa-s2c: block in module (Andrew Poelstra) 826bd04 add eccommit functionality (Andrew Poelstra) Pull request description: This is a backport and rebase of bitcoin-core/secp256k1#669 ACKs for top commit: jonasnick: ACK 47efb5e real-or-random: ACK 47efb5e Tree-SHA512: e1f3ee3985bc77197eb57c03884b5d4a5f8733523bba50e11309f86388471c6265b7241e9856e1b80a88f4c268f2826c0394e26161292aa438b2246a1ad86aa1

benma mentioned this pull request Oct 8, 2019

ecdsa sign-to-contract module, with anti nonce covert chan util functions #637

Closed

benma commented Oct 8, 2019

View reviewed changes

src/secp256k1.c Outdated Show resolved Hide resolved

benma commented Oct 8, 2019

View reviewed changes

src/secp256k1.c Outdated Show resolved Hide resolved

benma added a commit to BitBoxSwiss/libwally-core that referenced this pull request Oct 13, 2019

pull in ecdsa sign to contract PR

a110572

bitcoin-core/secp256k1#669

benma added a commit to BitBoxSwiss/libwally-core that referenced this pull request Oct 13, 2019

pull in ecdsa sign to contract PR

64e56f8

bitcoin-core/secp256k1#669

jonasnick reviewed Nov 13, 2019

View reviewed changes

benma force-pushed the ecdsa_nonce_sidechan_alternative branch from 93067b1 to ab49ce1 Compare December 29, 2019 19:54

benma changed the title ~~[alternative] ecdsa sign-to-contract module, with anti nonce covert chan util functions~~ ~[alternative] ~ecdsa sign-to-contract module, with anti nonce covert chan util functions Dec 30, 2019

benma changed the title ~~~[alternative] ~ecdsa sign-to-contract module, with anti nonce covert chan util functions~~ ~[alternative]~ ecdsa sign-to-contract module, with anti nonce covert chan util functions Dec 30, 2019

benma changed the title ~~~[alternative]~ ecdsa sign-to-contract module, with anti nonce covert chan util functions~~ ecdsa sign-to-contract module, with anti nonce covert chan util functions Dec 30, 2019

benma force-pushed the ecdsa_nonce_sidechan_alternative branch from ab49ce1 to e2728ee Compare December 30, 2019 14:06

benma force-pushed the ecdsa_nonce_sidechan_alternative branch 3 times, most recently from ceedfca to ffc0872 Compare January 17, 2020 12:38

benma force-pushed the ecdsa_nonce_sidechan_alternative branch from ffc0872 to 642fb4a Compare January 17, 2020 12:45

jonasnick and others added 8 commits April 21, 2020 13:08

Add ec_commitments which are essentially the pay-to-contract-style tw…

437ef77

…eaks of public keys. The functionality is not exposed.

Add and expose sign-to-contract opening with parse and serialize func…

ad4b685

…tions

modules: add ecdsa_sign_to_contract

43df8e1

Sign a message while comitting to some data at the same time by by adding `hash(k1*G, data)` to the rfc6979 deterministic nonce. Includes verification function to check that the signature commits to the data.

ecdsa_sign_to_contract: add Anti Nonce Covert Channel util functions

99d639b

recovery: re-use secp256k1_ecdsa_sign_helper

0b74a94

The body of the recovery sign function is 99% the same secp256k1_ecdsa_sign. Now that this has moved to the sign helper, it can be re-used not only by the sign-to-contract module, but also by the recovery module.

f serialize s2c_opening as 33 bytes instead of 34

59113ab

f Add ec_commitments

2fcd799

use reduce field element x modulo n, use secp256k1_scalar_eq over memcmp

1a0f2ed

benma force-pushed the ecdsa_nonce_sidechan_alternative branch from 642fb4a to 1a0f2ed Compare April 21, 2020 12:46

apoelstra mentioned this pull request Nov 26, 2020

Add ECDSA sign-to-contract module BlockstreamResearch/secp256k1-zkp#111

Merged

benma closed this Dec 16, 2020

generalsunion approved these changes Jul 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ecdsa sign-to-contract module, with anti nonce covert chan util functions #669

ecdsa sign-to-contract module, with anti nonce covert chan util functions #669

benma commented Oct 8, 2019 •

edited

Loading

benma commented Oct 10, 2019

jonasnick left a comment

jonasnick Nov 13, 2019

jonasnick Nov 13, 2019

benma Jan 16, 2020 •

edited

Loading

benma Jan 16, 2020 •

edited

Loading

benma Jan 16, 2020

benma Jan 17, 2020

benma commented Dec 29, 2019

benma commented Jan 17, 2020 •

edited

Loading

benma commented Apr 21, 2020

apoelstra commented Nov 27, 2020

benma commented Dec 3, 2020 •

edited

Loading

benma commented Dec 16, 2020

	secp256k1_fe_get_b32(c, &pr_ge.x);
	secp256k1_scalar_set_b32(&computed_r, c, NULL);
	return secp256k1_scalar_eq(sigr, &computed_r);

ecdsa sign-to-contract module, with anti nonce covert chan util functions #669

ecdsa sign-to-contract module, with anti nonce covert chan util functions #669

Conversation

benma commented Oct 8, 2019 • edited Loading

benma commented Oct 10, 2019

jonasnick left a comment

Choose a reason for hiding this comment

jonasnick Nov 13, 2019

Choose a reason for hiding this comment

jonasnick Nov 13, 2019

Choose a reason for hiding this comment

benma Jan 16, 2020 • edited Loading

Choose a reason for hiding this comment

benma Jan 16, 2020 • edited Loading

Choose a reason for hiding this comment

benma Jan 16, 2020

Choose a reason for hiding this comment

benma Jan 17, 2020

Choose a reason for hiding this comment

benma commented Dec 29, 2019

benma commented Jan 17, 2020 • edited Loading

benma commented Apr 21, 2020

apoelstra commented Nov 27, 2020

benma commented Dec 3, 2020 • edited Loading

benma commented Dec 16, 2020

benma commented Oct 8, 2019 •

edited

Loading

benma Jan 16, 2020 •

edited

Loading

benma Jan 16, 2020 •

edited

Loading

benma commented Jan 17, 2020 •

edited

Loading

benma commented Dec 3, 2020 •

edited

Loading