Changeset View
Standalone View
src/secp256k1/src/modules/schnorr/schnorr_impl.h
Show First 20 Lines • Show All 126 Lines • ▼ Show 20 Lines | ) { | ||||
secp256k1_sha256_finalize(&sha, buf); | secp256k1_sha256_finalize(&sha, buf); | ||||
secp256k1_scalar_set_b32(e, buf, &overflow); | secp256k1_scalar_set_b32(e, buf, &overflow); | ||||
return !overflow & !secp256k1_scalar_is_zero(e); | return !overflow & !secp256k1_scalar_is_zero(e); | ||||
} | } | ||||
static int secp256k1_schnorr_sig_sign( | static int secp256k1_schnorr_sig_sign( | ||||
const secp256k1_ecmult_gen_context* ctx, | const secp256k1_ecmult_gen_context* ctx, | ||||
unsigned char *sig64, | unsigned char *sig64, | ||||
const unsigned char *msg32, | |||||
const secp256k1_scalar *privkey, | const secp256k1_scalar *privkey, | ||||
secp256k1_ge *pubkey, | secp256k1_ge *pubkey, | ||||
const secp256k1_scalar *nonce, | secp256k1_nonce_function noncefp, | ||||
const unsigned char *msg32 | const void *ndata | ||||
) { | |||||
secp256k1_ge R; | |||||
secp256k1_scalar k; | |||||
int ret; | |||||
if (secp256k1_scalar_is_zero(privkey)) { | |||||
markblundeberg: Same comment as below -- why sanity check the private key but not also sanity-check the pubkey… | |||||
deadalnixAuthorUnsubmitted Done Inline ActionsThat function never use it, so there is really no point doing it here. deadalnix: That function never use it, so there is really no point doing it here. | |||||
return 0; | |||||
} | |||||
if (!secp256k1_schnorr_compute_k_R(ctx, &k, &R, msg32, privkey, noncefp, ndata)) { | |||||
return 0; | |||||
} | |||||
ret = secp256k1_schnorr_compute_sig(sig64, msg32, &k, &R, privkey, pubkey); | |||||
secp256k1_scalar_clear(&k); | |||||
jasonbcoxUnsubmitted Not Done Inline ActionsDoes R need to be cleared here as well, since it's derived from k? If not, why? jasonbcox: Does R need to be cleared here as well, since it's derived from k? If not, why? | |||||
markblundebergUnsubmitted Not Done Inline ActionsR is public so it should be fine. (technically, the jacobi index of Y is one additional bit of private information that is normally erased before publication, however this bit does not have a connection that can lead to compromise of private key since it is uncorrelated with the rest of the nonce) markblundeberg: R is public so it should be fine.
(technically, the jacobi index of Y is one additional bit of… | |||||
markblundebergUnsubmitted Not Done Inline Actionsjacobi symbol i mean markblundeberg: jacobi symbol i mean | |||||
return ret; | |||||
} | |||||
static int secp256k1_schnorr_compute_k_R( | |||||
const secp256k1_ecmult_gen_context* ctx, | |||||
secp256k1_scalar *k, | |||||
secp256k1_ge *R, | |||||
const unsigned char *msg32, | |||||
const secp256k1_scalar *privkey, | |||||
secp256k1_nonce_function noncefp, | |||||
const void *ndata | |||||
) { | ) { | ||||
secp256k1_gej Rj; | secp256k1_gej Rj; | ||||
secp256k1_ge Ra; | |||||
secp256k1_scalar e, s, k; | |||||
if (secp256k1_scalar_is_zero(privkey) || secp256k1_scalar_is_zero(nonce)) { | if (!secp256k1_schnorr_sig_generate_k(k, msg32, privkey, noncefp, ndata)) { | ||||
return 0; | return 0; | ||||
FabienUnsubmitted Not Done Inline ActionsClear k here, to avoid relying on secp256k1_schnorr_sig_generate_k behavior Fabien: Clear k here, to avoid relying on `secp256k1_schnorr_sig_generate_k` behavior | |||||
deadalnixAuthorUnsubmitted Done Inline ActionsNo. This function does not own k nor set k. Making responsabilities unclear is exactly how you end up with codepath that do not clean what they should. deadalnix: No. This function does not own k nor set k. Making responsabilities unclear is exactly how you… | |||||
FabienUnsubmitted Not Done Inline ActionsOK, so it can be cleared in secp256k1_schnorr_sig_sign Fabien: OK, so it can be cleared in `secp256k1_schnorr_sig_sign` | |||||
} | } | ||||
k = *nonce; | |||||
secp256k1_ecmult_gen(ctx, &Rj, &k); | secp256k1_ecmult_gen(ctx, &Rj, k); | ||||
secp256k1_ge_set_gej(&Ra, &Rj); | secp256k1_ge_set_gej(R, &Rj); | ||||
if (!secp256k1_fe_is_quad_var(&Ra.y)) { | return 1; | ||||
} | |||||
static int secp256k1_schnorr_compute_sig( | |||||
unsigned char *sig64, | |||||
const unsigned char *msg32, | |||||
secp256k1_scalar *k, | |||||
secp256k1_ge *R, | |||||
const secp256k1_scalar *privkey, | |||||
secp256k1_ge *pubkey | |||||
) { | |||||
secp256k1_scalar e, s; | |||||
if (secp256k1_scalar_is_zero(privkey) || secp256k1_scalar_is_zero(k)) { | |||||
markblundebergUnsubmitted Not Done Inline ActionsShould this also sanity-check the pubkey point argument (not point at infinity)? Alternatively if sanity checks are not needed, this function can return void. markblundeberg: Should this also sanity-check the `pubkey` point argument (not point at infinity)? | |||||
markblundebergUnsubmitted Not Done Inline ActionsSince this refactor is definitely intended to be used in cases where pubkey unrelated to privkey it's logical to check that pubkey is not point at infinity as that would cause a crash in secp256k1_schnorr_compute_e. markblundeberg: Since this refactor is definitely intended to be used in cases where `pubkey` unrelated to… | |||||
return 0; | |||||
} | |||||
if (!secp256k1_fe_is_quad_var(&R->y)) { | |||||
FabienUnsubmitted Not Done Inline ActionsMoving this part to secp256k1_schnorr_compute_k_R() would make the flow easier to follow. Fabien: Moving this part to `secp256k1_schnorr_compute_k_R()` would make the flow easier to follow. | |||||
deadalnixAuthorUnsubmitted Done Inline ActionsSee @markblundeberg 's comment. deadalnix: See @markblundeberg 's comment. | |||||
/** | /** | ||||
* R's y coordinate is not a quadratic residue, which is not allowed. | * R's y coordinate is not a quadratic residue, which is not allowed. | ||||
* Negate the nonce to ensure it is. | * Negate the nonce to ensure it is. | ||||
*/ | */ | ||||
secp256k1_scalar_negate(&k, &k); | secp256k1_scalar_negate(k, k); | ||||
markblundebergUnsubmitted Not Done Inline ActionsInteresting that this sign flip trick still works with MuSig. If signers produce an aggregate R that needs to have its sign flipped, then all signers can individually notice this, flip their k's, and thereby arrive at the proper result. markblundeberg: Interesting that this sign flip trick still works with MuSig. If signers produce an aggregate R… | |||||
deadalnixAuthorUnsubmitted Done Inline ActionsYes. deadalnix: Yes. | |||||
markblundebergUnsubmitted Not Done Inline ActionsThere was something bothering me about the way the k argument gets mutated here and I realized what it was... basically, if you were to call this function a second time with same variables, it may produce the wrong signature. In principle maybe you ought to flip the sign of R as well so that the caller learns the new corrected R. But meh, as long as it's clear that k is expected to be cleared immediately after and no second call, it doesn't matter. markblundeberg: There was something bothering me about the way the `k` argument gets mutated here and I… | |||||
deadalnixAuthorUnsubmitted Done Inline ActionsThe function is secp256k1_schnorr_compute_sig, not secp256k1_schnorr_compute_R deadalnix: The function is `secp256k1_schnorr_compute_sig`, not `secp256k1_schnorr_compute_R` | |||||
} | } | ||||
secp256k1_fe_normalize(&Ra.x); | secp256k1_fe_normalize(&R->x); | ||||
secp256k1_fe_get_b32(sig64, &Ra.x); | secp256k1_fe_get_b32(sig64, &R->x); | ||||
secp256k1_schnorr_compute_e(&e, sig64, pubkey, msg32); | secp256k1_schnorr_compute_e(&e, sig64, pubkey, msg32); | ||||
secp256k1_scalar_mul(&s, &e, privkey); | secp256k1_scalar_mul(&s, &e, privkey); | ||||
secp256k1_scalar_add(&s, &s, &k); | secp256k1_scalar_add(&s, &s, k); | ||||
secp256k1_scalar_clear(&k); | |||||
secp256k1_scalar_get_b32(sig64 + 32, &s); | secp256k1_scalar_get_b32(sig64 + 32, &s); | ||||
return 1; | return 1; | ||||
} | } | ||||
static int secp256k1_schnorr_sig_generate_k( | static int secp256k1_schnorr_sig_generate_k( | ||||
secp256k1_scalar *k, | secp256k1_scalar *k, | ||||
const unsigned char *msg32, | const unsigned char *msg32, | ||||
const unsigned char *seckey, | const secp256k1_scalar *privkey, | ||||
secp256k1_nonce_function noncefp, | secp256k1_nonce_function noncefp, | ||||
const void *ndata | const void *ndata | ||||
) { | ) { | ||||
int overflow = 0; | int overflow = 0; | ||||
int ret = 0; | int ret = 0; | ||||
unsigned int count = 0; | unsigned int count = 0; | ||||
unsigned char nonce32[32]; | unsigned char nonce32[32], seckey[32]; | ||||
/* Seed used to make sure we generate different values of k for schnorr */ | /* Seed used to make sure we generate different values of k for schnorr */ | ||||
const unsigned char secp256k1_schnorr_algo16[17] = "Schnorr+SHA256 "; | const unsigned char secp256k1_schnorr_algo16[17] = "Schnorr+SHA256 "; | ||||
if (noncefp == NULL) { | if (noncefp == NULL) { | ||||
noncefp = secp256k1_nonce_function_default; | noncefp = secp256k1_nonce_function_default; | ||||
} | } | ||||
secp256k1_scalar_get_b32(seckey, privkey); | |||||
while (1) { | while (1) { | ||||
ret = noncefp(nonce32, msg32, seckey, secp256k1_schnorr_algo16, (void*)ndata, count++); | ret = noncefp(nonce32, msg32, seckey, secp256k1_schnorr_algo16, (void*)ndata, count++); | ||||
if (!ret) { | if (!ret) { | ||||
break; | break; | ||||
} | } | ||||
secp256k1_scalar_set_b32(k, nonce32, &overflow); | secp256k1_scalar_set_b32(k, nonce32, &overflow); | ||||
if (!overflow && !secp256k1_scalar_is_zero(k)) { | if (!overflow && !secp256k1_scalar_is_zero(k)) { | ||||
break; | break; | ||||
} | } | ||||
secp256k1_scalar_clear(k); | |||||
} | } | ||||
memset(seckey, 0, 32); | |||||
memset(nonce32, 0, 32); | memset(nonce32, 0, 32); | ||||
return ret; | return ret; | ||||
} | } | ||||
#endif | #endif |
Same comment as below -- why sanity check the private key but not also sanity-check the pubkey point argument (not point at infinity)?