Skip to content

fix: security bugs: ReadHashSet allocation attack, Unbatcher overflow#4115

Merged
miwarnec merged 7 commits into
masterfrom
copilot/read-through-mirror-core-code
Jun 27, 2026
Merged

fix: security bugs: ReadHashSet allocation attack, Unbatcher overflow#4115
miwarnec merged 7 commits into
masterfrom
copilot/read-through-mirror-core-code

Conversation

Copilot AI commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Two independent security bugs, one per commit.

ReadHashSet missing allocation limit (NetworkReaderExtensions.cs)

ReadList and ReadArray both guard against allocation attacks via NetworkReader.AllocationLimit. ReadHashSet was missing this check — a malicious peer can send an arbitrarily large length and force unbounded allocation (DoS).

Unbatcher integer overflow on size prefix (Unbatcher.cs)

DecompressVarUInt returns ulong. The direct cast to int wraps values > int.MaxValue to negative, which silently passes reader.Remaining < size (negative is always less). A malformed batch bypasses size validation entirely.

// Before: silent wraparound
int size = (int)Compression.DecompressVarUInt(reader);

// After: explicit overflow guard
ulong rawSize = Compression.DecompressVarUInt(reader);
if (rawSize > int.MaxValue) { Clear(); throw ...; }
int size = (int)rawSize;

Copilot AI added 5 commits June 27, 2026 08:44
…ervice

ReadList and ReadArray guard against allocation attacks with
AllocationLimit, but ReadHashSet was missing this check entirely.
A malicious peer could send a huge length value and force the
receiver to allocate unbounded memory.
DecompressVarUInt returns ulong. Casting directly to int silently
wraps values > int.MaxValue to negative, which bypasses the
reader.Remaining < size validation. Now validates the raw ulong
against int.MaxValue before casting.
…ving

Previously only checked the key, ignoring the value entirely.
This violated the IDictionary<TKey,TValue> contract which requires
both key AND value to match for removal.
Copilot AI changed the title Fix 3 bugs: ReadHashSet allocation attack, Unbatcher integer overflow, SyncDictionary.Remove contract Fix 3 security/correctness bugs: ReadHashSet allocation attack, Unbatcher overflow, SyncDictionary contract Jun 27, 2026
Copilot AI requested a review from miwarnec June 27, 2026 08:53
@miwarnec

Copy link
Copy Markdown
Collaborator

@copilot drop the syncdictionary change

Copilot AI commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

@copilot drop the syncdictionary change

Reverted in revert: drop SyncDictionary.Remove(KeyValuePair) change per review.

Copilot AI changed the title Fix 3 security/correctness bugs: ReadHashSet allocation attack, Unbatcher overflow, SyncDictionary contract Fix 2 security bugs: ReadHashSet allocation attack, Unbatcher overflow Jun 27, 2026
@miwarnec

Copy link
Copy Markdown
Collaborator

@copilot add test coverage if its low overhead to our existin gtests

…rflow

- TestReadHashSet_LengthIsTooBig: mirrors existing ReadList/ReadArray tests
- GetNextMessage_SizePrefixOverflow_Throws: verifies size > int.MaxValue throws

Copilot AI commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

@copilot add test coverage if its low overhead to our existin gtests

Added in a2f387d:

  • TestReadHashSet_LengthIsTooBig — mirrors the existing TestReadList_LengthIsTooBig/TestReadArray_LengthIsTooBig pattern
  • GetNextMessage_SizePrefixOverflow_Throws — crafts a batch with a VarUInt size > int.MaxValue and asserts it throws

@miwarnec miwarnec marked this pull request as ready for review June 27, 2026 09:12
@miwarnec miwarnec changed the title Fix 2 security bugs: ReadHashSet allocation attack, Unbatcher overflow fix: security bugs: ReadHashSet allocation attack, Unbatcher overflow Jun 27, 2026
@codecov

codecov Bot commented Jun 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 42.22%. Comparing base (b25a14b) to head (a2f387d).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4115      +/-   ##
==========================================
+ Coverage   42.19%   42.22%   +0.03%     
==========================================
  Files         156      156              
  Lines       15092    15100       +8     
==========================================
+ Hits         6368     6376       +8     
  Misses       8724     8724              
Flag Coverage Δ
unittests 42.22% <100.00%> (+0.03%) ⬆️
unity-6000.5.0f1 ?
unity-6000.5.1f1 42.22% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
Assets/Mirror/Core/Batching/Unbatcher.cs 96.61% <100.00%> (+0.31%) ⬆️
Assets/Mirror/Core/NetworkReaderExtensions.cs 93.03% <100.00%> (+0.10%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@miwarnec miwarnec merged commit 6634734 into master Jun 27, 2026
10 checks passed
@miwarnec miwarnec deleted the copilot/read-through-mirror-core-code branch June 27, 2026 20:58
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