Skip to content

Commit 2cecbf5

Browse files
matthew-pilotteovl
matthew-pilot
and
teovl
authored
fix(security): add decompression bomb protection to untarUnder (PILOT-418) (#288)
* test: add decompression bomb protection tests for untarUnder (PILOT-418) Adds three test cases: - TestUntarUnder_RejectsDecompressionBomb — verifies a tar entry exceeding maxUntarFileSize is rejected - TestUntarUnder_AcceptsNormalBundle — regression test for normal tarballs - TestUntarUnder_AllowsFileUpToLimit — verifies files at exactly the limit are accepted These tests will pass once the fix is applied in the next commit. * fix(security): add decompression bomb protection to untarUnder (PILOT-418) The untarUnder function used io.Copy without a per-file size limit. A maliciously crafted tar.gz with a high compression ratio could pass the 1 MiB download cap (via fetchAll's LimitReader) but decompress to fill disk. Add a 64 MiB per-file cap using io.LimitReader, with a probe read to detect entries that overflow. Oversized entries are rejected and the partial file is removed from disk to prevent garbage accumulation. --------- Co-authored-by: Matthew (Pilot Bot) <teodor@vulturelabs.io>
1 parent 6beeb91 commit 2cecbf5

2 files changed

Lines changed: 240 additions & 4 deletions

File tree

‎cmd/pilotctl/appstore_catalogue.go‎

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -415,9 +415,17 @@ func httpGet(raw string) (io.ReadCloser, error) {
415415
return resp.Body, nil
416416
}
417417

418+
// maxUntarFileSize is the per-file limit for entries extracted by
419+
// untarUnder. 64 MiB covers legitimate bundles (the largest known
420+
// pilot app is ~4 MiB) while bounding decompression bombs that pass
421+
// the 1 MiB tarball download cap via a high compression ratio.
422+
const maxUntarFileSize int64 = 64 << 20 // 64 MiB
423+
418424
// untarUnder writes every entry in r under dst, refusing any path
419425
// that resolves outside dst (mirrors the supervisor's
420-
// resolveUnder guard on manifest.binary.path).
426+
// resolveUnder guard on manifest.binary.path). Per-file extraction
427+
// is capped at maxUntarFileSize to prevent decompression bombs from
428+
// filling disk via extreme compression ratios.
421429
func untarUnder(r io.Reader, dst string) error {
422430
tr := tar.NewReader(r)
423431
for {
@@ -446,12 +454,26 @@ func untarUnder(r io.Reader, dst string) error {
446454
if err != nil {
447455
return err
448456
}
449-
if _, err := io.Copy(f, tr); err != nil {
457+
lr := io.LimitReader(tr, maxUntarFileSize)
458+
written, err := io.Copy(f, lr)
459+
if err != nil {
450460
_ = f.Close()
451461
return err
452462
}
453-
if err := f.Close(); err != nil {
454-
return err
463+
overLimit := false
464+
if written >= maxUntarFileSize {
465+
// Read one more byte to check if the tar entry has leftover data.
466+
// If it does, the file exceeds the per-file limit.
467+
var probe [1]byte
468+
if _, err := io.ReadFull(tr, probe[:]); err == nil {
469+
overLimit = true
470+
}
471+
// err means the tar entry is exactly at the limit — that's fine.
472+
}
473+
_ = f.Close()
474+
if overLimit {
475+
os.Remove(out)
476+
return fmt.Errorf("extracted file exceeds maxUntarFileSize (%d bytes): %q", maxUntarFileSize, hdr.Name)
455477
}
456478
default:
457479
// Skip symlinks, devices, etc. — neither needed for a

‎cmd/pilotctl/appstore_catalogue_test.go‎

Lines changed: 214 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,214 @@
1+
package main
2+
3+
import (
4+
"archive/tar"
5+
"bytes"
6+
"compress/gzip"
7+
"os"
8+
"path/filepath"
9+
"strings"
10+
"testing"
11+
)
12+
13+
// TestUntarUnder_RejectsDecompressionBomb verifies that a tar entry
14+
// exceeding maxUntarFileSize is rejected (decompression bomb guard).
15+
func TestUntarUnder_RejectsDecompressionBomb(t *testing.T) {
16+
dst := t.TempDir()
17+
18+
var buf bytes.Buffer
19+
gz := gzip.NewWriter(&buf)
20+
tw := tar.NewWriter(gz)
21+
22+
// Create a file entry with size = maxUntarFileSize + 1
23+
size := maxUntarFileSize + 1
24+
hdr := &tar.Header{
25+
Name: "bomb.bin",
26+
Size: size,
27+
Typeflag: tar.TypeReg,
28+
Mode: 0644,
29+
}
30+
if err := tw.WriteHeader(hdr); err != nil {
31+
t.Fatal(err)
32+
}
33+
// Write exactly the declared size (128 MiB + 1)
34+
chunk := make([]byte, 65536)
35+
written := int64(0)
36+
for written < size {
37+
n := int64(len(chunk))
38+
if written+n > size {
39+
n = size - written
40+
}
41+
if _, err := tw.Write(chunk[:n]); err != nil {
42+
t.Fatal(err)
43+
}
44+
written += n
45+
}
46+
if err := tw.Close(); err != nil {
47+
t.Fatal(err)
48+
}
49+
if err := gz.Close(); err != nil {
50+
t.Fatal(err)
51+
}
52+
53+
gzr, err := gzip.NewReader(&buf)
54+
if err != nil {
55+
t.Fatal(err)
56+
}
57+
defer gzr.Close()
58+
err = untarUnder(gzr, dst)
59+
if err == nil {
60+
t.Fatal("expected decompression bomb rejection, got nil")
61+
}
62+
if !strings.Contains(err.Error(), "exceeds maxUntarFileSize") {
63+
t.Fatalf("expected maxUntarFileSize error, got: %v", err)
64+
}
65+
66+
// The entry must NOT have been written to disk
67+
entries, _ := os.ReadDir(dst)
68+
for _, e := range entries {
69+
if e.Name() == "bomb.bin" {
70+
t.Fatal("decompression bomb entry was written to disk despite being rejected")
71+
}
72+
}
73+
}
74+
75+
// TestUntarUnder_RejectsPathTraversal ensures the existing traversal
76+
// guard still works.
77+
func TestUntarUnder_RejectsPathTraversal(t *testing.T) {
78+
dst := t.TempDir()
79+
traversal := []string{
80+
"../../../etc/passwd",
81+
"foo/../../etc/passwd",
82+
"..",
83+
}
84+
for _, p := range traversal {
85+
var buf bytes.Buffer
86+
gz := gzip.NewWriter(&buf)
87+
tw := tar.NewWriter(gz)
88+
89+
hdr := &tar.Header{
90+
Name: p,
91+
Size: 4,
92+
Typeflag: tar.TypeReg,
93+
Mode: 0644,
94+
}
95+
if err := tw.WriteHeader(hdr); err != nil {
96+
t.Fatal(err)
97+
}
98+
if _, err := tw.Write([]byte("data")); err != nil {
99+
t.Fatal(err)
100+
}
101+
if err := tw.Close(); err != nil {
102+
t.Fatal(err)
103+
}
104+
if err := gz.Close(); err != nil {
105+
t.Fatal(err)
106+
}
107+
if err := untarUnder(&buf, dst); err == nil {
108+
t.Errorf("expected path traversal rejection for %q", p)
109+
}
110+
}
111+
}
112+
113+
// TestUntarUnder_AcceptsNormalBundle ensures a legitimate tarball
114+
// still extracts correctly.
115+
func TestUntarUnder_AcceptsNormalBundle(t *testing.T) {
116+
dst := t.TempDir()
117+
118+
var buf bytes.Buffer
119+
gz := gzip.NewWriter(&buf)
120+
tw := tar.NewWriter(gz)
121+
122+
if err := tw.WriteHeader(&tar.Header{Name: "app/", Typeflag: tar.TypeDir, Mode: 0755}); err != nil {
123+
t.Fatal(err)
124+
}
125+
if err := tw.WriteHeader(&tar.Header{Name: "app/v1", Size: 5, Typeflag: tar.TypeReg, Mode: 0644}); err != nil {
126+
t.Fatal(err)
127+
}
128+
if _, err := tw.Write([]byte("hello")); err != nil {
129+
t.Fatal(err)
130+
}
131+
if err := tw.WriteHeader(&tar.Header{Name: "app/manifest.json", Size: 2, Typeflag: tar.TypeReg, Mode: 0644}); err != nil {
132+
t.Fatal(err)
133+
}
134+
if _, err := tw.Write([]byte("{}")); err != nil {
135+
t.Fatal(err)
136+
}
137+
138+
if err := tw.Close(); err != nil {
139+
t.Fatal(err)
140+
}
141+
if err := gz.Close(); err != nil {
142+
t.Fatal(err)
143+
}
144+
gzr, err := gzip.NewReader(&buf)
145+
if err != nil {
146+
t.Fatal(err)
147+
}
148+
defer gzr.Close()
149+
if err := untarUnder(gzr, dst); err != nil {
150+
t.Fatalf("expected no error, got: %v", err)
151+
}
152+
153+
if _, err := os.Stat(filepath.Join(dst, "app", "v1")); os.IsNotExist(err) {
154+
t.Fatal("expected app/v1 to exist")
155+
}
156+
if _, err := os.Stat(filepath.Join(dst, "app", "manifest.json")); os.IsNotExist(err) {
157+
t.Fatal("expected app/manifest.json to exist")
158+
}
159+
}
160+
161+
// TestUntarUnder_AllowsFileUpToLimit ensures a file at exactly
162+
// maxUntarFileSize is accepted.
163+
func TestUntarUnder_AllowsFileUpToLimit(t *testing.T) {
164+
dst := t.TempDir()
165+
166+
var buf bytes.Buffer
167+
gz := gzip.NewWriter(&buf)
168+
tw := tar.NewWriter(gz)
169+
170+
hdr := &tar.Header{
171+
Name: "bigfile.bin",
172+
Size: maxUntarFileSize,
173+
Typeflag: tar.TypeReg,
174+
Mode: 0644,
175+
}
176+
if err := tw.WriteHeader(hdr); err != nil {
177+
t.Fatal(err)
178+
}
179+
chunk := make([]byte, 65536)
180+
written := int64(0)
181+
for written < maxUntarFileSize {
182+
n := int64(len(chunk))
183+
if written+n > maxUntarFileSize {
184+
n = maxUntarFileSize - written
185+
}
186+
if _, err := tw.Write(chunk[:n]); err != nil {
187+
t.Fatal(err)
188+
}
189+
written += n
190+
}
191+
if err := tw.Close(); err != nil {
192+
t.Fatal(err)
193+
}
194+
if err := gz.Close(); err != nil {
195+
t.Fatal(err)
196+
}
197+
198+
gzr, err := gzip.NewReader(&buf)
199+
if err != nil {
200+
t.Fatal(err)
201+
}
202+
defer gzr.Close()
203+
if err := untarUnder(gzr, dst); err != nil {
204+
t.Fatalf("expected no error for file at limit, got: %v", err)
205+
}
206+
207+
fi, err := os.Stat(filepath.Join(dst, "bigfile.bin"))
208+
if err != nil {
209+
t.Fatal(err)
210+
}
211+
if fi.Size() != maxUntarFileSize {
212+
t.Fatalf("expected file size %d, got %d", maxUntarFileSize, fi.Size())
213+
}
214+
}

0 commit comments

Comments
 (0)