cryptonote_basic: fix add_extra_nonce_to_tx_extra() length
What changed, and why it matters
This commit fixes a bug in how Monero builds the 'extra nonce' field inside transaction metadata. Previously, the code always reserved only 2 bytes for the length header, which is wrong for larger nonces because the protocol uses a variable-length integer (varint) that can take more than 1 byte. The patch now correctly calculates how many bytes the length needs and writes it as a proper varint. The bug could corrupt transaction extra data or cause parsing failures when nonces larger than 127 bytes are used, but the maximum allowed nonce size (255 bytes) limits the damage. There is no public statement from the Monero Project calling this a security issue, and no independent researcher is credited.
Treat as a correctness fix and include in normal release testing. Review callers of add_extra_nonce_to_tx_extra() to confirm no other code relies on the old fixed-size layout. Run the new unit tests. If this function is reachable from network-parsed inputs, consider a lightweight audit of tx_extra parsing for similar varint length assumptions elsewhere.
Security signals we found
Buffer sizing bug in transaction metadata serialization
Incorrect use of fixed 1-byte length where variable-length integer is required
Potential tx_extra corruption or parse failure for nonce sizes > 127
New unit tests added to cover varint length calculation and tx_extra helpers
No vendor security advisory or CVE referenced in commit
Evidence from the diff
The function add_extra_nonce_to_tx_extra() in cryptonote_format_utils.cpp previously assumed the length of extra_nonce could always be encoded in a single byte, so it resized tx_extra by 2 + extra_nonce.size() (1 tag byte + 1 length byte + payload). Monero’s tx extra format, however, uses a variable-length integer for the length field. For nonce sizes above 127, the length requires 2 varint bytes; for the maximum allowed 255 bytes, it still fits in 2 bytes, but the old code wrote a raw uint8_t size rather than a varint. The patch adds tools::get_varint_byte_size() to compute the encoded length, resizes the buffer to 1 (tag) + len_varint_bytes + payload, and writes the length with tools::write_varint(). It also adds unit tests for the nonce helper, merge-mining tag helpers, and varint byte-size computation. The change is a correctness fix; the worst direct outcome is malformed tx_extra serialization or parsing failure for larger nonces, with no obvious remote code execution or consensus-split path visible from the diff alone.
Changed components
src/cryptonote_basic/cryptonote_format_utils.cppsrc/common/varint.htests/unit_tests/cryptonote_format_utils.cpptests/unit_tests/varint.cpptests/unit_tests/CMakeLists.txtInspect captured patch +362 / −4
diff --git a/src/common/varint.h b/src/common/varint.h
index dfe9805..a69db80 100644
--- a/src/common/varint.h
+++ b/src/common/varint.h
@@ -125,4 +125,14 @@ namespace tools {
int read_varint(InputIt &&first, InputIt &&last, T &i) {
return read_varint<std::numeric_limits<T>::digits>(std::forward<InputIt>(first), std::forward<InputIt>(last), i);
}
+
+ template <typename T, typename = std::enable_if_t<std::is_integral_v<T> && std::is_unsigned_v<T>>>
+ constexpr std::size_t get_varint_byte_size(T val) {
+ std::size_t bytes = 0;
+ do {
+ ++bytes;
+ val >>= 7;
+ } while (val);
+ return bytes;
+ }
}
diff --git a/src/cryptonote_basic/cryptonote_format_utils.cpp b/src/cryptonote_basic/cryptonote_format_utils.cpp
index d3b71d3..7e21d78 100644
--- a/src/cryptonote_basic/cryptonote_format_utils.cpp
+++ b/src/cryptonote_basic/cryptonote_format_utils.cpp
@@ -718,15 +718,19 @@ namespace cryptonote
{
CHECK_AND_ASSERT_MES(extra_nonce.size() <= TX_EXTRA_NONCE_MAX_COUNT, false, "extra nonce could be 255 bytes max");
size_t start_pos = tx_extra.size();
- tx_extra.resize(tx_extra.size() + 2 + extra_nonce.size());
+ const std::size_t len_varint_bytes = tools::get_varint_byte_size(extra_nonce.size());
+ tx_extra.resize(tx_extra.size() + 1 + len_varint_bytes + extra_nonce.size());
//write tag
tx_extra[start_pos] = TX_EXTRA_NONCE;
//write len
++start_pos;
- tx_extra[start_pos] = static_cast<uint8_t>(extra_nonce.size());
+ unsigned char * vp = tx_extra.data() + start_pos;
+ tools::write_varint(vp, extra_nonce.size());
+ assert(vp == tx_extra.data() + tx_extra.size() - extra_nonce.size());
//write data
- ++start_pos;
- memcpy(&tx_extra[start_pos], extra_nonce.data(), extra_nonce.size());
+ start_pos += len_varint_bytes;
+ if (!extra_nonce.empty())
+ memcpy(&tx_extra[start_pos], extra_nonce.data(), extra_nonce.size());
return true;
}
//---------------------------------------------------------------
diff --git a/tests/unit_tests/CMakeLists.txt b/tests/unit_tests/CMakeLists.txt
index a25ea07..8bd5e15 100644
--- a/tests/unit_tests/CMakeLists.txt
+++ b/tests/unit_tests/CMakeLists.txt
@@ -42,6 +42,7 @@ set(unit_tests_sources
checkpoints.cpp
command_line.cpp
crypto.cpp
+ cryptonote_format_utils.cpp
decompose_amount_into_digits.cpp
device.cpp
difficulty.cpp
diff --git a/tests/unit_tests/cryptonote_format_utils.cpp b/tests/unit_tests/cryptonote_format_utils.cpp
new file mode 100644
index 0000000..dc9eb0d
--- /dev/null
+++ b/tests/unit_tests/cryptonote_format_utils.cpp
@@ -0,0 +1,245 @@
+// Copyright (c) 2025, The Monero Project
+//
+// All rights reserved.
+//
+// Redistribution and use in source and binary forms, with or without modification, are
+// permitted provided that the following conditions are met:
+//
+// 1. Redistributions of source code must retain the above copyright notice, this list of
+// conditions and the following disclaimer.
+//
+// 2. Redistributions in binary form must reproduce the above copyright notice, this list
+// of conditions and the following disclaimer in the documentation and/or other
+// materials provided with the distribution.
+//
+// 3. Neither the name of the copyright holder nor the names of its contributors may be
+// used to endorse or promote products derived from this software without specific
+// prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY
+// EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+// MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+// THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+// PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+// INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+// STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+// THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+#include "gtest/gtest.h"
+
+#include "crypto/generators.h"
+#include "cryptonote_basic/cryptonote_format_utils.h"
+#include "serialization/binary_utils.h"
+#include "serialization/string.h"
+
+TEST(cn_format_utils, add_extra_nonce_to_tx_extra)
+{
+ static constexpr std::size_t max_nonce_size = TX_EXTRA_NONCE_MAX_COUNT + 1; // we *can* test higher if desired
+
+ for (int empty_prefix = 0; empty_prefix < 2; ++empty_prefix)
+ {
+ std::vector<std::uint8_t> extra_prefix;
+ if (!empty_prefix)
+ cryptonote::add_tx_pub_key_to_extra(extra_prefix, crypto::get_H());
+
+ std::vector<std::uint8_t> extra;
+ std::string nonce;
+ std::vector<cryptonote::tx_extra_field> tx_extra_fields;
+ extra.reserve(extra_prefix.size() + max_nonce_size + 1 + 10);
+ nonce.reserve(max_nonce_size);
+ tx_extra_fields.reserve(2);
+ for (std::size_t nonce_size = 0; nonce_size <= max_nonce_size; ++nonce_size)
+ {
+ extra = extra_prefix;
+ nonce.resize(nonce_size);
+ if (nonce.size())
+ memset(&nonce[0], '%', nonce.size());
+ tx_extra_fields.clear();
+
+ const std::size_t expected_extra_size = extra_prefix.size() + 1
+ + tools::get_varint_byte_size(nonce_size) + nonce_size;
+ const bool expected_success = nonce_size <= TX_EXTRA_NONCE_MAX_COUNT;
+
+ // add nonce and do detailed test
+ const bool add_success = cryptonote::add_extra_nonce_to_tx_extra(extra, nonce);
+ ASSERT_EQ(expected_success, add_success);
+ if (!expected_success)
+ continue;
+ ASSERT_EQ(expected_extra_size, extra.size());
+ ASSERT_EQ(0, memcmp(extra_prefix.data(), extra.data(), extra_prefix.size()));
+ const std::uint8_t *p = extra.data() + extra_prefix.size();
+ ASSERT_EQ(TX_EXTRA_NONCE, *p);
+ ++p;
+ std::size_t read_nonce_size = 0;
+ const int varint_size = tools::read_varint((const uint8_t*)(p), // copy p
+ (const uint8_t*) extra.data() + extra.size(),
+ read_nonce_size);
+ ASSERT_EQ(tools::get_varint_byte_size(nonce_size), varint_size);
+ p += varint_size;
+ for (std::size_t i = 0; i < nonce_size; ++i)
+ {
+ ASSERT_EQ('%', *p);
+ ++p;
+ }
+ ASSERT_EQ(extra.data() + extra.size(), p);
+
+ // do integration test with higher-level tx_extra parsing code
+ ASSERT_TRUE(cryptonote::parse_tx_extra(extra, tx_extra_fields));
+ if (empty_prefix)
+ {
+ ASSERT_EQ(1, tx_extra_fields.size());
+ const auto &nonce_field = boost::get<cryptonote::tx_extra_nonce>(tx_extra_fields.at(0));
+ ASSERT_EQ(nonce, nonce_field.nonce);
+ }
+ else
+ {
+ ASSERT_EQ(2, tx_extra_fields.size());
+ const auto &pk_field = boost::get<cryptonote::tx_extra_pub_key>(tx_extra_fields.at(0));
+ ASSERT_EQ(crypto::get_H(), pk_field.pub_key);
+ const auto &nonce_field = boost::get<cryptonote::tx_extra_nonce>(tx_extra_fields.at(1));
+ ASSERT_EQ(nonce, nonce_field.nonce);
+ }
+ }
+ }
+}
+
+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};
+
+ const crypto::hash mm_merkle_root = crypto::rand<crypto::hash>();
+
+ for (int empty_prefix = 0; empty_prefix < 2; ++empty_prefix)
+ {
+ std::vector<std::uint8_t> extra_prefix;
+ if (!empty_prefix)
+ cryptonote::add_tx_pub_key_to_extra(extra_prefix, crypto::get_H());
+
+ std::vector<std::uint8_t> extra;
+ std::vector<cryptonote::tx_extra_field> tx_extra_fields;
+ extra.reserve(extra_prefix.size() + 1 + 1 + 10 + 32);
+ tx_extra_fields.reserve(2);
+ for (std::uint64_t mm_merkle_tree_depth : depths)
+ {
+ extra = extra_prefix;
+ tx_extra_fields.clear();
+
+ const std::size_t expected_extra_size = extra_prefix.size() + 1 + 1
+ + tools::get_varint_byte_size(mm_merkle_tree_depth) + 32;
+
+ // add nonce and do detailed test
+ const bool add_success = cryptonote::add_mm_merkle_root_to_tx_extra(extra, mm_merkle_root, mm_merkle_tree_depth);
+ ASSERT_TRUE(add_success);
+ ASSERT_EQ(expected_extra_size, extra.size());
+ ASSERT_EQ(0, memcmp(extra_prefix.data(), extra.data(), extra_prefix.size()));
+ const std::uint8_t *p = extra.data() + extra_prefix.size();
+ ASSERT_EQ(TX_EXTRA_MERGE_MINING_TAG, *p);
+ ++p;
+ ASSERT_EQ(32 + tools::get_varint_byte_size(mm_merkle_tree_depth), *p);
+ ++p;
+ std::uint64_t read_depth = 0;
+ const int varint_size = tools::read_varint((const uint8_t*)(p), // copy p
+ (const uint8_t*) extra.data() + extra.size(),
+ read_depth);
+ ASSERT_EQ(tools::get_varint_byte_size(mm_merkle_tree_depth), varint_size);
+ ASSERT_EQ(mm_merkle_tree_depth, read_depth);
+ p += varint_size;
+ ASSERT_EQ(0, memcmp(p, mm_merkle_root.data, sizeof(mm_merkle_root)));
+ p += sizeof(crypto::hash);
+ ASSERT_EQ(extra.data() + extra.size(), p);
+
+ // do integration test with higher-level tx_extra parsing code
+ ASSERT_TRUE(cryptonote::parse_tx_extra(extra, tx_extra_fields));
+ if (empty_prefix)
+ {
+ ASSERT_EQ(1, tx_extra_fields.size());
+ const auto &mm_field = boost::get<cryptonote::tx_extra_merge_mining_tag>(tx_extra_fields.at(0));
+ ASSERT_EQ(mm_merkle_root, mm_field.merkle_root);
+ ASSERT_EQ(mm_merkle_tree_depth, mm_field.depth);
+ }
+ else
+ {
+ ASSERT_EQ(2, tx_extra_fields.size());
+ const auto &pk_field = boost::get<cryptonote::tx_extra_pub_key>(tx_extra_fields.at(0));
+ ASSERT_EQ(crypto::get_H(), pk_field.pub_key);
+ const auto &mm_field = boost::get<cryptonote::tx_extra_merge_mining_tag>(tx_extra_fields.at(1));
+ ASSERT_EQ(mm_merkle_root, mm_field.merkle_root);
+ ASSERT_EQ(mm_merkle_tree_depth, mm_field.depth);
+ }
+ }
+ }
+}
+
+TEST(cn_format_utils, tx_extra_merge_mining_tag_store_load)
+{
+ const std::vector<std::uint64_t> depths{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 63, 64, 127, 128, 16383, 16384};
+
+ const crypto::hash mm_merkle_root = crypto::rand<crypto::hash>();
+
+ for (int empty_prefix = 0; empty_prefix < 2; ++empty_prefix)
+ {
+ std::vector<std::uint8_t> extra_prefix;
+ if (!empty_prefix)
+ cryptonote::add_tx_pub_key_to_extra(extra_prefix, crypto::get_H());
+
+ std::vector<std::uint8_t> extra;
+ std::vector<cryptonote::tx_extra_field> tx_extra_fields;
+ extra.reserve(extra_prefix.size() + 1 + 1 + 10 + 32);
+ tx_extra_fields.reserve(2);
+ for (std::uint64_t mm_merkle_tree_depth : depths)
+ {
+ extra = extra_prefix;
+ tx_extra_fields.clear();
+
+ const std::size_t expected_extra_size = extra_prefix.size() + 1 + 1
+ + tools::get_varint_byte_size(mm_merkle_tree_depth) + 32;
+
+ // add nonce and do detailed test
+ cryptonote::tx_extra_merge_mining_tag mm;
+ mm.depth = mm_merkle_tree_depth;
+ mm.merkle_root = mm_merkle_root;
+ cryptonote::tx_extra_field extra_field = mm;
+ std::string mm_blob;
+ ASSERT_TRUE(::serialization::dump_binary(extra_field, mm_blob));
+ extra.resize(extra.size() + mm_blob.size());
+ memcpy(extra.data() + extra.size() - mm_blob.size(), mm_blob.data(), mm_blob.size());
+ ASSERT_EQ(expected_extra_size, extra.size());
+ ASSERT_EQ(0, memcmp(extra_prefix.data(), extra.data(), extra_prefix.size()));
+ const std::uint8_t *p = extra.data() + extra_prefix.size();
+ ASSERT_EQ(TX_EXTRA_MERGE_MINING_TAG, *p);
+ ++p;
+ ASSERT_EQ(32 + tools::get_varint_byte_size(mm_merkle_tree_depth), *p);
+ ++p;
+ std::uint64_t read_depth = 0;
+ const int varint_size = tools::read_varint((const uint8_t*)(p), // copy p
+ (const uint8_t*) extra.data() + extra.size(),
+ read_depth);
+ ASSERT_EQ(tools::get_varint_byte_size(mm_merkle_tree_depth), varint_size);
+ ASSERT_EQ(mm_merkle_tree_depth, read_depth);
+ p += varint_size;
+ ASSERT_EQ(0, memcmp(p, mm_merkle_root.data, sizeof(mm_merkle_root)));
+ p += sizeof(crypto::hash);
+ ASSERT_EQ(extra.data() + extra.size(), p);
+
+ // do integration test with higher-level tx_extra parsing code
+ ASSERT_TRUE(cryptonote::parse_tx_extra(extra, tx_extra_fields));
+ if (empty_prefix)
+ {
+ ASSERT_EQ(1, tx_extra_fields.size());
+ const auto &mm_field = boost::get<cryptonote::tx_extra_merge_mining_tag>(tx_extra_fields.at(0));
+ ASSERT_EQ(mm_merkle_root, mm_field.merkle_root);
+ ASSERT_EQ(mm_merkle_tree_depth, mm_field.depth);
+ }
+ else
+ {
+ ASSERT_EQ(2, tx_extra_fields.size());
+ const auto &pk_field = boost::get<cryptonote::tx_extra_pub_key>(tx_extra_fields.at(0));
+ ASSERT_EQ(crypto::get_H(), pk_field.pub_key);
+ const auto &mm_field = boost::get<cryptonote::tx_extra_merge_mining_tag>(tx_extra_fields.at(1));
+ ASSERT_EQ(mm_merkle_root, mm_field.merkle_root);
+ ASSERT_EQ(mm_merkle_tree_depth, mm_field.depth);
+ }
+ }
+ }
+}
diff --git a/tests/unit_tests/varint.cpp b/tests/unit_tests/varint.cpp
index 5deac16..9654fe0 100644
--- a/tests/unit_tests/varint.cpp
+++ b/tests/unit_tests/varint.cpp
@@ -62,3 +62,101 @@ TEST(varint, equal)
ASSERT_TRUE(idx2 == idx);
}
}
+
+TEST(varint, max_uint64_t_bytes)
+{
+ unsigned char bytes[100]{};
+ unsigned char *p = bytes;
+ tools::write_varint(p, std::numeric_limits<std::uint64_t>::max());
+ std::size_t i = 0;
+ while (bytes[i]) ++i;
+ ASSERT_EQ(10, i); // tests size of max 64-bit uint
+ ASSERT_EQ(bytes + i, p); // tests iterator is modified in-place
+}
+
+template <typename T>
+static void subtest_varint_byte_size_for_single_value(const std::size_t expected_bytes, const T val)
+{
+ static_assert(std::is_integral_v<T> && std::is_unsigned_v<T>);
+ static constexpr int DIGITS = std::numeric_limits<T>::digits;
+ static_assert(DIGITS == CHAR_BIT * sizeof(T));
+ static_assert(sizeof(T));
+ static_assert(sizeof(T) <= 8); // 64-bit or less
+
+ unsigned char bytes[100]{};
+ unsigned char *p = bytes;
+ tools::write_varint(p, val);
+ ASSERT_EQ(expected_bytes, p - bytes);
+ ASSERT_EQ(expected_bytes, tools::get_varint_byte_size(val));
+}
+
+template <typename T>
+static void subtest_varint_byte_size_for_type()
+{
+ static_assert(std::is_integral_v<T> && std::is_unsigned_v<T>);
+ static constexpr int DIGITS = std::numeric_limits<T>::digits;
+ static_assert(DIGITS == CHAR_BIT * sizeof(T));
+ static_assert(sizeof(T));
+
+ for (T n = 0; n < 128; ++n)
+ {
+ subtest_varint_byte_size_for_single_value(1, n);
+ }
+
+ for (T n = 128; n < 255; ++n)
+ {
+ subtest_varint_byte_size_for_single_value(2, n);
+ }
+
+ subtest_varint_byte_size_for_single_value(2, T{255});
+
+ if constexpr (DIGITS > 8)
+ {
+ static_assert(DIGITS >= 16);
+ for (T n = 256; n < 16384; ++n)
+ {
+ subtest_varint_byte_size_for_single_value(2, n);
+ }
+
+ for (T n = 16384; n < 65535; ++n)
+ {
+ subtest_varint_byte_size_for_single_value(3, n);
+ }
+
+ subtest_varint_byte_size_for_single_value(3, T{65535});
+
+ if constexpr (DIGITS > 16)
+ {
+ static_assert(DIGITS >= 32);
+ subtest_varint_byte_size_for_single_value(3, T{2097151});
+ subtest_varint_byte_size_for_single_value(4, T{2097152});
+ subtest_varint_byte_size_for_single_value(4, T{268435455});
+ subtest_varint_byte_size_for_single_value(5, T{268435456});
+ subtest_varint_byte_size_for_single_value(5, T{4294967295});
+
+ if constexpr (DIGITS > 32)
+ {
+ static_assert(DIGITS >= 64);
+ subtest_varint_byte_size_for_single_value(5, T{34359738367});
+ subtest_varint_byte_size_for_single_value(6, T{34359738368});
+ subtest_varint_byte_size_for_single_value(6, T{4398046511103});
+ subtest_varint_byte_size_for_single_value(7, T{4398046511104});
+ subtest_varint_byte_size_for_single_value(7, T{562949953421311});
+ subtest_varint_byte_size_for_single_value(8, T{562949953421312});
+ subtest_varint_byte_size_for_single_value(8, T{72057594037927935});
+ subtest_varint_byte_size_for_single_value(9, T{72057594037927936});
+ subtest_varint_byte_size_for_single_value(9, T{9223372036854775807});
+ subtest_varint_byte_size_for_single_value(10, T{9223372036854775808ull});
+ subtest_varint_byte_size_for_single_value(10, T{18446744073709551615ull});
+ }
+ }
+ }
+};
+
+TEST(varint, get_varint_byte_size)
+{
+ subtest_varint_byte_size_for_type<std::uint8_t>();
+ subtest_varint_byte_size_for_type<std::uint16_t>();
+ subtest_varint_byte_size_for_type<std::uint32_t>();
+ subtest_varint_byte_size_for_type<std::uint64_t>();
+}
Why this scored 59/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.