From c8e0c11263fd58c94841c223c30ebcea7474d4af Mon Sep 17 00:00:00 2001 From: Tore-tto Date: Fri, 21 Aug 2026 18:00:49 +0530 Subject: [PATCH 1/2] Fix unguarded worker_index read and harden POS commit-reveal -master_node_voting: the signature-failure log read quorum.workers[vote.state_change.worker_index] for all vote types, but worker_index only exists on obligation votes -- for a checkpoint vote it aliases attacker-controlled bytes and indexes an empty vector. Guard it by vote type. Also re-enable the two success logs at MTRACE (level 3) instead of leaving them commented out. -pos: only accept a random value reveal from a quorum position that actually committed a hash, so the commit-reveal binding is enforced locally rather than relying on the stage timeout. -pos: drop the dead std::sample in signature selection and fix the comment -- the loop always used the first N. --- src/cryptonote_core/master_node_voting.cpp | 22 +++++++++--- src/cryptonote_core/pos.cpp | 39 +++++++++++++--------- 2 files changed, 42 insertions(+), 19 deletions(-) diff --git a/src/cryptonote_core/master_node_voting.cpp b/src/cryptonote_core/master_node_voting.cpp index 76d9919851f..cee43f75771 100755 --- a/src/cryptonote_core/master_node_voting.cpp +++ b/src/cryptonote_core/master_node_voting.cpp @@ -520,20 +520,34 @@ namespace master_nodes result = crypto::check_signature(hash, key, vote.signature); if (result){ - /*MDEBUG("Signature accepted for " << vote.type << " voter " << vote.index_in_group << "/" << key + MTRACE("Signature accepted for " << vote.type << " voter " << vote.index_in_group << "/" << key << (vote.type == quorum_type::obligations ? " voting for worker " + std::to_string( vote.state_change.worker_index) : "") << " at height " << vote.block_height); - */ + if(vote.type == quorum_type::obligations){ - //MDEBUG("Signature accepted for " << quorum.workers[vote.state_change.worker_index]); + MTRACE("Signature accepted for " << quorum.workers[vote.state_change.worker_index]); } } else { vvc.m_signature_not_valid = true; - MDEBUG("Signature not accepted for MN " << quorum.workers[vote.state_change.worker_index]); + + // NOTE: `worker_index` only exists on obligation votes -- it is the inactive member of the + // vote's union for every other type, so reading it here for e.g. a checkpoint vote both is + // UB and yields attacker-controlled bytes taken from the vote's block hash. Checkpoint + // quorums also leave `workers` empty, and this path never bounds-checks the index (only the + // obligations branch above calls bounds_check_worker_index), so an unguarded read here is an + // out-of-bounds access on an empty vector reachable from any peer that sends us a vote. + // + // Reaching this point on an obligations vote does imply the index was bounds-checked, so + // that branch is safe; every other vote type must log via its validator instead. + if (vote.type == quorum_type::obligations) + MDEBUG("Signature not accepted for MN " << quorum.workers[vote.state_change.worker_index]); + else + MDEBUG("Signature not accepted for " << vote.type << " voter " << vote.index_in_group + << "/" << key << " at height " << vote.block_height); } return result; diff --git a/src/cryptonote_core/pos.cpp b/src/cryptonote_core/pos.cpp index 5f9ee02b97d..762efa8b0b8 100755 --- a/src/cryptonote_core/pos.cpp +++ b/src/cryptonote_core/pos.cpp @@ -672,16 +672,21 @@ void POS::handle_message(void *quorumnet_state, POS::message const &msg) auto &value = quorum[msg.quorum_position]; if (value) return; - if (auto const &hash = context.transient.random_value_hashes.wait.data[msg.quorum_position]; hash) + auto const &hash = context.transient.random_value_hashes.wait.data[msg.quorum_position]; + if (!hash) { - auto derived = blake2b_hash(msg.random_value.value.data, sizeof(msg.random_value.value.data)); - if (derived != *hash) - { - MTRACE(log_prefix(context) << "Dropping " << msg_source_string(context, msg) - << ". Rederived random value hash " << derived << " does not match original hash " - << *hash); - return; - } + MTRACE(log_prefix(context) << "Dropping " << msg_source_string(context, msg) + << ". No random value hash was committed for this quorum position"); + return; + } + + auto derived = blake2b_hash(msg.random_value.value.data, sizeof(msg.random_value.value.data)); + if (derived != *hash) + { + MTRACE(log_prefix(context) << "Dropping " << msg_source_string(context, msg) + << ". Rederived random value hash " << derived << " does not match original hash " + << *hash); + return; } value = msg.random_value.value; @@ -1653,9 +1658,16 @@ round_state send_and_wait_for_signed_blocks(round_context &context, master_nodes if (!enforce_validator_participation_and_timeouts(context, stage, node_list, timed_out, all_received)) return goto_preparing_for_next_round(context); - // Select signatures randomly so we don't always just take the first N required signatures. - // Then sort just the first N required signatures, so signatures are added - // to the block in sorted order, but were chosen randomly. + // NOTE: Collect the quorum positions that signed, in ascending order, and take the first + // POS_BLOCK_REQUIRED_SIGNATURES of them. + // + // Any subset of the participating validators is equally valid here: verification only + // requires exactly N signatures, given in ascending order, from validators set in the + // block's participation bitset (see verify_quorum_signatures). Which subset we pick has no + // further effect either -- a validator's participation is scored from POS.validator_bitset, + // not from the signatures that made it into the block, so leaving a validator out does not + // count against it. Quorum positions are themselves re-randomised every block, so taking + // the lowest N does not favour any particular master node over time. std::array indices = {}; size_t indices_count = 0; @@ -1664,10 +1676,7 @@ round_state send_and_wait_for_signed_blocks(round_context &context, master_nodes if (quorum[index]) indices[indices_count++] = index; - // Random select from first 'N' POS_BLOCK_REQUIRED_SIGNATURES from indices_count entries. assert(indices_count >= master_nodes::POS_BLOCK_REQUIRED_SIGNATURES); - std::array selected = {}; - std::sample(indices.begin(), indices.begin() + indices_count, selected.begin(), selected.size(), tools::rng); // Add Signatures cryptonote::block &final_block = context.transient.signed_block.final_block; From 1f4912ffb02d3b30741ff08c8f5afb1d3faa74b9 Mon Sep 17 00:00:00 2001 From: Tore-tto Date: Tue, 25 Aug 2026 19:48:12 +0530 Subject: [PATCH 2/2] Fix out-of-bounds read in light wallet amount lookup When the requested amount is missing from the light wallet server's response, the search loop in light_wallet_get_outs exits with amount_key == amount_outs.size(). The error message then indexes amount_outs at that position, reading one past the end of the vector. Report `amount` instead: it is the value being searched for, and on the success path it equals amount_outs[amount_key].amount by definition. --- src/wallet/wallet2.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp index 0664dbc6d2b..d24c0b4342f 100755 --- a/src/wallet/wallet2.cpp +++ b/src/wallet/wallet2.cpp @@ -9802,7 +9802,7 @@ void wallet2::light_wallet_get_outs(std::vector(ores.amount_outs[amount_key].amount) + " not found" ); + THROW_WALLET_EXCEPTION_IF(!found_amount , error::wallet_internal_error, "Outputs for amount " + boost::lexical_cast(amount) + " not found" ); LOG_PRINT_L2("Index " << i << "/" << light_wallet_requested_outputs_count << ": idx " << ores.amount_outs[amount_key].outputs[i].global_index << " (real " << td.m_global_output_index << "), unlocked " << "(always in light)" << ", key " << ores.amount_outs[0].outputs[i].public_key);