From 3ddc355094d6bf33320d4ddfee2bc84fb960a139 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Sat, 25 Oct 2025 22:37:36 +0200 Subject: [PATCH 01/10] Address dir traversal in file backup storage `GetBackups` RPC Signed-off-by: Tim Vaillancourt --- go/fileutil/join.go | 49 ++++++++++++++++++++++++ go/fileutil/join_test.go | 40 +++++++++++++++++++ go/vt/mysqlctl/filebackupstorage/file.go | 16 +++++--- 3 files changed, 99 insertions(+), 6 deletions(-) create mode 100644 go/fileutil/join.go create mode 100644 go/fileutil/join_test.go diff --git a/go/fileutil/join.go b/go/fileutil/join.go new file mode 100644 index 00000000000..dda102ce3e8 --- /dev/null +++ b/go/fileutil/join.go @@ -0,0 +1,49 @@ +/* +Copyright 2025 The Vitess Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package fileutil + +import ( + "errors" + "fmt" + "os" + "path" + "path/filepath" + "strings" +) + +var ErrInvalidBackupDir = errors.New("invalid backup directory") + +// SafePathJoin joins file paths using a rootPath and one or many other paths, +// returning a single absolute path. An error is returned if the joined path +// causes a directory traversal to a path outside of the provided rootPath. +func SafePathJoin(rootPath string, joinPaths ...string) (string, error) { + allPaths := []string{rootPath} + allPaths = append(allPaths, joinPaths...) + p := path.Join(allPaths...) + absPath, err := filepath.Abs(p) + if err != nil { + return p, fmt.Errorf("failed to parse backup path %q: %w", p, err) + } + absRootPath, err := filepath.Abs(rootPath) + if err != nil { + return p, fmt.Errorf("failed to parse backup root path %q: %w", rootPath, err) + } + if absPath != absRootPath && !strings.HasPrefix(absPath, absRootPath+string(os.PathSeparator)) { + return p, ErrInvalidBackupDir + } + return p, nil +} diff --git a/go/fileutil/join_test.go b/go/fileutil/join_test.go new file mode 100644 index 00000000000..1be696f4b7c --- /dev/null +++ b/go/fileutil/join_test.go @@ -0,0 +1,40 @@ +/* +Copyright 2025 The Vitess Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package fileutil + +import ( + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestSafePathJoin(t *testing.T) { + rootDir := t.TempDir() + + t.Run("success", func(t *testing.T) { + rootDir := t.TempDir() + path, err := SafePathJoin(rootDir, "good/path") + require.NoError(t, err) + require.Equal(t, filepath.Join(rootDir, "good/path"), path) + }) + + t.Run("dir-traversal", func(t *testing.T) { + _, err := SafePathJoin(rootDir, "../../..") + require.ErrorIs(t, err, ErrInvalidBackupDir) + }) +} diff --git a/go/vt/mysqlctl/filebackupstorage/file.go b/go/vt/mysqlctl/filebackupstorage/file.go index 6bacf2581ff..4f8a27490bb 100644 --- a/go/vt/mysqlctl/filebackupstorage/file.go +++ b/go/vt/mysqlctl/filebackupstorage/file.go @@ -27,12 +27,12 @@ import ( "github.com/spf13/pflag" - "vitess.io/vitess/go/os2" - "vitess.io/vitess/go/vt/mysqlctl/errors" - + "vitess.io/vitess/go/fileutil" "vitess.io/vitess/go/ioutil" + "vitess.io/vitess/go/os2" stats "vitess.io/vitess/go/vt/mysqlctl/backupstats" "vitess.io/vitess/go/vt/mysqlctl/backupstorage" + "vitess.io/vitess/go/vt/mysqlctl/errors" "vitess.io/vitess/go/vt/servenv" "vitess.io/vitess/go/vt/utils" ) @@ -147,9 +147,13 @@ func newFileBackupStorage(params backupstorage.Params) *FileBackupStorage { // ListBackups is part of the BackupStorage interface func (fbs *FileBackupStorage) ListBackups(ctx context.Context, dir string) ([]backupstorage.BackupHandle, error) { - // ReadDir already sorts the results - p := path.Join(FileBackupStorageRoot, dir) - fi, err := os.ReadDir(p) + // Check dir is not a directory traversal. + path, err := fileutil.SafePathJoin(FileBackupStorageRoot, dir) + if err != nil { + return nil, fmt.Errorf("failed to parse backup path %q: %w", path, err) + } + + fi, err := os.ReadDir(path) if err != nil { if os.IsNotExist(err) { return nil, nil From 0d798b7bc8298ce39d0bd422e3818d1f94300fc9 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Sat, 25 Oct 2025 22:39:35 +0200 Subject: [PATCH 02/10] return abs path Signed-off-by: Tim Vaillancourt --- go/fileutil/join.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/fileutil/join.go b/go/fileutil/join.go index dda102ce3e8..dbbdd2fb367 100644 --- a/go/fileutil/join.go +++ b/go/fileutil/join.go @@ -45,5 +45,5 @@ func SafePathJoin(rootPath string, joinPaths ...string) (string, error) { if absPath != absRootPath && !strings.HasPrefix(absPath, absRootPath+string(os.PathSeparator)) { return p, ErrInvalidBackupDir } - return p, nil + return absPath, nil } From 77b5ae70b6de5bf5a5bd846b9559f0f9610996fb Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Sat, 25 Oct 2025 22:41:05 +0200 Subject: [PATCH 03/10] improve test Signed-off-by: Tim Vaillancourt --- go/fileutil/join_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/go/fileutil/join_test.go b/go/fileutil/join_test.go index 1be696f4b7c..ea288843a36 100644 --- a/go/fileutil/join_test.go +++ b/go/fileutil/join_test.go @@ -30,6 +30,7 @@ func TestSafePathJoin(t *testing.T) { rootDir := t.TempDir() path, err := SafePathJoin(rootDir, "good/path") require.NoError(t, err) + require.True(t, filepath.IsAbs(path)) require.Equal(t, filepath.Join(rootDir, "good/path"), path) }) From 3d9c0266292def988592004d5acaf7e74b76c5ec Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Sat, 25 Oct 2025 22:43:24 +0200 Subject: [PATCH 04/10] generic err name Signed-off-by: Tim Vaillancourt --- go/fileutil/join.go | 4 ++-- go/fileutil/join_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go/fileutil/join.go b/go/fileutil/join.go index dbbdd2fb367..4395e739dfe 100644 --- a/go/fileutil/join.go +++ b/go/fileutil/join.go @@ -25,7 +25,7 @@ import ( "strings" ) -var ErrInvalidBackupDir = errors.New("invalid backup directory") +var ErrInvalidJoinedPath = errors.New("invalid joined path") // SafePathJoin joins file paths using a rootPath and one or many other paths, // returning a single absolute path. An error is returned if the joined path @@ -43,7 +43,7 @@ func SafePathJoin(rootPath string, joinPaths ...string) (string, error) { return p, fmt.Errorf("failed to parse backup root path %q: %w", rootPath, err) } if absPath != absRootPath && !strings.HasPrefix(absPath, absRootPath+string(os.PathSeparator)) { - return p, ErrInvalidBackupDir + return p, ErrInvalidJoinedPath } return absPath, nil } diff --git a/go/fileutil/join_test.go b/go/fileutil/join_test.go index ea288843a36..8c3807f4e53 100644 --- a/go/fileutil/join_test.go +++ b/go/fileutil/join_test.go @@ -36,6 +36,6 @@ func TestSafePathJoin(t *testing.T) { t.Run("dir-traversal", func(t *testing.T) { _, err := SafePathJoin(rootDir, "../../..") - require.ErrorIs(t, err, ErrInvalidBackupDir) + require.ErrorIs(t, err, ErrInvalidJoinedPath) }) } From ae318d4ae32ff6903ede8e03ec4c148738a83f61 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Sat, 25 Oct 2025 22:44:25 +0200 Subject: [PATCH 05/10] fix err again Signed-off-by: Tim Vaillancourt --- go/fileutil/join.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/fileutil/join.go b/go/fileutil/join.go index 4395e739dfe..de73b4776d4 100644 --- a/go/fileutil/join.go +++ b/go/fileutil/join.go @@ -36,11 +36,11 @@ func SafePathJoin(rootPath string, joinPaths ...string) (string, error) { p := path.Join(allPaths...) absPath, err := filepath.Abs(p) if err != nil { - return p, fmt.Errorf("failed to parse backup path %q: %w", p, err) + return p, fmt.Errorf("failed to parse path %q: %w", p, err) } absRootPath, err := filepath.Abs(rootPath) if err != nil { - return p, fmt.Errorf("failed to parse backup root path %q: %w", rootPath, err) + return p, fmt.Errorf("failed to parse root path %q: %w", rootPath, err) } if absPath != absRootPath && !strings.HasPrefix(absPath, absRootPath+string(os.PathSeparator)) { return p, ErrInvalidJoinedPath From 37c6de07a87016010780059f7ea55521041c6310 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Sat, 25 Oct 2025 22:45:15 +0200 Subject: [PATCH 06/10] fix err again Signed-off-by: Tim Vaillancourt --- go/fileutil/join.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/go/fileutil/join.go b/go/fileutil/join.go index de73b4776d4..ea51923578a 100644 --- a/go/fileutil/join.go +++ b/go/fileutil/join.go @@ -18,7 +18,6 @@ package fileutil import ( "errors" - "fmt" "os" "path" "path/filepath" @@ -36,11 +35,11 @@ func SafePathJoin(rootPath string, joinPaths ...string) (string, error) { p := path.Join(allPaths...) absPath, err := filepath.Abs(p) if err != nil { - return p, fmt.Errorf("failed to parse path %q: %w", p, err) + return p, err } absRootPath, err := filepath.Abs(rootPath) if err != nil { - return p, fmt.Errorf("failed to parse root path %q: %w", rootPath, err) + return p, err } if absPath != absRootPath && !strings.HasPrefix(absPath, absRootPath+string(os.PathSeparator)) { return p, ErrInvalidJoinedPath From b466724edaa0911663f09674c38827da991ba816 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Sun, 26 Oct 2025 04:33:51 +0100 Subject: [PATCH 07/10] filepath.Join Signed-off-by: Tim Vaillancourt --- go/fileutil/join.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/go/fileutil/join.go b/go/fileutil/join.go index ea51923578a..1509c02dac4 100644 --- a/go/fileutil/join.go +++ b/go/fileutil/join.go @@ -19,7 +19,6 @@ package fileutil import ( "errors" "os" - "path" "path/filepath" "strings" ) @@ -32,17 +31,17 @@ var ErrInvalidJoinedPath = errors.New("invalid joined path") func SafePathJoin(rootPath string, joinPaths ...string) (string, error) { allPaths := []string{rootPath} allPaths = append(allPaths, joinPaths...) - p := path.Join(allPaths...) + p := filepath.Join(allPaths...) absPath, err := filepath.Abs(p) if err != nil { return p, err } absRootPath, err := filepath.Abs(rootPath) if err != nil { - return p, err + return absPath, err } if absPath != absRootPath && !strings.HasPrefix(absPath, absRootPath+string(os.PathSeparator)) { - return p, ErrInvalidJoinedPath + return absPath, ErrInvalidJoinedPath } return absPath, nil } From f22456c0b20b3c0a82f55a6481ec927bcb14eed8 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Sun, 26 Oct 2025 04:43:10 +0100 Subject: [PATCH 08/10] dupe rootDir Signed-off-by: Tim Vaillancourt --- go/fileutil/join_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/go/fileutil/join_test.go b/go/fileutil/join_test.go index 8c3807f4e53..6d1240fd0d8 100644 --- a/go/fileutil/join_test.go +++ b/go/fileutil/join_test.go @@ -27,7 +27,6 @@ func TestSafePathJoin(t *testing.T) { rootDir := t.TempDir() t.Run("success", func(t *testing.T) { - rootDir := t.TempDir() path, err := SafePathJoin(rootDir, "good/path") require.NoError(t, err) require.True(t, filepath.IsAbs(path)) From bb5e435f43edb3d50cebbae099ac8de08c66d96d Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Sun, 26 Oct 2025 12:40:37 +0100 Subject: [PATCH 09/10] cleanup Signed-off-by: Tim Vaillancourt --- go/vt/mysqlctl/filebackupstorage/file.go | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/go/vt/mysqlctl/filebackupstorage/file.go b/go/vt/mysqlctl/filebackupstorage/file.go index 4f8a27490bb..b4b0c601d43 100644 --- a/go/vt/mysqlctl/filebackupstorage/file.go +++ b/go/vt/mysqlctl/filebackupstorage/file.go @@ -127,7 +127,10 @@ func (fbh *FileBackupHandle) ReadFile(ctx context.Context, filename string) (io. if !fbh.readOnly { return nil, fmt.Errorf("ReadFile cannot be called on read-write backup") } - p := path.Join(FileBackupStorageRoot, fbh.dir, fbh.name, filename) + p, err := fileutil.SafePathJoin(FileBackupStorageRoot, fbh.dir, fbh.name, filename) + if err != nil { + return nil, err + } f, err := os.Open(p) if err != nil { return nil, err @@ -177,14 +180,17 @@ func (fbs *FileBackupStorage) ListBackups(ctx context.Context, dir string) ([]ba // StartBackup is part of the BackupStorage interface func (fbs *FileBackupStorage) StartBackup(ctx context.Context, dir, name string) (backupstorage.BackupHandle, error) { // Make sure the directory exists. - p := path.Join(FileBackupStorageRoot, dir) - if err := os2.MkdirAll(p); err != nil { + p, err := fileutil.SafePathJoin(FileBackupStorageRoot, dir) + if err != nil { + return nil, err + } + if err = os2.MkdirAll(p); err != nil { return nil, err } // Create the subdirectory for this named backup. p = path.Join(p, name) - if err := os2.Mkdir(p); err != nil { + if err = os2.Mkdir(p); err != nil { return nil, err } @@ -193,7 +199,10 @@ func (fbs *FileBackupStorage) StartBackup(ctx context.Context, dir, name string) // RemoveBackup is part of the BackupStorage interface func (fbs *FileBackupStorage) RemoveBackup(ctx context.Context, dir, name string) error { - p := path.Join(FileBackupStorageRoot, dir, name) + p, err := fileutil.SafePathJoin(FileBackupStorageRoot, dir, name) + if err != nil { + return err + } return os.RemoveAll(p) } From 56fe7e6da4d37ca75feb77a0bcf4640ebc3031eb Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Mon, 27 Oct 2025 00:18:20 +0100 Subject: [PATCH 10/10] pr suggestion Signed-off-by: Tim Vaillancourt --- go/fileutil/join.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/go/fileutil/join.go b/go/fileutil/join.go index 1509c02dac4..3b282ad9dca 100644 --- a/go/fileutil/join.go +++ b/go/fileutil/join.go @@ -29,7 +29,8 @@ var ErrInvalidJoinedPath = errors.New("invalid joined path") // returning a single absolute path. An error is returned if the joined path // causes a directory traversal to a path outside of the provided rootPath. func SafePathJoin(rootPath string, joinPaths ...string) (string, error) { - allPaths := []string{rootPath} + allPaths := make([]string, 0, len(joinPaths)+1) + allPaths = append(allPaths, rootPath) allPaths = append(allPaths, joinPaths...) p := filepath.Join(allPaths...) absPath, err := filepath.Abs(p)