Skip to content

component: avoid pre-allocating untrusted lengths in the binary set readers - #3091

Open
AlexandrKhromov2005 wants to merge 2 commits into
MetaCubeX:Alphafrom
AlexandrKhromov2005:fix/mrs-unbounded-allocation
Open

component: avoid pre-allocating untrusted lengths in the binary set readers#3091
AlexandrKhromov2005 wants to merge 2 commits into
MetaCubeX:Alphafrom
AlexandrKhromov2005:fix/mrs-unbounded-allocation

Conversation

@AlexandrKhromov2005

Copy link
Copy Markdown

component: avoid pre-allocating untrusted lengths in the binary set readers

ReadDomainSetBin (component/trie) and ReadIpCidrSet (component/cidr) read a length with
binary.Read and immediately allocate a slice of that size
(make([]uint64, length) / make([]byte, length) / make([]netipx.IPRange, length)) before
reading the elements. The length is only checked for < 1, not for an upper bound, so a crafted
binary set — reachable from an .mrs rule-set, including remote rule-providers
(rules/provider fetches provider content over HTTP and calls rulesMrsParse) — can declare a
huge length and either OOM the process on the allocation or panic with
makeslice: len out of range.

Found by fuzzing (go test -fuzz); the readers crash on a tiny input immediately.

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 hits EOF
    while reading the elements;
  • the byte slice is read through io.CopyN into a growable buffer.

Adds FuzzReadDomainSetBin / FuzzReadIpCidrSet. Existing tests pass; valid sets still decode.

…eaders

ReadDomainSetBin and ReadIpCidrSet read a length with binary.Read and then
immediately allocate a slice of that size (make([]uint64, length) /
make([]byte, length) / make([]netipx.IPRange, length)) before reading the
elements. The length is only checked for < 1, not for an upper bound, so a
crafted binary set (reached from an .mrs rule-set, including remote
rule-providers) can declare a huge length and 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; the byte slice is read through io.CopyN into a growable buffer. Add
FuzzReadDomainSetBin / FuzzReadIpCidrSet.
…trusted length

The .mrs reader read a reserved extra field via make([]byte, length) where length is an
untrusted int64 checked only for < 0. A crafted rule-set could declare a huge length and abort
the process (fatal runtime: out of memory, or a makeslice len-out-of-range panic) before any
body is read. The extra bytes are currently unused, so discard them with io.CopyN, which streams
and returns a short-read error for a bogus length. Adds a regression test.
@AlexandrKhromov2005

Copy link
Copy Markdown
Author

While re-auditing the .mrs parse path I found one more instance of the same pattern in the same code path, so I've added it to this PR (commit c51223f).

rules/provider/mrs_reader.go reads a reserved extra field as:

extra := make([]byte, length) // length is an untrusted int64, only checked for < 0
_, err = io.ReadFull(reader, extra)

length comes straight from the (attacker-controlled) rule-set, so a crafted .mrs can declare a huge length and abort the process (fatal error: runtime: out of memory, or a makeslice: len out of range panic) before any body is read — reachable earlier than the two set readers already fixed here. Since the extra bytes are currently unused ("reserved for future using"), the fix just discards them with io.CopyN(io.Discard, reader, length), which streams and returns a short-read error for a bogus length.

Added a regression test (mrs_reader_oom_test.go); go test ./rules/provider/ ./component/trie/ ./component/cidr/ passes.

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.

1 participant