From 97ae7bb5cbf2cb3ef14fa20c9af72b24b9fe3e6f Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Tue, 15 Oct 2019 09:12:24 +0000 Subject: wallet2: do not repeatedly ask for pool txes sent to us MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This lets a passive attacker with access to the network link between node and wallet perform traffic analysis to deduce when an idle wallet receives a transaction. Reported by Tramèr et al. --- src/wallet/wallet2.cpp | 5 ----- 1 file changed, 5 deletions(-) (limited to 'src/wallet/wallet2.cpp') diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp index 9b3e7e8b4..4f0e2c26a 100644 --- a/src/wallet/wallet2.cpp +++ b/src/wallet/wallet2.cpp @@ -2968,11 +2968,6 @@ void wallet2::update_pool_state(bool refreshed) LOG_PRINT_L1("We sent that one"); } } - else - { - LOG_PRINT_L1("Already saw that one, it's for us"); - txids.push_back({txid, true}); - } } // get those txes -- cgit v1.2.3 From d5472bd87b8e93706295d8aa7ff99e5ad594277d Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Tue, 15 Oct 2019 12:25:26 +0000 Subject: wallet2: do not send an unnecessary last getblocks.bin call on refresh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "everything refreshed" state was detected when a refresh call did not return any new blocks. This can be detected without that extra "empty" call by comparing the claimed node height to the height of the last block retrieved. Doing this avoids that last call, saves some bandwidth, and makes the common refresh case use only one call rather than two. As a side effect, it prevents an information leak reported by Tramèr et al: if the wallet retrieves a set of blocks which includes an output sent to the refreshing wallet, the wallet will prompt the user for the password to decode the amount and calculate the key image for the new output, and this will delay subsequent calls to getblocks.bin, allowing a passive adversary to note the delay and deduce when the wallet receives at least one output. This can still happen if the wallet downloads more than 1000 blocks, since this will be split in several calls, but then the most the adversary can tell is which 1000 block section the user received some monero (the adversary can estimate the heights of the blocks by calculating how many "large" transfers are done, which will be sections of blocks, the last of which will usually be below 1000, but the size of the data should allow the actual number of blocks sent to be determined fairly accurately). This timing trick still be used via the subsequent scan for incoming txes in the txpool, which will be fixed later. --- src/wallet/wallet2.cpp | 20 +++++++++++++++----- src/wallet/wallet2.h | 4 ++-- 2 files changed, 17 insertions(+), 7 deletions(-) (limited to 'src/wallet/wallet2.cpp') diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp index 4f0e2c26a..c7c49f445 100644 --- a/src/wallet/wallet2.cpp +++ b/src/wallet/wallet2.cpp @@ -2519,12 +2519,14 @@ void wallet2::parse_block_round(const cryptonote::blobdata &blob, cryptonote::bl error = !cryptonote::parse_and_validate_block_from_blob(blob, bl, bl_id); } //---------------------------------------------------------------------------------------------------- -void wallet2::pull_blocks(uint64_t start_height, uint64_t &blocks_start_height, const std::list &short_chain_history, std::vector &blocks, std::vector &o_indices) +void wallet2::pull_blocks(uint64_t start_height, uint64_t &blocks_start_height, const std::list &short_chain_history, std::vector &blocks, std::vector &o_indices, uint64_t ¤t_height) { cryptonote::COMMAND_RPC_GET_BLOCKS_FAST::request req = AUTO_VAL_INIT(req); cryptonote::COMMAND_RPC_GET_BLOCKS_FAST::response res = AUTO_VAL_INIT(res); req.block_ids = short_chain_history; + MDEBUG("Pulling blocks: start_height " << start_height); + req.prune = true; req.start_height = start_height; req.no_miner_tx = m_refresh_type == RefreshNoCoinbase; @@ -2544,6 +2546,10 @@ void wallet2::pull_blocks(uint64_t start_height, uint64_t &blocks_start_height, blocks_start_height = res.start_height; blocks = std::move(res.blocks); o_indices = std::move(res.output_indices); + current_height = res.current_height; + + MDEBUG("Pulled blocks: blocks_start_height " << blocks_start_height << ", count " << blocks.size() + << ", height " << blocks_start_height + blocks.size() << ", node height " << res.current_height); } //---------------------------------------------------------------------------------------------------- void wallet2::pull_hashes(uint64_t start_height, uint64_t &blocks_start_height, const std::list &short_chain_history, std::vector &hashes) @@ -2726,9 +2732,10 @@ void wallet2::refresh(bool trusted_daemon, uint64_t start_height, uint64_t & blo refresh(trusted_daemon, start_height, blocks_fetched, received_money); } //---------------------------------------------------------------------------------------------------- -void wallet2::pull_and_parse_next_blocks(uint64_t start_height, uint64_t &blocks_start_height, std::list &short_chain_history, const std::vector &prev_blocks, const std::vector &prev_parsed_blocks, std::vector &blocks, std::vector &parsed_blocks, bool &error, std::exception_ptr &exception) +void wallet2::pull_and_parse_next_blocks(uint64_t start_height, uint64_t &blocks_start_height, std::list &short_chain_history, const std::vector &prev_blocks, const std::vector &prev_parsed_blocks, std::vector &blocks, std::vector &parsed_blocks, bool &last, bool &error, std::exception_ptr &exception) { error = false; + last = false; exception = NULL; try @@ -2746,7 +2753,8 @@ void wallet2::pull_and_parse_next_blocks(uint64_t start_height, uint64_t &blocks // pull the new blocks std::vector o_indices; - pull_blocks(start_height, blocks_start_height, short_chain_history, blocks, o_indices); + uint64_t current_height; + pull_blocks(start_height, blocks_start_height, short_chain_history, blocks, o_indices, current_height); THROW_WALLET_EXCEPTION_IF(blocks.size() != o_indices.size(), error::wallet_internal_error, "Mismatched sizes of blocks and o_indices"); tools::threadpool& tpool = tools::threadpool::getInstance(); @@ -2784,6 +2792,7 @@ void wallet2::pull_and_parse_next_blocks(uint64_t start_height, uint64_t &blocks } } waiter.wait(&tpool); + last = !blocks.empty() && cryptonote::get_block_height(parsed_blocks.back().block) + 1 == current_height; } catch(...) { @@ -3250,7 +3259,7 @@ void wallet2::refresh(bool trusted_daemon, uint64_t start_height, uint64_t & blo }); auto scope_exit_handler_hwdev = epee::misc_utils::create_scope_leave_handler([&](){hwdev.computing_key_images(false);}); - bool first = true; + bool first = true, last = false; while(m_run.load(std::memory_order_relaxed)) { uint64_t next_blocks_start_height; @@ -3272,7 +3281,8 @@ void wallet2::refresh(bool trusted_daemon, uint64_t start_height, uint64_t & blo refreshed = true; break; } - tpool.submit(&waiter, [&]{pull_and_parse_next_blocks(start_height, next_blocks_start_height, short_chain_history, blocks, parsed_blocks, next_blocks, next_parsed_blocks, error, exception);}); + if (!last) + tpool.submit(&waiter, [&]{pull_and_parse_next_blocks(start_height, next_blocks_start_height, short_chain_history, blocks, parsed_blocks, next_blocks, next_parsed_blocks, last, error, exception);}); if (!first) { diff --git a/src/wallet/wallet2.h b/src/wallet/wallet2.h index 640565a4e..d99e6e8c5 100644 --- a/src/wallet/wallet2.h +++ b/src/wallet/wallet2.h @@ -1403,10 +1403,10 @@ private: void get_short_chain_history(std::list& ids, uint64_t granularity = 1) const; bool clear(); void clear_soft(bool keep_key_images=false); - void pull_blocks(uint64_t start_height, uint64_t& blocks_start_height, const std::list &short_chain_history, std::vector &blocks, std::vector &o_indices); + void pull_blocks(uint64_t start_height, uint64_t& blocks_start_height, const std::list &short_chain_history, std::vector &blocks, std::vector &o_indices, uint64_t ¤t_height); void pull_hashes(uint64_t start_height, uint64_t& blocks_start_height, const std::list &short_chain_history, std::vector &hashes); void fast_refresh(uint64_t stop_height, uint64_t &blocks_start_height, std::list &short_chain_history, bool force = false); - void pull_and_parse_next_blocks(uint64_t start_height, uint64_t &blocks_start_height, std::list &short_chain_history, const std::vector &prev_blocks, const std::vector &prev_parsed_blocks, std::vector &blocks, std::vector &parsed_blocks, bool &error, std::exception_ptr &exception); + void pull_and_parse_next_blocks(uint64_t start_height, uint64_t &blocks_start_height, std::list &short_chain_history, const std::vector &prev_blocks, const std::vector &prev_parsed_blocks, std::vector &blocks, std::vector &parsed_blocks, bool &last, bool &error, std::exception_ptr &exception); void process_parsed_blocks(uint64_t start_height, const std::vector &blocks, const std::vector &parsed_blocks, uint64_t& blocks_added, std::map, size_t> *output_tracker_cache = NULL); uint64_t select_transfers(uint64_t needed_money, std::vector unused_transfers_indices, std::vector& selected_transfers) const; bool prepare_file_names(const std::string& file_path); -- cgit v1.2.3 From fd35e2304ab5223edd9c6caf03a16c19330e972d Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Tue, 15 Oct 2019 18:10:29 +0000 Subject: wallet: fix another facet of "did I get some monero" information leak We get new pool txes before processing any tx, pool or not. This ensures that if we're asked for a password, this does not cause a measurable delay in the txpool query after the last block query. --- src/simplewallet/simplewallet.cpp | 12 ++++++++++-- src/wallet/wallet2.cpp | 40 +++++++++++++++++++++++++++++---------- src/wallet/wallet2.h | 3 ++- src/wallet/wallet_rpc_server.cpp | 10 ++++++++-- 4 files changed, 50 insertions(+), 15 deletions(-) (limited to 'src/wallet/wallet2.cpp') diff --git a/src/simplewallet/simplewallet.cpp b/src/simplewallet/simplewallet.cpp index 03693a57c..7cf0b4913 100644 --- a/src/simplewallet/simplewallet.cpp +++ b/src/simplewallet/simplewallet.cpp @@ -8356,7 +8356,11 @@ bool simple_wallet::get_transfers(std::vector& local_args, std::vec m_in_manual_refresh.store(true, std::memory_order_relaxed); epee::misc_utils::auto_scope_leave_caller scope_exit_handler = epee::misc_utils::create_scope_leave_handler([&](){m_in_manual_refresh.store(false, std::memory_order_relaxed);}); - m_wallet->update_pool_state(); + std::vector> process_txs; + m_wallet->update_pool_state(process_txs); + if (!process_txs.empty()) + m_wallet->process_pool_state(process_txs); + std::list> payments; m_wallet->get_unconfirmed_payments(payments, m_current_subaddress_account, subaddr_indices); for (std::list>::const_iterator i = payments.begin(); i != payments.end(); ++i) { @@ -10002,7 +10006,11 @@ bool simple_wallet::show_transfer(const std::vector &args) try { - m_wallet->update_pool_state(); + std::vector> process_txs; + m_wallet->update_pool_state(process_txs); + if (!process_txs.empty()) + m_wallet->process_pool_state(process_txs); + std::list> pool_payments; m_wallet->get_unconfirmed_payments(pool_payments, m_current_subaddress_account); for (std::list>::const_iterator i = pool_payments.begin(); i != pool_payments.end(); ++i) { diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp index c7c49f445..b23e8525b 100644 --- a/src/wallet/wallet2.cpp +++ b/src/wallet/wallet2.cpp @@ -2829,7 +2829,7 @@ void wallet2::remove_obsolete_pool_txs(const std::vector &tx_hashe } //---------------------------------------------------------------------------------------------------- -void wallet2::update_pool_state(bool refreshed) +void wallet2::update_pool_state(std::vector> &process_txs, bool refreshed) { MTRACE("update_pool_state start"); @@ -3019,13 +3019,7 @@ void wallet2::update_pool_state(bool refreshed) [tx_hash](const std::pair &e) { return e.first == tx_hash; }); if (i != txids.end()) { - process_new_transaction(tx_hash, tx, std::vector(), 0, 0, time(NULL), false, true, tx_entry.double_spend_seen, {}); - m_scanned_pool_txs[0].insert(tx_hash); - if (m_scanned_pool_txs[0].size() > 5000) - { - std::swap(m_scanned_pool_txs[0], m_scanned_pool_txs[1]); - m_scanned_pool_txs[0].clear(); - } + process_txs.push_back(std::make_pair(tx, tx_entry.double_spend_seen)); } else { @@ -3056,6 +3050,24 @@ void wallet2::update_pool_state(bool refreshed) MTRACE("update_pool_state end"); } //---------------------------------------------------------------------------------------------------- +void wallet2::process_pool_state(const std::vector> &txs) +{ + const time_t now = time(NULL); + for (const auto &e: txs) + { + const cryptonote::transaction &tx = e.first; + const bool double_spend_seen = e.second; + const crypto::hash tx_hash = get_transaction_hash(tx); + process_new_transaction(tx_hash, tx, std::vector(), 0, 0, now, false, true, double_spend_seen, {}); + m_scanned_pool_txs[0].insert(tx_hash); + if (m_scanned_pool_txs[0].size() > 5000) + { + std::swap(m_scanned_pool_txs[0], m_scanned_pool_txs[1]); + m_scanned_pool_txs[0].clear(); + } + } +} +//---------------------------------------------------------------------------------------------------- void wallet2::fast_refresh(uint64_t stop_height, uint64_t &blocks_start_height, std::list &short_chain_history, bool force) { std::vector hashes; @@ -3259,6 +3271,14 @@ void wallet2::refresh(bool trusted_daemon, uint64_t start_height, uint64_t & blo }); auto scope_exit_handler_hwdev = epee::misc_utils::create_scope_leave_handler([&](){hwdev.computing_key_images(false);}); + + // get updated pool state first, but do not process those txes just yet, + // since that might cause a password prompt, which would introduce a data + // leak allowing a passive adversary with traffic analysis capability to + // infer when we get an incoming output + std::vector> process_pool_txs; + update_pool_state(process_pool_txs, refreshed); + bool first = true, last = false; while(m_run.load(std::memory_order_relaxed)) { @@ -3389,8 +3409,8 @@ void wallet2::refresh(bool trusted_daemon, uint64_t start_height, uint64_t & blo try { // If stop() is called we don't need to check pending transactions - if (check_pool && m_run.load(std::memory_order_relaxed)) - update_pool_state(refreshed); + if (check_pool && m_run.load(std::memory_order_relaxed) && !process_pool_txs.empty()) + process_pool_state(process_pool_txs); } catch (...) { diff --git a/src/wallet/wallet2.h b/src/wallet/wallet2.h index d99e6e8c5..c86315f7c 100644 --- a/src/wallet/wallet2.h +++ b/src/wallet/wallet2.h @@ -1219,7 +1219,8 @@ private: bool import_key_images(signed_tx_set & signed_tx, size_t offset=0, bool only_selected_transfers=false); crypto::public_key get_tx_pub_key_from_received_outs(const tools::wallet2::transfer_details &td) const; - void update_pool_state(bool refreshed = false); + void update_pool_state(std::vector> &process_txs, bool refreshed = false); + void process_pool_state(const std::vector> &txs); void remove_obsolete_pool_txs(const std::vector &tx_hashes); std::string encrypt(const char *plaintext, size_t len, const crypto::secret_key &skey, bool authenticated = true) const; diff --git a/src/wallet/wallet_rpc_server.cpp b/src/wallet/wallet_rpc_server.cpp index ec21b2897..de501f056 100644 --- a/src/wallet/wallet_rpc_server.cpp +++ b/src/wallet/wallet_rpc_server.cpp @@ -2438,7 +2438,10 @@ namespace tools if (req.pool) { - m_wallet->update_pool_state(); + std::vector> process_txs; + m_wallet->update_pool_state(process_txs); + if (!process_txs.empty()) + m_wallet->process_pool_state(process_txs); std::list> payments; m_wallet->get_unconfirmed_payments(payments, account_index, subaddr_indices); @@ -2518,7 +2521,10 @@ namespace tools } } - m_wallet->update_pool_state(); + std::vector> process_txs; + m_wallet->update_pool_state(process_txs); + if (!process_txs.empty()) + m_wallet->process_pool_state(process_txs); std::list> pool_payments; m_wallet->get_unconfirmed_payments(pool_payments, req.account_index); -- cgit v1.2.3 From 5956beaa15161c3993f38c314a10633d679f0eb8 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Tue, 15 Oct 2019 18:35:11 +0000 Subject: wallet2: fix is_synced checking target height, not height Target height would be appropriate for the daemon, which syncs off other daemons, but the wallet syncs off the daemon it's connected to, and its target is the daemon's current height. --- src/wallet/wallet2.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/wallet/wallet2.cpp') diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp index b23e8525b..7f8b48b8d 100644 --- a/src/wallet/wallet2.cpp +++ b/src/wallet/wallet2.cpp @@ -13212,7 +13212,7 @@ uint64_t wallet2::get_blockchain_height_by_date(uint16_t year, uint8_t month, ui bool wallet2::is_synced() { uint64_t height; - boost::optional result = m_node_rpc_proxy.get_target_height(height); + boost::optional result = m_node_rpc_proxy.get_height(height); if (result && *result != CORE_RPC_STATUS_OK) return false; return get_blockchain_current_height() >= height; -- cgit v1.2.3