[PRODENG-3920] Replace hand-rolled quoting on license, credential and reset paths

pkg/configurer/common.go

@@ -38,8 +38,11 @@ func (c DockerConfigurer) GetDockerInfo(h Host) (commonconfig.DockerInfo, error)
 var errConfigEmpty = errors.New("the docker daemon config is empty")
 
 // GetDockerDaemonConfig parses docker daemon json string and populate DockerDaemonConfig struct.
+//
+// Callers that pass the result into commands (CleanupLingeringMCR) must
+// quote the returned path values; they are read from a host-controlled file.
 func (c DockerConfigurer) GetDockerDaemonConfig(dockerDaemon string) (commonconfig.DockerDaemonConfig, error) {
-	if dockerDaemon != "" {
+	if dockerDaemon == "" {
 		return commonconfig.DockerDaemonConfig{}, errConfigEmpty
 	}
 

pkg/configurer/linux.go

@@ -557,14 +557,23 @@ func (c LinuxConfigurer) CleanupLingeringMCR(h Host, dockerInfo commonconfig.Doc
 	if err != nil {
 		log.Debugf("%s: failed to create DockerDaemon config %s: %s", h, dockerConfig, err)
 	}
-
+	// dockerConfig.Root/ExecRoot come from a host-controlled daemon.json;
+	// anything that is not a clean absolute path would be executed via the
+	// sudo'd rm/umount below.
 	if dockerConfig.Root != "" {
-		dockerRootDir = dockerConfig.Root
+		if !isSafeRemotePath(dockerConfig.Root) {
+			log.Warnf("%s: ignoring unsafe docker root path from daemon config: %q", h, dockerConfig.Root)
+		} else {
+			dockerRootDir = dockerConfig.Root
+		}
 	}
 	if dockerConfig.ExecRoot != "" {
-		dockerExecRootDir = dockerConfig.ExecRoot
+		if !isSafeRemotePath(dockerConfig.ExecRoot) {
+			log.Warnf("%s: ignoring unsafe docker exec root path from daemon config: %q", h, dockerConfig.ExecRoot)
+		} else {
+			dockerExecRootDir = dockerConfig.ExecRoot
+		}
 	}
-
 	// /var/lib/ Root folder
 	c.attemptPathSudoDelete(h, dockerRootDir)
 	if idx := strings.LastIndex(dockerRootDir, "/"); idx != -1 {
@@ -576,7 +585,7 @@ func (c LinuxConfigurer) CleanupLingeringMCR(h Host, dockerInfo commonconfig.Doc
 
 	// /var/run/ Exec-root folder
 	execRootNetnsUnmount := path.Join(dockerExecRootDir, "netns/default")
-	if err := h.Sudo().Exec(fmt.Sprintf("umount %s", execRootNetnsUnmount)); err != nil {
+	if err := h.Sudo().Exec(sh.Command("umount", execRootNetnsUnmount)); err != nil {
 		log.Debugf("%s: failed to umount %s: %s", h, execRootNetnsUnmount, err)
 	}
 
@@ -585,6 +594,7 @@ func (c LinuxConfigurer) CleanupLingeringMCR(h Host, dockerInfo commonconfig.Doc
 		dockerExecRootDir = dockerExecRootDir[:idx]
 	}
 	c.attemptPathSudoDelete(h, path.Join(dockerExecRootDir, "cri-dockerd-mke.sock"))
+
 	c.attemptPathSudoDelete(h, path.Join(dockerExecRootDir, "docker.sock"))
 	c.attemptPathSudoDelete(h, constant.LinuxDefaultDockerExecRoot)
 
@@ -593,6 +603,25 @@ func (c LinuxConfigurer) CleanupLingeringMCR(h Host, dockerInfo commonconfig.Doc
 	c.attemptPathSudoDelete(h, "/lib/systemd/system/cri-dockerd-mke.socket")
 }
 
+// safeRemotePathRegexp matches an absolute path with no shell metacharacters,
+// no spaces and no traversal segments. Values that reach sudo'd rm/umount
+// commands are host-reported and must satisfy it.
+var safeRemotePathRegexp = regexp.MustCompile(`^/[a-zA-Z0-9._/-]*$`)
+
+// isSafeRemotePath reports whether a host-reported path is a plain absolute
+// path that can be used safely in a remote command.
+func isSafeRemotePath(p string) bool {
+	if !safeRemotePathRegexp.MatchString(p) ||
+		strings.Contains(p, "//") ||
+		strings.Contains(p, "/../") ||
+		strings.HasSuffix(p, "/..") ||
+		p == "/" {
+		return false
+	}
+
+	return true
+}
+
 func (c LinuxConfigurer) attemptPathSudoDelete(h Host, path string) {
 	fileInfo, err := h.Sudo().FS().Stat(path)
 	if err != nil {
@@ -605,9 +634,9 @@ func (c LinuxConfigurer) attemptPathSudoDelete(h Host, path string) {
 		return
 	}
 
-	command := fmt.Sprintf("rm %s", path)
+	command := sh.Command("rm", path)
 	if fileInfo.IsDir() {
-		command = fmt.Sprintf("rm -rf %s", path)
+		command = sh.Command("rm", "-rf", path)
 	}
 
 	if err := h.Sudo().Exec(command); err != nil {

pkg/configurer/reset_paths_test.go

@@ -0,0 +1,63 @@
+package configurer
+
+import (
+	"testing"
+
+	"github.com/k0sproject/rig/v2/sh"
+	"github.com/stretchr/testify/require"
+)
+
+func TestIsSafeRemotePath(t *testing.T) {
+	t.Parallel()
+
+	for _, tc := range []struct {
+		path string
+		safe bool
+	}{
+		{path: "/var/lib/docker", safe: true},
+		{path: "/var/lib/docker-1.2.3", safe: true},
+		{path: "/var/run/docker.sock", safe: true},
+		{path: "/var/lib/docker/cri-dockerd", safe: true},
+		{path: "var/lib/docker", safe: false},       // not absolute
+		{path: "./var/lib/docker", safe: false},     // not absolute
+		{path: "/var/lib/../docker", safe: false},   // traversal
+		{path: "/var/lib/docker/..", safe: false},   // traversal tail
+		{path: "/var/lib//docker", safe: false},     // empty segment
+		{path: "/", safe: false},                    // root itself
+		{path: "", safe: false},                     // empty
+		{path: "/var/lib/$(rm -rf /)", safe: false}, // command substitution
+		{path: "/var/lib/; rm -rf /", safe: false},  // command separator
+		{path: "/var/lib/my docker", safe: false},   // space
+		{path: "/var/lib/d;rm", safe: false},        // separator
+		{path: "`rm -rf /`", safe: false},           // backticks, not absolute
+		{path: "/tmp/x\nrm -rf /", safe: false},     // newline
+	} {
+		t.Run(tc.path, func(t *testing.T) {
+			t.Parallel()
+
+			require.Equal(t, tc.safe, isSafeRemotePath(tc.path), "path %q", tc.path)
+		})
+	}
+}
+
+// TestResetCommandsQuoteHostPaths pins the sink contract: sh.Command quotes
+// arguments by construction, so even a path that slipped past
+// isSafeRemotePath arrives as a single argument instead of executing.
+func TestResetCommandsQuoteHostPaths(t *testing.T) {
+	t.Parallel()
+
+	require.Equal(t,
+		`rm -rf /var/lib/docker`,
+		sh.Command("rm", "-rf", "/var/lib/docker"),
+	)
+	require.Contains(t,
+		sh.Command("umount", "/var/run/docker/netns/default"),
+		`/var/run/docker/netns/default`,
+	)
+	// A metacharacter-bearing path - the shape isSafeRemotePath rejects
+	// upstream - is quoted rather than executed.
+	require.Equal(t,
+		`rm -rf '/var/lib/$(id)'`,
+		sh.Command("rm", "-rf", "/var/lib/$(id)"),
+	)
+}

pkg/configurer/ubuntu/ubuntu.go

@@ -3,9 +3,11 @@ package ubuntu
 import (
 	"fmt"
 	"io/fs"
+	"net/url"
 
 	"github.com/Mirantis/launchpad/pkg/configurer"
 	commonconfig "github.com/Mirantis/launchpad/pkg/product/common/config"
+	"github.com/k0sproject/rig/v2/sh"
 )
 
 // Configurer is a generic Ubuntu level configurer implementation. Some of the configurer interface implementation
@@ -53,7 +55,12 @@ Signed-by: /usr/share/keyrings/mirantis-archive-keyring.gpg
 	// https://docs.mirantis.com/mcr/25.0/install/mcr-linux/ubuntu.html instructions
 
 	// 2. import the mirantis gpg key
-	if err := h.Sudo().Exec(fmt.Sprintf("gpg --batch --yes --output /usr/share/keyrings/mirantis-archive-keyring.gpg --dearmor <<< $(curl -fsSL %s)", gpgURL)); err != nil {
+	// engineConfig.RepoURL is user-supplied config; reject anything that is
+	// not a plain http(s) URL before it reaches a sudo'd command line.
+	if _, err := url.ParseRequestURI(gpgURL); err != nil {
+		return fmt.Errorf("invalid MCR repo URL %q: %w", gpgURL, err)
+	}
+	if err := h.Sudo().Exec(sh.Command("bash", "-c", fmt.Sprintf("gpg --batch --yes --output /usr/share/keyrings/mirantis-archive-keyring.gpg --dearmor <<< \"$(curl -fsSL %s)\"", gpgURL))); err != nil {
 		return fmt.Errorf("could not install the Mirantis Ubuntu GPG signing key")
 	}
 

pkg/product/mke/phase/install_mke.go

@@ -13,6 +13,7 @@ import (
 	mkeconfig "github.com/Mirantis/launchpad/pkg/product/mke/config"
 	"github.com/Mirantis/launchpad/pkg/util/installutil"
 	"github.com/k0sproject/rig/v2/cmd"
+	"github.com/k0sproject/rig/v2/sh/shellescape"
 	log "github.com/sirupsen/logrus"
 )
 
@@ -94,10 +95,10 @@ func (p *InstallMKE) Run() error {
 	}
 
 	if p.Config.Spec.MKE.AdminUsername != "" {
-		installFlags.AddUnlessExist(fmt.Sprintf("--admin-username='%s'", p.Config.Spec.MKE.AdminUsername))
+		installFlags.AddUnlessExist(fmt.Sprintf("--admin-username %s", shellescape.Quote(p.Config.Spec.MKE.AdminUsername)))
 	}
 	if p.Config.Spec.MKE.AdminPassword != "" {
-		installFlags.AddUnlessExist(fmt.Sprintf("--admin-password='%s'", p.Config.Spec.MKE.AdminPassword))
+		installFlags.AddUnlessExist(fmt.Sprintf("--admin-password %s", shellescape.Quote(p.Config.Spec.MKE.AdminPassword)))
 	}
 
 	log.Debugf("%s: install flags: %s", p.leader, installFlags.Join())

pkg/util/installutil/installutil.go

@@ -3,6 +3,9 @@ package installutil
 import (
 	"fmt"
 	"os"
+	"strings"
+
+	"github.com/k0sproject/rig/v2/sh/shellescape"
 )
 
 // Shared install utility between install phases for different products
@@ -13,6 +16,10 @@ func SetupLicenseFile(licenseFilePath string) (string, error) {
 	if err != nil {
 		return "", fmt.Errorf("failed to read license file: %w", err)
 	}
-	licenseFlag := fmt.Sprintf("--license '%s'", string(license))
+
+	// The license is embedded in a command line that a remote login shell
+	// executes; the whole flag value must survive as a single shell word.
+	licenseFlag := fmt.Sprintf("--license %s", shellescape.Quote(strings.TrimSpace(string(license))))
+
 	return licenseFlag, nil
 }