This adds sign-to-contract and anti-exfil protocol functionality to the schnorrsig module. It is based on the same functions that already exist for ECDSA here:
Some commits and functions were adapted from the original work here: https://github.com/bitcoin-core/secp256k1/pull/589/
10@@ -11,6 +11,63 @@
11 #include "../../../include/secp256k1_schnorrsig.h"
12 #include "../../hash.h"
13
14+static uint64_t s2c_opening_magic = 0x5d0520b8b7f2b168ULL;
15+
16+static const unsigned char s2c_data_tag[16] = "s2c/schnorr/data";
17+static const unsigned char s2c_point_tag[17] = "s2c/schnorr/point";
Nice!
By the way, is there a reason why this uses S2C instead of simple addition (Scheme 6 in https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2020-March/017667.html) besides the fact that the ECDSA code uses S2C. Are you aware of any reason why the S2C variant would be preferable over Scheme 6? (I think @jonasnick once told me that he wasn’t aware of all the various variants when the work was started.)
It seems the tests fails on s390x, which is a big-endian machine, see https://cirrus-ci.com/task/6416044628639744?logs=cat_tests_log#L4 Is there some code in this PR where endianness could be a problem? I don’t see any but I only had a brief look.
By the way, is there a reason why this uses S2C instead of simple addition
I did it this way for two reasons:
About s390x: I am puzzled, I don’t see anything related to endianness in the code related to the failing test, but maybe I am missing something. I also replaced the uint64_t magic by an 8-byte buffer just in case there could be problems with large integers, but it didn’t seem to be the cause.
About s390x: I am puzzled, I don’t see anything related to endianness in the code related to the failing test, but maybe I am missing something. I also replaced the uint64_t magic by an 8-byte buffer just in case there could be problems with large integers, but it didn’t seem to be the cause.
According to the implementation of counting_illegal_callback_fn excepts an int32_t and not size_t. Maybe this is the problem.
26+ * guaranteed to be portable between different platforms or versions. It can
27+ * be safely copied/moved.
28+ */
29+typedef struct {
30+ /* magic is set during initialization */
31+ uint64_t magic;
About s390x: I am puzzled, I don’t see anything related to endianness in the code related to the failing test, but maybe I am missing something. I also replaced the uint64_t magic by an 8-byte buffer just in case there could be problems with large integers, but it didn’t seem to be the cause.
According to the implementation of
counting_illegal_callback_fnexcepts anint32_tand notsize_t. Maybe this is the problem.
Great catch, that seemed to be the problem! Fixed now.
Most of the commits are taken as-is from @jonasnick and @apoelstra, with the added exfil functions working the same way as in the ECDSA version in secp256k1-zpk.
It would be great if you could review this and let me know if there is any obstacles to getting this merged. I’d like to use this in the BitBox02 to extend the anti-exfil protocol to Schnorr sigs in Taproot inputs.
It would be great if you could review this and let me know if there is any obstacles to getting this merged
I just want to let you know that this on our radar. Things can move slow here and I think most of the contributors have been pretty busy in the remaining weeks (and at least I will still be busy with other stuff for ~2 weeks). I can’t speak for the others but this is a feature I’d like to see included in the library, so I’ll certainly come back to this soon.
It would be great if you could review this and let me know if there is any obstacles to getting this merged
I just want to let you know that this on our radar. Things can move slow here and I think most of the contributors have been pretty busy in the remaining weeks (and at least I will still be busy with other stuff for ~2 weeks). I can’t speak for the others but this is a feature I’d like to see included in the library, so I’ll certainly come back to this soon.
Gentle reminder :smile: The last time I did this for ECDSA, the frequent rebasings were sometimes actually very time-consuming and complicated. If would be great to review and merge this before complicated conflicts appear.
master.
155 */
156 typedef struct {
157 unsigned char magic[4];
158 secp256k1_nonce_function_hardened noncefp;
159 void *ndata;
160+ secp256k1_schnorrsig_s2c_opening* s2c_opening;
Adding fields to this struct is, I think, an incompatible API break. If a user of the library uses an old version of the header, and links against a new version of the library, I believe the library will access uninitialized fields. Is that right?
In theory, that’s ok, because we’re still in 0.x releases, so any breaks are allowed by SemVer, but this seems unnecessarily burdensome for users. Ideally, extra fields to the struct only affect calls that are impossible to make in old code (e.g. only by new API functions, or when function arguments are set to constants that don’t exist in old versions).
Idly wondering: can we (ab)use the magic for this? Change the magic in the header, and make the library check whether the old or the new magic is used, and trigger behavior based on that.
I believe the library will access uninitialized fields. Is that right?
That is right, good catch.
There could be multiple solutions:
secp256k1_schnorrsig_s2c_opening_init(s2c_opening) to initialize it inside the signing function, we could have the caller do this instead and check that the magic is set properly.It seems to me that option 2 is worse, as if the opening magic is not set properly (e.g. by a user using the old headers with the new library), then the function would error. This is better than proceeding silently, but it still is incompatible and forces the user act. The alternative to erroring is to ignore these fields to maintain compatibility, but that would be strange for new library users that want to use the functionality and don’t see a proper error message when they fail to initialize the opening struct.
Option 3 seems backwards as the point of secp256k1_schnorrsig_extraparams struct was, I presume, to be able to extend it in a backwards compatible fashion.
Option 2 probably makes sense to do for the same reason that the extraparams struct is initialized by the user - to prevent corrupting memory in case the user passes the wrong pointer, and to possibly use it as a version field in the future.
TL;DR: I think defining a new magic in the extraparams struct is the best option, and we should also have the caller initialize the secp256k1_schnorrsig_s2c_opening struct with the magic. What do you think?
As a sidenote: maybe it would be a good to introduce a version check or init function for this library in general, so that if breaking changes are necessary in the future, it could be caught early at runtime.
The extraparams struct was built with the intention that magic acts as versioning for the struct and therefore allows backward-compatible changes (there’s some mention of this in the original schnorrsig_sign_custom PR). I implemented a variant of the idea here. I was imagining to change schnorrsig_sign_custom to something like this:
0int secp256k1_schnorrsig_sign_custom(..., secp256k1_schnorrsig_extraparams *params_in) {
1 secp256k1_schnorrsig_extraparams params;
2
3 if (memcmp(params_in->magic, old_magic, sizeof(params_in->magic)) == 0) {
4 secp256k1_extraparams_old *params_tmp;
5 copy_old_into_new(¶ms, params_tmp);
6 } else if (memcmp(params_in->magic, SECP256K1_SCHNORRSIG_EXTRAPARAMS_MAGIC, sizeof(params_in->magic)) == 0) {
7 params = *params_in;
8 }
9 /* ... */
10}
While this does work in practice, I’m not sure how compliant this is with the C standard. It would access the old extraparams struct through a pointer to a new extraparams struct. Padding shouldn’t be an issue here because it only accesses the first member of the struct.
It would be better if there was a solution where correctness is more obvious.
I’m not sure how compliant this is with the C standard.
If you read the standard strictly, then it’s just UB to have conflicting declarations for the same function: https://stackoverflow.com/a/69620952
In practice, and probably even with a more pragmatic reading of the standard, it’s unclear what should go wrong. Following your example code, the callee will read the magic value by accessing bytes through an lvalue of type unsigned char. That’s always allowed (even if the new struct type has stricter alignment requirements than the old struct type). Moreover, the magic is guaranteed to be at the beginning of the struct, and it’s explicitly allowed to use pointers to a struct as pointers to the first element (after an appropriate cast). After determining the version, the callee will read the struct members through an lvalue of the correct type (not identical type with same name but “compatible type”), which is also allowed.
As the callee starts with reading characters, it’s very much like passing an unsigned char* directly to the function and then casting it a pointer to the right struct type. I think we can do this in practice.
Thanks for the helpful inputs everyone!
I added a new commit use the magic in the schnorrsig extraparams struct for versioning that implements this for easier review. I can squash it in the end. Please take a look.
39+} secp256k1_schnorrsig_s2c_opening;
40+
41+/** The signer commitment in the anti-exfil protocol is the original public nonce. */
42+typedef secp256k1_pubkey secp256k1_schnorrsig_anti_exfil_signer_commitment;
43+
44+/** Parse a sign-to-contract opening.
Would it be possible to add a succinct description of the actual protocol to this header, or a reference to it?
The idea is that this is essentially defining a new cryptographic protocol that isn’t formally specified anywhere. If so, it’d be good to have the specification right here, so someone doesn’t need to go read through the implementation in order to figure out what the exact protocol is.
That makes it easier to separately review the protocol itself, and compare the implementation to that protocol.
As an example, see https://github.com/bitcoin-core/secp256k1/pull/1129/files#diff-d3089adfea8d8bbc31e130bb67389ab9ef45ea4abf4a79eceec63a037359f39dR10-R44 for example, which does so for ElligatorSwift.
I added a specification in the header (in a new commit for easier review, will squash in the end).
The spec is mostly a copy/paste from
A very similar spec was present in @jonasnick’s original PR:
259+ const unsigned char *data32,
260+ const secp256k1_schnorrsig_s2c_opening *opening
261+) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4);
262+
263+
264+/** Create the initial host commitment to `rho`. Part of the Anti-Exfil Protocol.
@jonasnick @sipa @real-or-random should I continue here or open an equivalent PR in secp256k1-zkp, or something else?
It seems close to being ready for production but got stuck due to uncertainty about whether this belongs in this repo in the first place.
The files are copied from:
- https://github.com/BlockstreamResearch/secp256k1-zkp/blob/6152622613fdf1c5af6f31f74c427c4e9ee120ce/src/eccommit.h
- https://github.com/ElementsProject/secp256k1-zkp/blob/6152622613fdf1c5af6f31f74c427c4e9ee120ce/src/eccommit_impl.h
The tests are copied from:
https://github.com/BlockstreamResearch/secp256k1-zkp/blob/6152622613fdf1c5af6f31f74c427c4e9ee120ce/src/tests.c#L4175
Originally introduced in:
https://github.com/ElementsProject/secp256k1-zkp/commit/826bd04b43f823813c633449223595031d5c31f7,
where it was used to implement sign-to-contract for ECDSA.
Co-authored-by: Marko Bencun <mbencun+pgp@gmail.com>
Co-authored-by: Jonas Nick <jonasd.nick@gmail.com>
Adapted from https://github.com/bitcoin-core/secp256k1/pull/589/.
Co-authored-by: Marko Bencun <mbencun+pgp@gmail.com>
Adapted from https://github.com/bitcoin-core/secp256k1/pull/589/.
The data is hashed using a tagged hash with the "s2c/schnorr/data"
tag, which is consistent with the data hashing done in the ECDSA
version of sign-to-contract (in ElementsProject/secp256k1-zkp), where
the "s2c/ecdsa/data" tag is used. Similarly, the tweak hash tag is
"s2c/schnorr/point".
Co-authored-by: Jonas Nick <jonasd.nick@gmail.com>
These functions allow to perform the anti-exfil protocol. It is very
similar to the implementation of the same protocol for ECDSA in
ElementsProject/secp256k1-zkp.
The opening struct can't be use in
`secp256k1_schnorrsig_anti_exfil_signer_commit()` as it contains the
``nonce_is_negated` field, which can only be set correctly during
signing with s2c data. As a result, we must use the opening in the
commitment verification, so we also must check that the signer
commitment is the same as the one used during signing. The alternative
is to only compare the x-coordinate, in which case the opening struct
could skip `nonce_is_negated` and the struct could be reused in
`secp256k1_schnorrsig_anti_exfil_signer_commit()`, but it seems to
have a downside that it would prevent batch-verification of the
commitments.
This ensures compatibility in that it makes sure that the
`secp256k1_schnorrsig_sign_custom()` works for users using an older
version of the headers but linking against a newer version of the
library.
Adapted from
- https://github.com/BlockstreamResearch/secp256k1-zkp/blob/6152622613fdf1c5af6f31f74c427c4e9ee120ce/include/secp256k1_ecdsa_s2c.h#L100
- https://github.com/bitcoin-core/secp256k1/pull/590/files#diff-75af67a78d848e432dcf1b975d64978c8d731530fcbde6c9b15edf7da556af00R65
@benma I agree with sipa’s comment above that a specification would be helpful. According to our CONTRIBUTING.md it is recommended to provide a specification and security arguments in order to add a new module. I think the relevance criterion for the new module is met.
Thanks. I added the specification in the comments, see #1140 (review).
Please take another look @jonasnick @sipa @real-or-random.
Labels
feature