Skip to content

fix(tx_pool): guard master node lookup before key_image_unlock dereference#193

Open
raw391 wants to merge 1 commit into
Beldex-Coin:devfrom
raw391:fix/tx-pool-mn-lookup-guard
Open

fix(tx_pool): guard master node lookup before key_image_unlock dereference#193
raw391 wants to merge 1 commit into
Beldex-Coin:devfrom
raw391:fix/tx-pool-mn-lookup-guard

Conversation

@raw391

@raw391 raw391 commented Jun 3, 2026

Copy link
Copy Markdown

In tx_pool::add_tx, the key_image_unlock handler at cryptonote_core/tx_pool.cpp:264-298 calls get_master_node_details(mnode_key) without first verifying that mnode_key exists in the master-node registry. mnode_key is read from the incoming transactions extra field (get_master_node_pubkey_from_tx_extra) and is attacker-controlled. The accessor at master_node_list.cpp:876-880 dereferences find() unconditionally:

master_node_info master_node_list::state_t::get_master_node_details(crypto::public_key mnode_key)
{
  auto it = master_nodes_infos.find(mnode_key);
  return *it->second;
}

For an unknown mnode_key the iterator equals end() and the dereference is undefined behaviour. A peer that relays a txtype::key_image_unlock transaction carrying an arbitrary master node pubkey reaches this path before any expensive validation.

Patch adds the existence check at the caller via the existing is_master_node wrapper (master_node_list.h:433), which is already used elsewhere in the codebase:

         uint64_t block_height = m_blockchain.get_current_blockchain_height();
+        if (!m_blockchain.get_master_node_list().is_master_node(mnode_key))
+          return false;
         const master_nodes::master_node_info &node_info = m_blockchain.get_master_node_list().get_master_node_details(mnode_key);

It also makes the accessor throw on a missing key, matching the early-return pattern at master_node_list.cpp:660-661:

   master_node_info master_node_list::state_t::get_master_node_details(crypto::public_key mnode_key)
   {
     auto it = master_nodes_infos.find(mnode_key);
+    if (it == master_nodes_infos.end())
+      throw std::invalid_argument("get_master_node_details: master node not found");
     return *it->second;
   }

The caller guard is the actual fix. The accessor throw is so other call sites of get_master_node_details are not left with the same shape.

…rence

In tx_pool::add_tx, the key_image_unlock handler calls
get_master_node_details(mnode_key) without first verifying that
mnode_key is a registered master node. The accessor dereferences
the iterator returned by find() unconditionally, so an unknown
mnode_key causes a segfault. mnode_key is read from the incoming
transactions tx_extra, so it is attacker-controlled.

Patch (a) checks existence at the caller via is_master_node()
before the lookup, and (b) makes the accessor throw on a missing
key so other callsites of get_master_node_details cant hit the
same shape. The caller already handles thrown exceptions as
failed verification.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants