multisig: force update option for key exchange [master] #8329

UkoeHB · 2022-05-15T01:49:46Z

Motivation

In the default multisig key exchange (account setup) ceremony, every key exchange round requires a message from every signer. That requirement may not be strictly necessary in some real-world use-cases (where it may be safe to assume some or all signers are trustworthy during account setup), so this PR adds an option to 'force-update' key exchange.

Changes

Adds a force_update_use_with_caution flag to the multisig_account::kex_update() method (it defaults to false). That interface change is propagated up the call stack to the exchange_multisig_keys() method in wallet2 and the RPC.
- When force-updating an intermediate key exchange round, you need at least num_signers - current_round_num messages from other signers (this is the bare minimum to actually make progress).
- When force-updating the post-kex verification round, you only need to pass in the local account's post-kex verification message (which was the local account's output from the previous round).
Adds a unit test demoing the new method.
Miscellaneous cleanup related to multisig (gen_multisig.cpp, unit tests, etc.).

Future Work

Currently, simplewallet and the MMS don't have an interface for the force update flag (I just defaulted it to false). A future PR can update the interface there if it's desired (@rbrunner7 ).

Dangers

The new flag is a foot-gun that can enable the following problems.

A signer's account created through force-updating may be unable to participate in signing (i.e. the final multisig address they obtain is malformed if there is a malicious player during account setup).
If a signer force-updates the post-kex verification round, they may believe their account is complete and the multisig group is able to create signatures. In reality, there may not be a threshold of honest signers that completed their accounts. Any funds sent to the multisig address obtained by the force-updater might be lost forever.
If honest multisig signers force-update any intermediate key exchange round, a malicious player could execute an 'address hostage' attack where the final address produced can only make signatures if that malicious player participates (although he wouldn't be able to make signatures on his own).

UkoeHB · 2022-08-16T17:36:18Z

Resolved merge conflicts. Updated wallet2::exchange_multisig_keys() method so if your account has completed the main kex rounds you can call this with NO kex messages to extract the account's final post-kex verification message (a kludge to avoid adding a whole new RPC endpoint). To migrate pre-v0.18 multisig accounts to v0.18, the following two workflows should be sufficient (assuming you have a multisig account that was completed before v0.18).

Workflow 1 (recommended)

Let all multisig group members call exchange_multisig_keys() with NO key exchange messages. Save the output strings as R1, R2, ..., Rn.
Each multisig group member calls exchange_multisig_keys(R1, R2, ..., Rn) again. Their accounts should be complete and usable after that.

Workflow 2 (fastest, less robust in general if there is a malicious group member)

Call exchange_multisig_keys() with NO key exchange messages locally. Save the output string as R_local.
Call exchange_multisig_keys(R_local, force_update_use_with_caution = true) (using the force update flag). The user's account should be complete and usable after that.

UkoeHB · 2022-08-16T22:02:38Z

I removed checks from exchange_multisig_keys() that would throw an error if calling it when your multisig account is already complete. This way you can always call it with no kex messages to extract the post-kex verification message.

If wallets can enter a state where the post-kex verification message can't be extracted, then malicious users could plausibly claim 'I can't get my post-kex verification message'.

vtnerd

Nothing critical, just a few questions (that may or may not require changes).

vtnerd · 2022-08-17T23:17:56Z

src/multisig/multisig_account_kex_impl.cpp

@@ -181,7 +181,8 @@ namespace multisig
  *    Key aggregation via aggregation coefficients prevents key cancellation attacks.
  *    See: https://www.getmonero.org/resources/research-lab/pubs/MRL-0009.pdf
  * param: final_keys - address components (public keys) obtained from other participants (not shared with local)
-  * param: privkeys_inout - private keys of address components known by local; each key will be multiplied by an aggregation coefficient (return by reference)
+  * param: privkeys_inout - private keys of address components known by local; each key will be multiplied by an aggregation


Pinging @moneromooo-monero some whitespace changes ... allow ?

vtnerd · 2022-08-17T23:18:04Z

src/multisig/multisig_account_kex_impl.cpp

@@ -199,7 +200,8 @@ namespace multisig
    for (std::size_t multisig_keys_index{0}; multisig_keys_index < privkeys_inout.size(); ++multisig_keys_index)
    {
      crypto::public_key pubkey;
-      CHECK_AND_ASSERT_THROW_MES(crypto::secret_key_to_public_key(privkeys_inout[multisig_keys_index], pubkey), "Failed to derive public key");
+      CHECK_AND_ASSERT_THROW_MES(crypto::secret_key_to_public_key(privkeys_inout[multisig_keys_index], pubkey),


Here again.

vtnerd · 2022-08-18T00:33:53Z

src/simplewallet/simplewallet.cpp

  return true;
 }

-bool simple_wallet::exchange_multisig_keys_main(const std::vector<std::string> &args, bool called_by_mms) {
-    CHECK_MULTISIG_ENABLED();


Was CHECK_MULTISIG_ENABLED() intentionally removed? Its not used in mms_next.

No, this was a rebase mistake, will fix.

vtnerd · 2022-08-18T00:56:13Z

src/wallet/wallet2.cpp

 {
  bool ready{false};
  CHECK_AND_ASSERT_THROW_MES(multisig(&ready), "The wallet is not multisig");
-  CHECK_AND_ASSERT_THROW_MES(!ready, "Multisig wallet creation process has already been finished");


Why was the !ready check removed? Seems like that should be done, even with the new if statement below.

Existing multisig wallets (which have ready == true) have no way to recover the post-kex verification message, which may be needed by other signers even after the local signer has done their post-kex verification round.

vtnerd · 2022-08-18T00:58:55Z

src/wallet/wallet2.cpp

@@ -5126,8 +5125,18 @@ std::string wallet2::exchange_multisig_keys(const epee::wipeable_string &passwor
      ""
    };

+  // KLUDGE: early return if there are no kex messages and main kex is complete (will return the post-kex verification round
+  //         message) (it's a kludge because this behavior would be more appropriate for a standalone wallet method)
+  if (kex_messages.size() == 0)


Is this just to retrieve the post-kex message multiple times or something? Otherwise I don't see why this was added.

Yes, because existing multisig wallets are stuck, unable to complete the post-kex verification round that was added in v0.18 (an oversight in testing the multisig changes).

They aren't guaranteed to be stuck though, right? Seems like this would only happen if other signers lost someone's post-kex message? If that could happen, couldn't any of the steps get stuck? I guess that's why the force option was added?

The post-kex round did not exist before v0.18, and the only way to get a post-kex message right now is from exchange_multisig_keys() as the output of a kex round (but existing accounts already completed their last kex round without getting a post-kex message).

I guess I am overloading this PR a little so we can fix multisig faster, the post-kex stuff isn't directly related to the force update option. However, you need force updating to get your account past the post-kex step if you aren't able to get post-kex messages from all other group members (which may be the case for old accounts).

vtnerd · 2022-08-18T00:59:58Z

src/wallet/wallet_rpc_server.cpp

@@ -4146,13 +4146,6 @@ namespace tools
      er.message = "This wallet is not multisig";
      return false;
    }
-
-    if (ready)


Why remove this check?

Existing multisig wallets (which have ready == true) have no way to recover the post-kex verification message, which may be needed by other signers even after the local signer has done their post-kex verification round.

vtnerd

Ok, so the hold up has been me confused by the naming scheme with the post-kex messaging. multisig_kex_rounds_required(...) appears to be the number of kex messages, not including the "post-round" message to verify that all participants are active. multisig_is_ready() only returns true after this additional message has been verified.

wallet2::multisig(...) check calls the first function, which was the source of my confusion in my prior comments. I kept transposing that function call to the latter function. It's also confusing, because there's a sense in which the local wallet truly is ready - this particular wallet can immediately sign valid messages - but it is not certain about the status of the remaining wallets. Ouch, kind of a mess, but I think you've done the a decent job correcting all this.

UkoeHB · 2022-09-10T19:04:35Z

Squashed commits

UkoeHB · 2022-09-12T15:52:33Z

@tmoravec do you want to test this PR? It should fix #8541

tmoravec · 2022-09-14T08:25:53Z

@tmoravec do you want to test this PR? It should fix #8541

I'd love to but I'd need to implement the simplewallet parts first. I can do that, but not today or tomorrow unfortunately :( .

selsta · 2022-09-20T16:06:45Z

@tmoravec I would like to include this in the next release, it would be great if you could test this in the next days :)

tmoravec · 2022-09-20T19:40:54Z

Thanks for the reminder, selsta :) .

I've added the new parameter to simplewallet to facilitate testing: UkoeHB#4

Trying to follow the Workflow 2, I'm getting an error: Error: Failed to perform multisig keys exchange: Multisig post-kex round messages from other signers did not all contain two pubkeys.

It looks like the check CHECK_AND_ASSERT_THROW_MES(pubkey_origins_map.size() == 2 in src/multisig/multisig_account_kex_impl.cpp is not taking incomplete_signer_set into account but I might be missing something.

EDIT: Here are the commands (on stagenet):

[wallet 51sehD (no daemon)]: exchange_multisig_keys force-update
Wallet password:
Another step is needed
MultisigxV2Rn1WBRa6XZrJ3A1kmifR2yXjDcbPGmrKfk6fknJiY9p4xf9AXnwJ8Gijm23iV9U3Vj8zHB8haMVVgsaMSkhBtksPzHfn1QuGXja3Hhifv9HSDNfsNQ7icCYh2oX6GAuKBc3uvj3bdGt9jyGyNWxxih4t88sT8XzrqUrU8guMkgXnoWDymd1gCttzgd2toGzVV3XUtPEGTfmWV6ETou95fNboLMuSaF2Ds
Send this multisig info to all other participants, then use exchange_multisig_keys <info1> [<info2>...] with others' multisig info
[wallet 51sehD (no daemon)]: exchange_multisig_keys force-update MultisigxV2Rn1WBRa6XZrJ3A1kmifR2yXjDcbPGmrKfk6fknJiY9p4xf9AXnwJ8Gijm23iV9U3Vj8zHB8haMVVgsaMSkhBtksPzHfn1QuGXja3Hhifv9HSDNfsNQ7icCYh2oX6GAuKBc3uvj3bdGt9jyGyNWxxih4t88sT8XzrqUrU8guMkgXnoWDymd1gCttzgd2toGzVV3XUtPEGTfmWV6ETou95fNboLMuSaF2Ds
Wallet password:
Error: Failed to perform multisig keys exchange: Multisig post-kex round messages from other signers did not all contain two pubkeys.

UkoeHB · 2022-09-20T23:44:53Z

src/wallet/wallet2.cpp

-  // open kex messages
-  std::vector<multisig::multisig_kex_msg> expanded_msgs;
-  expanded_msgs.reserve(kex_messages.size());
-
-  for (const auto &msg : kex_messages)
-    expanded_msgs.emplace_back(msg);
-


Moved this so expanded_msgs is defined right before its use.

UkoeHB · 2022-09-20T23:45:54Z

@tmoravec ok updated to hopefully fix that issue (also added your changes for simplewallet - note that I use the longer force-update-use-with-caution flag because it REALLY needs to be used with caution).

I am wondering if removing PRINT_USAGE(USAGE_EXCHANGE_MULTISIG_KEYS); will cause problems... is there another way to get the usage?

tmoravec · 2022-09-21T06:45:48Z

I am wondering if removing PRINT_USAGE(USAGE_EXCHANGE_MULTISIG_KEYS); will cause problems... is there another way to get the usage?

Now it supports zero arguments and any number of arguments. I can't think of wrong usage that would trigger this generic error. Happy to put it back if we can make up the condition.

FWIW There's always help exchange_multisig_keys. Plus more specific error messages, for example:

[wallet 51sehD (no daemon)]: exchange_multisig_keys test
Wallet password:
Error: Failed to perform multisig keys exchange: basic_string::substr: __pos (which is 13) > this->size() (which is 4)

tmoravec · 2022-09-21T07:04:18Z

ok updated to hopefully fix that issue

I've tested this and it works 👍 . What I did:

Open two parts of a 2/3 multisig wallet created with v0.17.2.0.
Finish the multisig setup with Workflow 2 above using exchange_multisig_keys force-update-use-with-caution ...
Verify that the addresses match the previous address (as seen in v0.17.2.0 software).
Refresh, exchange multisig info, receive transfer, send transfer.

Thank you, @UkoeHB !

…stances

UkoeHB · 2022-09-21T17:38:53Z

squashed commits

This was referenced May 15, 2022

multisig: add key exchange round booster #8203

Merged

Monero Multisig for HF #8237

Closed

UkoeHB force-pushed the multisig_force_update branch from 3eb0000 to b1c33c4 Compare May 15, 2022 13:00

UkoeHB force-pushed the multisig_force_update branch from b1c33c4 to 0f597c1 Compare August 16, 2022 17:30

UkoeHB force-pushed the multisig_force_update branch 2 times, most recently from 51c6f21 to 68f22f5 Compare August 16, 2022 22:01

vtnerd reviewed Aug 18, 2022

View reviewed changes

UkoeHB mentioned this pull request Sep 8, 2022

Multisig wallets are not backwards compatible #8541

Closed

vtnerd approved these changes Sep 9, 2022

View reviewed changes

UkoeHB force-pushed the multisig_force_update branch from 6b2e453 to 125f093 Compare September 10, 2022 19:04

vtnerd approved these changes Sep 11, 2022

View reviewed changes

tmoravec mentioned this pull request Sep 20, 2022

Add force_update_use_with_caution to simplewallet UkoeHB/monero#4

Closed

UkoeHB commented Sep 20, 2022

View reviewed changes

UkoeHB force-pushed the multisig_force_update branch from 99c9e3d to 2b6a716 Compare September 20, 2022 23:50

vtnerd approved these changes Sep 21, 2022

View reviewed changes

add an option to force-update multisig key exchange under some circum…

4b0785f

…stances

UkoeHB force-pushed the multisig_force_update branch from 2b6a716 to 4b0785f Compare September 21, 2022 17:38

UkoeHB mentioned this pull request Sep 21, 2022

multisig: force update option for key exchange [v0.18] #8582

Merged

UkoeHB changed the title ~~multisig: force update option for key exchange~~ multisig: force update option for key exchange [master] Sep 21, 2022

selsta approved these changes Sep 22, 2022

View reviewed changes

luigi1111 merged commit 0a10a4f into monero-project:master Sep 26, 2022

UkoeHB deleted the multisig_force_update branch June 28, 2023 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multisig: force update option for key exchange [master] #8329

multisig: force update option for key exchange [master] #8329

UkoeHB commented May 15, 2022

UkoeHB commented Aug 16, 2022 •

edited

Loading

UkoeHB commented Aug 16, 2022 •

edited

Loading

vtnerd left a comment

vtnerd Aug 17, 2022

vtnerd Aug 17, 2022

vtnerd Aug 18, 2022

UkoeHB Aug 18, 2022

vtnerd Aug 18, 2022

UkoeHB Aug 18, 2022

vtnerd Aug 18, 2022

UkoeHB Aug 18, 2022 •

edited

Loading

vtnerd Sep 9, 2022

UkoeHB Sep 9, 2022 •

edited

Loading

vtnerd Aug 18, 2022

UkoeHB Aug 18, 2022 •

edited

Loading

vtnerd left a comment

UkoeHB commented Sep 10, 2022

UkoeHB commented Sep 12, 2022

tmoravec commented Sep 14, 2022

selsta commented Sep 20, 2022

tmoravec commented Sep 20, 2022 •

edited

Loading

UkoeHB Sep 20, 2022

UkoeHB commented Sep 20, 2022 •

edited

Loading

tmoravec commented Sep 21, 2022 •

edited

Loading

tmoravec commented Sep 21, 2022

UkoeHB commented Sep 21, 2022

multisig: force update option for key exchange [master] #8329

multisig: force update option for key exchange [master] #8329

Conversation

UkoeHB commented May 15, 2022

Motivation

Changes

Future Work

Dangers

UkoeHB commented Aug 16, 2022 • edited Loading

Workflow 1 (recommended)

Workflow 2 (fastest, less robust in general if there is a malicious group member)

UkoeHB commented Aug 16, 2022 • edited Loading

vtnerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UkoeHB Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UkoeHB Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UkoeHB Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

vtnerd left a comment

Choose a reason for hiding this comment

UkoeHB commented Sep 10, 2022

UkoeHB commented Sep 12, 2022

tmoravec commented Sep 14, 2022

selsta commented Sep 20, 2022

tmoravec commented Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

UkoeHB commented Sep 20, 2022 • edited Loading

tmoravec commented Sep 21, 2022 • edited Loading

tmoravec commented Sep 21, 2022

UkoeHB commented Sep 21, 2022

UkoeHB commented Aug 16, 2022 •

edited

Loading

UkoeHB commented Aug 16, 2022 •

edited

Loading

UkoeHB Aug 18, 2022 •

edited

Loading

UkoeHB Sep 9, 2022 •

edited

Loading

UkoeHB Aug 18, 2022 •

edited

Loading

tmoravec commented Sep 20, 2022 •

edited

Loading

UkoeHB commented Sep 20, 2022 •

edited

Loading

tmoravec commented Sep 21, 2022 •

edited

Loading