Skip to content

Prevent deserialization memory exhaustion and ReadVarInt infinite loop (CWE-400) #105

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hairetikos wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Prevent deserialization memory exhaustion and ReadVarInt infinite loop (CWE-400) #105

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 45 additions & 51 deletions src/serialize.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,15 @@

static const unsigned int MAX_SIZE = 0x02000000;

/**
* Maximum element count accepted during deserialization of untrusted data.
* Set to 2 MiB (matching MAX_PROTOCOL_MESSAGE_LENGTH from net.h) since no
* single P2P message can carry more data than this on the wire.
* Disk serialization (SER_DISK) callers can bypass this via the original
* ReadCompactSize which still uses MAX_SIZE.
*/
static const unsigned int MAX_DESER_ELEMENTS = 2 * 1024 * 1024;
Comment on lines +32 to +39
Copy link
Copy Markdown

@VictorLux VictorLux Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FINDING-002 + FINDING-007: Rename to MAX_DESER_ELEMENT_COUNT (it's element count, not bytes), clarify the comment about units mismatch, and add MAX_EQUIHASH_SOLUTION_SIZE to prevent 320 MiB nSolution allocation via malicious headers messages (160 headers x 2 MiB each).

Suggested change
/**
* Maximum element count accepted during deserialization of untrusted data.
* Set to 2 MiB (matching MAX_PROTOCOL_MESSAGE_LENGTH from net.h) since no
* single P2P message can carry more data than this on the wire.
* Disk serialization (SER_DISK) callers can bypass this via the original
* ReadCompactSize which still uses MAX_SIZE.
*/
static const unsigned int MAX_DESER_ELEMENTS = 2 * 1024 * 1024;
/**
* Maximum element count accepted during deserialization of untrusted data.
* NOTE: This is an ELEMENT COUNT, not a byte count. For byte-sized
* containers (vector<unsigned char>, basic_string<char>) the numeric value
* matches MAX_PROTOCOL_MESSAGE_LENGTH (net.h). For non-byte containers
* (vector<CTransaction>, map<K,V>, etc.) each element is larger than one
* byte, so the effective byte cap is higher — the outer
* MAX_PROTOCOL_MESSAGE_LENGTH enforced in net.cpp provides the byte limit.
* Disk serialization (SER_DISK) callers can bypass this via the original
* ReadCompactSize which still uses MAX_SIZE.
*/
static const unsigned int MAX_DESER_ELEMENT_COUNT = 2 * 1024 * 1024;
/**
* Maximum Equihash solution size (bytes) accepted during deserialization.
* Equihash(200,9) = 1344 bytes (pre-Bubbles), Equihash(192,7) = 400 bytes
* (post-Bubbles height 585318+). Use the larger value + small margin.
* Without this bound, a malicious headers message can allocate up to
* 160 * MAX_DESER_ELEMENT_COUNT = 320 MiB of nSolution data.
*/
static const unsigned int MAX_EQUIHASH_SOLUTION_SIZE = 1408;

Then use MAX_EQUIHASH_SOLUTION_SIZE in primitives/block.h after READWRITE(nSolution) (see review body).


/**
* Dummy data type to identify deserializing constructors.
*
Expand Down Expand Up @@ -164,7 +173,6 @@ inline float ser_uint32_to_float(uint32_t y)
return tmp.x;
}


/////////////////////////////////////////////////////////////////
//
// Templates for serializing to anything that looks like a stream,
Expand Down Expand Up @@ -227,11 +235,6 @@ template<typename Stream> inline void Unserialize(Stream& s, double& a ) { a =
template<typename Stream> inline void Serialize(Stream& s, bool a) { char f=a; ser_writedata8(s, f); }
template<typename Stream> inline void Unserialize(Stream& s, bool& a) { char f=ser_readdata8(s); a=f; }






/**
* Compact Size
* size < 253 -- 1 byte
Expand Down Expand Up @@ -305,6 +308,23 @@ uint64_t ReadCompactSize(Stream& is)
return nSizeRet;
}

/**
* ReadCompactSize with a caller-specified upper bound.
* Use this instead of bare ReadCompactSize() when deserializing containers
* from untrusted data (P2P network) to prevent memory exhaustion attacks.
*
* @param nMaxElements The maximum element count to accept.
* @throws std::ios_base::failure if the decoded size exceeds nMaxElements.
*/
template<typename Stream>
uint64_t ReadCompactSizeWithLimit(Stream& is, uint64_t nMaxElements)
{
uint64_t nSizeRet = ReadCompactSize(is);
if (nSizeRet > nMaxElements)
throw std::ios_base::failure("ReadCompactSize(): element count exceeds context limit");
Comment on lines +322 to +324
Copy link
Copy Markdown

@VictorLux VictorLux Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FINDING-006: Include the actual values in the error message so operators can distinguish which guard fired (inner ReadCompactSize vs outer limit check) and see the exact size/limit during incidents.

Suggested change
uint64_t nSizeRet = ReadCompactSize(is);
if (nSizeRet > nMaxElements)
throw std::ios_base::failure("ReadCompactSize(): element count exceeds context limit");
uint64_t nSizeRet = ReadCompactSize(is);
if (nSizeRet > nMaxElements)
throw std::ios_base::failure(
"ReadCompactSizeWithLimit(): size " + std::to_string(nSizeRet) +
" exceeds element limit " + std::to_string(nMaxElements));

return nSizeRet;
}

/**
* Variable-length integers: bytes are a MSB base-128 encoding of the number.
* The high bit in each byte signifies whether another digit follows. To make
Expand Down Expand Up @@ -366,8 +386,19 @@ template<typename Stream, typename I>
I ReadVarInt(Stream& is)
{
I n = 0;
// Maximum possible VarInt encoding length for type I:
// Each byte encodes 7 bits, so sizeof(I)*8 bits needs at most
// ceil(sizeof(I)*8/7) bytes. Add 1 for safety.
const unsigned int max_iters = (sizeof(I) * 8 + 6) / 7;
unsigned int count = 0;
while(true) {
if (++count > max_iters)
throw std::ios_base::failure("ReadVarInt(): too many bytes");
unsigned char chData = ser_readdata8(is);
// Overflow check: if n already uses the top 7 bits, shifting
// left by 7 more would overflow type I.
if (n > (std::numeric_limits<I>::max() >> 7))
throw std::ios_base::failure("ReadVarInt(): overflow");
n = (n << 7) | (chData & 0x7F);
if (chData & 0x80)
n++;
Comment on lines 388 to 404
Copy link
Copy Markdown

@VictorLux VictorLux Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FINDING-001 + FINDING-003: Three changes here:

  1. static_assert (FINDING-003): Prevents signed-type UB at compile time. All current VARINT() callers use unsigned types (uint32_t via nCode/nVersion/nHeight in coins.h, unsigned int in CScriptCompressor).

  2. n++ overflow guard (FINDING-001): After n = (n << 7) | (chData & 0x7F), if n == MAX and chData & 0x80, then n++ wraps to 0. This one-line check prevents silent corruption.

  3. Braces on if/else: Required for the added guard block. Note: the else return n; on the line immediately after this hunk should also get braces for consistency.

Suggested change
I n = 0;
// Maximum possible VarInt encoding length for type I:
// Each byte encodes 7 bits, so sizeof(I)*8 bits needs at most
// ceil(sizeof(I)*8/7) bytes. Add 1 for safety.
const unsigned int max_iters = (sizeof(I) * 8 + 6) / 7;
unsigned int count = 0;
while(true) {
if (++count > max_iters)
throw std::ios_base::failure("ReadVarInt(): too many bytes");
unsigned char chData = ser_readdata8(is);
// Overflow check: if n already uses the top 7 bits, shifting
// left by 7 more would overflow type I.
if (n > (std::numeric_limits<I>::max() >> 7))
throw std::ios_base::failure("ReadVarInt(): overflow");
n = (n << 7) | (chData & 0x7F);
if (chData & 0x80)
n++;
static_assert(std::is_unsigned<I>::value,
"ReadVarInt requires an unsigned integer type");
I n = 0;
// Maximum possible VarInt encoding length for type I:
// Each byte encodes 7 bits, so sizeof(I)*8 bits needs at most
// ceil(sizeof(I)*8/7) bytes. Add 1 for safety.
const unsigned int max_iters = (sizeof(I) * 8 + 6) / 7;
unsigned int count = 0;
while(true) {
if (++count > max_iters)
throw std::ios_base::failure("ReadVarInt(): too many bytes");
unsigned char chData = ser_readdata8(is);
// Overflow check: if n already uses the top 7 bits, shifting
// left by 7 more would overflow type I.
if (n > (std::numeric_limits<I>::max() >> 7))
throw std::ios_base::failure("ReadVarInt(): overflow");
n = (n << 7) | (chData & 0x7F);
if (chData & 0x80) {
if (n == std::numeric_limits<I>::max())
throw std::ios_base::failure("ReadVarInt(): overflow on increment");
n++;
} else {
return n;
}

Note: This suggestion extends 2 lines past the diff hunk to include the else branch with braces. If GitHub can't apply it automatically, the full change is in the patch at the bottom of the audit comment.

Expand Down Expand Up @@ -570,8 +601,6 @@ template<typename Stream, typename T> void Unserialize(Stream& os, std::shared_p
template<typename Stream, typename T> void Serialize(Stream& os, const std::unique_ptr<const T>& p);
template<typename Stream, typename T> void Unserialize(Stream& os, std::unique_ptr<const T>& p);



/**
* If none of the specialized versions above matched, default to calling member function.
*/
Expand All @@ -587,10 +616,6 @@ inline void Unserialize(Stream& is, T& a)
a.Unserialize(is);
}





