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

Require message in musig protocol in an earlier state #73

Open
wants to merge 2 commits into
base: secp256k1-zkp
from

Conversation

Projects
None yet
3 participants
@jonasnick
Copy link
Contributor

commented Jun 21, 2019

This PR removes the set_msg function from the musig module and adds a msg argument get_public_nonce which must be non-null if the message has not been set during initialization.

The reason for this is that I think that our current set_msg logic is vulnerable to Wagner's attack. The main problem is that by grinding the message an attacker has control over the final hash being signed.

Assume the combined pubkey of Alice and the attacker is P.
The attacker wants to forge a signature on message m' and opens 3 parallel sessions with Alice.
They exchange nonce commitments and nonces.
Let's call the combined nonces Ri for sessions i = 0,1,2.
Now the attacker draws ka and sets R' = R0 + R1 + R2 + ka*G and runs Wagner's algorithm grinding mi to find

H(P, R',m') = H(P, R1, m1) + H(P, R2, m2) + H(P, R3, m3)

with about 2^100 work (with more parallel sessions requires much less work).

Then the attacker and Alice set the message in session i to mi and Alice creates partial signatures si = ki + H(P, Ri, mi).
Now (R', s' = s0 + s1 + s2) is a valid partial signature under Alice's public key A as well:

s'*G = (k0 + k1 + k2)*G + (H(P, R1, m1) + H(P, R2, m2) + H(P, R3, m3))*A
     = R' - ka*G + H(P, R', m')*A

This signature is completed by the attacker adding the partial signature using nonce ka which results in the forgery.

This PR does keeps allows to set the message after nonce commitments have been exchanged, unfortunately the possibility of pre-sharing nonces while not knowing the messages is removed.

@real-or-random

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

Oh nice observation. That the message is fixed upfront is apparently not an artefact of the formal model either...

Concept ACK.

@real-or-random
Copy link
Contributor

left a comment

Looks good except the nits. I haven't tested it yet.

@@ -225,7 +226,7 @@ SECP256K1_API int secp256k1_musig_session_initialize_verifier(
const unsigned char *pk_hash32,
const unsigned char *const *commitments,
size_t n_signers

This comment has been minimized.

Copy link
@real-or-random

real-or-random Jun 24, 2019

Contributor

(not touched by this PR:) maybe we can use ARG_CHECK also for the restrictions on n_signers

CHECK(secp256k1_musig_session_get_public_nonce(ctx, &session, signers, &nonce, ncs, 2) == 1);

/* Trying to get the nonce without providing a message fails. */
CHECK(secp256k1_musig_session_get_public_nonce(ctx, &session, signers, &nonce, ncs, 2, NULL) == 0);

This comment has been minimized.

Copy link
@real-or-random

real-or-random Jun 24, 2019

Contributor

Should we ARG_CHECK this violation?

This comment has been minimized.

Copy link
@jonasnick

jonasnick Jun 25, 2019

Author Contributor

If the rationale for ARG_CHECKing is still "everything that the rust typesystem would have caught" then I guess no.

This comment has been minimized.

Copy link
@apoelstra

apoelstra Jul 3, 2019

Member

We could prevent this with session types, i.e. have a Session<NonceUnset> and Session<NonceSet>

This comment has been minimized.

Copy link
@jonasnick

jonasnick Jul 5, 2019

Author Contributor

I considered this but thought that this is not exactly the rust type system as you could do something similar with C. But actually this is what we'll be doing in rust. Fixing.

This comment has been minimized.

Copy link
@jonasnick

jonasnick Jul 5, 2019

Author Contributor

Fixed

Show resolved Hide resolved src/modules/musig/tests_impl.h Outdated

@jonasnick jonasnick force-pushed the jonasnick:fix-musig-message branch from e940896 to f215a4c Jun 25, 2019

@real-or-random

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

ACK read the code, but did not test it

@@ -149,7 +149,7 @@ SECP256K1_API int secp256k1_musig_pubkey_combine(
* msg32: the 32-byte message to be signed. Shouldn't be NULL unless you
* require sharing public nonces before the message is known
* because it reduces nonce misuse resistance. If NULL, must be
* set with `musig_session_set_msg` before signing and verifying.
* set with `musig_session_get_public_nonce`.

This comment has been minimized.

Copy link
@apoelstra

apoelstra Jul 3, 2019

Member

We should probably cut this comment down even further to get rid of the "unless you require" clause.

This comment has been minimized.

Copy link
@jonasnick

jonasnick Jul 5, 2019

Author Contributor

Hm, not sure what exactly you're suggesting. Something like that?

 *              msg32: the 32-byte message to be signed. Shouldn't be NULL. If NULL, must be
 *                           set with `musig_session_get_public_nonce`.

Seems to be less clear. Also I assume sharing nonce commitments won't be a very exotic thing. Lightning will likely use it to avoid roundtrips. I have an open PR in the scriptless scripts repo (ElementsProject/scriptless-scripts#6) that explains how to do this because I was contacted by someone concerned that MuSig will end up being very expensive in communication costs.

This comment has been minimized.

Copy link
@apoelstra

apoelstra Jul 5, 2019

Member

I think if you change "require sharing public nonces" to "require sharing nonce commitments" I'll be happy

This comment has been minimized.

Copy link
@jonasnick

jonasnick Jul 5, 2019

Author Contributor

fixed

/* Providing a message should make get_public_nonce succeed. */
CHECK(secp256k1_musig_session_get_public_nonce(ctx, &session, signers, &nonce, ncs, 2, msg) == 1);
/* Trying to set the message again fails. */
CHECK(secp256k1_musig_session_get_public_nonce(ctx, &session, signers, &nonce, ncs, 2, msg) == 0);

This comment has been minimized.

Copy link
@apoelstra

apoelstra Jul 3, 2019

Member

I don't have a strong opinion but I feel like this should also be an ARG_CHECK (and that rust-secp should prevent it statically)

This comment has been minimized.

Copy link
@jonasnick

jonasnick Jul 5, 2019

Author Contributor

By using something like a state machine/session types? Yes, fixing.

This comment has been minimized.

Copy link
@jonasnick

jonasnick Jul 5, 2019

Author Contributor

fixed

@apoelstra

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

I see a bunch of stuff when I run valgrind ./tests 4 with this code.

Edit: oh for some reason I'm compiling with openssl, that's probably all it is. (Which means some broken Arch package must have depended on openssl recently, because it definitely was not installed before..)

@jonasnick jonasnick force-pushed the jonasnick:fix-musig-message branch from f215a4c to b934674 Jul 5, 2019

@jonasnick jonasnick force-pushed the jonasnick:fix-musig-message branch from b934674 to 6a57be0 Jul 9, 2019

@jonasnick

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

rebased

Require message in musig protocol in an earlier state. In particular,
remove the set_msg function and require the message in get_public_nonce
at the latest.
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.