Add Schnorr signature

[kurotych]

No description provided.

[prusnak]

Where do these test vectors come from?

If these are your own, can you please also add some that are not your own? Simple google search for "schnorr test vectors" returns some results such as:

• https://raw.githubusercontent.com/sipa/bips/bip-schnorr/bip-schnorr/test-vectors.csv
• https://gist.github.com/kallewoof/5d623445802a84f17cc7ff5572109074

[kurotych]

I got test vectors from results this function.

[prusnak]

Squashed and merged in 8114e0f

Thanks!

[prusnak]

Hey @armatusmiles

I have reverted the commit and removed the Schnorr implementation from master. @onvej-sl found out the Schnorr implementation you provided is insecure and he will tell you more.

[kurotych]

@prusnak Hello. Thank you for feedback. I’m sorry.

[hewigovens]

Hi @prusnak @onvej-sl

could you please share more details about the implementation? I believe @armatusmiles implemented Zilliqa scheme (https://github.com/Zilliqa/Zilliqa/blob/master/src/libCrypto/Schnorr.cpp), which is audited by NCC group (Zilliqa team tells us)

Thanks

[onvej-sl]

Hi @hewigovens,

I found the implementation insecure for the following reasons:

1. In schnorr_verify, you should check that both r and s parts of the signature are less than the curve order. That's how Schnorr signature is usually implemented, Zilliqa not being an exception.
   By the way, scalar_multiply assumes that exponent is less than the order, so in a special case verification failed with an assertion error.
2. You represent a signature by two bignum256 numbers. Since every number has several bignum256 representation (which may be partly reduced, reduced or normalised), great care must be taken when dealing with it. The raw format (an output of bn_write_be) would be better.
   Assume bignum256 r is the r part of a signature on secp256k1 curve with r.val = {0x10364142, 0x3f497a33, 0x348a03bb, 0x2bb739ab, 0x3ffffeba, 0x3fffffff, 0x3fffffff, 0xffffffff, 0x0000fffc}. Such r pass through the condition if (bn_is_less(&curve->order, &sign->r)) return 4;, however it shouldn't (it's equal to the curve order plus one).
3. Your nonce is user defined. Since a nonce reuse reveals the private key, it should be generated randomly or deterministically (for example as defined in RFC 6979) right in ecdsa_sign.

[hewigovens]

@onvej-sl Thanks for your elaborations, for 3 we did use generate_k_rfc6979 to generate the nonce in a wrapper method, it's just not ready to push to upstream. we'll address 1 and 2 shortly

[ansnunez]

Hi @onvej-sl for 1 I think the checks are already there? The 4 lines starting from https://github.com/TrustWallet/wallet-core/blob/1d67cd877ae495a603715a2b649c1271cbc8ff83/trezor-crypto/src/schnorr.c#L116

In Zilliqa, though, we also check for r and s being negative: https://github.com/Zilliqa/Zilliqa/blob/4ed7a542efc98ef00ae38688f66833492786536e/src/libCrypto/Schnorr.cpp#L295

[ansnunez]

@onvej-sl for 2, I think calc_r is missing a call to bn_nnmod to avoid the problem in your example.

Here is the equivalent code in Zilliqa:
https://github.com/Zilliqa/Zilliqa/blob/4ed7a542efc98ef00ae38688f66833492786536e/src/libCrypto/Schnorr.cpp#L158

If we put this code in place, would it then be OK to keep the two bignum256 representation?

[onvej-sl]

Hi @onvej-sl for 1 I think the checks are already there? The 4 lines starting from https://github.com/TrustWallet/wallet-core/blob/1d67cd877ae495a603715a2b649c1271cbc8ff83/trezor-crypto/src/schnorr.c#L116

They are not. What if r or s is equal to the order?

@onvej-sl for 2, I think calc_r is missing a call to bn_nnmod to avoid the problem in your example.

It won't solve the problem. The problem is that the r I provided is not normalized. Please see bignum.c comments for normalized, reduced and partly reduced numbers.

If we put this code in place, would it then be OK to keep the two bignum256 representation?

What is the reason you prefer bignum256 over raw format? I think I've proven it not easy to work with bignum256.

[hewigovens]

@onvej-sl could you please review this pr? @ansnunez tries to fix all the concerns.

Thanks!

[onvej-sl]

@onvej-sl could you please review this pr? @ansnunez tries to fix all the concerns.

Good job! It seems to be OK. Are you going to push it to trezor-firmware?

[hewigovens]

Thanks, we will push it to upstream later