multisig: fixes #8114

perfect-daemon · 2021-12-14T20:11:25Z

No description provided.

UkoeHB

This is a solid patch for the known vulnerabilities (to be disclosed publicly after this patch is merged). Thank you for all the hard work.

p.s. yes, I checked the diff against my local branch

moneromooo-monero · 2021-12-14T22:11:38Z

src/common/varint.h

+    static_assert(std::is_integral<T>::value, "");
+    static_assert(std::is_unsigned<T>::value, "");
+    static_assert((sizeof(T) * 8 + 6) / 7 <= N, "");  //output array must be large enough to store any varint encoding of 't'
+    for (std::size_t i = 0; i < N && t; ++i) {


This will write nothing if the value passed is 0. I don't think this is the intent. A unit test checking it writes the same as the existing varint writer (or that the reader gets back the same values) would find this.

And why the new function at all? Static bounds checking? Couldn't it just do the static asserts, and then forward to the other existing function (so there isn't unnecessary code duplication)? This does have an extra runtime check for the array bounds, but if that triggers a truncation occurs that isn't handled.

Also, trivial bikeshedding, but the parameters are flipped from the other function, which bothers my OCD.

vtnerd

My review isn't quite finished, but I have a question that I felt needed to go out sooner rather than later. It may require someone else from the MRL to respond (either here or directly me to IRC). The adaption of the relevant paper doesn't seem accurate, but the adaption is necessarily a bit more complex due to CLSAG (when compared to schnorr), so its possible that I misunderstood some key detail.

vtnerd · 2021-12-15T17:13:13Z

src/common/varint.h

+    static_assert(std::is_integral<T>::value, "");
+    static_assert(std::is_unsigned<T>::value, "");
+    static_assert((sizeof(T) * 8 + 6) / 7 <= N, "");  //output array must be large enough to store any varint encoding of 't'
+    for (std::size_t i = 0; i < N && t; ++i) {


And why the new function at all? Static bounds checking? Couldn't it just do the static asserts, and then forward to the other existing function (so there isn't unnecessary code duplication)? This does have an extra runtime check for the array bounds, but if that triggers a truncation occurs that isn't handled.

Also, trivial bikeshedding, but the parameters are flipped from the other function, which bothers my OCD.

vtnerd · 2021-12-15T20:52:26Z

src/multisig/signing_protocol.h

+  bool initialized;
+  bool reconstruction;
+  rct::keyV cached_w;
+  std::vector<cached_CLSAG_Gen_t> cached_CLSAG;


If this is the only place cached_CLSAG_Gen_t is referenced outside of the cpp, then the entire class declaration can be moved into the cpp and only a forward declare is needed in this header.

I don't think the headers can be reduced further, so its primarily just useful for keeping things out of the header and C++ parser.

vtnerd · 2021-12-15T21:57:50Z

src/multisig/signing_protocol.cpp

+      sc_muladd(cached_w[i].bytes, mu_C.bytes, z.bytes, cached_w[i].bytes);
+    }
+  }
+  unsigned_tx.rct_signatures = rv;


Might as well std::move this.

vtnerd · 2021-12-15T23:25:54Z

src/multisig/signing_protocol.cpp

