Skip to content

Commit 069f6cf

Browse files
jdomeracki-codergeokat
jdomeracki-coder
and
geokat
authored
fix: reject oversized and invalid zip uploads (#25877) (#26272)
Backport of #25877 to `release/2.29`. Cherry-picked with `git cherry-pick -x` (`2f011fd2a3`); the commit message references the original PR. <details> <summary>Cherry-pick conflict resolution</summary> The core fix (`archive/archive.go`, `coderd/files.go`) applied cleanly. `coderd/files_test.go` needed a small adaptation: this branch predates the shared test-client refactor, so the new `buildZipWithFile` helper was kept and the two new sub-tests create their own client, matching the other sub-tests on this branch. `go test ./archive/...` passes and the `coderd` test package compiles. </details> _Generated by Coder Agents on behalf of @jdomeracki-coder._ Co-authored-by: George K <george@coder.com>
1 parent e01d3f4 commit 069f6cf

4 files changed

Lines changed: 334 additions & 21 deletions

File tree

‎archive/archive.go‎

Lines changed: 128 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -6,43 +6,153 @@ import (
66
"bytes"
77
"errors"
88
"io"
9-
"log"
9+
"math"
1010
"strings"
11+
12+
"golang.org/x/xerrors"
13+
)
14+
15+
// Ref:
16+
// https://github.com/golang/go/blob/go1.24.0/src/archive/tar/format.go
17+
// https://github.com/golang/go/blob/go1.24.0/src/archive/tar/writer.go
18+
const (
19+
tarBlockSize = 512
20+
tarEndBlockBytes = 2 * tarBlockSize
1121
)
1222

23+
// ErrArchiveTooLarge reports that archive expansion would exceed the
24+
// configured limit.
25+
var ErrArchiveTooLarge = xerrors.New("archive exceeds maximum size")
26+
27+
// ErrInvalidZipContent reports that a ZIP entry is malformed or its
28+
// contents fail validation during conversion.
29+
var ErrInvalidZipContent = xerrors.New("invalid zip content")
30+
1331
// CreateTarFromZip converts the given zipReader to a tar archive.
32+
// maxSize limits the total tar output, including tar metadata.
1433
func CreateTarFromZip(zipReader *zip.Reader, maxSize int64) ([]byte, error) {
34+
err := validateZipArchiveSize(zipReader, maxSize)
35+
if err != nil {
36+
return nil, err
37+
}
38+
1539
var tarBuffer bytes.Buffer
16-
err := writeTarArchive(&tarBuffer, zipReader, maxSize)
40+
err = writeTarArchive(&tarBuffer, zipReader, maxSize)
1741
if err != nil {
1842
return nil, err
1943
}
2044
return tarBuffer.Bytes(), nil
2145
}
2246

23-
func writeTarArchive(w io.Writer, zipReader *zip.Reader, maxSize int64) error {
24-
tarWriter := tar.NewWriter(w)
25-
defer tarWriter.Close()
47+
// validateZipArchiveSize performs a metadata-based preflight size
48+
// check before conversion. The actual tar output limit will still be
49+
// enforced while streaming.
50+
func validateZipArchiveSize(zipReader *zip.Reader, maxSize int64) error {
51+
if maxSize < 0 {
52+
return ErrArchiveTooLarge
53+
}
54+
55+
maxBytes := uint64(maxSize)
56+
totalBytes := uint64(tarEndBlockBytes)
57+
if totalBytes > maxBytes {
58+
return ErrArchiveTooLarge
59+
}
2660

2761
for _, file := range zipReader.File {
28-
err := processFileInZipArchive(file, tarWriter, maxSize)
62+
entrySize, err := projectedTarEntrySize(file)
2963
if err != nil {
3064
return err
3165
}
66+
if entrySize > maxBytes-totalBytes {
67+
return ErrArchiveTooLarge
68+
}
69+
totalBytes += entrySize
3270
}
71+
3372
return nil
3473
}
3574

36-
func processFileInZipArchive(file *zip.File, tarWriter *tar.Writer, maxSize int64) error {
75+
func projectedTarEntrySize(file *zip.File) (uint64, error) {
76+
// Each tar entry contributes one header block plus its data
77+
// rounded up to the next tar block boundary.
78+
size := file.UncompressedSize64
79+
if remainder := size % tarBlockSize; remainder != 0 {
80+
padding := tarBlockSize - remainder
81+
if size > math.MaxUint64-padding {
82+
return 0, ErrArchiveTooLarge
83+
}
84+
size += padding
85+
}
86+
87+
if size > math.MaxUint64-tarBlockSize {
88+
return 0, ErrArchiveTooLarge
89+
}
90+
91+
return tarBlockSize + size, nil
92+
}
93+
94+
type limitedWriter struct {
95+
w io.Writer
96+
remaining int64
97+
}
98+
99+
func (w *limitedWriter) Write(p []byte) (int, error) {
100+
if len(p) == 0 {
101+
return 0, nil
102+
}
103+
if w.remaining <= 0 {
104+
return 0, ErrArchiveTooLarge
105+
}
106+
107+
origLen := len(p)
108+
if int64(origLen) > w.remaining {
109+
p = p[:int(w.remaining)]
110+
}
111+
112+
n, err := w.w.Write(p)
113+
// io.Writer may report both written bytes and an error, so
114+
// account for any accepted bytes before returning the error.
115+
w.remaining -= int64(n)
116+
if err != nil {
117+
return n, err
118+
}
119+
if n < origLen {
120+
return n, ErrArchiveTooLarge
121+
}
122+
return n, nil
123+
}
124+
125+
func writeTarArchive(w io.Writer, zipReader *zip.Reader, maxSize int64) error {
126+
tarWriter := tar.NewWriter(&limitedWriter{
127+
w: w,
128+
remaining: maxSize,
129+
})
130+
131+
for _, file := range zipReader.File {
132+
err := processFileInZipArchive(file, tarWriter)
133+
if err != nil {
134+
return err
135+
}
136+
}
137+
138+
return tarWriter.Close()
139+
}
140+
141+
func processFileInZipArchive(file *zip.File, tarWriter *tar.Writer) error {
37142
fileReader, err := file.Open()
38143
if err != nil {
39144
return err
40145
}
41146
defer fileReader.Close()
42147

148+
size := file.FileInfo().Size()
149+
if size < 0 {
150+
return ErrArchiveTooLarge
151+
}
152+
43153
err = tarWriter.WriteHeader(&tar.Header{
44154
Name: file.Name,
45-
Size: file.FileInfo().Size(),
155+
Size: size,
46156
Mode: int64(file.Mode()),
47157
ModTime: file.Modified,
48158
// Note: Zip archives do not store ownership information.
@@ -53,12 +163,17 @@ func processFileInZipArchive(file *zip.File, tarWriter *tar.Writer, maxSize int6
53163
return err
54164
}
55165

56-
n, err := io.CopyN(tarWriter, fileReader, maxSize)
57-
log.Println(file.Name, n, err)
58-
if errors.Is(err, io.EOF) {
59-
err = nil
166+
_, err = io.CopyN(tarWriter, fileReader, size)
167+
switch {
168+
case errors.Is(err, io.EOF), errors.Is(err, io.ErrUnexpectedEOF):
169+
return ErrInvalidZipContent
170+
case errors.Is(err, zip.ErrChecksum), errors.Is(err, zip.ErrFormat):
171+
return ErrInvalidZipContent
172+
case err != nil:
173+
return err
174+
default:
175+
return nil
60176
}
61-
return err
62177
}
63178

64179
// CreateZipFromTar converts the given tarReader to a zip archive.

‎archive/archive_test.go‎

Lines changed: 96 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"archive/tar"
55
"archive/zip"
66
"bytes"
7+
"encoding/binary"
78
"io/fs"
89
"os"
910
"os/exec"
@@ -35,21 +36,113 @@ func TestCreateTarFromZip(t *testing.T) {
3536
zr, err := zip.NewReader(bytes.NewReader(zipBytes), int64(len(zipBytes)))
3637
require.NoError(t, err, "failed to parse sample zip file")
3738

38-
tarBytes, err := archive.CreateTarFromZip(zr, int64(len(zipBytes)))
39+
wantTar := archivetest.TestTarFileBytes()
40+
gotTar, err := archive.CreateTarFromZip(zr, int64(len(wantTar)))
3941
require.NoError(t, err, "failed to convert zip to tar")
4042

41-
archivetest.AssertSampleTarFile(t, tarBytes)
43+
archivetest.AssertSampleTarFile(t, gotTar)
4244

4345
tempDir := t.TempDir()
4446
tempFilePath := filepath.Join(tempDir, "test.tar")
45-
err = os.WriteFile(tempFilePath, tarBytes, 0o600)
47+
err = os.WriteFile(tempFilePath, gotTar, 0o600)
4648
require.NoError(t, err, "failed to write converted tar file")
4749

4850
cmd := exec.CommandContext(ctx, "tar", "--extract", "--verbose", "--file", tempFilePath, "--directory", tempDir)
4951
require.NoError(t, cmd.Run(), "failed to extract converted tar file")
5052
assertExtractedFiles(t, tempDir, true)
5153
}
5254

55+
func buildTestZip(t *testing.T, files map[string]string) []byte {
56+
t.Helper()
57+
58+
var zipBytes bytes.Buffer
59+
zw := zip.NewWriter(&zipBytes)
60+
for name, contents := range files {
61+
w, err := zw.Create(name)
62+
require.NoError(t, err)
63+
64+
_, err = w.Write([]byte(contents))
65+
require.NoError(t, err)
66+
}
67+
require.NoError(t, zw.Close())
68+
69+
return zipBytes.Bytes()
70+
}
71+
72+
func TestCreateTarFromZip_RejectsOversizedAggregateExpansion(t *testing.T) {
73+
t.Parallel()
74+
75+
zipBytes := buildTestZip(t, map[string]string{
76+
"a.txt": strings.Repeat("a", 600),
77+
"b.txt": strings.Repeat("b", 600),
78+
"c.txt": strings.Repeat("c", 600),
79+
})
80+
81+
zr, err := zip.NewReader(bytes.NewReader(zipBytes), int64(len(zipBytes)))
82+
require.NoError(t, err)
83+
84+
tarBytes, err := archive.CreateTarFromZip(zr, 1024)
85+
require.Error(t, err)
86+
require.Nil(t, tarBytes)
87+
}
88+
89+
func TestCreateTarFromZip_RejectsInvalidZipMetadata(t *testing.T) {
90+
t.Parallel()
91+
92+
// Ref: https://github.com/golang/go/blob/go1.24.0/src/archive/zip/struct.go
93+
corruptZipUncompressedSize := func(t *testing.T, zipBytes []byte, size uint32) []byte {
94+
t.Helper()
95+
96+
const (
97+
directoryHeaderSignature = "PK\x01\x02"
98+
uncompressedSizeOffset = 24
99+
)
100+
hdrOffset := bytes.Index(zipBytes, []byte(directoryHeaderSignature))
101+
require.NotEqual(t, -1, hdrOffset, "missing ZIP central directory header")
102+
corrupted := bytes.Clone(zipBytes)
103+
sizeBytes := corrupted[hdrOffset+uncompressedSizeOffset : hdrOffset+uncompressedSizeOffset+4]
104+
binary.LittleEndian.PutUint32(sizeBytes, size)
105+
106+
return corrupted
107+
}
108+
109+
zipBytes := buildTestZip(t, map[string]string{
110+
"hello.txt": "hello",
111+
})
112+
zipBytes = corruptZipUncompressedSize(t, zipBytes, 6)
113+
114+
zr, err := zip.NewReader(bytes.NewReader(zipBytes), int64(len(zipBytes)))
115+
require.NoError(t, err)
116+
117+
// Keep the size limit large so this test exercises the invalid
118+
// ZIP metadata path rather than the tar output limit.
119+
maxSize := int64(4096)
120+
tarBytes, err := archive.CreateTarFromZip(zr, maxSize)
121+
require.ErrorIs(t, err, archive.ErrInvalidZipContent)
122+
require.Nil(t, tarBytes)
123+
}
124+
125+
func TestCreateTarFromZip_RejectsOversizedTarOverhead(t *testing.T) {
126+
t.Parallel()
127+
128+
// Empty files keep the ZIP payload tiny while still forcing tar
129+
// headers and end-of-archive blocks to consume output budget.
130+
zipBytes := buildTestZip(t, map[string]string{
131+
"empty-a.txt": "",
132+
"empty-b.txt": "",
133+
})
134+
135+
zr, err := zip.NewReader(bytes.NewReader(zipBytes), int64(len(zipBytes)))
136+
require.NoError(t, err)
137+
138+
// Two empty tar entries still need 2 header blocks plus the 2
139+
// end-of-archive blocks, so the output is 2048 bytes and must
140+
// exceed this limit.
141+
tarBytes, err := archive.CreateTarFromZip(zr, 2047)
142+
require.Error(t, err)
143+
require.Nil(t, tarBytes)
144+
}
145+
53146
func TestCreateZipFromTar(t *testing.T) {
54147
t.Parallel()
55148
if runtime.GOOS != "linux" {

‎coderd/files.go‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -79,11 +79,24 @@ func (api *API) postFile(rw http.ResponseWriter, r *http.Request) {
7979

8080
data, err = archive.CreateTarFromZip(zipReader, HTTPFileMaxBytes)
8181
if err != nil {
82-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
83-
Message: "Internal error processing .zip archive.",
84-
Detail: err.Error(),
85-
})
86-
return
82+
switch {
83+
case errors.Is(err, archive.ErrArchiveTooLarge):
84+
httpapi.Write(ctx, rw, http.StatusRequestEntityTooLarge, codersdk.Response{
85+
Message: "Expanded .zip archive exceeds maximum size.",
86+
})
87+
return
88+
case errors.Is(err, archive.ErrInvalidZipContent):
89+
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
90+
Message: "Invalid .zip archive contents.",
91+
})
92+
return
93+
default:
94+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
95+
Message: "Internal error processing .zip archive.",
96+
Detail: err.Error(),
97+
})
98+
return
99+
}
87100
}
88101
contentType = tarMimeType
89102
}

0 commit comments

Comments
 (0)