backupengine: disallow path traversals via backup MANIFEST on restore (#19470)

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

Author: vitess-bot[bot]

go/vt/mysqlctl/builtinbackupengine.go

@@ -36,6 +36,7 @@ import (
 	"github.com/spf13/pflag"
 	"golang.org/x/sync/errgroup"
 
+	"vitess.io/vitess/go/fileutil"
 	"vitess.io/vitess/go/ioutil"
 	"vitess.io/vitess/go/mysql"
 	"vitess.io/vitess/go/mysql/replication"
@@ -179,7 +180,9 @@ func registerBuiltinBackupEngineFlags(fs *pflag.FlagSet) {
 	fs.StringVar(&builtinIncrementalRestorePath, "builtinbackup-incremental-restore-path", builtinIncrementalRestorePath, "the directory where incremental restore files, namely binlog files, are extracted to. In k8s environments, this should be set to a directory that is shared between the vttablet and mysqld pods. The path should exist. When empty, the default OS temp dir is assumed.")
 }
 
-// fullPath returns the full path of the entry, based on its type
+// fullPath returns the full path of the entry, based on its type.
+// It validates that the resolved path does not escape the base directory
+// via path traversal (e.g. "../../" sequences in fe.Name).
 func (fe *FileEntry) fullPath(cnf *Mycnf) (string, error) {
 	// find the root to use
 	var root string
@@ -196,7 +199,7 @@ func (fe *FileEntry) fullPath(cnf *Mycnf) (string, error) {
 		return "", vterrors.Errorf(vtrpcpb.Code_UNKNOWN, "unknown base: %v", fe.Base)
 	}
 
-	return path.Join(fe.ParentPath, root, fe.Name), nil
+	return fileutil.SafePathJoin(path.Join(fe.ParentPath, root), fe.Name)
 }
 
 // open attempts to open the file

go/vt/mysqlctl/builtinbackupengine_test.go

@@ -21,7 +21,9 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
+	"vitess.io/vitess/go/fileutil"
 	tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata"
 )
 
@@ -70,6 +72,93 @@ func TestGetIncrementalFromPosGTIDSet(t *testing.T) {
 	}
 }
 
+func TestFileEntryFullPath(t *testing.T) {
+	cnf := &Mycnf{
+		DataDir:               "/vt/data",
+		InnodbDataHomeDir:     "/vt/innodb-data",
+		InnodbLogGroupHomeDir: "/vt/innodb-log",
+		BinLogPath:            "/vt/binlogs/mysql-bin",
+	}
+
+	tests := []struct {
+		name      string
+		entry     FileEntry
+		wantPath  string
+		wantError error
+	}{
+		{
+			name:     "valid relative path in DataDir",
+			entry:    FileEntry{Base: backupData, Name: "mydb/table1.ibd"},
+			wantPath: "/vt/data/mydb/table1.ibd",
+		},
+		{
+			name:     "valid relative path in InnodbDataHomeDir",
+			entry:    FileEntry{Base: backupInnodbDataHomeDir, Name: "ibdata1"},
+			wantPath: "/vt/innodb-data/ibdata1",
+		},
+		{
+			name:     "valid relative path in InnodbLogGroupHomeDir",
+			entry:    FileEntry{Base: backupInnodbLogGroupHomeDir, Name: "ib_logfile0"},
+			wantPath: "/vt/innodb-log/ib_logfile0",
+		},
+		{
+			name:     "valid relative path in BinlogDir",
+			entry:    FileEntry{Base: backupBinlogDir, Name: "mysql-bin.000001"},
+			wantPath: "/vt/binlogs/mysql-bin.000001",
+		},
+		{
+			name:     "valid path with ParentPath",
+			entry:    FileEntry{Base: backupData, Name: "mydb/table1.ibd", ParentPath: "/tmp/restore"},
+			wantPath: "/tmp/restore/vt/data/mydb/table1.ibd",
+		},
+		{
+			name:      "path traversal escapes base directory",
+			entry:     FileEntry{Base: backupData, Name: "../../etc/passwd"},
+			wantError: fileutil.ErrInvalidJoinedPath,
+		},
+		{
+			name:      "path traversal with deeper nesting",
+			entry:     FileEntry{Base: backupData, Name: "mydb/../../../etc/shadow"},
+			wantError: fileutil.ErrInvalidJoinedPath,
+		},
+		{
+			name:      "path traversal to root",
+			entry:     FileEntry{Base: backupData, Name: "../../../../../etc/crontab"},
+			wantError: fileutil.ErrInvalidJoinedPath,
+		},
+		{
+			name:      "path traversal escapes ParentPath",
+			entry:     FileEntry{Base: backupData, Name: "../../../../etc/passwd", ParentPath: "/tmp/restore"},
+			wantError: fileutil.ErrInvalidJoinedPath,
+		},
+		{
+			name:     "relative path with dot-dot that stays within base",
+			entry:    FileEntry{Base: backupData, Name: "mydb/../mydb/table1.ibd"},
+			wantPath: "/vt/data/mydb/table1.ibd",
+		},
+	}
+
+	// Test unknown base separately since it returns a different error type.
+	t.Run("unknown base", func(t *testing.T) {
+		entry := FileEntry{Base: "unknown", Name: "file"}
+		_, err := entry.fullPath(cnf)
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "unknown base")
+	})
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := tt.entry.fullPath(cnf)
+			if tt.wantError != nil {
+				require.ErrorIs(t, err, tt.wantError)
+			} else {
+				require.NoError(t, err)
+				assert.Equal(t, tt.wantPath, got)
+			}
+		})
+	}
+}
+
 func TestShouldDrainForBackupBuiltIn(t *testing.T) {
 	be := &BuiltinBackupEngine{}