+    for (const auto& e: sources[i].outputs)
+      offsets.emplace_back(e.first);
+    unsigned_tx.vin[i] = cryptonote::txin_to_key{
+      .amount = 0,


Is there a check somewhere to ensure the inputs are ringct, as opposed to version 0? Its probably in wallet2 somewhere?

EDIT: I meant to delete this, because I noticed later there was a check for this further up the call stack.

vtnerd · 2021-12-16T00:02:08Z

src/multisig/signing_protocol.cpp

+      tx_aux_secret_keys,
+      tx_aux_public_keys,
+      output_amount_secret_keys,
+      reinterpret_cast<crypto::public_key &>(output_public_keys[i])


This is an aliasing violation (but probably won't blow up). Still safer to run the conversion function.

vtnerd · 2021-12-16T19:52:19Z

src/multisig/signing_protocol.cpp

+  }
+
+  // musig2-style combination factor 'b'
+  rct::key b = rct::hash_to_scalar(b_params);


const rct::key -> don't want this one to accidentally change.

vtnerd · 2021-12-17T02:59:24Z

src/multisig/signing_protocol.cpp

+  b_params.insert(b_params.end(), s.begin(), s.begin() + l);    //fake responses before 'l'
+  b_params.insert(b_params.end(), s.begin() + l + 1, s.end());  //fake responses after 'l'
+  b_params.emplace_back();
+  tools::encode_varint(l, b_params.back().bytes);  //real signing index 'l'


I don't see why this needs to be encoded as a varint at all - copying to a uint32_t with SWAP32LE (or uint64_t if concerned about portability), and two static_asserts would do the same thing without the specialized function for varint.

b_params is hashed by key cn_fast_hash(const keyV &keys), so all elements need to be rct::keys (not ints).

It doesn't matter - the bit encoding just needs to be same. Do an endianess swap and memcpy into a rct::key.

vtnerd · 2021-12-17T23:04:03Z

src/multisig/signing_protocol.cpp

+      s.resize(ring_size);
+      for (std::size_t j = 0; j < ring_size; ++j) {
+        if (j != l)
+          s[j] = rct::skGen();  //make fake responses


At a glance, this doesn't seem correct. In the source paper (Musig2), the randomly point (R) is chosen cooperatively so that secret shares never leak to the other participants. It seems like something similar would have to be done here, but I haven't gone through the math details to verify any potential issue.

Has the adaption of musig2 been reviewed by anyone else (besides me)?

This conditional is generating the fake CLSAG responses for all decoy ring members. Yes, I did a thorough review already (hence why I approved this PR...).

I will probably finish up my review tomorrow, the hardest part is done (going through the relevant papers to track how it mapped to code, and changes necessary for the custom CLSAG scheme). So far it appears to be conforming to the two relevant papers; my review is "downhill" from here.

I don't have anything else to add, but there are some comments that the OP should comment on or update.

but there are some comments that the OP should comment on or update.

Care to elaborate?

An aliasing issue that can be trivial removed

The new varint function isn't needed (chop it for maintenance purposes)

A scalarmult can be easily removed

A std::move can be added easily

A simple const can be added

They are all simple changes, so it seemed easiest to make them now. Only the first two are useful before merge. The varint thing in particular would need to be chopped before a release, otherwise it becomes part of the protocol and must be maintained. The simple swap+memcpy is preferable to the maintenance of that specific function.

Oh, I thought you meant code comments.

vtnerd · 2021-12-17T23:11:38Z

src/multisig/signing_protocol.cpp

+    if (i == 0)
+      c_0 = c;
+    rct::addKeys3(c_params[c_params_L_offset], s[i], G_precomp.k, c, W_precomp[i].k);
+    rct::addKeys3(c_params[c_params_R_offset], s[i], H_precomp[i].k, c, wH_l_precomp.k);


This is a follow-up to my other comment about the random s values being selected by a single participant. The participants providing randomized public keys affect the outcome of these iterative c hash values, but that output value was never controllable (that's the point of this hash function usage from the verifiers perspective). That should also mean that the randomized public keys are trivial in terms of the purpose with respect to the musig algorithm - it doesn't seem like they serve any useful purpose unless they affect the selection of the s values.

EDIT: Ignore this comment, and focus on the other one about the s value. There's another randomly generated value in here, and the current implementation uses each participants random for that.

This code block is a bit hard to read due to the indexing. It's only using the decoy ring members' fake responses (not the responses from the multisig participants).

You can think of a ring signature as a Schnorr signature, where the real signature challenge is just a complicated hashing sequence of decoy members and fake responses. This code block is just computing the real signature challenge for multisig participants to respond to.

The line c = rct::hash_to_scalar(c_params); above this block is where the multisig participants' nonces are hashed (the real responses s[l] aren't defined in this function, they are defined way down in finalize_tx()).

I had a reasonably good idea of what this was doing (as in, you just explained bits I already knew).

My question (below, this was mostly noise here as I was thinking outloud) was whether s was a set of nonce values, and since you did a review already, presumably your answer is that they are not nonce values.

The CLSAG paper doesn't list any values as nonce, only that alpha and a series of s values are selected randomly. At a glance this musig algorithm appears to be violating the paper, because randomness isn't provided/enforced for the s values (the initiator gets to select all of the values).

The "fake"/random s values are definitely just for symmetry (logical "OR") to conceal the real P in use - more obvious after some fresh eyes (was staring at this too long earlier). You can't recover anything from a re-use in those values because they are used in equations isolated from any secret keys

'Nonce' is the same as alpha. IIRC the newer papers all use nonce, which imo is a better semantic than alpha.

The "fake"/random s values are definitely just for symmetry (logical "OR") to conceal the real P in use - more obvious after some fresh eyes (was staring at this too long earlier). You can't recover anything from a re-use in those values because they are used in equations isolated from any secret keys

Right, the real s is the real response, which is computed from a sum of multisig partial responses. The fake s are just random numbers/decoy responses. Do you still think these fake responses need to be computed from material provided by all multisig participants? Afaict, the only advantage this offers is if the tx proposer has weak RNG, then adding RNG from other participants will improve it (i.e. prevent observers from figuring out the real spend's index). If the tx proposer is malicious (or any of the signers), then adding more entropy from other participants is meaningless (the malicious individual can just publish the real index).

Do you still think these fake responses need to be computed from material provided by all multisig participants?

I do not think the s values need to be included in the shared random computation.

Afaict, the only advantage this offers is if the tx proposer has weak RNG, then adding RNG from other participants will improve it (i.e. prevent observers from figuring out the real spend's index). If the tx proposer is malicious (or any of the signers), then adding more entropy from other participants is meaningless (the malicious individual can just publish the real index).

The "penalty" for making the scheme easier appears to be a standard mechanism for publicly leaking the real spend. Since any of the signers can do this anyway, as you stated, this doesn't appear to be an issue worth fixing.

vtnerd · 2021-12-17T23:13:27Z

src/multisig/signing_protocol.cpp

+      );
+    }
+    for (std::size_t i = 0; i < num_sources - 1; ++i) {
+      rct::skGen(a[i]);


FWIW, I don't think the randomized values from all participants needs to influence this value, just the s values.

This code block is only executed if (not reconstruction), which is only true for the initial tx proposer. All other participants reconstruct his tx.

Yes, I know.

Sorry - this comment was (somewhat) pointless, I was stating that I think all participants need to alter s somehow, but I didn't think these randomly generated a values needed that.

rbrunner7 · 2022-01-02T09:12:09Z

I did some functional tests on this, i.e. I took the code, compiled it and made multisig transactions in the normal way, to check whether something might be broken. I did not look at the code, and I did not try to check whether the security problems this code tries to solve are in fact solved. Still, I thought just checking whether anything still runs normally should be worthwhile.

Running the CLI wallet plus MMS on the branch I built new testnet multisig wallets for 2/2, 2/3 and 3/5 multisig, funded those and then made outgoing transactions. Especially the 3/5 case gave me the opportunity to do a row of transactions with varying signers.

I connected to a daemon running current master code.

Everything worked without any problem.

I could only test a single error condition: Does the wallet detect that a signer tries to sign a transaction a second time? Result: It does.

jonathancross · 2022-03-04T15:10:35Z

test-ubuntu:

Testing monero: URI
Traceback (most recent call last):
  File "/home/runner/work/monero/monero/tests/functional_tests/uri.py", line 234, in <module>
    URITest().run_test()
  File "/home/runner/work/monero/monero/tests/functional_tests/uri.py", line 46, in run_test
    self.test_monero_uri()
  File "/home/runner/work/monero/monero/tests/functional_tests/uri.py", line 145, in test_monero_uri
    res = wallet.make_uri(address = address, recipient_name = utf8string[0], tx_description = utf8string[1], amount = 1000000000000, payment_id = '1' * 64)
  File "/home/runner/work/monero/monero/utils/python-rpc/framework/wallet.py", line 982, in make_uri
    return self.rpc.send_json_rpc_request(make_uri)
  File "/home/runner/work/monero/monero/utils/python-rpc/framework/rpc.py", line 80, in send_json_rpc_request
    return self.send_request("/json_rpc", inputs, 'result')
  File "/home/runner/work/monero/monero/utils/python-rpc/framework/rpc.py", line 72, in send_request
    assert 'error' not in res, res
AssertionError: {'error': {'code': -11, 'message': 'Cannot make URI from supplied parameters: Standalone payment id deprecated, use integrated address instead'}, 'id': '0', 'jsonrpc': '2.0'}
[TEST FAILED] uri

selsta · 2022-03-08T04:22:29Z

@jonathancross this is fixed in master

anon and others added 6 commits December 14, 2021 20:40

multisig: forbid signing nonce reusage

94cf745

multisig: reproduce challenge before signing

5c62cdc

multisig: use combination of multicomponent nonce

b555ad7

multisig: remove unused code

c68abe4

multisig: misc review

84907ee

review comments 2

e98c6be

perfect-daemon force-pushed the pr011_master branch from 9246396 to e98c6be Compare December 14, 2021 20:42

UkoeHB approved these changes Dec 14, 2021

View reviewed changes

moneromooo-monero reviewed Dec 14, 2021

View reviewed changes

vtnerd reviewed Dec 17, 2021

View reviewed changes

UkoeHB mentioned this pull request Jan 18, 2022

Multisig: signature fixes #8149

Merged

UkoeHB mentioned this pull request Apr 4, 2022

Monero Multisig for HF #8237

Closed

UkoeHB mentioned this pull request Jul 13, 2022

multisig: fixes #8113

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multisig: fixes #8114

multisig: fixes #8114

perfect-daemon commented Dec 14, 2021

UkoeHB left a comment

moneromooo-monero Dec 14, 2021

vtnerd Dec 15, 2021

vtnerd left a comment

vtnerd Dec 15, 2021

vtnerd Dec 15, 2021

vtnerd Dec 15, 2021

vtnerd Dec 15, 2021 •

edited

Loading

vtnerd Dec 16, 2021

vtnerd Dec 16, 2021

vtnerd Dec 17, 2021

UkoeHB Dec 17, 2021

vtnerd Dec 18, 2021 •

edited

Loading

vtnerd Dec 17, 2021

UkoeHB Dec 17, 2021 •

edited

Loading

vtnerd Dec 18, 2021

vtnerd Dec 23, 2021

UkoeHB Dec 23, 2021

vtnerd Dec 24, 2021

UkoeHB Dec 30, 2021

vtnerd Dec 17, 2021 •

edited

Loading

UkoeHB Dec 17, 2021 •

edited

Loading

vtnerd Dec 18, 2021 •

edited

Loading

UkoeHB Dec 18, 2021

UkoeHB Dec 18, 2021

vtnerd Dec 22, 2021

vtnerd Dec 17, 2021

UkoeHB Dec 17, 2021 •

edited

Loading

vtnerd Dec 18, 2021

vtnerd Dec 18, 2021

rbrunner7 commented Jan 2, 2022

jonathancross commented Mar 4, 2022

selsta commented Mar 8, 2022

multisig: fixes #8114

Are you sure you want to change the base?

multisig: fixes #8114

Conversation

perfect-daemon commented Dec 14, 2021

UkoeHB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

vtnerd Dec 15, 2021 • edited Loading

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

vtnerd Dec 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UkoeHB Dec 17, 2021 • edited Loading

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

vtnerd Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

UkoeHB Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

vtnerd Dec 18, 2021 • edited Loading

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 Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbrunner7 commented Jan 2, 2022

jonathancross commented Mar 4, 2022

selsta commented Mar 8, 2022

vtnerd Dec 15, 2021 •

edited

Loading

vtnerd Dec 18, 2021 •

edited

Loading

UkoeHB Dec 17, 2021 •

edited

Loading

vtnerd Dec 17, 2021 •

edited

Loading

UkoeHB Dec 17, 2021 •

edited

Loading

vtnerd Dec 18, 2021 •

edited

Loading

UkoeHB Dec 17, 2021 •

edited

Loading