[RELEASE] Several mitigations for attacking privacy via key reusing forks #3322

moneromooo-monero · 2018-02-28T11:41:41Z

No description provided.

moneromooo-monero · 2018-02-28T15:42:16Z

I've just thought there should be a "set_ring" command, in case the forkers don't merge this. I'll do this momentarily.

SamsungGalaxyPlayer · 2018-02-28T18:01:48Z

I can't speak for the code itself, but I support the general approach. It boils down to three things:

When spending on a fork, use the same exact ring member set.
If spending on the fork and/or main chain, consider using new outputs created after the split. These outputs will not be compromised by the key image attack, since they cannot be spent on both chains. Wallet software can select exclusively from this set at a cost of standing out, or it can select one to make sure there will be at least some plausible deniability while likely not standing out significantly.
Blacklist known bad outputs compromised through key image reuse and 0-mixin (effects for the latter have largely faded). These are known to be decoys.

Of course, a disclaimer that if a large proportion of transactions (for ringsize 5 >33%; ringsize 7 >50%) on the forked chain don't take advantage of 1, then there is a relatively likely chance that the real input can be revealed.

For ringsize 5, suppose 50% of initial transactions on a fork use this tool. Then, approx. 6.25% of other transactions will be compromised (total 53%, since 50%*6.25%) on the fork chain. Suppose half of the transactions on Monero are related to the fork, then the proportion of compromised inputs on the Monero chain falls to approx. 53/2 = 27%.

These are terribly rough estimates, but even small proportions of people using this tool help the forked chain to a small extent and Monero to a larger extent.

stoffu

Looks good, although my confidence is particularly low regarding the get_outs function which is quite long.

stoffu · 2018-02-28T15:53:29Z

src/cryptonote_core/blockchain.h

