Second attempt to address exploit

andyfeller · andyfeller · commit 8da27d2c8ac8 · 2024-11-19T17:55:18.000-05:00
This builds off suggestion to reuse logic used already within `gh run download` for detecting path traversals.

This largely works but runs into an issue where detection logic doesn't handle non-separated traversal.
diff --git a/pkg/cmd/run/download/download.go b/pkg/cmd/run/download/download.go
@@ -169,6 +169,11 @@ func runDownload(opts *DownloadOptions) error {
 		if len(wantPatterns) != 0 || len(wantNames) != 1 {
 			destDir = filepath.Join(destDir, a.Name)
 		}
+
+		if !filepathDescendsFrom(destDir, opts.DestinationDir) {
+			return fmt.Errorf("error downloading %s: would result in path traversal", a.Name)
+		}
+
 		err := opts.Platform.Download(a.DownloadURL, destDir)
 		if err != nil {
 			return fmt.Errorf("error downloading %s: %w", a.Name, err)
diff --git a/pkg/cmd/run/download/download_test.go b/pkg/cmd/run/download/download_test.go
@@ -289,6 +289,108 @@ func Test_runDownload(t *testing.T) {
 					})
 			},
 		},
+		{
+			name: "given artifact name contains `..`, verify an error about path traversal is returned",
+			opts: DownloadOptions{
+				RunID:          "2345",
+				DestinationDir: ".",
+			},
+			mockAPI: func(p *mockPlatform) {
+				p.On("List", "2345").Return([]shared.Artifact{
+					{
+						Name:        "..",
+						DownloadURL: "http://download.com/artifact1.zip",
+						Expired:     false,
+					},
+				}, nil)
+			},
+			wantErr: "error downloading ..: would result in path traversal",
+		},
+		{
+			name: "given artifact name contains `..`, verify an error about path traversal is returned",
+			opts: DownloadOptions{
+				RunID:          "2345",
+				DestinationDir: "imaginary-dir",
+			},
+			mockAPI: func(p *mockPlatform) {
+				p.On("List", "2345").Return([]shared.Artifact{
+					{
+						Name:        "..",
+						DownloadURL: "http://download.com/artifact1.zip",
+						Expired:     false,
+					},
+				}, nil)
+			},
+			wantErr: "error downloading ..: would result in path traversal",
+		},
+		{
+			name: "given artifact name contains `../etc/passwd`, verify an error about path traversal is returned",
+			opts: DownloadOptions{
+				RunID:          "2345",
+				DestinationDir: ".",
+			},
+			mockAPI: func(p *mockPlatform) {
+				p.On("List", "2345").Return([]shared.Artifact{
+					{
+						Name:        "../etc/passwd",
+						DownloadURL: "http://download.com/artifact1.zip",
+						Expired:     false,
+					},
+				}, nil)
+			},
+			wantErr: "error downloading ../etc/passwd: would result in path traversal",
+		},
+		{
+			name: "given artifact name contains `../etc/passwd`, verify an error about path traversal is returned",
+			opts: DownloadOptions{
+				RunID:          "2345",
+				DestinationDir: "imaginary-dir",
+			},
+			mockAPI: func(p *mockPlatform) {
+				p.On("List", "2345").Return([]shared.Artifact{
+					{
+						Name:        "../etc/passwd",
+						DownloadURL: "http://download.com/artifact1.zip",
+						Expired:     false,
+					},
+				}, nil)
+			},
+			wantErr: "error downloading ../etc/passwd: would result in path traversal",
+		},
+		{
+			name: "given artifact name contains `../../etc/passwd`, verify an error about path traversal is returned",
+			opts: DownloadOptions{
+				RunID:          "2345",
+				DestinationDir: ".",
+			},
+			mockAPI: func(p *mockPlatform) {
+				p.On("List", "2345").Return([]shared.Artifact{
+					{
+						Name:        "../../etc/passwd",
+						DownloadURL: "http://download.com/artifact1.zip",
+						Expired:     false,
+					},
+				}, nil)
+			},
+			wantErr: "error downloading ../../etc/passwd: would result in path traversal",
+		},
+		{
+			name: "given artifact name contains `../../etc/passwd`, verify an error about path traversal is returned",
+			opts: DownloadOptions{
+				RunID:          "2345",
+				DestinationDir: "imaginary-dir",
+			},
+			mockAPI: func(p *mockPlatform) {
+				p.On("List", "2345").Return([]shared.Artifact{
+					{
+						Name:        "../../etc/passwd",
+						DownloadURL: "http://download.com/artifact1.zip",
+						Expired:     false,
+					},
+				}, nil)
+			},
+			wantErr: "error downloading ../../etc/passwd: would result in path traversal",
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
diff --git a/pkg/cmd/run/download/zip.go b/pkg/cmd/run/download/zip.go
@@ -73,6 +73,9 @@ func getPerm(m os.FileMode) os.FileMode {
 func filepathDescendsFrom(p, dir string) bool {
 	p = filepath.Clean(p)
 	dir = filepath.Clean(dir)
+	if dir == "." && p == ".." {
+		return false
+	}
 	if dir == "." && !filepath.IsAbs(p) {
 		return !strings.HasPrefix(p, ".."+string(filepath.Separator))
 	}
