Sharon KatzandLucas Manuel Rodriguez
24e64614aa
Resolves https://github.com/fleetdm/confidential/issues/16902 - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually ## Reproduction **Attack vector:** An authenticated Fleet admin uploads a crafted `.msi` file via the software upload API. The server's MSI metadata parser (`pkg/file/msi.go` > `ExtractMSIMetadata` > `decodeStrings`) processes the file's `_StringPool` and `_StringData` streams during upload. A malicious `_StringPool` can claim arbitrarily large string sizes that trigger speculative memory allocation before the actual data is read. **Manual reproduction:** Wrote a standalone Go program that constructs the two raw byte streams the parser consumes (a 12-byte `_StringPool` claiming a 64 MB string, and an empty 0-byte `_StringData`), then measures heap allocation via `runtime.MemStats` before and after calling the vulnerable code path. **Before fix (vulnerable):** ``` Pool input size: 12 bytes StringData size: 0 bytes Claimed string size: 67108864 bytes (64 MB) Error returned: failed to read string data: EOF Heap allocated: 67117016 bytes (64.0 MB) Amplification: 12 input bytes -> 67117016 byte allocation (5593084x) ``` 12 bytes of pool input forced a 64 MB heap allocation via `buf.Grow()` before `io.CopyN` discovered there was no data to read. **After fix:** ``` Error returned: failed to read string data: EOF Heap allocated: 3072 bytes (3.0 KB) Memory amplification eliminated: true ``` Same input, 3 KB allocated instead of 64 MB. Without the speculative `buf.Grow()`, `io.CopyN` grows the buffer incrementally based on actual available data and immediately hits EOF. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Fixed a potential resource exhaustion issue when processing MSI metadata with unusually large string-size declarations. - MSI files with missing string data are now handled without excessive memory allocation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Lucas Manuel Rodriguez <lucas@fleetdm.com>
52 lines
1.9 KiB
Go
52 lines
1.9 KiB
Go
package file
|
|
|
|
import (
|
|
"bytes"
|
|
"encoding/binary"
|
|
"runtime"
|
|
"testing"
|
|
|
|
"github.com/stretchr/testify/require"
|
|
)
|
|
|
|
func TestDecodeStringsMemoryAmplification(t *testing.T) {
|
|
// Build a _StringPool that claims a single string of 64 MB.
|
|
// The pool format is: 4-byte header (codepage + unknown), then 4-byte entries (size uint16 + refcount uint16).
|
|
var pool bytes.Buffer
|
|
|
|
// Pool header: codepage=0, unknown=0
|
|
require.NoError(t, binary.Write(&pool, binary.LittleEndian, uint16(0))) // codepage
|
|
require.NoError(t, binary.Write(&pool, binary.LittleEndian, uint16(0))) // unknown
|
|
|
|
// One entry claiming a huge size. The "large string" path is triggered
|
|
// when Size==0 and RefCount!=0, then reads a uint32 for the actual size.
|
|
require.NoError(t, binary.Write(&pool, binary.LittleEndian, uint16(0))) // Size=0 triggers large-string path
|
|
require.NoError(t, binary.Write(&pool, binary.LittleEndian, uint16(1))) // RefCount!=0
|
|
const claimedSize = 64 * 1024 * 1024 // 64 MB
|
|
require.NoError(t, binary.Write(&pool, binary.LittleEndian, uint32(claimedSize)))
|
|
|
|
// _StringData is empty: zero actual bytes of string data.
|
|
var data bytes.Buffer
|
|
|
|
// Measure memory before
|
|
var before runtime.MemStats
|
|
runtime.GC()
|
|
runtime.ReadMemStats(&before)
|
|
|
|
// decodeStrings should fail because there is no data to read,
|
|
// but it must NOT allocate 64 MB first.
|
|
_, err := decodeStrings(&data, &pool)
|
|
require.Error(t, err, "expected an error because string data is empty")
|
|
|
|
// Measure memory after
|
|
var after runtime.MemStats
|
|
runtime.ReadMemStats(&after)
|
|
|
|
// With the fix, TotalAlloc should increase by well under 1 MB.
|
|
// Without the fix, it would jump by ~64 MB from the speculative Grow call.
|
|
allocated := after.TotalAlloc - before.TotalAlloc
|
|
const maxAllowed = 1024 * 1024 // 1 MB
|
|
require.Less(t, allocated, uint64(maxAllowed),
|
|
"decodeStrings allocated %d bytes; expected less than %d (memory amplification detected)", allocated, maxAllowed)
|
|
}
|