Skip to content

Commit 317e689

Browse files
authored
fix: add path sanitization to prevent directory traversal in downloader (#889)
1 parent bd1c520 commit 317e689

2 files changed

Lines changed: 94 additions & 2 deletions

File tree

pkg/distribution/huggingface/downloader.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"sync"
1212

1313
"github.com/docker/model-runner/pkg/distribution/internal/progress"
14+
"github.com/docker/model-runner/pkg/internal/archive"
1415
)
1516

1617
// Downloader manages file downloads from HuggingFace repositories
@@ -142,8 +143,11 @@ func (d *Downloader) DownloadAll(ctx context.Context, files []RepoFile, progress
142143

143144
// downloadFileWithProgress downloads a single file with progress reporting
144145
func (d *Downloader) downloadFileWithProgress(ctx context.Context, file RepoFile, totalImageSize uint64, progressWriter io.Writer) (string, error) {
145-
// Create local file path (preserve directory structure)
146-
localPath := filepath.Join(d.tempDir, file.Path)
146+
// Validate file path to prevent directory traversal attacks
147+
localPath, err := archive.CheckRelative(d.tempDir, file.Path)
148+
if err != nil {
149+
return "", fmt.Errorf("invalid file path %q: %w", file.Path, err)
150+
}
147151

148152
// Ensure parent directory exists
149153
if err := os.MkdirAll(filepath.Dir(localPath), 0o755); err != nil {
Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
package huggingface
2+
3+
import (
4+
"path/filepath"
5+
"testing"
6+
7+
"github.com/docker/model-runner/pkg/internal/archive"
8+
)
9+
10+
func TestCheckRelativeRejectsPathTraversal(t *testing.T) {
11+
baseDir := t.TempDir()
12+
13+
tests := []struct {
14+
name string
15+
filePath string
16+
wantErr bool
17+
wantPath string // expected result when no error
18+
}{
19+
{
20+
name: "simple filename",
21+
filePath: "model.safetensors",
22+
wantErr: false,
23+
wantPath: filepath.Join(baseDir, "model.safetensors"),
24+
},
25+
{
26+
name: "nested subdirectory",
27+
filePath: "subdir/model.gguf",
28+
wantErr: false,
29+
wantPath: filepath.Join(baseDir, "subdir/model.gguf"),
30+
},
31+
{
32+
name: "deeply nested path",
33+
filePath: "a/b/c/config.json",
34+
wantErr: false,
35+
wantPath: filepath.Join(baseDir, "a/b/c/config.json"),
36+
},
37+
{
38+
name: "path traversal with dot-dot",
39+
filePath: "../../etc/passwd",
40+
wantErr: true,
41+
},
42+
{
43+
name: "path traversal single level",
44+
filePath: "../evil.txt",
45+
wantErr: true,
46+
},
47+
{
48+
name: "path traversal embedded in subdir",
49+
filePath: "subdir/../../etc/shadow",
50+
wantErr: true,
51+
},
52+
{
53+
name: "absolute path is rejected",
54+
filePath: "/etc/passwd",
55+
wantErr: true,
56+
},
57+
{
58+
name: "dot-dot at boundary of base name",
59+
filePath: "../" + filepath.Base(baseDir) + "-other/evil.txt",
60+
wantErr: true,
61+
},
62+
{
63+
name: "path with dot segment that stays inside",
64+
filePath: "subdir/../model.gguf",
65+
wantErr: false,
66+
wantPath: filepath.Join(baseDir, "model.gguf"),
67+
},
68+
}
69+
70+
for _, tt := range tests {
71+
t.Run(tt.name, func(t *testing.T) {
72+
got, err := archive.CheckRelative(baseDir, tt.filePath)
73+
if tt.wantErr {
74+
if err == nil {
75+
t.Errorf("CheckRelative(%q, %q) = %q, want error", baseDir, tt.filePath, got)
76+
}
77+
return
78+
}
79+
if err != nil {
80+
t.Errorf("CheckRelative(%q, %q) returned unexpected error: %v", baseDir, tt.filePath, err)
81+
return
82+
}
83+
if got != tt.wantPath {
84+
t.Errorf("CheckRelative(%q, %q) = %q, want %q", baseDir, tt.filePath, got, tt.wantPath)
85+
}
86+
})
87+
}
88+
}

0 commit comments

Comments
 (0)