-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Multisig: signature fixes #8149
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh sorry, found one last thing to comment about with the commitment portion of musig. I don't think it matters, but I was curious about your thoughts (and I will probably think about it some more in the interim).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The remaining comments are trivial.
Just wanted to note here that I tested these changes on ubuntu:20.04:
Probably not all possible cases, but still. Thanks for fixing! |
I care about this getting merged. Is there anything I can do to help? |
Thank you @domol! @arnuschky I'm not sure... there are some non-public review processes going on that I need to respect. It's mostly a waiting game I guess. Note that this PR is not super useful without #7877. |
Would you mind elaborating a bit, please? #7877 is a major change that updates the flows, changes the APIs, and so on. It requires heavy integration work. This one is closer to a bugfix that can more or less be merged. Is it possible to merge this one to get the security improvement, and leave #7877 for later, possibly several months later for example? Thank you! |
@tmoravec Without #7877, you cannot be confident that an M-of-N multisig wallet is actually an M-of-N multisig wallet. With key cancellation or an address hostage attack during the setup ceremony, it could be 1-of-N or (1+(M-1))-of-N, where the attacker always must participate in signing. I'm pretty sure the only API/flow change with #7877 is changing
Unfortunately it's pretty much out of my hands. Review doesn't happen instantaneously. |
@UkoeHB Fantastic, thanks for the explanation!
I meant merge to downstream libraries and products, not to monero core master. I would never dare to push anyone :) . |
@tmoravec Yes, you are free to use any PR as you wish (assuming it isn't marked WIP or has an active discussion), with the caveat that an unmerged PR is probably a not-fully-reviewed PR (anything you do with it is 'experimental'). |
If it helps, I've backported the changes to
It works :) . Thanks for the fixes! |
Note (todo): It will take a bit of work to rebase this PR onto #8061. |
Seems this needs a rebase? |
6bf408b
to
4b2c4fc
Compare
rebased (to clean up merge conflicts) |
4b2c4fc
to
dd95270
Compare
dd95270
to
78448a6
Compare
rebased (onto BP+) |
1f0cee2
to
d7875ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read through the code, except the tests due to the other people saying this does work according to external expectations. The cryptography seems solid and well implemented to me, yet Ukoe definitely is the more informed here. I had one comment arguably more about ringct and then I think one comment is miswritten with what would be a critical security flaw, by my reading of it (yet not one that seems to be present in the code). Once that's clarified, I'd be happy to put my approval behind this :)
I can read/write C++, but I haven't contributed to Monero before so it's not my strongest suit. My only other concern would be if the TX handling code had some level of duplication which could be reduced (input sorting, yet it's a pretty trivial few lines...).
cf1956d
to
810b1ed
Compare
rebased (for view tags) |
cdfef46
to
b9a3625
Compare
792a9b6
to
b1f649a
Compare
4e9885a
to
9ce4b4a
Compare
Squashed |
9ce4b4a
to
4e9885a
Compare
Undid squash (some miscommunication sorry). |
4e9885a
to
a717af3
Compare
Another rebase |
a717af3
to
97904eb
Compare
I rolled back the deterministic sender-receiver secrets, which were a bit of scope creep on this PR (they will be proposed in a follow-up PR after this one). EDIT: here is a branch with a commit for deterministic sender-receiver secrets on top of this PR's branch. |
97904eb
to
4081f00
Compare
@UkoeHB FYI I've just tested this branch and I'm getting an error when creating a transaction (2/3 multisig wallet, stagenet):
|
@tmoravec is BP+ active on your branch (i.e. is your wallet2 trying to make BP+ txs?)? This PR only works with BP+. Edit: the current testnet is running BP+, so that might be a better place to test it right now. |
FWIW - multisig is a moving target for quite a while already: Over the last few days I tested this as current master plus all commits in this PR, on testnet. I built 2/2, 2/3 and 3/5 wallets, funded, synced and then built out-transactions to spend some funds. Everything worked flawlessly. |
I tested the latest update today, building transactions for stagenet in 2/2 and 4/4 wallets. Again no problems detected. I also did a 2/2 testnet transaction with this code, making sure those did not break. Worked as well. |
Thanks @UkoeHB for the pre-BP+ patch! I've tested it and it works with my 2/3 wallet tests. Creating and funding wallets, sending transactions, etc. on stagenet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of these are required, yet there's a few questions/nice to haves.
I'm not the most familiar with wallet2 as a whole, leading to C++ omissions in my review such as not checking for proper usage of memwipe everywhere, yet the cryptography in this change set should be secure and implement a secure multisig signing process for Monero as a whole. User tests confirm it meets the expected UX, and nonces are explicitly removed from memory on first usage (as distinct from a previously noted work with Monero multisig).
EDIT: I have to clarify multisig transaction creation is insecure without the burning bug fix, which was moved to another PR, which I missed when doing my review here. Accordingly, this review is just on the "signing" process.
const rct::key& D, | ||
const unsigned int l, | ||
const rct::keyV& s, | ||
const std::size_t num_alpha_components |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why num_alpha_components is practically variable. While Musig2 does leave it ambiguous, consensus seems to have been practically reached around 2. Leaving this variable potentially poses issues as it's half static, half variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's a little overkill, although from a design standpoint it's nice to have a centralized config parameter instead of a hard-coded value.
c_params_L_offset = c_params.size(); | ||
b_params_L_offset = b_params.size(); | ||
c_params.resize(c_params.size() + 1); //this is where L will be inserted later | ||
b_params.resize(b_params.size() + num_alpha_components); //multisig aggregate public nonces for L will be inserted here later |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, the variable num_alpha_components enables a transcript conflict. A ring of 11 with 3 alpha components has the exact same length as a ring of 12 with 2 alpha components. While, practically, this isn't an issue due to the message hash being based off ring size and therefore shifting if this happens, there's a lack of a clear distinction here with this naive transcript (which I don't mean to criticize on its own as this is consistent with the rest of Monero). Adding
b_params.emplace_back();
encode_int_to_key_le(num_alpha_components, b_params.back());
would be appreciated to make num_alpha_components properly sized in this transcript though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, since this binds to the aggregate nonces, mutability is trivial. While this follows MuSig2, FROST not only defined a binomial nonce scheme, yet also transcripted them as participant, D, E
, removing any chance at mutability. Not an issue here, yet I do believe that'd be an improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Musig2 is simpler and there is no concrete reason to change the design at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been resolved with the most recent edits :)
alpha_combined = rct::zero(); | ||
for (std::size_t i = 0; i < num_alpha_components; ++i) { | ||
rct::addKeys(L_l, L_l, rct::scalarmultKey(total_alpha_G[i], b_i)); | ||
rct::addKeys(R_l, R_l, rct::scalarmultKey(total_alpha_H[i], b_i)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using sc_add and doing the scalarmultKey at the end outside of the loop would be more efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically more efficient yes, but negligible in practice. If this were verification code or if it were egregious then I'd change it. I generally prefer legibility over marginal efficiency gains in cases like this.
src/wallet/wallet2.h
Outdated
FIELD(sigs) | ||
FIELD(ignore) | ||
FIELD(used_L) | ||
FIELD(signing_keys) | ||
FIELD(msout) | ||
if (version < 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe we ever check a version 1 struct is used. I assume these fields will be left uninitialized and therefore naturally error without issue, yet may be good to enforce this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the standard pattern for updating serialization versions in this codebase (when the old version is assumed to be deprecated).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although it does make me wonder if this should return false instead, since old stuff won't work anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
const rct::keyV& total_alpha_H, | ||
const rct::keyV& alpha, | ||
rct::key& alpha_combined, | ||
rct::key& c_0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CLSAG code calls this c1. While I honestly kind of prefer c0, and it turns out that's the term from the paper, the discrepancy isn't preferred. We also can't change it within the CLSAG code anymore, which is unfortunate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this isn't ideal... however, c_0
is set here:
for (std::size_t i = (l + 1) % n; i != l; i = (i + 1) % n) {
if (i == 0)
c_0 = c;
...
}
So I think c_0
makes more sense than doubling down on c1
.
I added a comment about the notation discrepancy.
bool view_tag_required(const int bp_version) | ||
{ | ||
// view tags were introduced at the same time as BP+, so they are needed after BP+ (v4 and later) | ||
if (bp_version <= 3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd hope we could utilize a proper enum here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, however the version is defined as an integer upstream so we are kind of stuck in this case.
s.resize(ring_size); | ||
for (std::size_t j = 0; j < ring_size; ++j) { | ||
if (j != l) | ||
s[j] = rct::skGen(); //make fake responses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be converted into a RNG in a manner similar to tx_secret_key to reduce bandwidth and consolidate code between creator/reconstructors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree you can do this, and it does reduce bandwidth. I'm not sure it is useful in practice, because the payload size would only shrink by a few hundred bytes at most (in the typical case). The goal here is to get something that works correctly, rather than engineer it into the ideal state.
bool tx_builder_ringct_t::init( | ||
const cryptonote::account_keys& account_keys, | ||
const std::vector<std::uint8_t>& extra, | ||
const std::uint64_t unlock_time, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I understand why this is technically valid, the unlock time has minimal benefit and is largely considered extremely detrimental. Including it here in multisig does not seem beneficial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The calling code (wallet2 tx builder) expects timelocks to work as intended by the user (who has to manually specify the unlock time). Deprecating timelocks is a bigger project that's out of scope here.
src/wallet/wallet2.cpp
Outdated
auto ignore_set = ignore_sets.empty() ? std::unordered_set<crypto::public_key>() : ignore_sets.front(); | ||
src.multisig_kLRki = get_multisig_composite_kLRki(idx, ignore_set, used_L, used_L); | ||
} | ||
//TODO: multisig_kLRki as used in tx_source_entry is just a key image shuttle into the multisig tx builder, need to simplify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left over TODO?
sig.ignore = ignore_sets[i]; | ||
sig.signing_keys = signing_keys; //the local signer signed with ALL of their multisig key shares, record their pubkeys for reference by other signers | ||
} | ||
if (m_multisig_threshold <= 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I dare ask what 0 is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't exist in the current implementation. IIRC I added the <=
as defense in depth against bugs.
Inference AG recently completed an audit of this PR funded by RINO. Overall, the actionable points in the report are minor. Here are my comments on the points raised (bolded points were addressed in the latest commit):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I have nitpicks, my noted flaws (except the burning bug which was moved out of the PR) appear fixed, and Inference AG's appears competently responded to as well.
040b093
to
c7b2944
Compare
Commits squashed |
Follow up to PR #8114. I lost contact with the original author two weeks ago.
This PR has the same commits as #8114, with [EDIT: additional commits to address reviewers, fix rebases, and general code quality].
Summary (added post-merge)
src/multisig/signing_protocol.h/.cpp
, largly by copying and slightly modifying code fromconstruct_tx_with_tx_key()
), because it is necessary for all multisig participants to fully reconstruct the message they are asked to sign.on_sign_multisig()
). This PR only ensures that whensign_multisig_tx()
is called, the tx signed actually corresponds to the tx details passed in.Possible Future Work
Here are items of work/research related to multisig that are out-of-scope for this PR.
set_events()
, but it's blocked by this PR.M < N - 1
).export_multisig()
information). The lack of this ability isn't a big UX problem for escrowed 2-of-3 markets, but is sub-optimal for generic use of multisig.