/**
* string
*/
Expand All @@ -605,14 +630,12 @@ void Serialize(Stream& os, const std::basic_string<C>& str)
template<typename Stream, typename C>
void Unserialize(Stream& is, std::basic_string<C>& str)
{
unsigned int nSize = ReadCompactSize(is);
unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);
Copy link
Copy Markdown

@VictorLux VictorLux Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FINDING-007: Rename MAX_DESER_ELEMENTS → MAX_DESER_ELEMENT_COUNT (consistent with the constant rename above).

Suggested change
unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);
unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENT_COUNT);

str.resize(nSize);
if (nSize != 0)
is.read((char*)&str[0], nSize * sizeof(str[0]));
}



/**
* prevector
*/
Expand All @@ -638,13 +661,12 @@ inline void Serialize(Stream& os, const prevector<N, T>& v)
Serialize_impl(os, v, T());
}


template<typename Stream, unsigned int N, typename T>
void Unserialize_impl(Stream& is, prevector<N, T>& v, const unsigned char&)
{
// Limit size per read so bogus size value won't cause out of memory
v.clear();
unsigned int nSize = ReadCompactSize(is);
unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);
unsigned int i = 0;
while (i < nSize)
{
Copy link
Copy Markdown

@VictorLux VictorLux Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FINDING-007: Rename.

Suggested change
{
unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENT_COUNT);

Expand All @@ -659,7 +681,7 @@ template<typename Stream, unsigned int N, typename T, typename V>
void Unserialize_impl(Stream& is, prevector<N, T>& v, const V&)
{
v.clear();
unsigned int nSize = ReadCompactSize(is);
unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);
Copy link
Copy Markdown

@VictorLux VictorLux Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FINDING-007: Rename.

Suggested change
unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);
unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENT_COUNT);

