Prevent deserialization memory exhaustion and ReadVarInt infinite loop (CWE-400) by hairetikos · Pull Request #105 · ZclassicCommunity/zclassic

hairetikos · 2026-02-15T13:10:01Z

NOTE: this is a fix for an extreme (D)DoS scenario, it theoretically would overwhelm a system with limited RAM like a 4GB VPS... but still might be good to merge. please use Claude Opus/other LLM to scrutinize this fix, as maybe it could be more elegant

Updated deserialization functions to use a limit on the maximum number of elements accepted, preventing potential memory exhaustion attacks.

(fixes an extreme DDoS scenario: if an attacker made hundreds or thousands of simultaneous connections and overwhelmed the node with data, it could've caused RAM/mem exhaustion, leading to a frozen system and possible node termination if for example the Linux OOM killer decided to kill zclassicd to free up memory)

Updated deserialization functions to use a limit on the maximum number of elements accepted, preventing potential memory exhaustion attacks.

VictorLux · 2026-02-25T05:49:34Z

Security Audit: PR #105 — "Prevent deserialization memory exhaustion and ReadVarInt infinite loop (CWE-400)"

Auditor: AI-assisted review (Claude/Opus)
Date: 2026-02-24 (original), 2026-02-25 (revised for posting)
Scope: src/serialize.h — MAX_DESER_ELEMENTS, ReadCompactSizeWithLimit, ReadVarInt hardening, all container Unserialize updates

Disclaimer

This review was performed by an AI assistant (Claude by Anthropic) and has not been peer-reviewed by a professional security auditor. While findings are derived from direct source code analysis against the Zclassic codebase (master branch + PR diff), they may contain errors, omissions, or misinterpretations. This is not a formal security audit engagement. All findings should be independently verified before acting on them. Use at your own risk.

Executive Summary

The PR correctly identifies and partially fixes two real vulnerabilities: unbounded container allocation and a potential infinite loop in ReadVarInt. The overall approach is sound and the deserialization hardening is a meaningful improvement. However, the analysis identifies 2 High, 3 Medium, 2 Low, and 1 Informational findings that should be addressed before merge.

Inline suggested changes for src/serialize.h are attached as a code review. Additional changes needed in src/main.cpp and src/primitives/block.h (not in this PR's scope) are described below.

FINDING-001 — `n++` Post-Shift Overflow Not Guarded in `ReadVarInt`

Severity: HIGH | Confidence: Confirmed (mathematical proof) | Category: Integer Overflow

The proposed patch adds a pre-shift overflow guard, but n++ executes unguarded when chData & 0x80 is set:

n = (n << 7) | (chData & 0x7F);
if (chData & 0x80)
    n++;   // <-- NOT guarded

Concrete example (uint64_t): Pre-shift value 0x01FFFFFFFFFFFFFF passes the guard. After shift+OR: n = 0xFFFFFFFFFFFFFFFF. Then n++ wraps to 0x0000000000000000 — silent wrong value instead of exception.

For signed types: Wraps through INT64_MAX to INT64_MIN — undefined behavior in C++.

Impact: Attacker can cause ReadVarInt to return 0 where it should throw. Affects CCoins::Unserialize (VARINT(nCode), VARINT(nVersion), VARINT(nHeight)) and CScriptCompressor — corrupts deserialized values silently.

FINDING-002 — `MAX_DESER_ELEMENTS` Units Mismatch + `nSolution` 320 MiB Allocation

Severity: HIGH | Confidence: Confirmed | Category: DoS Mitigation Gap

MAX_DESER_ELEMENTS = 2,097,152 is described as "matching MAX_PROTOCOL_MESSAGE_LENGTH" (net.h:51) — but that's bytes, while this limits element count. For nSolution (std::vector<unsigned char> in block.h:33,51): Equihash(192,7) = 400 bytes, but limit allows 2 MiB per solution. With MAX_HEADERS_RESULTS = 160: 160 x 2 MiB = 320 MiB from a single peer.

Proposed fix (requires src/primitives/block.h — not in this PR):

// In serialize.h, add after MAX_DESER_ELEMENT_COUNT:
static const unsigned int MAX_EQUIHASH_SOLUTION_SIZE = 1408;
// (covers Equihash(200,9)=1344 pre-Bubbles + Equihash(192,7)=400 post-Bubbles)

// In primitives/block.h, after READWRITE(nSolution):
if (ser_action.ForRead() && nSolution.size() > MAX_EQUIHASH_SOLUTION_SIZE) {
    throw std::ios_base::failure("nSolution size " +
        std::to_string(nSolution.size()) +
        " exceeds Equihash maximum " +
        std::to_string(MAX_EQUIHASH_SOLUTION_SIZE));
}

FINDING-003 — `ReadVarInt` Signed Type Undefined Behavior

Severity: MEDIUM | Confidence: Confirmed | Category: Undefined Behavior

When I is signed, n << 7 can produce a positive value that overflows to negative on n++ — UB in C++. Iteration guard catches it in practice but through a different mechanism.

FINDING-004 — `main.cpp:6117` Bare `ReadCompactSize` Not Hardened

Severity: MEDIUM | Confidence: Confirmed | Category: Incomplete Coverage

Requires src/main.cpp — not in this PR:

// main.cpp:6117 — CURRENT:
ReadCompactSize(vRecv); // ignore tx count; assume it is 0.

// PROPOSED FIX:
ReadCompactSizeWithLimit(vRecv, 0); // tx count must be 0 in headers msg

The tx count in headers messages is expected to always be 0. Currently a malicious peer sending non-zero is silently accepted. ReadCompactSizeWithLimit(vRecv, 0) throws (disconnecting the peer) if non-zero.

FINDING-005 — `LimitedString::Unserialize` Not Hardened

Severity: MEDIUM | Confidence: Confirmed | Category: Incomplete Coverage

LimitedString<Limit>::Unserialize (serialize.h:503) still uses bare ReadCompactSize (up to 32 MiB) before checking Limit. This is a P2P path via CAlert (alert.h:64: LIMITED_STRING(strComment, 65536)). Note: This line is not in the PR diff, so the suggested fix is shown below instead of inline:

// CURRENT (serialize.h:501-510):
void Unserialize(Stream& s)
{
    size_t size = ReadCompactSize(s);
    if (size > Limit) {
        throw std::ios_base::failure("String length limit exceeded");
    }
    string.resize(size);
    if (size != 0)
        s.read((char*)&string[0], size);
}

// PROPOSED FIX:
void Unserialize(Stream& s)
{
    size_t size = ReadCompactSizeWithLimit(s, Limit);
    string.resize(size);
    if (size != 0)
        s.read((char*)&string[0], size);
}

FINDING-006 — Duplicate Error Messages Hinder Debugging

Severity: LOW | Confidence: Confirmed | Category: Diagnostics

ReadCompactSizeWithLimit and inner ReadCompactSize both throw similar "size too large" messages. Including the actual values helps debugging.

FINDING-007 — `MAX_DESER_ELEMENTS` Naming Inconsistency

Severity: LOW | Confidence: Confirmed | Category: Code Quality

Name suggests bytes but it's element count. Rename to MAX_DESER_ELEMENT_COUNT for clarity.

FINDING-008 — `ReadVarInt` Iteration Guard Correctly Protects Disk Paths (Positive)

Severity: INFO | Confidence: Confirmed | Category: Positive Finding

max_iters = (sizeof(I) * 8 + 6) / 7 is mathematically correct. Protects both P2P and disk paths (LevelDB CCoins::Unserialize). No action needed.

Sapling/JoinSplit Vector Safety (Positive)

All Sapling vectors are safe under the element limit — bounded by MAX_TX_SIZE_AFTER_SAPLING = 102,000 bytes:

Vector	Element Size	Max Elements	vs. 2M Limit
`vShieldedSpend`	384 bytes	~265	Safe
`vShieldedOutput`	948 bytes	~107	Safe
`vjoinsplit`	~700+ bytes	~145	Safe
`vin`	~41 bytes	~2,487	Safe
`vout`	~9 bytes	~11,333	Safe

Risk & Impact Summary

#	Finding	Severity	Impact	Consensus Risk	P2P Exploitable?
001	`n++` overflow in `ReadVarInt`	HIGH	Silent wrong return value; corrupted `CCoins` deserialization	Low (mitigated by iteration guard)	Indirectly
002	`nSolution` 320 MiB allocation	HIGH	160 headers x 2 MiB = 320 MiB from single peer	None (Equihash check post-deser)	Yes
003	Signed type UB in `ReadVarInt`	MEDIUM	UB; compiler may optimize unexpectedly	Low	Indirectly
004	Bare `ReadCompactSize` in `main.cpp`	MEDIUM	Malformed headers silently accepted	None	Yes
005	`LimitedString` not hardened	MEDIUM	Inconsistent coverage on P2P alert path	None	Minimal
006	Duplicate error messages	LOW	Harder debugging	None	N/A
007	Naming inconsistency	LOW	Developer confusion	None	N/A
008	Iteration guard on disk paths	INFO	Positive finding	N/A	N/A

Testing Required Before Merge

Mandatory:

Full chain sync regression — Genesis to tip on mainnet. Confirm no valid block rejected (especially verify MAX_EQUIHASH_SOLUTION_SIZE = 1408 covers Equihash(200,9) = 1344 pre-Bubbles and Equihash(192,7) = 400 post-Bubbles).
ReadVarInt round-trip test — Boundary values for uint32_t/uint64_t (0, 0x7F, 0x80, MAX-1, MAX).
ReadVarInt overflow negative test — Crafted stream triggering n++ wrap must throw.
ReadCompactSizeWithLimit negative test — Element count MAX_DESER_ELEMENT_COUNT + 1 must throw for each container type.

Recommended:

Malicious headers stress test — 160 headers with >1408-byte nSolution. Verify disconnect.
Cross-version peer compatibility — Patched vs unpatched nodes exchanging blocks/headers.

Files Summary

File	In This PR?	Changes Needed
`src/serialize.h`	Yes	Inline suggestions attached (FINDING 001-003, 006-007)
`src/serialize.h` (LimitedString)	Line not in diff	FINDING-005 fix shown above
`src/main.cpp:6117`	No	FINDING-004 fix shown above
`src/primitives/block.h`	No	FINDING-002 `nSolution` bound shown above

Overall Verdict

Directionally correct, needs targeted fixes before merge. The PR addresses real vulnerabilities. FINDING-001 is a one-line fix. FINDING-002 needs a per-field limit. See inline suggested changes and the additional file fixes above.

This review was generated by an AI assistant and should be independently verified. It is not a substitute for professional security auditing.

VictorLux

Code Review — Proposed Fixes for All Findings

Inline suggestions below address FINDING-001 through FINDING-007 for src/serialize.h.

Additional files needing changes (not in this PR's diff):

`src/main.cpp:6117` (FINDING-004)

// CURRENT:
ReadCompactSize(vRecv); // ignore tx count; assume it is 0.

// PROPOSED:
ReadCompactSizeWithLimit(vRecv, 0); // tx count must be 0 in headers msg

`src/primitives/block.h` (FINDING-002 — nSolution bound)

After READWRITE(nSolution); (line 51), add:

if (ser_action.ForRead() && nSolution.size() > MAX_EQUIHASH_SOLUTION_SIZE) {
    throw std::ios_base::failure("nSolution size " +
        std::to_string(nSolution.size()) +
        " exceeds Equihash maximum " +
        std::to_string(MAX_EQUIHASH_SOLUTION_SIZE));
}

`src/serialize.h` — LimitedString (FINDING-005)

Line 501-510 (not in diff). Replace:

void Unserialize(Stream& s)
{
    size_t size = ReadCompactSize(s);
    if (size > Limit) {
        throw std::ios_base::failure("String length limit exceeded");
    }
    string.resize(size);
    if (size != 0)
        s.read((char*)&string[0], size);
}

With:

void Unserialize(Stream& s)
{
    size_t size = ReadCompactSizeWithLimit(s, Limit);
    string.resize(size);
    if (size != 0)
        s.read((char*)&string[0], size);
}

VictorLux · 2026-02-25T05:50:19Z

src/serialize.h

+/**
+ * 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;


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).

VictorLux · 2026-02-25T05:50:20Z

src/serialize.h

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


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));

VictorLux · 2026-02-25T05:50:20Z

src/serialize.h

    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++;


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

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).

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.

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.

VictorLux · 2026-02-25T05:50:20Z

src/serialize.h

 void Unserialize(Stream& is, std::basic_string<C>& str)
 {
-    unsigned int nSize = ReadCompactSize(is);
+    unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);


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);

VictorLux · 2026-02-25T05:50:20Z

src/serialize.h

+    unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);
    unsigned int i = 0;
    while (i < nSize)
    {


FINDING-007: Rename.

Suggested change

{

unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENT_COUNT);

VictorLux · 2026-02-25T05:50:20Z

src/serialize.h

+    unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);
    unsigned int i = 0;
    while (i < nSize)
    {


FINDING-007: Rename.

Suggested change

{

unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENT_COUNT);

VictorLux · 2026-02-25T05:50:20Z

src/serialize.h

 {
    v.clear();
-    unsigned int nSize = ReadCompactSize(is);
+    unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);


FINDING-007: Rename.

Suggested change

unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);

unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENT_COUNT);

VictorLux · 2026-02-25T05:50:20Z

src/serialize.h

 {
    m.clear();
-    unsigned int nSize = ReadCompactSize(is);
+    unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);


FINDING-007: Rename.

Suggested change

unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);

unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENT_COUNT);

VictorLux · 2026-02-25T05:50:20Z

src/serialize.h

 {
    m.clear();
-    unsigned int nSize = ReadCompactSize(is);
+    unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);


FINDING-007: Rename.

Suggested change

unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);

unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENT_COUNT);

VictorLux · 2026-02-25T05:50:20Z

src/serialize.h

 {
    l.clear();
-    unsigned int nSize = ReadCompactSize(is);
+    unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);


FINDING-007: Rename.

Suggested change

unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENTS);

unsigned int nSize = ReadCompactSizeWithLimit(is, MAX_DESER_ELEMENT_COUNT);

Add limits to deserialization element count

841b30d

Updated deserialization functions to use a limit on the maximum number of elements accepted, preventing potential memory exhaustion attacks.

VictorLux suggested changes Feb 25, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Prevent deserialization memory exhaustion and ReadVarInt infinite loop (CWE-400)#105
hairetikos wants to merge 1 commit intoZclassicCommunity:masterfrom
hairetikos:patch-3

hairetikos commented Feb 15, 2026 •

edited

Loading

Uh oh!

VictorLux commented Feb 25, 2026

Uh oh!

VictorLux left a comment

Uh oh!

VictorLux Feb 25, 2026

Uh oh!

VictorLux Feb 25, 2026

Uh oh!

VictorLux Feb 25, 2026

Uh oh!

VictorLux Feb 25, 2026

Uh oh!

VictorLux Feb 25, 2026

Uh oh!

VictorLux Feb 25, 2026

Uh oh!

VictorLux Feb 25, 2026

Uh oh!

VictorLux Feb 25, 2026

Uh oh!

VictorLux Feb 25, 2026

Uh oh!

VictorLux Feb 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-/**
- * 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;

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

Conversation

hairetikos commented Feb 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VictorLux commented Feb 25, 2026

Security Audit: PR #105 — "Prevent deserialization memory exhaustion and ReadVarInt infinite loop (CWE-400)"

Executive Summary

FINDING-001 — n++ Post-Shift Overflow Not Guarded in ReadVarInt

FINDING-002 — MAX_DESER_ELEMENTS Units Mismatch + nSolution 320 MiB Allocation

FINDING-003 — ReadVarInt Signed Type Undefined Behavior

FINDING-004 — main.cpp:6117 Bare ReadCompactSize Not Hardened

FINDING-005 — LimitedString::Unserialize Not Hardened

FINDING-006 — Duplicate Error Messages Hinder Debugging

FINDING-007 — MAX_DESER_ELEMENTS Naming Inconsistency

FINDING-008 — ReadVarInt Iteration Guard Correctly Protects Disk Paths (Positive)

Sapling/JoinSplit Vector Safety (Positive)

Risk & Impact Summary

Testing Required Before Merge

Files Summary

Overall Verdict

Uh oh!

VictorLux left a comment

Choose a reason for hiding this comment

Code Review — Proposed Fixes for All Findings

src/main.cpp:6117 (FINDING-004)

src/primitives/block.h (FINDING-002 — nSolution bound)

src/serialize.h — LimitedString (FINDING-005)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hairetikos commented Feb 15, 2026 •

edited

Loading

FINDING-001 — `n++` Post-Shift Overflow Not Guarded in `ReadVarInt`

FINDING-002 — `MAX_DESER_ELEMENTS` Units Mismatch + `nSolution` 320 MiB Allocation

FINDING-003 — `ReadVarInt` Signed Type Undefined Behavior

FINDING-004 — `main.cpp:6117` Bare `ReadCompactSize` Not Hardened

FINDING-005 — `LimitedString::Unserialize` Not Hardened

FINDING-007 — `MAX_DESER_ELEMENTS` Naming Inconsistency

FINDING-008 — `ReadVarInt` Iteration Guard Correctly Protects Disk Paths (Positive)

`src/main.cpp:6117` (FINDING-004)

`src/primitives/block.h` (FINDING-002 — nSolution bound)

`src/serialize.h` — LimitedString (FINDING-005)