+    /**
+     * @brief gets per block distribution of rct outputs
+     *
+     * @return amount the amount to get a ditribution for


stoffu · 2018-02-28T15:56:16Z

src/cryptonote_core/blockchain.cpp

+
+  distribution.clear();
+  uint64_t db_height = m_db->height();
+  distribution.resize(db_height - start_height);


Initialize with zero: distribution.resize(db_height - start_height, 0);

stoffu · 2018-02-28T16:26:10Z

src/wallet/wallet2.cpp

+  uint32_t rpc_version;
+  boost::optional<std::string> result = m_node_rpc_proxy.get_rpc_version(rpc_version);
+  // no error
+  if (!!result)


I find the design of NodeRPCProxy a bit strange, because all the query methods always return boost::optional<std::string>() which equals to boost::none. So this section is always no-op.

It returns an optional error string.

I'd also expect so, but all the return statement I see in node_rpc_proxy.cpp is always return boost::optional<std::string>();. Or am I missing places where such an optional error string gets returned?

Various calls to CHECK_AND_ASSERT_MES, which typically return resp_t.result.status.

Oops, sorry I was completely overlooking them.

stoffu · 2018-02-28T16:29:35Z

src/wallet/wallet2.cpp

+  req.params.amounts.push_back(0);
+  m_daemon_rpc_mutex.lock();
+  MDEBUG("calling get_output_distribution");
+  bool r = net_utils::invoke_http_json("/json_rpc", req, res, m_http_client, rpc_timeout);


invoke_http_json_rpc can be used instead to eliminate boilerplate lines like req.jsonrpc = "2.0";.

stoffu · 2018-02-28T16:33:00Z

src/wallet/wallet_errors.h

@@ -36,6 +36,7 @@
 #include <vector>

 #include "cryptonote_basic/cryptonote_format_utils.h"
+#include "cryptonote_core/cryptonote_tx_utils.h"


This should be in ringdb.cpp instead.

wallet_errors.h uses cryptonote::tx_source_entry in tx_not_constructed

Ah, right. The code has been compiling fine merely because of the specific order wallet_errors.h was included, I suppose.

stoffu · 2018-02-28T18:21:25Z

src/simplewallet/simplewallet.cpp

+  m_cmd_binder.set_handler("blackball",
+                           boost::bind(&simple_wallet::blackball, this, _1),
+                           tr("blackball <output public key> | <filename> [<add>]"),
+                           tr("Blackball an output so it never gets selected as a fake output in a ring"));


an output (or a set of outputs in a file)

stoffu · 2018-02-28T19:08:27Z

src/blockchain_utilities/blockchain_blackball.cpp

+
+  po::options_description desc_cmd_only("Command line options");
+  po::options_description desc_cmd_sett("Command line options and settings options");
+  const command_line::arg_descriptor<std::string> arg_blackball_db_dir = {"blackball-db-dir", "Specify blackball database directory",


Perhaps this could use the dependent argument as in #3170?

stoffu · 2018-02-28T19:26:25Z

src/blockchain_utilities/blockchain_blackball.cpp

+          continue;
+
+        const std::vector<uint64_t> absolute = cryptonote::relative_output_offsets_to_absolute(txin.key_offsets);
+        if (n == 0)


Why only for the n==0 case?

After a fork, there'll be (amount,index) duplicates. You want to use the main db only.

OK. It wasn't clear in the code that n==0 is supposed be the main db. I think some more words should be added to the arg description.

stoffu · 2018-02-28T19:58:29Z

src/wallet/wallet2.cpp

+{
+  m_ring_database = filename;
+  MGINFO("ringdb path set to " << filename);
+  delete m_ringdb;


Check if not null?

delete does so.

Ouch, embarrassing :)

stoffu · 2018-02-28T20:02:33Z

src/simplewallet/simplewallet.cpp

+    }
+    if (relative)
+    {
+      if (!ring.back())


Doesn’t this exclude the case with the first index being 0?

Yes, I removed some "useless code", was not useless after all :/

stoffu · 2018-02-28T20:21:54Z

Some of my review comments got hidden due to push -f, but they're still unaddressed. Please take a look.

stoffu · 2018-03-02T00:52:51Z

src/wallet/wallet2.h

+    bool remove_rings(const cryptonote::transaction_prefix &tx);
+    bool get_ring(const crypto::chacha_key &key, const crypto::key_image &key_image, std::vector<uint64_t> &outs);
+
+    bool get_output_distribution(uint64_t &start_height, std::vector<uint64_t> &offsets);


I don't see this function being used anywhere. What's the intent?

Why is the second argument named offsets instead of distribution?

It's used by my patch to use a gamma distribution for fake out selection, which hasn't been PR'd. Since I picked this code from this patch, I might as well bring in the function that uses it too.

stoffu · 2018-03-02T01:15:14Z

src/simplewallet/simplewallet.cpp

+    }
+    else
+    {
+      if (ring.size() > 1 && ring[ring.size() - 2] >= ring[ring.size() - 1])


Shouldn't this test be performed for every index?

if (ring.size() > 1) { for (size_t i = 1; i < ring.size(); ++i) { if (ring[i - 1] >= ring[i]) { ...failure... return true; } } }

It's done for every index, there's a loop above.

Sorry, I misread.

stoffu · 2018-03-02T01:23:11Z

src/simplewallet/simplewallet.cpp

+        }
+        fclose(f);
+        bool add = false;
+        if (args.size() > 1 && args[1] != "add")


This logic seems incorrect, where add never becomes true. My suggestion:

if (args.size() > 1) { if (args[1] != "add) { ...failure... return true; } add = true; }

stoffu · 2018-03-02T01:50:57Z

src/wallet/ringdb.cpp

+
+    MDEBUG("Removing ring data for key image " << txin.k_image);
+    dbr = mdb_del(txn, dbi_rings, &key, NULL);
+    THROW_WALLET_EXCEPTION_IF(dbr, tools::error::wallet_internal_error, "Failed to add ring to database: " + std::string(mdb_strerror(dbr)));


"Failed to remove ring from database: "

stoffu · 2018-03-02T02:07:12Z

src/wallet/ringdb.h

+    bool clear_blackballs();
+
+  private:
+    bool blackball_worker(const crypto::public_key &output, int op);


Looks like further refactoring is possible using exactly the same strategy; i.e. by introducing these internal worker functions:

bool rings_worker(const crypto::chacha_key &chacha_key, const cryptonote::transaction_prefix &tx, int op); bool ring_worker(const crypto::chacha_key &chacha_key, const crypto::key_image &key_image, std::vector<uint64_t> &outs, bool relative, int op);

For the second one, the parameter outs would act as input/output when op is SET/GET. Also, I think it makes sense to have the function get_ring accept the parameter relative.

Yes, but I did those before the blackball one, and when I started doing this it created lots of conflicts.

What kind of conflicts? Can't you just add some switches to the worker function and direct the execution flow based on the op code?

Here's a patch that I came up with:

diff --git a/src/wallet/ringdb.cpp b/src/wallet/ringdb.cpp index 522a7ca7..6b22292e 100644 --- a/src/wallet/ringdb.cpp +++ b/src/wallet/ringdb.cpp @@ -170,6 +170,8 @@ static size_t get_ring_data_size(size_t n_entries) return n_entries * (32 + 1024); // highball 1kB for the ring data to make sure } +enum { RINGS_ADD, RINGS_REMOVE}; +enum { RING_GET, RING_SET}; enum { BLACKBALL_BLACKBALL, BLACKBALL_UNBLACKBALL, BLACKBALL_QUERY, BLACKBALL_CLEAR}; namespace tools @@ -218,13 +220,24 @@ ringdb::~ringdb() mdb_env_close(env); } -bool ringdb::add_rings(const crypto::chacha_key &chacha_key, const cryptonote::transaction_prefix &tx) +bool ringdb::rings_worker(const crypto::chacha_key &chacha_key, const cryptonote::transaction_prefix &tx, int op) { MDB_txn *txn; int dbr; bool tx_active = false; - dbr = resize_env(env, filename.c_str(), get_ring_data_size(tx.vin.size())); + if (op == RINGS_ADD) + { + dbr = resize_env(env, filename.c_str(), get_ring_data_size(tx.vin.size())); + } + else if (op == RINGS_REMOVE) + { + dbr = resize_env(env, filename.c_str(), 0); + } + else + { + THROW_WALLET_EXCEPTION(tools::error::wallet_internal_error, "Invalid rings op"); + } THROW_WALLET_EXCEPTION_IF(dbr, tools::error::wallet_internal_error, "Failed to set env map size"); dbr = mdb_txn_begin(env, NULL, 0, &txn); THROW_WALLET_EXCEPTION_IF(dbr, tools::error::wallet_internal_error, "Failed to create LMDB transaction: " + std::string(mdb_strerror(dbr))); @@ -244,130 +257,119 @@ bool ringdb::add_rings(const crypto::chacha_key &chacha_key, const cryptonote::t std::string key_ciphertext = encrypt(txin.k_image, chacha_key); key.mv_data = (void*)key_ciphertext.data(); key.mv_size = key_ciphertext.size(); - MDEBUG("Saving relative ring for key image " << txin.k_image << ": " << - boost::join(txin.key_offsets | boost::adaptors::transformed([](uint64_t out){return std::to_string(out);}), " ")); - std::string compressed_ring = compress_ring(txin.key_offsets); - std::string data_ciphertext = encrypt(compressed_ring, txin.k_image, chacha_key); - data.mv_size = data_ciphertext.size(); - data.mv_data = (void*)data_ciphertext.c_str(); - dbr = mdb_put(txn, dbi_rings, &key, &data, 0); - THROW_WALLET_EXCEPTION_IF(dbr, tools::error::wallet_internal_error, "Failed to add ring to database: " + std::string(mdb_strerror(dbr))); + + if (op == RINGS_ADD) + { + MDEBUG("Saving relative ring for key image " << txin.k_image << ": " << + boost::join(txin.key_offsets | boost::adaptors::transformed([](uint64_t out){return std::to_string(out);}), " ")); + std::string compressed_ring = compress_ring(txin.key_offsets); + std::string data_ciphertext = encrypt(compressed_ring, txin.k_image, chacha_key); + data.mv_size = data_ciphertext.size(); + data.mv_data = (void*)data_ciphertext.c_str(); + dbr = mdb_put(txn, dbi_rings, &key, &data, 0); + } + else + { + dbr = mdb_get(txn, dbi_rings, &key, &data); + THROW_WALLET_EXCEPTION_IF(dbr && dbr != MDB_NOTFOUND, tools::error::wallet_internal_error, "Failed to look for key image in LMDB table: " + std::string(mdb_strerror(dbr))); + if (dbr == MDB_NOTFOUND) + continue; + THROW_WALLET_EXCEPTION_IF(data.mv_size <= 0, tools::error::wallet_internal_error, "Invalid ring data size"); + + MDEBUG("Removing ring data for key image " << txin.k_image); + dbr = mdb_del(txn, dbi_rings, &key, NULL); + } + THROW_WALLET_EXCEPTION_IF(dbr, tools::error::wallet_internal_error, std::string("Failed to ") + (op == RINGS_ADD ? "add ring to" : "remove ring from") + " database: " + std::string(mdb_strerror(dbr))); } dbr = mdb_txn_commit(txn); - THROW_WALLET_EXCEPTION_IF(dbr, tools::error::wallet_internal_error, "Failed to commit txn adding ring to database: " + std::string(mdb_strerror(dbr))); + THROW_WALLET_EXCEPTION_IF(dbr, tools::error::wallet_internal_error, std::string("Failed to commit txn ") + (op == RING_GET ? "adding rings to" : "removing rings from") + " database: " + std::string(mdb_strerror(dbr))); tx_active = false; return true; } +bool ringdb::add_rings(const crypto::chacha_key &chacha_key, const cryptonote::transaction_prefix &tx) +{ + return rings_worker(chacha_key, tx, RINGS_ADD); +} + bool ringdb::remove_rings(const crypto::chacha_key &chacha_key, const cryptonote::transaction_prefix &tx) +{ + return rings_worker(chacha_key, tx, RINGS_REMOVE); +} + +bool ringdb::ring_worker(const crypto::chacha_key &chacha_key, const crypto::key_image &key_image, std::vector<uint64_t> &outs, bool relative, int op) { MDB_txn *txn; int dbr; bool tx_active = false; - dbr = resize_env(env, filename.c_str(), 0); - THROW_WALLET_EXCEPTION_IF(dbr, tools::error::wallet_internal_error, "Failed to set env map size"); + if (op == RING_GET) + { + dbr = resize_env(env, filename.c_str(), 0); + } + else if (op == RING_SET) + { + dbr = resize_env(env, filename.c_str(), outs.size() * 64); + } + else + { + THROW_WALLET_EXCEPTION(tools::error::wallet_internal_error, "Invalid ring op"); + } + THROW_WALLET_EXCEPTION_IF(dbr, tools::error::wallet_internal_error, "Failed to set env map size: " + std::string(mdb_strerror(dbr))); dbr = mdb_txn_begin(env, NULL, 0, &txn); THROW_WALLET_EXCEPTION_IF(dbr, tools::error::wallet_internal_error, "Failed to create LMDB transaction: " + std::string(mdb_strerror(dbr))); epee::misc_utils::auto_scope_leave_caller txn_dtor = epee::misc_utils::create_scope_leave_handler([&](){if (tx_active) mdb_txn_abort(txn);}); tx_active = true; - for (const auto &in: tx.vin) + MDB_val key, data; + std::string key_ciphertext = encrypt(key_image, chacha_key); + key.mv_data = (void*)key_ciphertext.data(); + key.mv_size = key_ciphertext.size(); + if (op == RING_GET) { - if (in.type() != typeid(cryptonote::txin_to_key)) - continue; - const auto &txin = boost::get<cryptonote::txin_to_key>(in); - const uint32_t ring_size = txin.key_offsets.size(); - if (ring_size == 1) - continue; - - MDB_val key, data; - std::string key_ciphertext = encrypt(txin.k_image, chacha_key); - key.mv_data = (void*)key_ciphertext.data(); - key.mv_size = key_ciphertext.size(); - dbr = mdb_get(txn, dbi_rings, &key, &data); THROW_WALLET_EXCEPTION_IF(dbr && dbr != MDB_NOTFOUND, tools::error::wallet_internal_error, "Failed to look for key image in LMDB table: " + std::string(mdb_strerror(dbr))); if (dbr == MDB_NOTFOUND) - continue; + return false; THROW_WALLET_EXCEPTION_IF(data.mv_size <= 0, tools::error::wallet_internal_error, "Invalid ring data size"); - MDEBUG("Removing ring data for key image " << txin.k_image); - dbr = mdb_del(txn, dbi_rings, &key, NULL); - THROW_WALLET_EXCEPTION_IF(dbr, tools::error::wallet_internal_error, "Failed to add ring to database: " + std::string(mdb_strerror(dbr))); + std::string data_plaintext = decrypt(std::string((const char*)data.mv_data, data.mv_size), key_image, chacha_key); + outs = decompress_ring(data_plaintext); + MDEBUG("Found ring for key image " << key_image << ":"); + MDEBUG("Relative: " << boost::join(outs | boost::adaptors::transformed([](uint64_t out){return std::to_string(out);}), " ")); + if (!relative) + { + outs = cryptonote::relative_output_offsets_to_absolute(outs); + MDEBUG("Absolute: " << boost::join(outs | boost::adaptors::transformed([](uint64_t out){return std::to_string(out);}), " ")); + } + + } + else + { + std::string compressed_ring = compress_ring(relative ? outs : cryptonote::absolute_output_offsets_to_relative(outs)); + std::string data_ciphertext = encrypt(compressed_ring, key_image, chacha_key); + data.mv_size = data_ciphertext.size(); + data.mv_data = (void*)data_ciphertext.c_str(); + dbr = mdb_put(txn, dbi_rings, &key, &data, 0); + THROW_WALLET_EXCEPTION_IF(dbr, tools::error::wallet_internal_error, "Failed to set ring for key image in LMDB table: " + std::string(mdb_strerror(dbr))); } dbr = mdb_txn_commit(txn); - THROW_WALLET_EXCEPTION_IF(dbr, tools::error::wallet_internal_error, "Failed to commit txn adding ring to database: " + std::string(mdb_strerror(dbr))); + THROW_WALLET_EXCEPTION_IF(dbr, tools::error::wallet_internal_error, std::string("Failed to commit txn ") + (op == RING_GET ? "getting ring from" : "setting ring to") + " database: " + std::string(mdb_strerror(dbr))); tx_active = false; return true; } -bool ringdb::get_ring(const crypto::chacha_key &chacha_key, const crypto::key_image &key_image, std::vector<uint64_t> &outs) +bool ringdb::get_ring(const crypto::chacha_key &chacha_key, const crypto::key_image &key_image, std::vector<uint64_t> &outs, bool relative) { - MDB_txn *txn; - int dbr; - bool tx_active = false; - - dbr = resize_env(env, filename.c_str(), 0); - THROW_WALLET_EXCEPTION_IF(dbr, tools::error::wallet_internal_error, "Failed to set env map size: " + std::string(mdb_strerror(dbr))); - dbr = mdb_txn_begin(env, NULL, 0, &txn); - THROW_WALLET_EXCEPTION_IF(dbr, tools::error::wallet_internal_error, "Failed to create LMDB transaction: " + std::string(mdb_strerror(dbr))); - epee::misc_utils::auto_scope_leave_caller txn_dtor = epee::misc_utils::create_scope_leave_handler([&](){if (tx_active) mdb_txn_abort(txn);}); - tx_active = true; - - MDB_val key, data; - std::string key_ciphertext = encrypt(key_image, chacha_key); - key.mv_data = (void*)key_ciphertext.data(); - key.mv_size = key_ciphertext.size(); - dbr = mdb_get(txn, dbi_rings, &key, &data); - THROW_WALLET_EXCEPTION_IF(dbr && dbr != MDB_NOTFOUND, tools::error::wallet_internal_error, "Failed to look for key image in LMDB table: " + std::string(mdb_strerror(dbr))); - if (dbr == MDB_NOTFOUND) - return false; - THROW_WALLET_EXCEPTION_IF(data.mv_size <= 0, tools::error::wallet_internal_error, "Invalid ring data size"); - - std::string data_plaintext = decrypt(std::string((const char*)data.mv_data, data.mv_size), key_image, chacha_key); - outs = decompress_ring(data_plaintext); - MDEBUG("Found ring for key image " << key_image << ":"); - MDEBUG("Relative: " << boost::join(outs | boost::adaptors::transformed([](uint64_t out){return std::to_string(out);}), " ")); - outs = cryptonote::relative_output_offsets_to_absolute(outs); - MDEBUG("Absolute: " << boost::join(outs | boost::adaptors::transformed([](uint64_t out){return std::to_string(out);}), " ")); - - dbr = mdb_txn_commit(txn); - THROW_WALLET_EXCEPTION_IF(dbr, tools::error::wallet_internal_error, "Failed to commit txn getting ring from database: " + std::string(mdb_strerror(dbr))); - tx_active = false; - return true; + return ring_worker(chacha_key, key_image, outs, relative, RING_GET); } bool ringdb::set_ring(const crypto::chacha_key &chacha_key, const crypto::key_image &key_image, const std::vector<uint64_t> &outs, bool relative) { - MDB_txn *txn; - int dbr; - bool tx_active = false; - - dbr = resize_env(env, filename.c_str(), outs.size() * 64); - THROW_WALLET_EXCEPTION_IF(dbr, tools::error::wallet_internal_error, "Failed to set env map size: " + std::string(mdb_strerror(dbr))); - dbr = mdb_txn_begin(env, NULL, 0, &txn); - THROW_WALLET_EXCEPTION_IF(dbr, tools::error::wallet_internal_error, "Failed to create LMDB transaction: " + std::string(mdb_strerror(dbr))); - epee::misc_utils::auto_scope_leave_caller txn_dtor = epee::misc_utils::create_scope_leave_handler([&](){if (tx_active) mdb_txn_abort(txn);}); - tx_active = true; - - MDB_val key, data; - std::string key_ciphertext = encrypt(key_image, chacha_key); - key.mv_data = (void*)key_ciphertext.data(); - key.mv_size = key_ciphertext.size(); - std::string compressed_ring = compress_ring(relative ? outs : cryptonote::absolute_output_offsets_to_relative(outs)); - std::string data_ciphertext = encrypt(compressed_ring, key_image, chacha_key); - data.mv_size = data_ciphertext.size(); - data.mv_data = (void*)data_ciphertext.c_str(); - dbr = mdb_put(txn, dbi_rings, &key, &data, 0); - THROW_WALLET_EXCEPTION_IF(dbr, tools::error::wallet_internal_error, "Failed to set ring for key image in LMDB table: " + std::string(mdb_strerror(dbr))); - - dbr = mdb_txn_commit(txn); - THROW_WALLET_EXCEPTION_IF(dbr, tools::error::wallet_internal_error, "Failed to commit txn setting ring to database: " + std::string(mdb_strerror(dbr))); - tx_active = false; - return true; + std::vector<uint64_t> outs_copy = outs; + return ring_worker(chacha_key, key_image, outs_copy, relative, RING_SET); } bool ringdb::blackball_worker(const crypto::public_key &output, int op) diff --git a/src/wallet/ringdb.h b/src/wallet/ringdb.h index 5cc41958..7d17421f 100644 --- a/src/wallet/ringdb.h +++ b/src/wallet/ringdb.h @@ -45,7 +45,7 @@ namespace tools bool add_rings(const crypto::chacha_key &chacha_key, const cryptonote::transaction_prefix &tx); bool remove_rings(const crypto::chacha_key &chacha_key, const cryptonote::transaction_prefix &tx); - bool get_ring(const crypto::chacha_key &chacha_key, const crypto::key_image &key_image, std::vector<uint64_t> &outs); + bool get_ring(const crypto::chacha_key &chacha_key, const crypto::key_image &key_image, std::vector<uint64_t> &outs, bool relative = false); bool set_ring(const crypto::chacha_key &chacha_key, const crypto::key_image &key_image, const std::vector<uint64_t> &outs, bool relative); bool blackball(const crypto::public_key &output); @@ -54,6 +54,8 @@ namespace tools bool clear_blackballs(); private: + bool rings_worker(const crypto::chacha_key &chacha_key, const cryptonote::transaction_prefix &tx, int op); + bool ring_worker(const crypto::chacha_key &chacha_key, const crypto::key_image &key_image, std::vector<uint64_t> &outs, bool relative, int op); bool blackball_worker(const crypto::public_key &output, int op); private:

A few duplicate lines of code in {add,remove}_rings and {get,set}_ring are eliminated.

I'm not sure what you are asking by "kind of conflict". Git conflicts. Did you try squashing this in the relevant commits ? You'll see the conflicts you get.

I see. Here's my attempt in reordering and squashing commits: https://github.com/stoffu/monero/commits/pr-3322-rebase

Notable changes I had to make are:

squash the commit turning ringdb from namespace-based to object-based

squash the last commit introducing the set_ring command into the first commit introducing ringdb

separate the parameter name change in wallet2::tx_add_fake_output

Do you think this is reasonable?

Looks reasonable by the commit messages, but (1) I'm not PRing something I did not write unless I can ensure there's nothing fishy in it and (2) I didn't want to spend time on this before, so I'm not spending time on it now either. Maybe once I have time if this is not merged already.

stoffu · 2018-03-02T03:48:50Z

src/cryptonote_core/blockchain.h

+     *
+     * @param amount the amount to get a ditribution for
+     * @param start_height the height of the first rct output
+     * @param distribution the start offset of the first rct output in this block (same as previous if none)


return-by-reference to the above two params, just like other member functions

Well, it's blindingly obvious, no ? But OK.

It's mostly for keeping the consistency in this header file. If existing declarations are given verbose comments, so should be the new ones, IMO.

stoffu · 2018-03-02T04:26:40Z

src/blockchain_utilities/blockchain_usage.cpp

+      auto it = outputs.find(od);
+      if (it == outputs.end())
+      {
+        outputs[output_data(out.amount, indices[out.amount], coinbase, height)];


Perhaps outputs[od] = {}; can make the intent a bit clearer?

stoffu · 2018-03-02T04:28:15Z

src/blockchain_utilities/blockchain_usage.cpp

+      }
+      else
+      {
+        it->first.info(coinbase, height);


I'm not sure how/why this code path can get executed, because indices for any amount gets incremented every time od is added to outputs. In other words, there will never be two output_data with the same amount and index during the loop.

Transactions are not processed in blockchain order so you can get an output used before it's created.

Ah, didn't know that.

stoffu · 2018-03-02T04:44:58Z

src/blockchain_utilities/blockchain_blackball.cpp

+            {
+              if (std::find(r1.begin(), r1.end(), out) != r1.end())
+                common.push_back(out);
+            }


How about using std::set_intersection?

std::set_intersection(r1.begin(), r1.end(), r2.begin(), r2.end(), std::back_inserter(common));

Mostly because I did not know that existed. At least I know what the current code does. The "equivalent code" in the docs isn't immediately obviously the same as mine, so I'm keeping mine.

stoffu · 2018-03-02T04:48:24Z

src/blockchain_utilities/blockchain_blackball.cpp

+            }
+            else
+            {
+              MINFO("The intersection has more than one element, it's still ok");


If r0 and r1 are of different lengths, ring members that aren't common in both get identified as fakes. To make use of this information, relative_rings[txin.k_image] should be set to the smallest ring among all that share the same key image.

Very good observation, thanks.

should be set to the smallest ring among all that share the same key image.

Actually, not just the smallest among all the relevant rings, but the set intersection of all of them which can be smaller than any one of them.

That's what I've done, yes.

stoffu · 2018-03-02T05:09:38Z

src/wallet/wallet2.cpp

+    THROW_WALLET_EXCEPTION_IF(*result == CORE_RPC_STATUS_BUSY, tools::error::daemon_busy, "getversion");
+    if (*result != CORE_RPC_STATUS_OK)
+    {
+      MDEBUG("Cannot determined daemon RPC version, not requesting rct distribution");


"determined" -> "determine"

To prevent "fix typo" commits :)

stoffu · 2018-03-02T05:13:39Z

src/simplewallet/simplewallet.cpp

+  m_cmd_binder.set_handler("blackballed",
+                           boost::bind(&simple_wallet::blackballed, this, _1),
+                           tr("blackballed <output public key>"),
+                           tr("Checks whether an output is blackballed"));


"Checks" -> "Check"

That's correct too.

The same reason as with the unblackball, just for the linguistic consistency.

stoffu

Confirmed the bug was fixed. Thanks again for developing this great tool!

stoffu · 2018-03-03T13:12:10Z

src/wallet/wallet2.cpp

+        num_outs = segregation_limit[amount].first;
+      else for (const auto &he: resp_t.result.histogram)
+        if (he.amount == amount)
+          num_outs = he.unlocked_instances;


Maybe break once found?

vtnerd

Will finish later. The comments on line 358-364 in blockchain_blackball.cpp are worth pushing this review sonner than later ...

vtnerd · 2018-03-05T17:42:44Z

src/cryptonote_core/blockchain.cpp

+  if (amount == 0)
+    start_height = m_testnet ? testnet_hard_forks[2].height : mainnet_hard_forks[2].height;
+  else
+    start_height = 0;


The comment indicates this is for ringct, so the comment needs to be updated or this need to be a return false. Also, this would require a ~11-12MiB vector currently. Should the height be user provided/selected? The callee already needs to know which heights are unique and important for the algorithm that is using this API to work.

It used to be rct only when first written, I'll change the comment. Good idea about the user being able to clamp.

vtnerd · 2018-03-05T17:45:58Z

src/cryptonote_core/blockchain.cpp

+  if (!r)
+    return false;
+  for (size_t n = 1; n < distribution.size(); ++n)
+    distribution[n] += distribution[n-1];


Why have this logic here? Can't the callee do this if desired? Also, since these values are being serialized via varint code, so this increasing the transmission size.

Because that is what I needed. Admittedly not a very complicated job to leave on the caller though.

I made it optional (cumulative bool) in the request.

vtnerd · 2018-03-05T23:23:20Z

src/blockchain_utilities/blockchain_blackball.cpp

+    LOG_PRINT_L0("No inputs given");
+    return 1;
+  }
+  std::vector<Blockchain*> core_storage(inputs.size(), NULL);


std::vector<std::unique_ptr<Blockchain>>

vtnerd · 2018-03-05T23:25:01Z

src/blockchain_utilities/blockchain_usage.cpp

+
+int main(int argc, char* argv[])
+{
+  TRY_ENTRY();


vtnerd · 2018-03-05T23:25:49Z

src/blockchain_utilities/blockchain_usage.cpp

+  // tx_memory_pool, Blockchain's constructor takes tx_memory_pool object.
+  LOG_PRINT_L0("Initializing source blockchain (BlockchainDB)");
+  const std::string input = command_line::get_arg(vm, arg_input);
+  Blockchain *core_storage = NULL;


std::unique_ptr

vtnerd · 2018-03-06T03:32:38Z

src/blockchain_utilities/blockchain_blackball.cpp

+              MINFO("The intersection has more than one element, it's still ok");
+              std::vector<uint64_t> new_ring;
+              for (const auto &out: relative_rings[txin.k_image])
+                if (std::find(common.begin(), common.end(), out) != common.end())


common contains absolute offsets, whereas relative_rings[txin.k_image] contains relative offsets. This find is unlikely to return any useful information.

vtnerd · 2018-03-06T03:39:15Z

src/blockchain_utilities/blockchain_blackball.cpp

+              for (const auto &out: relative_rings[txin.k_image])
+                if (std::find(common.begin(), common.end(), out) != common.end())
+                  new_ring.push_back(out);
+              relative_rings[txin.k_image] = new_ring;


This write to relative_rings[txin.k_image] is always overwritten on line 364 without any reads in-between. Presumably this is supposed to update relative_rings[txin.k_image] so that it only contains the common elements in both rings. If that is the goal, then relative_offsets must store absolute offsets instead of relative offsets, because the expansion algorithm will not work correctly (unless you re-packed from absolute to relative right here). There is no advantage in doing relative offsets though - that optimization is for reducing the wire/stored db size.

vtnerd · 2018-03-06T03:51:58Z

src/blockchain_utilities/blockchain_blackball.cpp

+
+  size_t done = 0;
+  std::unordered_map<crypto::key_image, std::vector<uint64_t>> relative_rings;
+  std::map<output_data, std::unordered_set<crypto::key_image>> outputs;


These should most likely be hash maps. They will have many elements, and are called frequently enough to notice. boost::hash<> has built-in functionality for std::pair<>, and so does boost::unorderded_set<>. so the key could be std::pair<uint64_t, uint64_t> to save on typing (if you didn't want to use boost::hash<> with std::unordered_set due to extra long typename). The interfaces are identical.

vtnerd · 2018-03-06T05:18:53Z

src/blockchain_utilities/blockchain_usage.cpp

+  mutable uint64_t height;
+  output_data(uint64_t a, uint64_t i, bool cb, uint64_t h): amount(a), index(i), coinbase(cb), height(h) {}
+  bool operator<(const output_data &other) const
+    { if (amount < other.amount) return true; if (amount == other.amount && index < other.index) return true; return false; }


This could also be return std::make_pair(amount, index) < std::make_pair(other.amount, other.index);.

vtnerd · 2018-03-06T05:22:33Z

src/blockchain_utilities/blockchain_blackball.cpp

+  while (!newly_spent.empty())
+  {
+    LOG_PRINT_L0("Secondary pass due to " << newly_spent.size() << " newly found spent outputs");
+    std::set<output_data> work_spent = newly_spent;


work_spend = std::move(newly_spent) for a quick/easy perfornance gain.

vtnerd · 2018-03-07T04:19:23Z

src/rpc/core_rpc_server.cpp

+          for (size_t n = 1; n < distribution.size(); ++n)
+            distribution[n] += distribution[n-1];
+        }
+        res.distributions.push_back({amount, start_height, distribution, base});


std::move(distribution)

stoffu reviewed Feb 28, 2018

View reviewed changes

moneromooo-monero force-pushed the detfake-master branch 4 times, most recently from 2eaeb4f to 7985728 Compare March 1, 2018 12:40

stoffu reviewed Mar 2, 2018

View reviewed changes

moneromooo-monero force-pushed the detfake-master branch from 7985728 to 0b65078 Compare March 2, 2018 12:47

moneromooo-monero mentioned this pull request Mar 2, 2018

[Fork Attack Mitigation] add sweep_all_fork that accepts two destination addresses #3265

Closed

moneromooo-monero force-pushed the detfake-master branch from 0b65078 to 6fe5be8 Compare March 3, 2018 10:33

stoffu approved these changes Mar 3, 2018

View reviewed changes

moneromooo-monero force-pushed the detfake-master branch from 6fe5be8 to 2b02c65 Compare March 3, 2018 14:04

moneromooo-monero changed the title ~~Several mitigations for attacking privacy via key reusing forks~~ [RELEASE] Several mitigations for attacking privacy via key reusing forks Mar 4, 2018

vtnerd reviewed Mar 6, 2018

View reviewed changes

moneromooo-monero force-pushed the detfake-master branch 4 times, most recently from f45e326 to ef10a92 Compare March 7, 2018 13:18

vtnerd reviewed Mar 8, 2018

View reviewed changes

[RELEASE] Several mitigations for attacking privacy via key reusing forks #3322

[RELEASE] Several mitigations for attacking privacy via key reusing forks #3322

Conversation

moneromooo-monero commented Feb 28, 2018

moneromooo-monero commented Feb 28, 2018

SamsungGalaxyPlayer commented Feb 28, 2018 • edited

stoffu 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

stoffu Mar 1, 2018 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoffu Mar 1, 2018 • edited

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

stoffu commented Feb 28, 2018 • edited

stoffu Mar 2, 2018 • edited

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

stoffu Mar 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoffu Mar 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoffu Mar 2, 2018 • edited

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

stoffu Mar 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoffu Mar 2, 2018 • edited

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

stoffu Mar 2, 2018 • edited

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

stoffu left a 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

vtnerd Mar 5, 2018 • edited

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

SamsungGalaxyPlayer commented Feb 28, 2018 •

edited

stoffu Mar 1, 2018 •

edited

stoffu Mar 1, 2018 •

edited

stoffu commented Feb 28, 2018 •

edited

stoffu Mar 2, 2018 •

edited

stoffu Mar 2, 2018 •

edited

stoffu Mar 2, 2018 •

edited

stoffu Mar 2, 2018 •

edited

stoffu Mar 2, 2018 •

edited

stoffu Mar 2, 2018 •

edited

stoffu Mar 2, 2018 •

edited

vtnerd Mar 5, 2018 •

edited