vttablet: harden ExecuteHook RPC and backup engine flag inputs (#19486)

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

Author: timvaillancourt

go/vt/hook/hook.go

@@ -23,11 +23,12 @@ import (
 	"io"
 	"os"
 	"os/exec"
-	"path"
+	"path/filepath"
 	"strings"
 	"syscall"
 	"time"
 
+	"vitess.io/vitess/go/fileutil"
 	vtenv "vitess.io/vitess/go/vt/env"
 	"vitess.io/vitess/go/vt/log"
 )
@@ -98,19 +99,17 @@ func NewHookWithEnv(name string, params []string, env map[string]string) *Hook {
 
 // findHook tries to locate the hook, and returns the exec.Cmd for it.
 func (hook *Hook) findHook(ctx context.Context) (*exec.Cmd, int, error) {
-	// Check the hook path.
-	if strings.Contains(hook.Name, "/") {
-		return nil, HOOK_INVALID_NAME, errors.New("hook cannot contain '/'")
-	}
-
 	// Find our root.
 	root, err := vtenv.VtRoot()
 	if err != nil {
 		return nil, HOOK_VTROOT_ERROR, fmt.Errorf("cannot get VTROOT: %v", err)
 	}
 
 	// See if the hook exists.
-	vthook := path.Join(root, "vthook", hook.Name)
+	vthook, err := fileutil.SafePathJoin(filepath.Join(root, "vthook"), hook.Name)
+	if err != nil {
+		return nil, HOOK_INVALID_NAME, fmt.Errorf("invalid hook name %q: %v", hook.Name, err)
+	}
 	_, err = os.Stat(vthook)
 	if err != nil {
 		if os.IsNotExist(err) {

go/vt/mysqlctl/mysqlshellbackupengine.go

@@ -18,6 +18,7 @@ package mysqlctl
 
 import (
 	"bufio"
+	"bytes"
 	"context"
 	"encoding/json"
 	"errors"
@@ -32,6 +33,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/google/shlex"
 	"github.com/spf13/pflag"
 
 	"vitess.io/vitess/go/fileutil"
@@ -41,6 +43,7 @@ import (
 	"vitess.io/vitess/go/vt/log"
 	"vitess.io/vitess/go/vt/mysqlctl/backupstorage"
 	tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata"
+	vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc"
 	"vitess.io/vitess/go/vt/servenv"
 	"vitess.io/vitess/go/vt/vterrors"
 )
@@ -140,14 +143,20 @@ func (be *MySQLShellBackupEngine) ExecuteBackup(ctx context.Context, params Back
 		return BackupUnusable, vterrors.Wrap(err, "can't get MySQL version")
 	}
 
-	args := []string{}
-	if mysqlShellFlags != "" {
-		args = append(args, strings.Fields(mysqlShellFlags)...)
+	args, err := shlex.Split(mysqlShellFlags)
+	if err != nil {
+		return BackupUnusable, vterrors.Wrap(err, "failed to parse --mysql-shell-flags")
+	}
+
+	// compact and validate the json input from mysqlShellDumpFlags.
+	var compactDumpFlags bytes.Buffer
+	if err := json.Compact(&compactDumpFlags, []byte(mysqlShellDumpFlags)); err != nil {
+		return BackupUnusable, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "failed to parse --mysql-shell-dump-flags as JSON: %v", err)
 	}
 
 	args = append(args, "-e", fmt.Sprintf("util.dumpInstance(%q, %s)",
 		location,
-		mysqlShellDumpFlags,
+		compactDumpFlags.String(),
 	))
 
 	// to be able to get the consistent GTID sets, we will acquire a global read lock before starting mysql shell.
@@ -362,15 +371,20 @@ func (be *MySQLShellBackupEngine) ExecuteRestore(ctx context.Context, params Res
 	}
 	defer resetFunc()
 
-	args := []string{}
+	args, err := shlex.Split(mysqlShellFlags)
+	if err != nil {
+		return nil, vterrors.Wrap(err, "failed to parse --mysql-shell-flags")
+	}
 
-	if mysqlShellFlags != "" {
-		args = append(args, strings.Fields(mysqlShellFlags)...)
+	// compact and validate the json input from mysqlShellLoadFlags.
+	var compactLoadFlags bytes.Buffer
+	if err := json.Compact(&compactLoadFlags, []byte(mysqlShellLoadFlags)); err != nil {
+		return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "failed to parse --mysql-shell-load-flags as JSON: %v", err)
 	}
 
 	args = append(args, "-e", fmt.Sprintf("util.loadDump(%q, %s)",
 		location,
-		mysqlShellLoadFlags,
+		compactLoadFlags.String(),
 	))
 
 	cmd := exec.CommandContext(ctx, "mysqlsh", args...)

go/vt/mysqlctl/xtrabackupengine.go

@@ -30,6 +30,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/google/shlex"
 	"github.com/spf13/pflag"
 
 	"vitess.io/vitess/go/ioutil"
@@ -318,7 +319,11 @@ func (be *XtrabackupEngine) backupFiles(
 		flagsToExec = append(flagsToExec, "--stream="+xtrabackupStreamMode)
 	}
 	if xtrabackupBackupFlags != "" {
-		flagsToExec = append(flagsToExec, strings.Fields(xtrabackupBackupFlags)...)
+		backupFlags, err := shlex.Split(xtrabackupBackupFlags)
+		if err != nil {
+			return replicationPosition, vterrors.Wrap(err, "failed to parse --xtrabackup-backup-flags")
+		}
+		flagsToExec = append(flagsToExec, backupFlags...)
 	}
 
 	// Create a cancellable Context for calls to bh.AddFile().
@@ -545,7 +550,11 @@ func (be *XtrabackupEngine) restoreFromBackup(ctx context.Context, cnf *Mycnf, b
 		"--target-dir=" + tempDir,
 	}
 	if xtrabackupPrepareFlags != "" {
-		flagsToExec = append(flagsToExec, strings.Fields(xtrabackupPrepareFlags)...)
+		prepareFlags, err := shlex.Split(xtrabackupPrepareFlags)
+		if err != nil {
+			return vterrors.Wrap(err, "failed to parse --xtrabackup-prepare-flags")
+		}
+		flagsToExec = append(flagsToExec, prepareFlags...)
 	}
 	prepareCmd := exec.CommandContext(ctx, restoreProgram, flagsToExec...)
 	prepareOut, err := prepareCmd.StdoutPipe()
@@ -719,7 +728,11 @@ func (be *XtrabackupEngine) extractFiles(ctx context.Context, logger logutil.Log
 		xbstreamProgram := path.Join(xtrabackupEnginePath, xbstream)
 		flagsToExec := []string{"-C", tempDir, "-xv"}
 		if xbstreamRestoreFlags != "" {
-			flagsToExec = append(flagsToExec, strings.Fields(xbstreamRestoreFlags)...)
+			restoreFlags, err := shlex.Split(xbstreamRestoreFlags)
+			if err != nil {
+				return vterrors.Wrap(err, "failed to parse --xbstream-restore-flags")
+			}
+			flagsToExec = append(flagsToExec, restoreFlags...)
 		}
 		xbstreamCmd := exec.CommandContext(ctx, xbstreamProgram, flagsToExec...)
 		logger.Infof("Executing xbstream cmd: %v %v", xbstreamProgram, flagsToExec)

go/vt/vttablet/grpctmserver/server.go

@@ -18,6 +18,7 @@ package grpctmserver
 
 import (
 	"context"
+	"path/filepath"
 	"time"
 
 	"google.golang.org/grpc"
@@ -35,6 +36,7 @@ import (
 	querypb "vitess.io/vitess/go/vt/proto/query"
 	tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata"
 	tabletmanagerservicepb "vitess.io/vitess/go/vt/proto/tabletmanagerservice"
+	vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc"
 )
 
 // server is the gRPC implementation of the RPC server
@@ -64,6 +66,9 @@ func (s *server) Sleep(ctx context.Context, request *tabletmanagerdatapb.SleepRe
 func (s *server) ExecuteHook(ctx context.Context, request *tabletmanagerdatapb.ExecuteHookRequest) (response *tabletmanagerdatapb.ExecuteHookResponse, err error) {
 	defer s.tm.HandleRPCPanic(ctx, "ExecuteHook", request, response, true /*verbose*/, &err)
 	ctx = callinfo.GRPCCallInfo(ctx)
+	if request.Name == "" || filepath.Base(request.Name) != request.Name {
+		return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "hook name must be a basename, got %q", request.Name)
+	}
 	response = &tabletmanagerdatapb.ExecuteHookResponse{}
 	hr := s.tm.ExecuteHook(ctx, &hook.Hook{
 		Name:       request.Name,

go/vt/vttablet/tmrpctest/test_tm_rpc.go

@@ -27,6 +27,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"google.golang.org/protobuf/proto"
 
@@ -594,6 +595,13 @@ func tmRPCTestExecuteHook(ctx context.Context, t *testing.T, client tmclient.Tab
 	compareError(t, "ExecuteHook", err, hr, testExecuteHookHookResult)
 }
 
+func tmRPCTestExecuteHookInvalidName(ctx context.Context, t *testing.T, client tmclient.TabletManagerClient, tablet *topodatapb.Tablet) {
+	for _, name := range []string{"", "../etc/passwd", "/bin/ls"} {
+		_, err := client.ExecuteHook(ctx, tablet, &hook.Hook{Name: name})
+		assert.ErrorContains(t, err, "hook name must be a basename")
+	}
+}
+
 func tmRPCTestExecuteHookPanic(ctx context.Context, t *testing.T, client tmclient.TabletManagerClient, tablet *topodatapb.Tablet) {
 	_, err := client.ExecuteHook(ctx, tablet, testExecuteHookHook)
 	expectHandleRPCPanic(t, "ExecuteHook", true /*verbose*/, err)
@@ -1597,6 +1605,7 @@ func Run(t *testing.T, client tmclient.TabletManagerClient, tablet *topodatapb.T
 	tmRPCTestChangeType(ctx, t, client, tablet)
 	tmRPCTestSleep(ctx, t, client, tablet)
 	tmRPCTestExecuteHook(ctx, t, client, tablet)
+	tmRPCTestExecuteHookInvalidName(ctx, t, client, tablet)
 	tmRPCTestRefreshState(ctx, t, client, tablet)
 	tmRPCTestRunHealthCheck(ctx, t, client, tablet)
 	tmRPCTestReloadSchema(ctx, t, client, tablet)