STACKITGit/cli
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Second attempt to address exploit

Browse source 
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.
This commit is contained in:
Andy Feller 2024-11-19 17:55:18 -05:00
parent 9decf1b526
commit 8da27d2c8a
3 changed files with 110 additions and 0 deletions
Download patch file Download diff file Expand all files Collapse all files

5
pkg/cmd/run/download/download.go
View file

@ -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)

102
pkg/cmd/run/download/download_test.go
View file

@ -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) {

3
pkg/cmd/run/download/zip.go
View file

@ -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))
}