unsigned int i = 0;
unsigned int nMid = 0;
while (nMid < nSize)
Expand All @@ -679,8 +701,6 @@ inline void Unserialize(Stream& is, prevector<N, T>& v)
Unserialize_impl(is, v, T());
}



/**
* vector
*/
Expand All @@ -706,13 +726,12 @@ inline void Serialize(Stream& os, const std::vector<T, A>& v)
Serialize_impl(os, v, T());
}


template<typename Stream, typename T, typename A>
void Unserialize_impl(Stream& is, std::vector<T, A>& v, const unsigned char&)
{
// Limit size per read so bogus size value won't cause out of memory
v.clear();
unsigned int nSize = ReadCompactSize(is);
unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);
unsigned int i = 0;
while (i < nSize)
{
Copy link
Copy Markdown

@VictorLux VictorLux Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FINDING-007: Rename.

Suggested change
{
unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENT_COUNT);

Expand All @@ -727,7 +746,7 @@ template<typename Stream, typename T, typename A, typename V>
void Unserialize_impl(Stream& is, std::vector<T, A>& v, const V&)
{
v.clear();
unsigned int nSize = ReadCompactSize(is);
unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);
Copy link
Copy Markdown

@VictorLux VictorLux Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FINDING-007: Rename.

Suggested change
unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);
unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENT_COUNT);

