Schnorr sign-to-contract and anti-exfil

[benma]

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:

https://github.com/ElementsProject/secp256k1-zkp/blob/d22774e248c703a191049b78f8d04f37d6fcfa05/include/secp256k1_ecdsa_s2c.h

Some commits and functions were adapted from the original work here: #589

[benma]

If these tags are okay, I can add functions with precomputed states if desired.

[real-or-random]

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.

[benma]

By the way, is there a reason why this uses S2C instead of simple addition

I did it this way for two reasons:

• sign-to-contract seems useful also for other uses, see Implement sign-to-contract scheme for BIP-340 signatures BlockstreamResearch/secp256k1-zkp#154
• It seemed to be the easiest approach, seeing that the same was already implemented for ECDSA. That means it should need less time to code and review and maintain.

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.

[real-or-random]

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.

[benma]

I took this for a commit from @jonasnick, but I am actually unsure what the purpose is of the magic. Absent programming errors inside the library, could something go wrong if there was no magic field? Are there guidelines of when to use a magic (most structs don't contain any)?

[sipa]

I think the magics are a relative recent addition, and just there for assurance, so that incorrect use can be detected. In correct usage, they serve no function.

[benma]

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.

Great catch, that seemed to be the problem! Fixed now.

[benma]

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.

[real-or-random]

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.

[benma]

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

[benma]

Rebased to fix conflicts with master.

[benma]

@real-or-random @jonasnick @apoelstra any chance of progress here? The code should be straight forward, with a little bit of concentrated effort we could get this merged and used in production 🙏

[sipa]

I think the magics are a relative recent addition, and just there for assurance, so that incorrect use can be detected. In correct usage, they serve no function.

[sipa]

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.

[benma]

I believe the library will access uninitialized fields. Is that right?

That is right, good catch.

There could be multiple solutions:

1. As you suggest, the magic can be used as a version field.
2. The s2c_opening struct itself contains a magic field. Instead of calling 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.
3. not adding these fields to extraparams but add another function secp256k1_schneorrsig_sign_s2c instead.

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.

[jonasnick]

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:

int secp256k1_schnorrsig_sign_custom(..., secp256k1_schnorrsig_extraparams *params_in) {
    secp256k1_schnorrsig_extraparams params;

    if (memcmp(params_in->magic, old_magic, sizeof(params_in->magic)) == 0) {
        secp256k1_extraparams_old *params_tmp;
        copy_old_into_new(&params, params_tmp);
    } else if (memcmp(params_in->magic, SECP256K1_SCHNORRSIG_EXTRAPARAMS_MAGIC, sizeof(params_in->magic)) == 0) {
        params = *params_in;
    }
    /* ... */
}

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.

[real-or-random]

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.

[benma]

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.

[sipa]

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.

[benma]

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

• https://github.com/BlockstreamResearch/secp256k1-zkp/blob/6152622613fdf1c5af6f31f74c427c4e9ee120ce/include/secp256k1_ecdsa_s2c.h#L100

A very similar spec was present in @jonasnick's original PR:

• https://github.com/bitcoin-core/secp256k1/pull/590/files#diff-75af67a78d848e432dcf1b975d64978c8d731530fcbde6c9b15edf7da556af00R65

[sipa]

Here as well, I think it'd be good to add a specification of the Anti-Exfil protocol, and/or references to such a specification.

[benma]

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

[jonasnick]

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

[benma]

@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 (comment).

Please take another look @jonasnick @sipa @real-or-random.