Skip to content

Commit 3712416

Browse files
CopilotCopilot
authored
Use public permissions for extracted ZIP logs (#65463)
1 parent 78618c6 commit 3712416

2 files changed

Lines changed: 18 additions & 14 deletions

File tree

‎pkg/cli/logs_download_zip.go‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ import (
2020

2121
var logsDownloadZipLog = logger.New("cli:logs_download_zip")
2222

23+
const maxZipEntrySize = 1 * 1024 * 1024 * 1024 // 1GB
24+
2325
// unzipFile extracts a zip file to a destination directory
2426
func unzipFile(zipPath, destDir string, verbose bool) error {
2527
// Open the zip file
@@ -72,8 +74,7 @@ func extractZipFile(f *zip.File, destDir string, verbose bool) (extractErr error
7274

7375
// Decompression bomb protection - limit individual file size to 1GB
7476
// #nosec G110 -- Decompression bomb is mitigated by size check below
75-
const maxFileSize = 1 * 1024 * 1024 * 1024 // 1GB
76-
if f.UncompressedSize64 > maxFileSize {
77+
if f.UncompressedSize64 > maxZipEntrySize {
7778
logsDownloadZipLog.Printf("Rejected oversized zip entry (decompression bomb guard): %s (%d bytes)", f.Name, f.UncompressedSize64)
7879
return fmt.Errorf("file too large in zip (>1GB): %s (%d bytes)", f.Name, f.UncompressedSize64)
7980
}
@@ -83,6 +84,10 @@ func extractZipFile(f *zip.File, destDir string, verbose bool) (extractErr error
8384
return fmt.Errorf("failed to create directory: %w", err)
8485
}
8586

87+
return extractZipFileContents(f, filePath)
88+
}
89+
90+
func extractZipFileContents(f *zip.File, filePath string) (extractErr error) {
8691
// Open the file in the zip
8792
srcFile, err := f.Open()
8893
if err != nil {
@@ -91,7 +96,7 @@ func extractZipFile(f *zip.File, destDir string, verbose bool) (extractErr error
9196
defer srcFile.Close()
9297

9398
// Create the destination file
94-
destFile, err := os.OpenFile(filePath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode())
99+
destFile, err := os.OpenFile(filePath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, constants.FilePermSensitive)
95100
if err != nil {
96101
return fmt.Errorf("failed to create destination file: %w", err)
97102
}
@@ -107,7 +112,7 @@ func extractZipFile(f *zip.File, destDir string, verbose bool) (extractErr error
107112
// Copy the content with size limit enforcement.
108113
// Limit to maxFileSize+1 bytes: if exactly maxFileSize+1 bytes can be read
109114
// the archive is over the limit and must be rejected.
110-
limitedReader := io.LimitReader(srcFile, int64(maxFileSize)+1)
115+
limitedReader := io.LimitReader(srcFile, int64(maxZipEntrySize)+1)
111116
written, err := io.Copy(destFile, limitedReader)
112117
if err != nil {
113118
extractErr = fmt.Errorf("failed to extract file: %w", err)
@@ -117,7 +122,7 @@ func extractZipFile(f *zip.File, destDir string, verbose bool) (extractErr error
117122
// Verify we didn't exceed the size limit.
118123
// written == maxFileSize+1 means the reader was not exhausted, i.e. the
119124
// actual content is larger than maxFileSize.
120-
if written > int64(maxFileSize) {
125+
if written > int64(maxZipEntrySize) {
121126
extractErr = fmt.Errorf("file extraction exceeded size limit: %s", f.Name)
122127
return extractErr
123128
}

‎pkg/cli/logs_extract_zip_test.go‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"path/filepath"
1010
"testing"
1111

12+
"github.com/github/gh-aw/pkg/constants"
1213
"github.com/stretchr/testify/assert"
1314
"github.com/stretchr/testify/require"
1415
)
@@ -110,22 +111,22 @@ func TestExtractZipFileZipSlipPrevention(t *testing.T) {
110111
require.ErrorContains(t, err, "invalid file path", "Error should mention invalid path")
111112
}
112113

113-
// TestExtractZipFilePreservesMode tests that file permissions are preserved
114-
func TestExtractZipFilePreservesMode(t *testing.T) {
114+
// TestExtractZipFileUsesSensitivePermissions tests that ZIP permissions do not override the local permission policy
115+
func TestExtractZipFileUsesSensitivePermissions(t *testing.T) {
115116
t.Parallel()
116117
// Create a temporary directory for extraction
117118
tempDir := t.TempDir()
118119

119-
// Create an in-memory zip with specific file mode
120+
// Create an in-memory zip with restrictive file mode
120121
buf := new(bytes.Buffer)
121122
zipWriter := zip.NewWriter(buf)
122123

123-
// Create a file with specific mode (executable)
124+
// Create a file with owner-only permissions
124125
header := &zip.FileHeader{
125126
Name: "executable.sh",
126127
Method: zip.Deflate,
127128
}
128-
header.SetMode(0755) // Executable mode
129+
header.SetMode(0600)
129130

130131
writer, err := zipWriter.CreateHeader(header)
131132
require.NoError(t, err, "Failed to create file with header in zip")
@@ -145,14 +146,12 @@ func TestExtractZipFilePreservesMode(t *testing.T) {
145146
err = extractZipFile(zipReader.File[0], tempDir, false)
146147
require.NoError(t, err, "extractZipFile should succeed")
147148

148-
// Verify the extracted file has the correct mode
149+
// Verify the extracted file uses sensitive permissions despite the restrictive zip mode
149150
extractedPath := filepath.Join(tempDir, "executable.sh")
150151
info, err := os.Stat(extractedPath)
151152
require.NoError(t, err, "Failed to stat extracted file")
152153

153-
// Check that file is executable (at least one execute bit set)
154-
mode := info.Mode()
155-
assert.NotEqual(t, os.FileMode(0), mode&0111, "File should have execute permission")
154+
assert.Equal(t, constants.FilePermSensitive, info.Mode().Perm(), "File should have sensitive permissions")
156155
}
157156

158157
// TestExtractZipFileWithNestedDirectories tests extraction with nested paths

0 commit comments

Comments
 (0)