diff --git a/internal/templaterepo/client.go b/internal/templaterepo/client.go index 2de078fd..c3001976 100644 --- a/internal/templaterepo/client.go +++ b/internal/templaterepo/client.go @@ -4,6 +4,7 @@ import ( "archive/tar" "compress/gzip" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -26,6 +27,20 @@ const ( templateMetadataFile = ".cre/template.yaml" ) +// maxExtractTotalSize and maxExtractFileSize bound decompression (a "tar bomb" +// can expand to far more bytes than the compressed download), and maxExtractFileCount +// bounds the number of entries, mirroring the caps used by the update extractor. +// maxTarballDownloadSize bounds the raw (still-compressed) tarball written to the +// on-disk cache, guarding against disk exhaustion on the download side. +// These are vars (not consts) so tests can shrink them instead of building +// multi-hundred-megabyte fixtures. +var ( + maxExtractTotalSize int64 = 500 * 1024 * 1024 + maxExtractFileSize int64 = 100 * 1024 * 1024 + maxTarballDownloadSize int64 = 500 * 1024 * 1024 + maxExtractFileCount = 10000 +) + // standardIgnores are files/dirs always excluded when extracting templates. var standardIgnores = []string{ ".git", @@ -253,9 +268,20 @@ func (c *Client) DownloadTarball(source RepoSource, destPath string) error { } defer f.Close() - if _, err := io.Copy(f, resp.Body); err != nil { + // Cap the raw download so a compromised or malicious source cannot exhaust + // disk by streaming an unbounded tarball into the cache. + written, err := io.CopyN(f, resp.Body, maxTarballDownloadSize+1) + if err != nil && !errors.Is(err, io.EOF) { return fmt.Errorf("failed to write tarball: %w", err) } + if written > maxTarballDownloadSize { + // Drop the partial file so it can't poison the cache. + f.Close() + if rmErr := os.Remove(destPath); rmErr != nil { + c.logger.Warn().Err(rmErr).Msgf("Failed to remove oversized partial tarball %s", destPath) + } + return fmt.Errorf("tarball exceeds maximum allowed download size (limit %d bytes)", maxTarballDownloadSize) + } return nil } @@ -346,9 +372,17 @@ func (c *Client) extractTarball(r io.Reader, templatePath, destDir string, exclu // We need to detect it and strip it. var topLevelPrefix string + absDestDir, err := filepath.Abs(destDir) + if err != nil { + return fmt.Errorf("failed to resolve destination directory: %w", err) + } + + var totalSize int64 + var fileCount int + for { header, err := tr.Next() - if err == io.EOF { + if errors.Is(err, io.EOF) { break } if err != nil { @@ -365,6 +399,11 @@ func (c *Client) extractTarball(r io.Reader, templatePath, destDir string, exclu return fmt.Errorf("illegal file path in archive: %s", header.Name) } + fileCount++ + if fileCount > maxExtractFileCount { + return fmt.Errorf("archive contains too many entries (limit: %d)", maxExtractFileCount) + } + // Detect top-level prefix from the first real directory entry if topLevelPrefix == "" { parts := strings.SplitN(header.Name, "/", 2) @@ -410,6 +449,17 @@ func (c *Client) extractTarball(r io.Reader, templatePath, destDir string, exclu targetPath := filepath.Join(destDir, relPath) + // Belt-and-suspenders Zip Slip guard: verify the resolved path is still + // contained within destDir, in case the ".." check above is bypassed by + // some other means (e.g. an absolute path in the header). + absTargetPath, err := filepath.Abs(targetPath) + if err != nil { + return fmt.Errorf("failed to resolve target path for %s: %w", name, err) + } + if absTargetPath != absDestDir && !strings.HasPrefix(absTargetPath, absDestDir+string(os.PathSeparator)) { + return fmt.Errorf("resolved file path escapes destination directory: %s", header.Name) + } + switch header.Typeflag { case tar.TypeDir: c.logger.Debug().Msgf("Extracting dir: %s -> %s", name, targetPath) @@ -429,16 +479,30 @@ func (c *Client) extractTarball(r io.Reader, templatePath, destDir string, exclu return fmt.Errorf("failed to create parent directory: %w", err) } + if header.Size > maxExtractFileSize { + return fmt.Errorf("file %s exceeds maximum allowed size (%d bytes, limit %d)", name, header.Size, maxExtractFileSize) + } + f, err := os.OpenFile(targetPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, os.FileMode(header.Mode)&0755|0600) //nolint:gosec // mode is masked to safe range if err != nil { return fmt.Errorf("failed to create file %s: %w", targetPath, err) } - if _, err := io.Copy(f, tr); err != nil { //nolint:gosec // tar size is bounded by GitHub API tarball limits + written, err := io.CopyN(f, tr, maxExtractFileSize+1) + if err != nil && !errors.Is(err, io.EOF) { f.Close() return fmt.Errorf("failed to write file %s: %w", targetPath, err) } + if written > maxExtractFileSize { + f.Close() + return fmt.Errorf("file %s exceeds maximum allowed size (limit %d bytes)", name, maxExtractFileSize) + } f.Close() + + totalSize += written + if totalSize > maxExtractTotalSize { + return fmt.Errorf("archive exceeds maximum total extracted size (limit %d bytes)", maxExtractTotalSize) + } } } diff --git a/internal/templaterepo/client_test.go b/internal/templaterepo/client_test.go index eec8f630..2fb77ffb 100644 --- a/internal/templaterepo/client_test.go +++ b/internal/templaterepo/client_test.go @@ -1,6 +1,9 @@ package templaterepo import ( + "archive/tar" + "bytes" + "compress/gzip" "encoding/json" "net/http" "net/http/httptest" @@ -14,6 +17,38 @@ import ( "github.com/smartcontractkit/cre-cli/internal/testutil" ) +// buildTarGz builds an in-memory gzip'd tarball with a synthetic top-level +// prefix directory (mirroring GitHub's tarball layout) followed by the given +// files, each written with the requested content. +func buildTarGz(t *testing.T, files map[string]string) *bytes.Buffer { + t.Helper() + + var buf bytes.Buffer + gw := gzip.NewWriter(&buf) + tw := tar.NewWriter(gw) + + require.NoError(t, tw.WriteHeader(&tar.Header{ + Name: "owner-repo-abc123/", + Typeflag: tar.TypeDir, + Mode: 0755, + })) + + for name, content := range files { + require.NoError(t, tw.WriteHeader(&tar.Header{ + Name: "owner-repo-abc123/" + name, + Typeflag: tar.TypeReg, + Mode: 0644, + Size: int64(len(content)), + })) + _, err := tw.Write([]byte(content)) + require.NoError(t, err) + } + + require.NoError(t, tw.Close()) + require.NoError(t, gw.Close()) + return &buf +} + func TestDiscoverTemplates_FindsTemplateYaml(t *testing.T) { logger := testutil.NewTestLogger() @@ -143,3 +178,68 @@ func TestExtractTarball_BasicExtraction(t *testing.T) { _ = client } + +func TestExtractTarball_EnforcesPerFileSizeLimit(t *testing.T) { + orig := maxExtractFileSize + maxExtractFileSize = 10 + defer func() { maxExtractFileSize = orig }() + + client := NewClient(testutil.NewTestLogger()) + tarball := buildTarGz(t, map[string]string{ + ".cre/template.yaml": "kind: x\n", + "main.go": "this file is way over the ten byte cap", + }) + + err := client.extractTarball(tarball, "", t.TempDir(), nil, false) + require.Error(t, err) + assert.Contains(t, err.Error(), "exceeds maximum allowed size") +} + +func TestExtractTarball_EnforcesTotalSizeLimit(t *testing.T) { + orig := maxExtractTotalSize + maxExtractTotalSize = 10 + defer func() { maxExtractTotalSize = orig }() + + client := NewClient(testutil.NewTestLogger()) + tarball := buildTarGz(t, map[string]string{ + "a.txt": "123456", + "b.txt": "789012", + }) + + err := client.extractTarball(tarball, "", t.TempDir(), nil, false) + require.Error(t, err) + assert.Contains(t, err.Error(), "maximum total extracted size") +} + +func TestExtractTarball_EnforcesFileCountLimit(t *testing.T) { + orig := maxExtractFileCount + maxExtractFileCount = 2 + defer func() { maxExtractFileCount = orig }() + + client := NewClient(testutil.NewTestLogger()) + tarball := buildTarGz(t, map[string]string{ + "a.txt": "1", + "b.txt": "2", + "c.txt": "3", + }) + + err := client.extractTarball(tarball, "", t.TempDir(), nil, false) + require.Error(t, err) + assert.Contains(t, err.Error(), "too many entries") +} + +func TestExtractTarball_RejectsPathTraversal(t *testing.T) { + client := NewClient(testutil.NewTestLogger()) + destDir := t.TempDir() + + tarball := buildTarGz(t, map[string]string{ + "../../etc/passwd": "evil", + }) + + err := client.extractTarball(tarball, "", destDir, nil, false) + require.Error(t, err) + assert.Contains(t, err.Error(), "illegal file path") + + entries, _ := os.ReadDir(destDir) + assert.Empty(t, entries) +}