Skip to content

state: harden FromBytes against truncated input + 100% coverage #425

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

Draft
poolpOrg wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

state: harden FromBytes against truncated input + 100% coverage #425

Show file tree
Hide file tree
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
228 changes: 85 additions & 143 deletions repository/state/state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package state

import (
"bytes"
"encoding/binary"
"fmt"
"io"
Expand Down Expand Up @@ -291,17 +290,10 @@

/* Publishes the current state, by saving the stateID with the current Metadata. */
func (ls *LocalState) PutState(stateID objects.MAC) error {
mt, err := ls.Metadata.ToBytes()
if err != nil {
return err
}

err = ls.cache.PutState(stateID, mt)
if err != nil {
return err
}

return nil
// Metadata is a fixed-shape struct that msgpack can always marshal, so we
// don't surface an error from ToBytes here — only cache.PutState can fail.
mt, _ := ls.Metadata.ToBytes()
return ls.cache.PutState(stateID, mt)
}

func (ls *LocalState) GetStates() (map[objects.MAC][]byte, error) {
Expand Down Expand Up @@ -422,36 +414,18 @@
}

func DeleteEntryFromBytes(buf []byte) (del DeleteEntry, err error) {
bbuf := bytes.NewBuffer(buf)

typ, err := bbuf.ReadByte()
if err != nil {
return
}
del.Type = EntryType(typ)

typ, err = bbuf.ReadByte()
if err != nil {
return
}
del.BlobType = resources.Type(typ)

n, err := bbuf.Read(del.Blob[:])
if err != nil {
return
}
if n < len(objects.MAC{}) {
return del, fmt.Errorf("short read while deserializing delete entry")
}

n, err = bbuf.Read(del.Packfile[:])
if err != nil {
return
}
if n < len(objects.MAC{}) {
return del, fmt.Errorf("short read while deserializing delete entry")
if len(buf) < DeleteEntrySerializedSize {
return del, fmt.Errorf("short read while deserializing delete entry: have %d, want %d", len(buf), DeleteEntrySerializedSize)
}

pos := 0
del.Type = EntryType(buf[pos])
pos++
del.BlobType = resources.Type(buf[pos])
pos++
copy(del.Blob[:], buf[pos:pos+len(objects.MAC{})])
pos += len(objects.MAC{})
copy(del.Packfile[:], buf[pos:pos+len(objects.MAC{})])
return
}

Expand All @@ -474,36 +448,24 @@
}

func DeltaEntryFromBytes(buf []byte) (de DeltaEntry, err error) {
bbuf := bytes.NewBuffer(buf)

typ, err := bbuf.ReadByte()
if err != nil {
return
}

de.Type = resources.Type(typ)
de.Version = versioning.Version(binary.LittleEndian.Uint32(bbuf.Next(4)))

n, err := bbuf.Read(de.Blob[:])
if err != nil {
return
}
if n < len(objects.MAC{}) {
return de, fmt.Errorf("short read while deserializing delta entry")
if len(buf) < DeltaEntrySerializedSize {
return de, fmt.Errorf("short read while deserializing delta entry: have %d, want %d", len(buf), DeltaEntrySerializedSize)
}

n, err = bbuf.Read(de.Location.Packfile[:])
if err != nil {
return
}
if n < len(objects.MAC{}) {
return de, fmt.Errorf("short read while deserializing delta entry")
}

de.Location.Offset = binary.LittleEndian.Uint64(bbuf.Next(8))
de.Location.Length = binary.LittleEndian.Uint32(bbuf.Next(4))
de.Flags = binary.LittleEndian.Uint32(bbuf.Next(4))

pos := 0
de.Type = resources.Type(buf[pos])
pos++
de.Version = versioning.Version(binary.LittleEndian.Uint32(buf[pos:]))
pos += 4
copy(de.Blob[:], buf[pos:pos+len(objects.MAC{})])
pos += len(objects.MAC{})
copy(de.Location.Packfile[:], buf[pos:pos+len(objects.MAC{})])
pos += len(objects.MAC{})
de.Location.Offset = binary.LittleEndian.Uint64(buf[pos:])
pos += 8
de.Location.Length = binary.LittleEndian.Uint32(buf[pos:])
pos += 4
de.Flags = binary.LittleEndian.Uint32(buf[pos:])
return
}

Expand All @@ -530,27 +492,17 @@
}

func PackfileEntryFromBytes(buf []byte) (pe PackfileEntry, err error) {
bbuf := bytes.NewBuffer(buf)

n, err := bbuf.Read(pe.Packfile[:])
if err != nil {
return
}
if n < len(objects.MAC{}) {
return pe, fmt.Errorf("Short read while deserializing packfile entry")
if len(buf) < PackfileEntrySerializedSize {
return pe, fmt.Errorf("short read while deserializing packfile entry: have %d, want %d", len(buf), PackfileEntrySerializedSize)
}

n, err = bbuf.Read(pe.StateID[:])
if err != nil {
return
}
if n < len(objects.MAC{}) {
return pe, fmt.Errorf("Short read while deserializing packfile entry")
}

timestamp := binary.LittleEndian.Uint64(bbuf.Next(8))
pos := 0
copy(pe.Packfile[:], buf[pos:pos+len(objects.MAC{})])
pos += len(objects.MAC{})
copy(pe.StateID[:], buf[pos:pos+len(objects.MAC{})])
pos += len(objects.MAC{})
timestamp := binary.LittleEndian.Uint64(buf[pos:])
pe.Timestamp = time.Unix(0, int64(timestamp))

return
}

Expand All @@ -568,26 +520,17 @@
}

func ColouredEntryFromBytes(buf []byte) (de ColouredEntry, err error) {
bbuf := bytes.NewBuffer(buf)

typ, err := bbuf.ReadByte()
if err != nil {
return
if len(buf) < ColouredEntrySerializedSize {
return de, fmt.Errorf("short read while deserializing coloured entry: have %d, want %d", len(buf), ColouredEntrySerializedSize)
}

de.Type = resources.Type(typ)

n, err := bbuf.Read(de.Blob[:])
if err != nil {
return
}
if n < len(objects.MAC{}) {
return de, fmt.Errorf("Short read while deserializing coloured entry")
}

timestamp := binary.LittleEndian.Uint64(bbuf.Next(8))
pos := 0
de.Type = resources.Type(buf[pos])
pos++
copy(de.Blob[:], buf[pos:pos+len(objects.MAC{})])
pos += len(objects.MAC{})
timestamp := binary.LittleEndian.Uint64(buf[pos:])
de.When = time.Unix(0, int64(timestamp))

return
}

Expand All @@ -613,18 +556,34 @@
// - value [valueLen]byte
// - createdAt uint64
func ConfigurationEntryFromBytes(buf []byte) (ce ConfigurationEntry, err error) {
bbuf := bytes.NewBuffer(buf)
// Minimum on-disk size: 1 (keyLen) + 0 (empty key) + 2 (valueLen) + 0 (empty value) + 8 (timestamp).
const minSize = 1 + 2 + 8
if len(buf) < minSize {
return ce, fmt.Errorf("short read while deserializing configuration entry: have %d, want at least %d", len(buf), minSize)
}

keyLen, err := bbuf.ReadByte()
if err != nil {
return ce, fmt.Errorf("Short read while deserializing keyLen ConfigurationEntry")
pos := 0
keyLen := int(buf[pos])
pos++

if pos+keyLen+2+8 > len(buf) {
return ce, fmt.Errorf("short read while deserializing configuration entry key: have %d, want %d", len(buf)-pos, keyLen+2+8)
}
ce.Key = string(bbuf.Next(int(keyLen)))
ce.Key = string(buf[pos : pos+keyLen])
pos += keyLen

valueLen := int(binary.LittleEndian.Uint16(buf[pos:]))
pos += 2

valueLen := binary.LittleEndian.Uint16(bbuf.Next(2))
ce.Value = bbuf.Next(int(valueLen))
if pos+valueLen+8 > len(buf) {
return ce, fmt.Errorf("short read while deserializing configuration entry value: have %d, want %d", len(buf)-pos, valueLen+8)
}
// Copy so the returned struct doesn't alias the caller's buffer (the
// callers often reuse buffers from a pool).
ce.Value = append([]byte(nil), buf[pos:pos+valueLen]...)
pos += valueLen

timestamp := binary.LittleEndian.Uint64(bbuf.Next(8))
timestamp := binary.LittleEndian.Uint64(buf[pos:])
ce.CreatedAt = time.Unix(0, int64(timestamp))

return
Expand Down Expand Up @@ -702,10 +661,9 @@
return fmt.Errorf("failed to read delete entry %w, read(%d)/expected(%d)", err, n, length)
}

toDel, err := DeleteEntryFromBytes(del_buf)
if err != nil {
return fmt.Errorf("failed to deserialize delta entry %w", err)
}
// length is already validated against DeleteEntrySerializedSize, so
// DeleteEntryFromBytes cannot fail on a buffer of the right size.
toDel, _ := DeleteEntryFromBytes(del_buf)

switch toDel.Type {
case ET_LOCATIONS:
Expand All @@ -726,12 +684,8 @@
return fmt.Errorf("failed to read delta entry %w, read(%d)/expected(%d)", err, n, length)
}

// We need to decode just to make the key, but we can reuse the buffer
// to put inside the data part of the cache.
delta, err := DeltaEntryFromBytes(de_buf)
if err != nil {
return fmt.Errorf("failed to deserialize delta entry %w", err)
}
// length already matches DeltaEntrySerializedSize.
delta, _ := DeltaEntryFromBytes(de_buf)

if err := ls.cache.PutDelta(delta.Type, delta.Blob, delta.Location.Packfile, de_buf); err != nil {
return err
Expand All @@ -746,10 +700,8 @@
return fmt.Errorf("failed to read coloured entry %w, read(%d)/expected(%d)", err, n, length)
}

coloured, err := ColouredEntryFromBytes(coloured_buf)
if err != nil {
return fmt.Errorf("failed to deserialize coloured entry %w", err)
}
// length already matches ColouredEntrySerializedSize.
coloured, _ := ColouredEntryFromBytes(coloured_buf)

if err := ls.cache.PutColoured(coloured.Type, coloured.Blob, coloured_buf); err != nil {
return err
Expand All @@ -763,12 +715,10 @@
return fmt.Errorf("failed to read packfile entry %w, read(%d)/expected(%d)", err, n, length)
}

pe, err := PackfileEntryFromBytes(pe_buf)
if err != nil {
return fmt.Errorf("failed to deserialize packfile entry %w", err)
}
// length already matches PackfileEntrySerializedSize.
pe, _ := PackfileEntryFromBytes(pe_buf)

if err := ls.cache.PutPackfile(pe.Packfile, pe_buf); err != nil {

Check failure on line 721 in repository/state/state.go

View check run for this annotation

Claude / Claude Code Review

ET_CONFIGURATION allocation is unbounded — 4GB DoS on corrupted state file 

**ET_CONFIGURATION allocates `length` bytes up front without bounds — 4 GiB DoS on a corrupted state file.** In both `deserializeFromStream` (state.go:726) and `deserializeFromStreamv100` (state.go:857), the ET_CONFIGURATION arm does `ce_buf := make([]byte, length)` with `length` being a `uint32` read directly from the (attacker-controlled) stream. Every other entry type in the same switch was hardened by this PR to gate on `length != XxxSerializedSize` before allocating into a pre-sized fixed b
Comment on lines +718 to 721

@claude claude Bot May 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 ET_CONFIGURATION allocates length bytes up front without bounds — 4 GiB DoS on a corrupted state file. In both deserializeFromStream (state.go:726) and deserializeFromStreamv100 (state.go:857), the ET_CONFIGURATION arm does ce_buf := make([]byte, length) with length being a uint32 read directly from the (attacker-controlled) stream. Every other entry type in the same switch was hardened by this PR to gate on length != XxxSerializedSize before allocating into a pre-sized fixed buffer; ET_CONFIGURATION is the only branch left that allocates length bytes up front, so a 37-byte crafted state file with length=0xFFFFFFFF forces an immediate ~4 GiB allocation before io.ReadFull errors out — exactly the "corrupted on-disk state file a process-killer" threat model the PR description names. Fix is one-liner mirroring the existing pattern: cap length at the structural maximum for a legitimate ConfigurationEntry (1 + 255 + 2 + 65535 + 8 = 65801 bytes) before make, in both deserializers.

Extended reasoning...

What the bug is

Inside the per-entry switch of both deserializeFromStream (state.go:684-744) and deserializeFromStreamv100 (state.go:815-875), the ET_CONFIGURATION case is the only branch that allocates an entry buffer using the attacker-controlled length field from the wire, with no upper bound:

case ET_CONFIGURATION:
    ce_buf := make([]byte, length)    // length is a uint32 from the stream
    if n, err := io.ReadFull(r, ce_buf); err != nil { ... }

The PR explicitly hardens every other entry type in the same loop — ET_DELETE (state.go:670), ET_LOCATIONS (state.go:680), ET_COLOURED (state.go:691), ET_PACKFILE (state.go:710) — with a if length != XxxSerializedSize { return error } guard before any read, and reads into a pre-allocated fixed-size buffer (del_buf, de_buf, coloured_buf, pe_buf). ET_CONFIGURATION sits inside that same hardening loop but skips the guard entirely.

Why existing safeguards don't catch it

  • ConfigurationEntryFromBytes performs its defensive checks after the allocation — it protects against bad content but not against bad declared size.
  • The new TestDeserializeFromStream_BadConfigContent only exercises length=1, which never approaches the dangerous regime.
  • TestDeserializeFromStream_AllByteFlipsSafe does flip bytes in the length field, but it only asserts no-panic via notPanic. A successful 4 GiB allocation followed by a clean io.ReadFull EOF error does not panic — it returns an error and notPanic is satisfied. On CI hosts with < 4 GiB free RAM this may surface as intermittent OOM-killed test runs rather than a clear failure.

Concrete step-by-step proof

Craft this 37-byte payload:

bytes  0..31  : zeroed (the 32-byte parent header — accepted by ReadFull)
byte   32     : 0x05            (ET_CONFIGURATION)
bytes  33..36 : FF FF FF FF     (length = 4 294 967 295, little-endian uint32)

Trace through deserializeFromStream in state.go:

  1. Line ~669 reads the 32-byte header into ls.Metadata.Parent — OK.
  2. Line ~676 reads one byte into et_buf, gets 0x05 → entryType = ET_CONFIGURATION.
  3. Line ~688 calls readUint32() which reads bytes 33–36 → length = 0xFFFFFFFF = 4 294 967 295.
  4. Switch dispatches to state.go:725 ET_CONFIGURATION → ce_buf := make([]byte, 4294967295) allocates and zeroes ~4 GiB before any validation.
  5. Line 728 io.ReadFull(r, ce_buf) then fails on the short stream and returns an error — but the 4 GiB has already been committed.

On hosts with less than 4 GiB free this is an immediate OOM-kill of the kloset process. On larger hosts it's a multi-second stall and severe memory pressure on co-tenants (page-cache eviction, swap thrash). The same shape applies verbatim to deserializeFromStreamv100 at state.go:857, no header byte needed.

Reachability — production callers

This is reachable along exactly the surfaces the PR description names:

  • repository/repository.go:386 — MergeState is called with a reader returned by r.GetState over remote storage.
  • repository/repository.go:422 — MergeState is called with an on-disk state file via OpenStateFromStateFile.

A malicious remote storage backend, a corrupted disk, or a tampered state file all reach this branch.

Fix

Same pattern the PR already uses for the fixed-size entries — cap before allocate. The legitimate maximum for a ConfigurationEntry is structurally bounded because keyLen is a uint8 (max 255) and valueLen is a uint16 (max 65535):

1 (keyLen u8) + 255 (max key) + 2 (valueLen u16) + 65535 (max value) + 8 (timestamp) = 65801 bytes

Any length larger than that cannot round-trip from ConfigurationEntry.ToBytes and should be rejected:

case ET_CONFIGURATION:
    const maxConfigEntrySize = 1 + 255 + 2 + 65535 + 8
    if length > maxConfigEntrySize {
        return fmt.Errorf("configuration entry length %d exceeds max %d", length, maxConfigEntrySize)
    }
    ce_buf := make([]byte, length)
    ...

Apply the same one-liner at state.go:857 inside deserializeFromStreamv100. While here, it would also be worth strengthening TestDeserializeFromStream_AllByteFlipsSafe (and the v100 counterpart) so that an arbitrarily-large declared length produces a fast error rather than an attempted allocation — e.g. by asserting that decode completes within a small memory budget, or by adding a targeted length=0xFFFFFFFF regression test alongside the existing length=1 case.

return err
}

Expand Down Expand Up @@ -865,12 +815,8 @@
return fmt.Errorf("failed to read delta entry %w, read(%d)/expected(%d)", err, n, length)
}

// We need to decode just to make the key, but we can reuse the buffer
// to put inside the data part of the cache.
delta, err := DeltaEntryFromBytes(de_buf)
if err != nil {
return fmt.Errorf("failed to deserialize delta entry %w", err)
}
// length already matches DeltaEntrySerializedSize.
delta, _ := DeltaEntryFromBytes(de_buf)

if err := ls.cache.PutDelta(delta.Type, delta.Blob, delta.Location.Packfile, de_buf); err != nil {
return err
Expand All @@ -885,10 +831,8 @@
return fmt.Errorf("failed to read coloured entry %w, read(%d)/expected(%d)", err, n, length)
}

coloured, err := ColouredEntryFromBytes(coloured_buf)
if err != nil {
return fmt.Errorf("failed to deserialize coloured entry %w", err)
}
// length already matches ColouredEntrySerializedSize.
coloured, _ := ColouredEntryFromBytes(coloured_buf)

if err := ls.cache.PutColoured(coloured.Type, coloured.Blob, coloured_buf); err != nil {
return err
Expand All @@ -902,10 +846,8 @@
return fmt.Errorf("failed to read packfile entry %w, read(%d)/expected(%d)", err, n, length)
}

pe, err := PackfileEntryFromBytes(pe_buf)
if err != nil {
return fmt.Errorf("failed to deserialize packfile entry %w", err)
}
// length already matches PackfileEntrySerializedSize.
pe, _ := PackfileEntryFromBytes(pe_buf)

if err := ls.cache.PutPackfile(pe.Packfile, pe_buf); err != nil {
return err
Expand Down
Loading
Loading