Skip to content

Commit 6a75570

Browse files
jdomeracki-coderjdomeracki-coder
committed
fix: improve archive extraction handling
1 parent 3a9adbd commit 6a75570

5 files changed

Lines changed: 155 additions & 19 deletions

File tree

‎go.mod‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
module github.com/coder/code-marketplace
22

3-
go 1.24.11
3+
go 1.25.8
44

55
require (
66
cdr.dev/slog v1.6.1

‎storage/local.go‎

Lines changed: 32 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -94,20 +94,32 @@ func (s *Local) list(ctx context.Context) []extension {
9494
return list
9595
}
9696

97+
9798
func (s *Local) AddExtension(ctx context.Context, manifest *VSIXManifest, vsix []byte, extra ...File) (string, error) {
9899
// Extract the zip to the correct path.
99100
identity := manifest.Metadata.Identity
100101
dir := filepath.Join(s.extdir, identity.Publisher, identity.ID, Version{
101102
Version: identity.Version,
102103
TargetPlatform: identity.TargetPlatform,
103104
}.String())
104-
err := easyzip.ExtractZip(vsix, func(name string, r io.Reader) error {
105-
path := filepath.Join(dir, name)
106-
err := os.MkdirAll(filepath.Dir(path), 0o755)
107-
if err != nil {
105+
106+
// Ensure the target directory exists before opening a root on it.
107+
if err := os.MkdirAll(dir, 0o755); err != nil {
108+
return "", err
109+
}
110+
// os.Root restricts all file operations to dir, preventing path traversal
111+
// via ".." components, absolute paths, and symlink escapes (Go 1.24+).
112+
root, err := os.OpenRoot(dir)
113+
if err != nil {
114+
return "", err
115+
}
116+
defer root.Close()
117+
118+
err = easyzip.ExtractZip(vsix, func(name string, r io.Reader) error {
119+
if err := root.MkdirAll(filepath.Dir(name), 0o755); err != nil {
108120
return err
109121
}
110-
w, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o644)
122+
w, err := root.OpenFile(name, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o644)
111123
if err != nil {
112124
return err
113125
}
@@ -120,21 +132,29 @@ func (s *Local) AddExtension(ctx context.Context, manifest *VSIXManifest, vsix [
120132
}
121133

122134
// Copy the VSIX itself as well.
123-
vsixPath := filepath.Join(dir, fmt.Sprintf("%s.vsix", ExtensionVSIXNameFromManifest(manifest)))
124-
err = os.WriteFile(vsixPath, vsix, 0o644)
135+
vsixName := fmt.Sprintf("%s.vsix", ExtensionVSIXNameFromManifest(manifest))
136+
w, err := root.OpenFile(vsixName, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o644)
137+
if err != nil {
138+
return "", err
139+
}
140+
_, err = w.Write(vsix)
141+
w.Close()
125142
if err != nil {
126143
return "", err
127144
}
128145

129146
for _, file := range extra {
130-
path := filepath.Join(dir, file.RelativePath)
131-
err := os.MkdirAll(filepath.Dir(path), 0o755)
132-
if err != nil {
147+
if err := root.MkdirAll(filepath.Dir(file.RelativePath), 0o755); err != nil {
133148
return "", err
134149
}
135-
err = os.WriteFile(path, file.Content, 0o644)
150+
w, err := root.OpenFile(file.RelativePath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o644)
151+
if err != nil {
152+
return dir, xerrors.Errorf("write extra file %q: %w", file.RelativePath, err)
153+
}
154+
_, err = w.Write(file.Content)
155+
w.Close()
136156
if err != nil {
137-
return dir, xerrors.Errorf("write extra file %q: %w", path, err)
157+
return dir, xerrors.Errorf("write extra file %q: %w", file.RelativePath, err)
138158
}
139159
}
140160

‎storage/local_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ func localFactory(t *testing.T) testStorage {
1919
require.NoError(t, err)
2020
return testStorage{
2121
storage: s,
22+
dir: extdir,
2223
write: func(content []byte, elem ...string) {
2324
dest := filepath.Join(extdir, filepath.Join(elem...))
2425
err := os.MkdirAll(filepath.Dir(dest), 0o755)

‎storage/signature_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ func signed(signer bool, factory func(t *testing.T) testStorage) func(t *testing
3030
storage: storage.NewSignatureStorage(slog.Make(), key, st.storage),
3131
write: st.write,
3232
exists: st.exists,
33+
dir: st.dir,
3334
expectedManifest: exp,
3435
}
3536
}

‎storage/storage_test.go‎

Lines changed: 120 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package storage_test
22

33
import (
4+
"archive/zip"
5+
"bytes"
46
"context"
57
"errors"
68
"fmt"
@@ -25,6 +27,8 @@ type testStorage struct {
2527
storage storage.Storage
2628
write func(content []byte, elem ...string)
2729
exists func(elem ...string) bool
30+
// dir is the base extension directory (local storage only).
31+
dir string
2832

2933
expectedManifest func(man *storage.VSIXManifest)
3034
}
@@ -123,20 +127,23 @@ func TestNewStorage(t *testing.T) {
123127
func TestStorage(t *testing.T) {
124128
t.Parallel()
125129
factories := []struct {
126-
name string
127-
factory storageFactory
130+
name string
131+
factory storageFactory
132+
localOnly bool
128133
}{
129134
{
130-
name: "Local",
131-
factory: localFactory,
135+
name: "Local",
136+
factory: localFactory,
137+
localOnly: true,
132138
},
133139
{
134140
name: "Artifactory",
135141
factory: artifactoryFactory,
136142
},
137143
{
138-
name: "SignedLocal",
139-
factory: signed(true, localFactory),
144+
name: "SignedLocal",
145+
factory: signed(true, localFactory),
146+
localOnly: true,
140147
},
141148
{
142149
name: "SignedArtifactory",
@@ -148,6 +155,23 @@ func TestStorage(t *testing.T) {
148155
t.Run("AddExtension", func(t *testing.T) {
149156
testAddExtension(t, sf.factory)
150157
})
158+
if sf.localOnly {
159+
t.Run("AddExtensionZipTraversal", func(t *testing.T) {
160+
testAddExtensionZipTraversal(t, sf.factory)
161+
})
162+
t.Run("AddExtensionExtraTraversal", func(t *testing.T) {
163+
testAddExtensionExtraTraversal(t, sf.factory)
164+
})
165+
t.Run("AddExtensionZipAbsolutePath", func(t *testing.T) {
166+
testAddExtensionZipAbsolutePath(t, sf.factory)
167+
})
168+
t.Run("AddExtensionExtraAbsolutePath", func(t *testing.T) {
169+
testAddExtensionExtraAbsolutePath(t, sf.factory)
170+
})
171+
t.Run("AddExtensionSymlinkEscape", func(t *testing.T) {
172+
testAddExtensionSymlinkEscape(t, sf.factory)
173+
})
174+
}
151175
t.Run("RemoveExtension", func(t *testing.T) {
152176
testRemoveExtension(t, sf.factory)
153177
})
@@ -927,6 +951,96 @@ func testAddExtension(t *testing.T, factory storageFactory) {
927951
}
928952
}
929953

954+
// createTraversalVSIX returns a valid zip whose sole entry has a path-traversal
955+
// name, used to test zip-slip protection in AddExtension.
956+
func createTraversalVSIX(t *testing.T, entryName string) []byte {
957+
t.Helper()
958+
buf := bytes.NewBuffer(nil)
959+
zw := zip.NewWriter(buf)
960+
fw, err := zw.Create(entryName)
961+
require.NoError(t, err)
962+
_, err = fw.Write([]byte("evil"))
963+
require.NoError(t, err)
964+
require.NoError(t, zw.Close())
965+
return buf.Bytes()
966+
}
967+
968+
func testAddExtensionZipTraversal(t *testing.T, factory storageFactory) {
969+
t.Parallel()
970+
971+
f := factory(t)
972+
ext := testutil.Extensions[0]
973+
manifest := testutil.ConvertExtensionToManifest(ext, storage.Version{Version: ext.LatestVersion})
974+
vsix := createTraversalVSIX(t, "../../../../tmp/evil")
975+
_, err := f.storage.AddExtension(context.Background(), manifest, vsix)
976+
require.Error(t, err)
977+
require.Contains(t, err.Error(), "path escapes from parent")
978+
}
979+
980+
func testAddExtensionExtraTraversal(t *testing.T, factory storageFactory) {
981+
t.Parallel()
982+
983+
f := factory(t)
984+
ext := testutil.Extensions[0]
985+
manifest := testutil.ConvertExtensionToManifest(ext, storage.Version{Version: ext.LatestVersion})
986+
vsix := testutil.CreateVSIXFromManifest(t, manifest)
987+
evil := storage.File{RelativePath: "../../../../tmp/evil", Content: []byte("evil")}
988+
_, err := f.storage.AddExtension(context.Background(), manifest, vsix, evil)
989+
require.Error(t, err)
990+
require.Contains(t, err.Error(), "path escapes from parent")
991+
}
992+
993+
func testAddExtensionZipAbsolutePath(t *testing.T, factory storageFactory) {
994+
t.Parallel()
995+
996+
f := factory(t)
997+
ext := testutil.Extensions[0]
998+
manifest := testutil.ConvertExtensionToManifest(ext, storage.Version{Version: ext.LatestVersion})
999+
vsix := createTraversalVSIX(t, "/tmp/evil")
1000+
_, err := f.storage.AddExtension(context.Background(), manifest, vsix)
1001+
require.Error(t, err)
1002+
require.Contains(t, err.Error(), "path escapes from parent")
1003+
}
1004+
1005+
func testAddExtensionExtraAbsolutePath(t *testing.T, factory storageFactory) {
1006+
t.Parallel()
1007+
1008+
f := factory(t)
1009+
ext := testutil.Extensions[0]
1010+
manifest := testutil.ConvertExtensionToManifest(ext, storage.Version{Version: ext.LatestVersion})
1011+
vsix := testutil.CreateVSIXFromManifest(t, manifest)
1012+
evil := storage.File{RelativePath: "/tmp/evil", Content: []byte("evil")}
1013+
_, err := f.storage.AddExtension(context.Background(), manifest, vsix, evil)
1014+
require.Error(t, err)
1015+
require.Contains(t, err.Error(), "path escapes from parent")
1016+
}
1017+
1018+
func testAddExtensionSymlinkEscape(t *testing.T, factory storageFactory) {
1019+
t.Parallel()
1020+
1021+
f := factory(t)
1022+
ext := testutil.Extensions[0]
1023+
manifest := testutil.ConvertExtensionToManifest(ext, storage.Version{Version: ext.LatestVersion})
1024+
vsix := testutil.CreateVSIXFromManifest(t, manifest)
1025+
1026+
// Pre-create the extension directory and plant a symlink inside it that
1027+
// points to a directory outside the root. An extra file written through
1028+
// this symlink must be rejected by os.Root's symlink-escape protection.
1029+
identity := manifest.Metadata.Identity
1030+
extDir := filepath.Join(f.dir, identity.Publisher, identity.ID, identity.Version)
1031+
require.NoError(t, os.MkdirAll(extDir, 0o755))
1032+
outside := t.TempDir()
1033+
require.NoError(t, os.Symlink(outside, filepath.Join(extDir, "link")))
1034+
1035+
evil := storage.File{RelativePath: "link/evil", Content: []byte("evil")}
1036+
_, err := f.storage.AddExtension(context.Background(), manifest, vsix, evil)
1037+
require.Error(t, err)
1038+
1039+
// Confirm the file was not written to the target outside the root.
1040+
_, statErr := os.Stat(filepath.Join(outside, "evil"))
1041+
require.True(t, os.IsNotExist(statErr))
1042+
}
1043+
9301044
func testRemoveExtension(t *testing.T, factory storageFactory) {
9311045
t.Parallel()
9321046

0 commit comments

Comments
 (0)