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

Merged

Conversation

jonasnick
Copy link
Contributor

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
Copy link
Collaborator

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

Concept ACK.

Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we ARG_CHECK this violation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

src/modules/musig/tests_impl.h Outdated Show resolved Hide resolved
@real-or-random
Copy link
Collaborator

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`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (BlockstreamResearch/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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@apoelstra
Copy link
Contributor

apoelstra 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
Copy link
Contributor Author

rebased

remove the set_msg function and require the message in get_public_nonce
at the latest.
@jonasnick
Copy link
Contributor Author

ping @apoelstra

@real-or-random
Copy link
Collaborator

ACK 6a57be0

@apoelstra
Copy link
Contributor

ACK 6a57be0

@apoelstra apoelstra merged commit cad7cc8 into BlockstreamResearch:secp256k1-zkp Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants