Skip to content

Commit 67a415c

Browse files
xavpaicexavpaice
authored
fix: prevent tar-slip path traversal in archive extraction (#5979)
* fix: prevent tar-slip path traversal in archive extraction Add a shared SafeArchivePath helper in pkg/archiveutil that validates tar entry paths before writing them. Use it in all hand-rolled tar extractors (pkg/archives, pkg/util, pkg/archiveutil, pkg/image, pkg/upstream) and add regression tests for dot-dot and absolute path traversal attempts. * fix: detect symlink escapes in archive extraction SafeArchivePath now walks each parent component of the target path and rejects any existing symlink as a potential escape. This prevents an archive entry like link/pwned from being written outside the extraction root when dest/link points to a directory outside the root. Add a regression test that creates a symlink in the destination and verifies the entry is rejected.
1 parent 1b05bb7 commit 67a415c

8 files changed

Lines changed: 369 additions & 18 deletions

File tree

‎pkg/archives/archives.go‎

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

1111
"github.com/pkg/errors"
12+
"github.com/replicatedhq/kots/pkg/archiveutil"
1213
)
1314

1415
// todo, figure out why this doesn't use the mholt tgz archiver that we
@@ -146,10 +147,13 @@ func ExtractTGZArchiveFromReader(tgzReader io.Reader, destDir string) error {
146147
}
147148

148149
err = func() error {
149-
fileName := filepath.Join(destDir, hdr.Name)
150+
fileName, err := archiveutil.SafeArchivePath(destDir, hdr.Name)
151+
if err != nil {
152+
return errors.Wrapf(err, "invalid archive entry %q", hdr.Name)
153+
}
150154

151155
filePath, _ := filepath.Split(fileName)
152-
err := os.MkdirAll(filePath, 0755)
156+
err = os.MkdirAll(filePath, 0755)
153157
if err != nil {
154158
return errors.Wrapf(err, "failed to create directory %q", filePath)
155159
}

‎pkg/archives/archives_test.go‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,16 @@
11
package archives
22

33
import (
4+
"archive/tar"
5+
"bytes"
6+
"compress/gzip"
47
"encoding/base64"
8+
"os"
9+
"path/filepath"
510
"testing"
11+
12+
"github.com/stretchr/testify/assert"
13+
"github.com/stretchr/testify/require"
614
)
715

816
func TestIsTGZ(t *testing.T) {
@@ -41,3 +49,71 @@ func TestIsTGZ(t *testing.T) {
4149
})
4250
}
4351
}
52+
53+
func TestExtractTGZArchiveFromReader_TarSlip(t *testing.T) {
54+
dir := t.TempDir()
55+
destDir := filepath.Join(dir, "dest")
56+
57+
tests := []struct {
58+
name string
59+
entryName string
60+
wantErr bool
61+
wantFile string
62+
}{
63+
{
64+
name: "traversal with dot-dot",
65+
entryName: "../../pwned",
66+
wantErr: true,
67+
},
68+
{
69+
name: "nested traversal",
70+
entryName: "foo/../../pwned",
71+
wantErr: true,
72+
},
73+
{
74+
name: "absolute path",
75+
entryName: "/etc/passwd",
76+
wantErr: true,
77+
},
78+
{
79+
name: "normal file",
80+
entryName: "app/foo.txt",
81+
wantErr: false,
82+
wantFile: "app/foo.txt",
83+
},
84+
}
85+
86+
for _, tt := range tests {
87+
t.Run(tt.name, func(t *testing.T) {
88+
require.NoError(t, os.RemoveAll(destDir))
89+
require.NoError(t, os.MkdirAll(destDir, 0755))
90+
91+
var buf bytes.Buffer
92+
gw := gzip.NewWriter(&buf)
93+
tw := tar.NewWriter(gw)
94+
err := tw.WriteHeader(&tar.Header{
95+
Typeflag: tar.TypeReg,
96+
Name: tt.entryName,
97+
Size: 4,
98+
Mode: 0644,
99+
})
100+
require.NoError(t, err)
101+
_, err = tw.Write([]byte("data"))
102+
require.NoError(t, err)
103+
require.NoError(t, tw.Close())
104+
require.NoError(t, gw.Close())
105+
106+
err = ExtractTGZArchiveFromReader(bytes.NewReader(buf.Bytes()), destDir)
107+
if tt.wantErr {
108+
require.Error(t, err)
109+
_, err := os.Stat(filepath.Join(dir, "pwned"))
110+
require.True(t, os.IsNotExist(err), "traversal file was written outside dest dir")
111+
return
112+
}
113+
require.NoError(t, err)
114+
content, err := os.ReadFile(filepath.Join(destDir, tt.wantFile))
115+
require.NoError(t, err)
116+
assert.Equal(t, "data", string(content))
117+
})
118+
}
119+
}

‎pkg/archiveutil/tgz.go‎

Lines changed: 66 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,11 @@ func extractFileToDisk(fi archives.FileInfo, dest string, stripComponents int) e
9595
}
9696
}
9797

98-
destPath := filepath.Join(dest, name)
98+
destPath, err := SafeArchivePath(dest, name)
99+
if err != nil {
100+
return err
101+
}
102+
99103
if fi.IsDir() {
100104
return os.MkdirAll(destPath, os.ModePerm)
101105
}
@@ -106,8 +110,7 @@ func extractFileToDisk(fi archives.FileInfo, dest string, stripComponents int) e
106110
}
107111
defer src.Close()
108112

109-
err = os.MkdirAll(filepath.Dir(destPath), os.ModePerm)
110-
if err != nil {
113+
if err = os.MkdirAll(filepath.Dir(destPath), os.ModePerm); err != nil {
111114
return err
112115
}
113116

@@ -123,3 +126,63 @@ func extractFileToDisk(fi archives.FileInfo, dest string, stripComponents int) e
123126

124127
return os.Chmod(destPath, fi.Mode().Perm())
125128
}
129+
130+
// SafeArchivePath returns the absolute path within destDir where an archive
131+
// entry named hdrName should be written, or an error if hdrName attempts to
132+
// escape destDir. Existing symlinks in the destination are treated as escapes
133+
// so that an entry whose parent path is a symlink cannot be written outside
134+
// the extraction root.
135+
func SafeArchivePath(destDir, hdrName string) (string, error) {
136+
if filepath.IsAbs(hdrName) {
137+
return "", errors.Errorf("illegal absolute path in archive: %q", hdrName)
138+
}
139+
140+
destDir, err := filepath.Abs(destDir)
141+
if err != nil {
142+
return "", errors.Wrap(err, "failed to resolve destination directory")
143+
}
144+
145+
fileName := filepath.Join(destDir, hdrName)
146+
rel, err := filepath.Rel(destDir, fileName)
147+
if err != nil {
148+
return "", errors.Errorf("illegal path in archive: %q", hdrName)
149+
}
150+
if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
151+
return "", errors.Errorf("illegal path in archive: %q", hdrName)
152+
}
153+
154+
if err := checkSymlinkEscape(destDir, fileName); err != nil {
155+
return "", err
156+
}
157+
158+
return fileName, nil
159+
}
160+
161+
// checkSymlinkEscape verifies that no existing component of fileName below
162+
// destDir is a symbolic link. This prevents an archive entry whose parent path
163+
// is a symlink from being written outside the extraction root.
164+
func checkSymlinkEscape(destDir, fileName string) error {
165+
rel, err := filepath.Rel(destDir, fileName)
166+
if err != nil {
167+
return err
168+
}
169+
170+
current := destDir
171+
for _, part := range strings.Split(rel, string(filepath.Separator)) {
172+
if part == "" || part == "." {
173+
continue
174+
}
175+
current = filepath.Join(current, part)
176+
fi, err := os.Lstat(current)
177+
if err != nil {
178+
if os.IsNotExist(err) {
179+
continue
180+
}
181+
return err
182+
}
183+
if fi.Mode()&os.ModeSymlink != 0 {
184+
return errors.Errorf("symlink escape attempt in archive path: %q", current)
185+
}
186+
}
187+
return nil
188+
}

‎pkg/archiveutil/tgz_test.go‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,3 +66,112 @@ func TestExtractTGZ_overwriteExisting(t *testing.T) {
6666
require.NoError(t, err)
6767
assert.Equal(t, "Hello, Another World!", string(content))
6868
}
69+
70+
func TestExtractTGZ_TarSlip(t *testing.T) {
71+
dir := t.TempDir()
72+
destDir := filepath.Join(dir, "dest")
73+
74+
tests := []struct {
75+
name string
76+
entryName string
77+
wantErr bool
78+
wantFile string
79+
}{
80+
{
81+
name: "traversal with dot-dot",
82+
entryName: "../../pwned",
83+
wantErr: true,
84+
},
85+
{
86+
name: "nested traversal",
87+
entryName: "foo/../../pwned",
88+
wantErr: true,
89+
},
90+
{
91+
name: "absolute path",
92+
entryName: "/etc/passwd",
93+
wantErr: true,
94+
},
95+
{
96+
name: "normal file",
97+
entryName: "app/foo.txt",
98+
wantErr: false,
99+
wantFile: "app/foo.txt",
100+
},
101+
}
102+
103+
for _, tt := range tests {
104+
t.Run(tt.name, func(t *testing.T) {
105+
require.NoError(t, os.RemoveAll(destDir))
106+
require.NoError(t, os.MkdirAll(destDir, 0755))
107+
108+
src := filepath.Join(dir, "test.tar.gz")
109+
f, err := os.Create(src)
110+
require.NoError(t, err)
111+
defer f.Close()
112+
113+
gw := gzip.NewWriter(f)
114+
tw := tar.NewWriter(gw)
115+
err = tw.WriteHeader(&tar.Header{
116+
Typeflag: tar.TypeReg,
117+
Name: tt.entryName,
118+
Size: 4,
119+
Mode: 0644,
120+
})
121+
require.NoError(t, err)
122+
_, err = tw.Write([]byte("data"))
123+
require.NoError(t, err)
124+
require.NoError(t, tw.Close())
125+
require.NoError(t, gw.Close())
126+
127+
err = ExtractTGZ(t.Context(), src, destDir)
128+
if tt.wantErr {
129+
require.Error(t, err)
130+
_, err := os.Stat(filepath.Join(dir, "pwned"))
131+
require.True(t, os.IsNotExist(err), "traversal file was written outside dest dir")
132+
return
133+
}
134+
require.NoError(t, err)
135+
content, err := os.ReadFile(filepath.Join(destDir, tt.wantFile))
136+
require.NoError(t, err)
137+
assert.Equal(t, "data", string(content))
138+
})
139+
}
140+
}
141+
142+
func TestExtractTGZ_TarSlip_SymlinkBypass(t *testing.T) {
143+
dir := t.TempDir()
144+
destDir := filepath.Join(dir, "dest")
145+
outsideDir := filepath.Join(dir, "outside")
146+
require.NoError(t, os.MkdirAll(destDir, 0755))
147+
require.NoError(t, os.MkdirAll(outsideDir, 0755))
148+
149+
linkPath := filepath.Join(destDir, "link")
150+
if err := os.Symlink(outsideDir, linkPath); err != nil {
151+
t.Skipf("unable to create symlink for test: %v", err)
152+
}
153+
154+
src := filepath.Join(dir, "test.tar.gz")
155+
f, err := os.Create(src)
156+
require.NoError(t, err)
157+
defer f.Close()
158+
159+
gw := gzip.NewWriter(f)
160+
tw := tar.NewWriter(gw)
161+
err = tw.WriteHeader(&tar.Header{
162+
Typeflag: tar.TypeReg,
163+
Name: "link/pwned",
164+
Size: 4,
165+
Mode: 0644,
166+
})
167+
require.NoError(t, err)
168+
_, err = tw.Write([]byte("data"))
169+
require.NoError(t, err)
170+
require.NoError(t, tw.Close())
171+
require.NoError(t, gw.Close())
172+
173+
err = ExtractTGZ(t.Context(), src, destDir)
174+
require.Error(t, err)
175+
_, err = os.Stat(filepath.Join(outsideDir, "pwned"))
176+
require.True(t, os.IsNotExist(err), "symlink bypass wrote file outside dest dir")
177+
}

‎pkg/image/airgap.go‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
imagespecsv1 "github.com/opencontainers/image-spec/specs-go/v1"
2121
"github.com/pkg/errors"
2222
"github.com/replicatedhq/kots/pkg/archives"
23+
"github.com/replicatedhq/kots/pkg/archiveutil"
2324
dockerarchive "github.com/replicatedhq/kots/pkg/docker/archive"
2425
dockerregistry "github.com/replicatedhq/kots/pkg/docker/registry"
2526
dockerregistrytypes "github.com/replicatedhq/kots/pkg/docker/registry/types"
@@ -79,7 +80,10 @@ func ExtractAppAirgapArchive(archive string, destDir string, excludeImages bool,
7980
continue
8081
}
8182

82-
dstFileName := filepath.Join(destDir, header.Name)
83+
dstFileName, err := archiveutil.SafeArchivePath(destDir, header.Name)
84+
if err != nil {
85+
return errors.Wrapf(err, "invalid archive entry %q", header.Name)
86+
}
8387
if err := os.MkdirAll(filepath.Dir(dstFileName), 0755); err != nil {
8488
return errors.Wrap(err, "failed to create path")
8589
}
@@ -842,7 +846,10 @@ func PushEmbeddedClusterArtifacts(airgapBundle string, artifactsToPush *kotsv1be
842846
continue
843847
}
844848

845-
dstFilePath := filepath.Join(tmpDir, header.Name)
849+
dstFilePath, err := archiveutil.SafeArchivePath(tmpDir, header.Name)
850+
if err != nil {
851+
return errors.Wrapf(err, "invalid archive entry %q", header.Name)
852+
}
846853
if err := os.MkdirAll(filepath.Dir(dstFilePath), 0755); err != nil {
847854
return errors.Wrap(err, "failed to create path")
848855
}

0 commit comments

Comments
 (0)