cryptonote_basic: copy tx extra payment IDs
What the developer wrote
Authored by Ap4sh
cryptonote_basic: copy tx extra payment IDs
What changed, and why it matters
This commit changes how Monero reads payment IDs embedded in transaction data. Previously, the code used a type-casting pointer trick to read the bytes; now it uses memcpy, which is the safer, standard way to copy raw bytes. The change is defensive and reduces the risk of subtle memory alignment or aliasing problems, but the commit itself does not claim to fix an active security bug.
Treat as a low-risk defensive hardening patch. Review whether the old reinterpret_cast could have caused observable issues on any supported platform, and consider backporting if the project maintains stable branches.
Security signals we found
Replaces reinterpret_cast with memcpy for raw byte copying
Adds unit tests for payment ID parsing and validation
Hardens transaction extra nonce parsing
Evidence from the diff
The patch replaces two reinterpret_cast-based reads of payment_id and encrypted payment_id from tx extra nonce buffers with memcpy. The affected functions are get_payment_id_from_tx_extra_nonce and get_encrypted_payment_id_from_tx_extra_nonce in src/cryptonote_basic/cryptonote_format_utils.cpp. It also adds unit tests covering round-trip serialization and tag/size validation. The change is a hardening measure against potential undefined behavior from unaligned or type-punned access, not a demonstrated exploit fix.
Changed components
src/cryptonote_basic/cryptonote_format_utils.cpptests/unit_tests/cryptonote_format_utils.cppInspect captured patch +58 / −2
diff --git a/src/cryptonote_basic/cryptonote_format_utils.cpp b/src/cryptonote_basic/cryptonote_format_utils.cpp
index e4ef2b1..ce5e5ba 100644
--- a/src/cryptonote_basic/cryptonote_format_utils.cpp
+++ b/src/cryptonote_basic/cryptonote_format_utils.cpp
@@ -806,7 +806,7 @@ namespace cryptonote
return false;
if(TX_EXTRA_NONCE_PAYMENT_ID != extra_nonce[0])
return false;
- payment_id = *reinterpret_cast<const crypto::hash*>(extra_nonce.data() + 1);
+ memcpy(&payment_id, extra_nonce.data() + 1, sizeof(payment_id));
return true;
}
//---------------------------------------------------------------
@@ -816,7 +816,7 @@ namespace cryptonote
return false;
if (TX_EXTRA_NONCE_ENCRYPTED_PAYMENT_ID != extra_nonce[0])
return false;
- payment_id = *reinterpret_cast<const crypto::hash8*>(extra_nonce.data() + 1);
+ memcpy(&payment_id, extra_nonce.data() + 1, sizeof(payment_id));
return true;
}
//---------------------------------------------------------------
diff --git a/tests/unit_tests/cryptonote_format_utils.cpp b/tests/unit_tests/cryptonote_format_utils.cpp
index dc9eb0d..1b15305 100644
--- a/tests/unit_tests/cryptonote_format_utils.cpp
+++ b/tests/unit_tests/cryptonote_format_utils.cpp
@@ -104,6 +104,62 @@ TEST(cn_format_utils, add_extra_nonce_to_tx_extra)
}
}
+TEST(cn_format_utils, payment_id_tx_extra_nonce_roundtrip)
+{
+ const crypto::hash payment_id = crypto::rand<crypto::hash>();
+ std::string extra_nonce;
+ cryptonote::set_payment_id_to_tx_extra_nonce(extra_nonce, payment_id);
+ ASSERT_EQ(sizeof(payment_id) + 1, extra_nonce.size());
+
+ crypto::hash parsed_payment_id = crypto::null_hash;
+ ASSERT_TRUE(cryptonote::get_payment_id_from_tx_extra_nonce(extra_nonce, parsed_payment_id));
+ EXPECT_EQ(payment_id, parsed_payment_id);
+
+ const crypto::hash8 encrypted_payment_id = crypto::rand<crypto::hash8>();
+ std::string encrypted_extra_nonce;
+ cryptonote::set_encrypted_payment_id_to_tx_extra_nonce(encrypted_extra_nonce, encrypted_payment_id);
+ ASSERT_EQ(sizeof(encrypted_payment_id) + 1, encrypted_extra_nonce.size());
+
+ crypto::hash8 parsed_encrypted_payment_id = crypto::null_hash8;
+ ASSERT_TRUE(cryptonote::get_encrypted_payment_id_from_tx_extra_nonce(encrypted_extra_nonce, parsed_encrypted_payment_id));
+ EXPECT_EQ(encrypted_payment_id, parsed_encrypted_payment_id);
+}
+
+TEST(cn_format_utils, payment_id_tx_extra_nonce_rejects_wrong_tag_or_size)
+{
+ const crypto::hash payment_id = crypto::rand<crypto::hash>();
+ std::string extra_nonce;
+ cryptonote::set_payment_id_to_tx_extra_nonce(extra_nonce, payment_id);
+
+ crypto::hash parsed_payment_id = crypto::null_hash;
+ std::string wrong_tag = extra_nonce;
+ wrong_tag[0] = TX_EXTRA_NONCE_ENCRYPTED_PAYMENT_ID;
+ EXPECT_FALSE(cryptonote::get_payment_id_from_tx_extra_nonce(wrong_tag, parsed_payment_id));
+
+ std::string wrong_size = extra_nonce;
+ wrong_size.resize(wrong_size.size() - 1);
+ EXPECT_FALSE(cryptonote::get_payment_id_from_tx_extra_nonce(wrong_size, parsed_payment_id));
+ wrong_size = extra_nonce;
+ wrong_size.push_back('\0');
+ EXPECT_FALSE(cryptonote::get_payment_id_from_tx_extra_nonce(wrong_size, parsed_payment_id));
+
+ const crypto::hash8 encrypted_payment_id = crypto::rand<crypto::hash8>();
+ std::string encrypted_extra_nonce;
+ cryptonote::set_encrypted_payment_id_to_tx_extra_nonce(encrypted_extra_nonce, encrypted_payment_id);
+
+ crypto::hash8 parsed_encrypted_payment_id = crypto::null_hash8;
+ wrong_tag = encrypted_extra_nonce;
+ wrong_tag[0] = TX_EXTRA_NONCE_PAYMENT_ID;
+ EXPECT_FALSE(cryptonote::get_encrypted_payment_id_from_tx_extra_nonce(wrong_tag, parsed_encrypted_payment_id));
+
+ wrong_size = encrypted_extra_nonce;
+ wrong_size.resize(wrong_size.size() - 1);
+ EXPECT_FALSE(cryptonote::get_encrypted_payment_id_from_tx_extra_nonce(wrong_size, parsed_encrypted_payment_id));
+ wrong_size = encrypted_extra_nonce;
+ wrong_size.push_back('\0');
+ EXPECT_FALSE(cryptonote::get_encrypted_payment_id_from_tx_extra_nonce(wrong_size, parsed_encrypted_payment_id));
+}
+
TEST(cn_format_utils, add_mm_merkle_root_to_tx_extra)
{
const std::vector<std::uint64_t> depths{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 63, 64, 127, 128, 16383, 16384};
Why this scored 29/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.