Skip to content

common/srs: avoid pre-allocating untrusted lengths in the rule-set reader - #4399

Open
AlexandrKhromov2005 wants to merge 224 commits into
SagerNet:testingfrom
AlexandrKhromov2005:fix/srs-unbounded-allocation
Open

common/srs: avoid pre-allocating untrusted lengths in the rule-set reader#4399
AlexandrKhromov2005 wants to merge 224 commits into
SagerNet:testingfrom
AlexandrKhromov2005:fix/srs-unbounded-allocation

Conversation

@AlexandrKhromov2005

Copy link
Copy Markdown

common/srs: avoid pre-allocating untrusted lengths in the rule-set reader

The binary rule-set (.srs) reader repeatedly reads a length with
binary.ReadUvarint and immediately allocates a slice of that size
(make([]T, length)) before reading the elements. The length comes from the
(untrusted) rule-set data, so a crafted .srs can declare an enormous count and
either OOM the process on the allocation or panic with
makeslice: len out of range. This is reachable when parsing rule-set content,
including remote rule-sets (route/rule/rule_set_remote.go calls
srs.Read(..., false) on downloaded content).

Found by fuzzing (go test -fuzz); a ~40-byte input is enough to trigger it.

Fix

Read incrementally instead of trusting the length for a single allocation:

  • element slices grow via append with a capped initial capacity, so a bogus
    length simply hits EOF while reading the elements;
  • byte slices go through a small readBytes helper backed by io.CopyN, which
    grows the buffer only as bytes actually arrive.

Covers Read, readRuleItemString, readRuleItemUint8, readRuleItemUint16,
readLogicalRule, readPrefix and readIPSet. Adds FuzzSRSRead. Existing
tests pass; valid rule-sets still decode.

Note: the same "read length, then make" pattern also exists in the
succinct-set reader used for AdGuard-format domains
(sing/common/domain/set.go), reached from .srs via AdGuard domain items;
that lives in the sagernet/sing dependency and would need the same treatment
there. Happy to follow up.

nekohasekai and others added 24 commits August 5, 2026 17:39
Recent Windows 11 builds remove TCP estats, which made writeAndWaitAck
return without waiting for acknowledgment. sing now queries
SIO_TCP_INFO on the socket instead, and falls back to estats on
systems predating it (Windows 10 1703).
Batched darwin packet I/O now also covers connected sockets on iOS.
CI took the library from the head of refs/heads/go. The packages carried a
library that differs from the bindings in the binary. CI reads the library
file from the commit in CRONET_GO_VERSION, and the update script points that
pin at the generated branch.
…ader

The binary rule-set (.srs) reader repeatedly reads a length via
binary.ReadUvarint and then immediately allocates a slice of that size
(make([]T, length)) before reading the elements. The length is untrusted, so a
crafted rule-set can declare a huge count and either OOM the process on the
allocation or panic with "makeslice: len out of range".

Read incrementally instead:
- element slices grow via append with a capped initial capacity, so a bogus
  length simply hits EOF while reading the elements;
- byte slices are read through a small readBytes helper backed by io.CopyN,
  which grows the buffer only as bytes actually arrive.

Also add FuzzSRSRead covering srs.Read.

Covers: Read, readRuleItemString, readRuleItemUint8, readRuleItemUint16,
readLogicalRule, readPrefix (ip_cidr) and readIPSet (ip_set).
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.

5 participants