From fa5e7feda66f2b64d8d2976f9110456a1f86989b Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Fri, 24 Jun 2022 22:55:25 +0200 Subject: [PATCH] fix(crypto): Fix nonce bias in CoSi signing. --- .../modtrezorcrypto/modtrezorcrypto-ed25519.h | 36 +++++++++++++++---- core/mocks/generated/trezorcrypto/ed25519.pyi | 7 ++++ .../test_trezor.crypto.curve.ed25519_cosi.py | 3 +- crypto/ed25519-donna/ed25519.c | 33 +++++++++++++---- crypto/ed25519-donna/ed25519.h | 3 +- crypto/tests/test_check.c | 7 ++-- .../firmware/.changelog.d/noissue.security.2 | 1 + legacy/firmware/fsm_msg_crypto.h | 17 +++++---- 8 files changed, 82 insertions(+), 25 deletions(-) create mode 100644 legacy/firmware/.changelog.d/noissue.security.2 diff --git a/core/embed/extmod/modtrezorcrypto/modtrezorcrypto-ed25519.h b/core/embed/extmod/modtrezorcrypto/modtrezorcrypto-ed25519.h index 3aac1d9b3..17fea3cbc 100644 --- a/core/embed/extmod/modtrezorcrypto/modtrezorcrypto-ed25519.h +++ b/core/embed/extmod/modtrezorcrypto/modtrezorcrypto-ed25519.h @@ -240,6 +240,25 @@ STATIC MP_DEFINE_CONST_FUN_OBJ_2( mod_trezorcrypto_ed25519_cosi_combine_signatures_obj, mod_trezorcrypto_ed25519_cosi_combine_signatures); +/// def cosi_commit() -> tuple[bytes, bytes]: +/// """ +/// Generate a nonce and commitment for the CoSi cosigning scheme. +/// """ +STATIC mp_obj_t mod_trezorcrypto_ed25519_cosi_commit() { + vstr_t nonce = {0}; + vstr_t commitment = {0}; + vstr_init_len(&nonce, 32); + vstr_init_len(&commitment, 32); + ed25519_cosi_commit(*(ed25519_secret_key *)nonce.buf, + *(ed25519_public_key *)commitment.buf); + mp_obj_tuple_t *tuple = MP_OBJ_TO_PTR(mp_obj_new_tuple(2, NULL)); + tuple->items[0] = mp_obj_new_str_from_vstr(&mp_type_bytes, &nonce); + tuple->items[1] = mp_obj_new_str_from_vstr(&mp_type_bytes, &commitment); + return MP_OBJ_FROM_PTR(tuple); +} +STATIC MP_DEFINE_CONST_FUN_OBJ_0(mod_trezorcrypto_ed25519_cosi_commit_obj, + mod_trezorcrypto_ed25519_cosi_commit); + /// def cosi_sign( /// secret_key: bytes, /// message: bytes, @@ -272,12 +291,15 @@ STATIC mp_obj_t mod_trezorcrypto_ed25519_cosi_sign(size_t n_args, } vstr_t sig = {0}; vstr_init_len(&sig, sizeof(ed25519_cosi_signature)); - ; - ed25519_cosi_sign(msg.buf, msg.len, *(const ed25519_secret_key *)sk.buf, - *(const ed25519_secret_key *)nonce.buf, - *(const ed25519_public_key *)sigR.buf, - *(const ed25519_secret_key *)pk.buf, - *(ed25519_cosi_signature *)sig.buf); + if (0 != ed25519_cosi_sign(msg.buf, msg.len, + *(const ed25519_secret_key *)sk.buf, + *(const ed25519_secret_key *)nonce.buf, + *(const ed25519_public_key *)sigR.buf, + *(const ed25519_secret_key *)pk.buf, + *(ed25519_cosi_signature *)sig.buf)) { + vstr_clear(&sig); + mp_raise_ValueError("Signing failed"); + } return mp_obj_new_str_from_vstr(&mp_type_bytes, &sig); } STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN( @@ -301,6 +323,8 @@ STATIC const mp_rom_map_elem_t mod_trezorcrypto_ed25519_globals_table[] = { MP_ROM_PTR(&mod_trezorcrypto_ed25519_cosi_combine_publickeys_obj)}, {MP_ROM_QSTR(MP_QSTR_cosi_combine_signatures), MP_ROM_PTR(&mod_trezorcrypto_ed25519_cosi_combine_signatures_obj)}, + {MP_ROM_QSTR(MP_QSTR_cosi_commit), + MP_ROM_PTR(&mod_trezorcrypto_ed25519_cosi_commit_obj)}, {MP_ROM_QSTR(MP_QSTR_cosi_sign), MP_ROM_PTR(&mod_trezorcrypto_ed25519_cosi_sign_obj)}, }; diff --git a/core/mocks/generated/trezorcrypto/ed25519.pyi b/core/mocks/generated/trezorcrypto/ed25519.pyi index 54d527a1e..451b34682 100644 --- a/core/mocks/generated/trezorcrypto/ed25519.pyi +++ b/core/mocks/generated/trezorcrypto/ed25519.pyi @@ -53,6 +53,13 @@ def cosi_combine_signatures(R: bytes, signatures: list[bytes]) -> bytes: """ +# extmod/modtrezorcrypto/modtrezorcrypto-ed25519.h +def cosi_commit() -> tuple[bytes, bytes]: + """ + Generate a nonce and commitment for the CoSi cosigning scheme. + """ + + # extmod/modtrezorcrypto/modtrezorcrypto-ed25519.h def cosi_sign( secret_key: bytes, diff --git a/core/tests/test_trezor.crypto.curve.ed25519_cosi.py b/core/tests/test_trezor.crypto.curve.ed25519_cosi.py index 7a398241e..92d65869b 100644 --- a/core/tests/test_trezor.crypto.curve.ed25519_cosi.py +++ b/core/tests/test_trezor.crypto.curve.ed25519_cosi.py @@ -26,8 +26,7 @@ class TestCryptoEd25519Cosi(unittest.TestCase): nonces = [None] * N Rs = [None] * N for j in range(N): - nonces[j] = ed25519.generate_secret() - Rs[j] = ed25519.publickey(nonces[j]) + nonces[j], Rs[j] = ed25519.cosi_commit() R = ed25519.cosi_combine_publickeys(Rs) diff --git a/crypto/ed25519-donna/ed25519.c b/crypto/ed25519-donna/ed25519.c index e25407a2a..6b7413421 100644 --- a/crypto/ed25519-donna/ed25519.c +++ b/crypto/ed25519-donna/ed25519.c @@ -18,6 +18,7 @@ #include "ed25519.h" #include "ed25519-hash-custom.h" +#include "rand.h" #include "memzero.h" /* @@ -50,16 +51,34 @@ ED25519_FN(ed25519_publickey) (const ed25519_secret_key sk, ed25519_public_key p } void +ED25519_FN(ed25519_cosi_commit) (ed25519_secret_key nonce, ed25519_public_key commitment) { + bignum256modm r = {0}; + ge25519 ALIGN(16) R; + unsigned char extnonce[64] = {0}; + + /* r = random512 mod L */ + random_buffer(extnonce, sizeof(extnonce)); + expand256_modm(r, extnonce, sizeof(extnonce)); + memzero(&extnonce, sizeof(extnonce)); + contract256_modm(nonce, r); + + /* R = rB */ + ge25519_scalarmult_base_niels(&R, ge25519_niels_base_multiples, r); + memzero(&r, sizeof(r)); + ge25519_pack(commitment, &R); +} + +int ED25519_FN(ed25519_cosi_sign) (const unsigned char *m, size_t mlen, const ed25519_secret_key sk, const ed25519_secret_key nonce, const ed25519_public_key R, const ed25519_public_key pk, ed25519_cosi_signature sig) { bignum256modm r = {0}, S = {0}, a = {0}; - hash_512bits extsk = {0}, extnonce = {0}, hram = {0}; + hash_512bits extsk = {0}, hram = {0}; ed25519_extsk(extsk, sk); - ed25519_extsk(extnonce, nonce); - /* r = nonce */ - expand256_modm(r, extnonce, 32); - memzero(&extnonce, sizeof(extnonce)); + /* r */ + expand_raw256_modm(r, nonce); + if (!is_reduced256_modm(r)) + return -1; /* S = H(R,A,m).. */ ed25519_hram(hram, R, pk, m, mlen); @@ -77,6 +96,8 @@ ED25519_FN(ed25519_cosi_sign) (const unsigned char *m, size_t mlen, const ed2551 /* S = (r + H(R,A,m)a) mod L */ contract256_modm(sig, S); + + return 0; } void @@ -155,7 +176,7 @@ ED25519_FN(ed25519_sign_open) (const unsigned char *m, size_t mlen, const ed2551 /* S */ expand_raw256_modm(S, RS + 32); if (!is_reduced256_modm(S)) - return -1; + return -1; /* SB - H(R,A,m)A */ ge25519_double_scalarmult_vartime(&R, &A, hram, S); diff --git a/crypto/ed25519-donna/ed25519.h b/crypto/ed25519-donna/ed25519.h index f01957561..b5fe78950 100644 --- a/crypto/ed25519-donna/ed25519.h +++ b/crypto/ed25519-donna/ed25519.h @@ -35,7 +35,8 @@ void curve25519_scalarmult_basepoint(curve25519_key mypublic, const curve25519_k int ed25519_cosi_combine_publickeys(ed25519_public_key res, CONST ed25519_public_key *pks, size_t n); void ed25519_cosi_combine_signatures(ed25519_signature res, const ed25519_public_key R, CONST ed25519_cosi_signature *sigs, size_t n); -void ed25519_cosi_sign(const unsigned char *m, size_t mlen, const ed25519_secret_key key, const ed25519_secret_key nonce, const ed25519_public_key R, const ed25519_public_key pk, ed25519_cosi_signature sig); +void ed25519_cosi_commit(ed25519_secret_key nonce, ed25519_public_key commitment); +int ed25519_cosi_sign(const unsigned char *m, size_t mlen, const ed25519_secret_key key, const ed25519_secret_key nonce, const ed25519_public_key R, const ed25519_public_key pk, ed25519_cosi_signature sig); #if defined(__cplusplus) } diff --git a/crypto/tests/test_check.c b/crypto/tests/test_check.c index 562f06656..2a4068e6f 100644 --- a/crypto/tests/test_check.c +++ b/crypto/tests/test_check.c @@ -6926,8 +6926,7 @@ START_TEST(test_ed25519_cosi) { /* phase 1: create nonces, commitments (R values) and combine commitments */ for (int j = 0; j < N; j++) { - generate_rfc6979(nonces[j], &rng); - ed25519_publickey(nonces[j], Rs[j]); + ed25519_cosi_commit(nonces[j], Rs[j]); } res = ed25519_cosi_combine_publickeys(R, Rs, N); ck_assert_int_eq(res, 0); @@ -6935,7 +6934,9 @@ START_TEST(test_ed25519_cosi) { MARK_SECRET_DATA(keys, sizeof(keys)); /* phase 2: sign and combine signatures */ for (int j = 0; j < N; j++) { - ed25519_cosi_sign(msg, sizeof(msg), keys[j], nonces[j], R, pk, sigs[j]); + res = ed25519_cosi_sign(msg, sizeof(msg), keys[j], nonces[j], R, pk, + sigs[j]); + ck_assert_int_eq(res, 0); } UNMARK_SECRET_DATA(sigs, sizeof(sigs)); diff --git a/legacy/firmware/.changelog.d/noissue.security.2 b/legacy/firmware/.changelog.d/noissue.security.2 new file mode 100644 index 000000000..625584fe6 --- /dev/null +++ b/legacy/firmware/.changelog.d/noissue.security.2 @@ -0,0 +1 @@ +Fix nonce bias in CoSi signing. diff --git a/legacy/firmware/fsm_msg_crypto.h b/legacy/firmware/fsm_msg_crypto.h index a790bc7c7..1cdd6d541 100644 --- a/legacy/firmware/fsm_msg_crypto.h +++ b/legacy/firmware/fsm_msg_crypto.h @@ -18,6 +18,7 @@ */ static uint8_t cosi_nonce[32] = {0}; +static uint8_t cosi_commitment[32] = {0}; static bool cosi_nonce_is_set = false; void fsm_msgCipherKeyValue(const CipherKeyValue *msg) { @@ -271,7 +272,7 @@ void fsm_msgCosiCommit(const CosiCommit *msg) { if (!node) return; if (!cosi_nonce_is_set) { - random_buffer(cosi_nonce, sizeof(cosi_nonce)); + ed25519_cosi_commit(cosi_nonce, cosi_commitment); cosi_nonce_is_set = true; } @@ -280,7 +281,7 @@ void fsm_msgCosiCommit(const CosiCommit *msg) { resp->commitment.size = 32; resp->pubkey.size = 32; - ed25519_publickey(cosi_nonce, resp->commitment.bytes); + memcpy(resp->commitment.bytes, cosi_commitment, sizeof(cosi_commitment)); ed25519_publickey(node->private_key, resp->pubkey.bytes); msg_write(MessageType_MessageType_CosiCommitment, resp); @@ -326,11 +327,13 @@ void fsm_msgCosiSign(const CosiSign *msg) { resp->signature.size = 32; cosi_nonce_is_set = false; - ed25519_cosi_sign(msg->data.bytes, msg->data.size, node->private_key, - cosi_nonce, msg->global_commitment.bytes, - msg->global_pubkey.bytes, resp->signature.bytes); + if (ed25519_cosi_sign(msg->data.bytes, msg->data.size, node->private_key, + cosi_nonce, msg->global_commitment.bytes, + msg->global_pubkey.bytes, resp->signature.bytes) == 0) { + msg_write(MessageType_MessageType_CosiSignature, resp); + } else { + fsm_sendFailure(FailureType_Failure_FirmwareError, NULL); + } memzero(cosi_nonce, sizeof(cosi_nonce)); - - msg_write(MessageType_MessageType_CosiSignature, resp); layoutHome(); }