diff --git a/vorbis.go b/vorbis.go index f1c6a19..aa068b3 100644 --- a/vorbis.go +++ b/vorbis.go @@ -129,8 +129,11 @@ func (m *metadataVorbis) readPictureBlock(r io.Reader) error { if err != nil { return err } - data := make([]byte, dataLen) - _, err = io.ReadFull(r, data) + // Use readBytes rather than allocating dataLen up front: dataLen comes + // straight from the file, so a bogus value could otherwise force a huge + // allocation (memory amplification) from a tiny input. readBytes caps the + // up-front allocation and streams larger amounts. + data, err := readBytes(r, uint(dataLen)) if err != nil { return err } diff --git a/vorbis_picture_alloc_test.go b/vorbis_picture_alloc_test.go new file mode 100644 index 0000000..92be6ea --- /dev/null +++ b/vorbis_picture_alloc_test.go @@ -0,0 +1,55 @@ +package tag + +import ( + "bytes" + "encoding/binary" + "runtime" + "testing" +) + +// flacWithPictureDataLen builds a minimal FLAC file whose single (last) +// metadata block is a PICTURE block advertising dataLen bytes of image data +// while carrying none. It exercises readPictureBlock with an untrusted length. +func flacWithPictureDataLen(dataLen uint32) []byte { + b := &bytes.Buffer{} + b.WriteString("fLaC") + b.WriteByte(0x80 | byte(pictureBlock)) // last-block flag + picture block type + b.Write([]byte{0, 0, 0}) // block length (unused for picture) + be := func(v uint32) { binary.Write(b, binary.BigEndian, v) } + be(0) // picture type = Other + be(0) // mime length + be(0) // description length + be(0) // width + be(0) // height + be(0) // color depth + be(0) // colors used + be(dataLen) // picture data length (bogus) + return b.Bytes() +} + +// TestFLACPictureAllocationBounded ensures a bogus picture-data length can't +// force a large allocation. Before the fix, readPictureBlock did +// make([]byte, dataLen) directly, so a ~40 byte file advertising a large +// picture forced that many bytes to be allocated (memory-amplification DoS). +func TestFLACPictureAllocationBounded(t *testing.T) { + const maxAlloc = 20 << 20 // decoding a tiny file must stay well under this + data := flacWithPictureDataLen(100 << 20) + + if len(data) > 4096 { + t.Fatalf("test input unexpectedly large: %d bytes", len(data)) + } + + var m1, m2 runtime.MemStats + runtime.GC() + runtime.ReadMemStats(&m1) + + // A truncated picture block is expected to error; we only care that it + // does not allocate based on the advertised length. + _, _ = ReadFrom(bytes.NewReader(data)) + + runtime.ReadMemStats(&m2) + if alloc := m2.TotalAlloc - m1.TotalAlloc; alloc > maxAlloc { + t.Fatalf("decoding a %d-byte file allocated %d bytes (%.1f MiB); want < %d", + len(data), alloc, float64(alloc)/(1<<20), maxAlloc) + } +}