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 schnorrsig module which implements BIP-schnorr compliant signatures #558

Open
wants to merge 2 commits into
base: master
from

Conversation

@jonasnick
Copy link
Contributor

commented Sep 25, 2018

This PR implements signing, verification and batch verification as described in BIP-Schnorr (https://github.com/sipa/bips/blob/bip-schnorr/bip-schnorr.mediawiki) in an experimental module schnorrsig. It includes the test vectors and a benchmarking tool and also adds ChaCha20 as a CSPRNG for batch verification.

In order to enable the module run ./configure with --enable-experimental --enable-module-schnorrsig.

Based on @apoelstra's work.

@jonasnick jonasnick force-pushed the jonasnick:schnorrsig branch 2 times, most recently from 220012e to 547ad32 Sep 25, 2018

@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

Replaced the chacha20 commit with a similar commit from secp256k1-zkp (ElementsProject/secp256k1-zkp@c3794f9).

@apoelstra

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

ACK except nit about sha object, bikeshedding about the module name, and also I did not check that the static test vectors match those in the BIP.

@gmaxwell

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2018

Maybe we should consider adopting an anti-covert-channel warden workflow as the standard interface for this function?

@jimpo
Copy link

left a comment

Nice! Such hype.

I skipped the tests, hoping to get back around to them.

Show resolved Hide resolved src/bench_schnorrsig.c Outdated
Show resolved Hide resolved src/bench_schnorrsig.c Outdated
Show resolved Hide resolved src/bench_schnorrsig.c
Show resolved Hide resolved src/bench_schnorrsig.c
Show resolved Hide resolved include/secp256k1_schnorrsig.h
Show resolved Hide resolved src/modules/schnorrsig/main_impl.h Outdated
Show resolved Hide resolved src/modules/schnorrsig/main_impl.h
Show resolved Hide resolved src/modules/schnorrsig/main_impl.h Outdated
@apoelstra

This comment has been minimized.

Copy link
Member

commented Oct 1, 2018

@gmaxwell By anti-covert channel do you mean essentially sign-to-contracting random data? I would like this. One thing blocking it is that our nonce function does not take a secp context currently, which makes sign-to-contract unergonmic -- see in sighacker how the sign-to-contract context needs to contain a pointer to the secp context.

I think we should fix that but it should probably be in another PR.

@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2018

Thanks @jimpo. I added a commit that addresses your comments.

@floreslorca

This comment has been minimized.

Copy link

commented Oct 8, 2018

how does this relate to #212 ?

@apoelstra

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

#212 is not secure against rogue-key attacks nor does it commit to the public key being signed for.

@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2018

and #212 was removed in #425

@real-or-random

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2018

Oh I was not aware of this PR.
FYI, I suggested that a CSPRNG is not enough for batch validation and we need a hash function to generate the seed (https://github.com/sipa/bips/pull/15/files) but now I see that this PR is doing that anyway. :)

Show resolved Hide resolved src/modules/schnorrsig/main_impl.h Outdated
Show resolved Hide resolved .gitignore Outdated
Show resolved Hide resolved include/secp256k1_schnorrsig.h Outdated
Show resolved Hide resolved include/secp256k1_schnorrsig.h Outdated
Show resolved Hide resolved src/modules/schnorrsig/main_impl.h
Show resolved Hide resolved src/modules/schnorrsig/main_impl.h Outdated
@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

@real-or-random Thanks for the review. I added a commit to address your comments.

@jonasnick jonasnick force-pushed the jonasnick:schnorrsig branch from 7d8391d to 8193edd Oct 16, 2018

@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2018

Added a test to increase the coverage of schnorrsig_sign. Now coverage in the schnorrsig module is 100% when excluding the lines that can't be hit. See https://htmlpreview.github.io/?https://raw.githubusercontent.com/jonasnick/secp256k1/schnorrsig-stats/coverage.src_modules_schnorrsig_main_impl.h.html

@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2018

squashed and rebased on master

Show resolved Hide resolved src/tests.c
Show resolved Hide resolved src/scalar_8x32_impl.h Outdated
* (https://github.com/sipa/bips/blob/bip-schnorr/bip-schnorr.mediawiki).
*/

/** Opaque data structure that holds a parsed Schnorr signature.

This comment has been minimized.

Copy link
@sipa

sipa Nov 5, 2018

Contributor

As the Schnorr signature BIP defines the actual byte representation of signatures, there is no distinction between "invalid because incorrect values" and "invalid because incorrect serialization" (as is the case with ECDSA and DER encoding).

Because of that, there seems to not be any strict need for separately exposed parse/serialize functions. That is unless we expect operations besides sign/verify that operate on signatures, which one is expected to use without intervening serialization step. In that case we may opt to later change the representation inside to be a secp256k1_fe_storage and a secp256k1_scalar.

This comment has been minimized.

Copy link
@apoelstra

apoelstra Nov 6, 2018

Member

We cannot use secp256k1_fe_storage or secp256k1_scalar because the size of these objects is not exported, so it would prevent users building them on the stack.

Eliminating parse/serialize operations will prevent any future changes we might want to do that could use a more efficient format. On the other hand, I can't imagine what future changes those might be.

This comment has been minimized.

Copy link
@sipa

sipa Nov 6, 2018

Contributor

Look at how secp256k1_pubkey_load works :)

And yes, I can't imagine what those operations would be either. I'd like to make sure we're not committing to an unnecessarily complex API without reason.

This comment has been minimized.

Copy link
@jonasnick

jonasnick Nov 7, 2018

Author Contributor

Makes sense to me to remove the parse/serialize functions. In the event that there's a better representation it could be its own struct.

This comment has been minimized.

Copy link
@jonasnick

jonasnick Jan 29, 2019

Author Contributor

That is unless we expect operations besides sign/verify that operate on signatures, which one is expected to use without intervening serialization step.

There are definitely operations besides sign/verify that operate on signatures, for example verifying a sign-to-contract commitment as implemented here https://github.com/bitcoin-core/secp256k1/pull/572/files#diff-b19c5ee427283d4d82bc5beb4e2f4777R202

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Apr 17, 2019

Contributor

We shouldn't speculate on future needs that we're not sure of too much-- if something useful turns up we can change the API. A guessed API might not be right in any case.

@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2018

Added commit that will switch to little endian format when interpreting chacha20 output, replace chacha20 tests with test vectors from the RFC, add sipa's chacha20 test.

@jonasnick jonasnick referenced this pull request Dec 22, 2018

Merged

Add MuSig module #35

@real-or-random

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

To save others from looking through it, the interesting lines are

+ ./tests
BUILDSTDERR: src/tests.c:1044: test condition failed: secp256k1_scalar_eq(&exp_r1, &r1)
BUILDSTDERR: /var/tmp/rpm-tmp.SSskCl: line 31: 56834 Aborted                 (core dumped) ./tests
secp256k1_scalar_chacha20(&r1, &r2, seed0, 0);
secp256k1_scalar_set_b32(&exp_r1, &expected1[0], NULL);
secp256k1_scalar_set_b32(&exp_r2, &expected1[32], NULL);
CHECK(secp256k1_scalar_eq(&exp_r1, &r1));

This comment has been minimized.

Copy link
@real-or-random

real-or-random May 23, 2019

Contributor

(here)

@real-or-random

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

@hegjon
Do you happen to have the stdout too (instead of only the stderr)? It contains a random seed for the entire test run.
Is the failure reproducible?

(Sorry for 3 comments in 5 min...)

@hegjon

This comment has been minimized.

Copy link

commented May 23, 2019

@hegjon
Do you happen to have the stdout too (instead of only the stderr)? It contains a random seed for the entire test run.
Is the failure reproducible?

I can check and see if I am able to find them, I could also try to redirect stdout to stderr while debugging.

(Sorry for 3 comments in 5 min...)

No worries, thanks for replying

@hegjon

This comment has been minimized.

Copy link

commented May 23, 2019

Is the failure reproducible?

Seems like it fails every time for s390x, will try to get the stdout

@hegjon

This comment has been minimized.

Copy link

commented May 24, 2019

If someone want to debug this bug on a s390x system, paste your public ssh key, I and will try to grant you access to one

@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2019

I can confirm that I get the same error on a big endian 64 bit mips system.

@@ -946,4 +949,94 @@ SECP256K1_INLINE static void secp256k1_scalar_mul_shift_var(secp256k1_scalar *r,
secp256k1_scalar_cadd_bit(r, 0, (l[(shift - 1) >> 6] >> ((shift - 1) & 0x3f)) & 1);
}

#define ROTL32(x,n) ((x) << (n) | (x) >> (32-(n)))

This comment has been minimized.

Copy link
@jb55

jb55 May 27, 2019

the result of this will be invalid if the integer type of x has a storage size larger than 32 bits.

for instance ROTL32((1UL << 31), 1) will not equal 1. It doesn't look like it's relevant for the current PR as it stands, but this is a common gotcha when using 64 bit literals.

suggest: ((uint32_t)(v) << (n)) | ((uint32_t)(v) >> (32 - (n)))

This comment has been minimized.

Copy link
@jonasnick

jonasnick May 27, 2019

Author Contributor

Integers given to this macro are always 32 bits

@@ -718,4 +720,102 @@ SECP256K1_INLINE static void secp256k1_scalar_mul_shift_var(secp256k1_scalar *r,
secp256k1_scalar_cadd_bit(r, 0, (l[(shift - 1) >> 5] >> ((shift - 1) & 0x1f)) & 1);
}

#define ROTL32(x,n) ((x) << (n) | (x) >> (32-(n)))

This comment has been minimized.

Copy link
@jb55
@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

The suggestion by @real-or-random above is correct. Removing the LE32 macro from the integer assignments makes the tests on my big endian machine pass. I'll add a commit to fix that later.

@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

Pushed a commit that removes the LE32 macro when doing the final chacha20 assignments. This didn't appear in the little endian tests because LE32 is a noop there. I only ran the tests with 32 bit scalars on the big endian machine, because using 64 bit scalars is prohibited by __int128 support not being available.

@hegjon

This comment has been minimized.

Copy link

commented May 28, 2019

@hegjon

This comment has been minimized.

Copy link

commented May 28, 2019

It builds+all tests passes on all arches for Fedora with the latest changes.

I also added these configure flags:

--enable-experimental --enable-module-schnorrsig

are they required?

@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

@hegjon Nice, thank you!

are they required?

yes. Will add to OP.

@jonasnick jonasnick force-pushed the jonasnick:schnorrsig branch 2 times, most recently from 8daac6e to 9d86983 May 30, 2019

@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

rebased

@jonasnick jonasnick force-pushed the jonasnick:schnorrsig branch from 9d86983 to a81efa5 May 30, 2019

int *nonce_is_negated,
const unsigned char *msg32,
const unsigned char *seckey,
secp256k1_nonce_function noncefp,

This comment has been minimized.

Copy link
@elichai

elichai Jun 13, 2019

Using @sipa logic from above

As the Schnorr signature BIP defines the actual byte representation of signatures, there is no distinction between "invalid because incorrect values" and "invalid because incorrect serialization"

Shouldn't the deterministic derivation that's described in BIP Schnorr be part of the function itself?
(I guess if you see someone using this directly for Musig or something like that he will want to provide a different source of randomness)

This comment has been minimized.

Copy link
@jonasnick

jonasnick Jun 17, 2019

Author Contributor

This is an optional argument in the same way as in ecdsa_sign. In almost all the cases users will provide NULL. I don't know a specific case where you would want to provide your own nonce function (outside of testing).

* Returns 1 on success, 0 on failure.
* Args: ctx: pointer to a context object, initialized for signing (cannot be NULL)
* Out: sig: pointer to the returned signature (cannot be NULL)
* nonce_is_negated: a pointer to an integer indicates if signing algorithm negated the

This comment has been minimized.

Copy link
@elichai

elichai Jun 14, 2019

Could you expand on why would a user of the library want to know if the generated nonce was negated or not?

This comment has been minimized.

Copy link
@jonasnick

jonasnick Jun 17, 2019

Author Contributor

The caller needs to know that if they want to create sign-to-contract commitments because this information is necessary to batch verify these commitments. PR #589 changes this argument to return nonce_is_negated as part of the full opening which is better.

#define LE32(p) (p)
#endif

static void secp256k1_scalar_chacha20(secp256k1_scalar *r1, secp256k1_scalar *r2, const unsigned char *seed, uint64_t idx) {

This comment has been minimized.

Copy link
@elichai

elichai Jun 14, 2019

A thought about multiple implementations of the same cryptography.
is this library becoming a general cryptography library?
should there be a different general crypto library with sha2/chacha20 etc.?

Because right now both libsecp and bitcoin implements SHA256 and HMAC, and after this they'll also both implement Chacha20 (https://github.com/bitcoin/bitcoin/blob/master/src/crypto/chacha20.cpp)

Any ideas on the matter? (I myself don't like duplicate implementations especially of sensitive cryptography(and there's also a performance difference too))

This comment has been minimized.

Copy link
@sipa

sipa Jun 14, 2019

Contributor

libsecp256k1 is absolutely not a generic low-level cryptographc primitives library; it's a library implementing high-level cryptographic algorithms efficiently with a safe-to-use interface.

As far as duplicate implementations goes, these are included for necessity of exposing fully-functional versions of the high-level functions it implements. I personally don't think code duplication for such low-level primitives is an issue, as this code doesn't "rot" (it won't go out of date ever, at worst very rarely needs to be extended, and possibly doesn't need to be touched ever, at all), so it isn't subject to the same priorities most software engineering places.

That said, there are some practical reasons why having a built-in implementation may be undesirable:

  • In very low-memory systems which already have versions of SHA256/... somewhere, the additional code size may be undesirable.
  • The SHA256/Chacha20 code inside libsecp256k1 isn't very optimized, and probably never will gain high levels of architecture-specific optimization. Generally in these protocols, the performance of these is not very important, but if it is, it'd be great to have libsecp256k1 use more efficient versions.

Because of that reason, I think we should see the internal primitives as default, naive, private implementations, and if there is ever a reason to use another one, we should add functionality to make libsecp256k1 use external versions (rather than exposing/improving the internal ones).

This comment has been minimized.

Copy link
@elichai

elichai Jun 14, 2019

Yeah, I didn't consider the first option a real one, just a possibility about how will the library look with more primitives being added.

I guess your answer is fair, optimized sha256 might already be a nice speedup in a rapid verification scenario(which is already considered in the whole batch verification).

Maybe making a general low level library that is shared between libsecp and bitcoin is an option? Otherwise your suggestion that if someone really wanted then it's possible to add a feature to support these functions as extern.

if (nonce_is_negated != NULL) {
*nonce_is_negated = 0;
}
if (!secp256k1_fe_is_quad_var(&r.y)) {

This comment has been minimized.

Copy link
@elichai

elichai Jun 15, 2019

Is this constant time?
As far as I could tell this uses https://github.com/bitcoin-core/secp256k1/blob/master/src/num_gmp_impl.h#L147

Which uses GMP.
Am I missing something?

This comment has been minimized.

Copy link
@sipa

sipa Jun 15, 2019

Contributor

It isn't (all functions ending with _var are variable time).

This isn't a problem, as R (including its original Y coordinate) is not secret.

This comment has been minimized.

Copy link
@elichai

elichai Jun 15, 2019

Oh, ok. I assumed the whole "sign" function is constant time.
Constant time, constant memory access signing and pubkey generation.
Thanks!

This comment has been minimized.

Copy link
@sipa

sipa Jun 15, 2019

Contributor

The whole "constant time" naming is confusing really, as we can't actually guarantee actual constant time (e.g. the div x86 instruction is variable time!).

Really what we're aiming for is sidechannel resistance, and one of the criteria is that no execution branches are memory access locations are dependent on secret data.

@real-or-random

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

Can you add a line here?

echo " module recovery = $enable_module_recovery"

@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

done

@jonasnick jonasnick force-pushed the jonasnick:schnorrsig branch from 1f5ca39 to 4a5b0a6 Jul 4, 2019

@jonasnick jonasnick force-pushed the jonasnick:schnorrsig branch from 4a5b0a6 to a228e2f Jul 4, 2019

@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

Removed nonce_is_negated out argument from schnorrsig_sign because it was only required for a follow-up sign-to-contract PR that I closed in favor of a different sign-to-contract PR which does not use a nonce_is_negated argument but instead adds s2c_opening and s2c_data arguments (PR #589).

Also squashed in order to allow to cleanly build the sign-to-contract PR on top.

secp256k1_ecmult_gen(&ctx->ecmult_gen_ctx, &pkj, &x);
secp256k1_ge_set_gej(&pk, &pkj);

if (!noncefp(buf, msg32, seckey, NULL, (void*)ndata, 0)) {

This comment has been minimized.

Copy link
@afk11

afk11 Jul 8, 2019

Contributor

Per the documentation for secp256k1_nonce_function the algo16 parameter should be written to and non null for schemes besides ECDSA - should the NULL be replaced with an algorithm identifier?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.