unsigned int i = 0;
unsigned int nMid = 0;
while (nMid < nSize)
Expand All @@ -747,8 +766,6 @@ inline void Unserialize(Stream& is, std::vector<T, A>& v)
Unserialize_impl(is, v, T());
}



/**
* optional
*/
Expand Down Expand Up @@ -784,8 +801,6 @@ void Unserialize(Stream& is, boost::optional<T>& item)
}
}



/**
* array
*/
Expand All @@ -805,7 +820,6 @@ void Unserialize(Stream& is, std::array<T, N>& item)
}
}


/**
* pair
*/
Expand All @@ -823,8 +837,6 @@ void Unserialize(Stream& is, std::pair<K, T>& item)
Unserialize(is, item.second);
}



/**
* map
*/
Expand All @@ -840,7 +852,7 @@ template<typename Stream, typename K, typename T, typename Pred, typename A>
void Unserialize(Stream& is, std::map<K, T, Pred, A>& m)
{
m.clear();
unsigned int nSize = ReadCompactSize(is);
unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);
Copy link
Copy Markdown

@VictorLux VictorLux Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FINDING-007: Rename.

Suggested change
unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);
unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENT_COUNT);

typename std::map<K, T, Pred, A>::iterator mi = m.begin();
for (unsigned int i = 0; i < nSize; i++)
{
Expand All @@ -850,8 +862,6 @@ void Unserialize(Stream& is, std::map<K, T, Pred, A>& m)
}
}



/**
* set
*/
Expand All @@ -867,7 +877,7 @@ template<typename Stream, typename K, typename Pred, typename A>
void Unserialize(Stream& is, std::set<K, Pred, A>& m)
{
m.clear();
unsigned int nSize = ReadCompactSize(is);
unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);
Copy link
Copy Markdown

@VictorLux VictorLux Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FINDING-007: Rename.

Suggested change
unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);
unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENT_COUNT);

typename std::set<K, Pred, A>::iterator it = m.begin();
for (unsigned int i = 0; i < nSize; i++)
{
Expand All @@ -877,8 +887,6 @@ void Unserialize(Stream& is, std::set<K, Pred, A>& m)
}
}



/**
* list
*/
Expand All @@ -894,7 +902,7 @@ template<typename Stream, typename T, typename A>
void Unserialize(Stream& is, std::list<T, A>& l)
{
l.clear();
unsigned int nSize = ReadCompactSize(is);
unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);
Copy link
Copy Markdown

@VictorLux VictorLux Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FINDING-007: Rename.

Suggested change
unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);
unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENT_COUNT);

typename std::list<T, A>::iterator it = l.begin();
for (unsigned int i = 0; i < nSize; i++)
{
Expand All @@ -904,8 +912,6 @@ void Unserialize(Stream& is, std::list<T, A>& l)
}
}



/**
* unique_ptr
*/
Expand All @@ -921,8 +927,6 @@ void Unserialize(Stream& is, std::unique_ptr<const T>& p)
p.reset(new T(deserialize, is));
}



/**
* shared_ptr
*/
Expand All @@ -938,8 +942,6 @@ void Unserialize(Stream& is, std::shared_ptr<const T>& p)
p = std::make_shared<const T>(deserialize, is);
}



/**
* Support for ADD_SERIALIZE_METHODS and READWRITE macro
*/
Expand All @@ -964,14 +966,6 @@ inline void SerReadWrite(Stream& s, T& obj, CSerActionUnserialize ser_action)
::Unserialize(s, obj);
}









/* ::GetSerializeSize implementations
*
* Computing the serialized size of objects is done through a special stream
Expand Down
Loading