improve ssh runner copying cert files

pkg/minikube/bootstrapper/certs.go

@@ -552,9 +552,11 @@ func installCertSymlinks(cr command.Runner, caCerts map[string]string) error {
 	for _, caCertFile := range caCerts {
 		dstFilename := path.Base(caCertFile)
 		certStorePath := path.Join(vmpath.GuestCertStoreDir, dstFilename)
-
-		cmd := fmt.Sprintf("test -s %s && ln -fs %s %s", caCertFile, caCertFile, certStorePath)
-		if _, err := cr.RunCmd(exec.Command("sudo", "/bin/bash", "-c", cmd)); err != nil {
+		// to avoid shell-based command exploitation will run these separately not in one command
+		if _, err := cr.RunCmd(exec.Command("sudo", "test", "-s", caCertFile)); err != nil {
+			return errors.Wrapf(err, "verify ca cert %s", caCertFile)
+		}
+		if _, err := cr.RunCmd(exec.Command("sudo", "ln", "-fs", caCertFile, certStorePath)); err != nil {
 			return errors.Wrapf(err, "create symlink for %s", caCertFile)
 		}
 
@@ -569,8 +571,11 @@ func installCertSymlinks(cr command.Runner, caCerts map[string]string) error {
 		subjectHashLink := path.Join(vmpath.GuestCertStoreDir, fmt.Sprintf("%s.0", subjectHash))
 
 		// NOTE: This symlink may exist, but point to a missing file
-		cmd = fmt.Sprintf("test -L %s || ln -fs %s %s", subjectHashLink, certStorePath, subjectHashLink)
-		if _, err := cr.RunCmd(exec.Command("sudo", "/bin/bash", "-c", cmd)); err != nil {
+		if _, err := cr.RunCmd(exec.Command("sudo", "test", "-L", subjectHashLink)); err == nil {
+			// equivalent to previous unsafe code: fmt.Sprintf("test -L %s || ln -fs %s %s", subjectHashLink, certStorePath, subjectHashLink)
+			continue
+		}
+		if _, err := cr.RunCmd(exec.Command("sudo", "ln", "-fs", certStorePath, subjectHashLink)); err != nil {
 			return errors.Wrapf(err, "create symlink for %s", caCertFile)
 		}
 	}

pkg/minikube/bootstrapper/certs_test.go

@@ -29,6 +29,7 @@ import (
 	"k8s.io/minikube/pkg/util"
 )
 
+// TestSetupCerts verifies that the certificate setup logic initializes the required files and directories without error.
 func TestSetupCerts(t *testing.T) {
 	tempDir := tests.MakeTempDir(t)
 
@@ -53,13 +54,15 @@ func TestSetupCerts(t *testing.T) {
 		t.Fatalf("error generating certificate: %v", err)
 	}
 
-	expected := map[string]string{
-		`sudo /bin/bash -c "test -s /usr/share/ca-certificates/mycert.pem && ln -fs /usr/share/ca-certificates/mycert.pem /etc/ssl/certs/mycert.pem"`:             "-",
-		`sudo /bin/bash -c "test -s /usr/share/ca-certificates/minikubeCA.pem && ln -fs /usr/share/ca-certificates/minikubeCA.pem /etc/ssl/certs/minikubeCA.pem"`: "-",
+	expectedToRun := map[string]string{
+		`sudo test -s /usr/share/ca-certificates/mycert.pem`:                                  "-",
+		`sudo ln -fs /usr/share/ca-certificates/mycert.pem /etc/ssl/certs/mycert.pem`:         "-",
+		`sudo test -s /usr/share/ca-certificates/minikubeCA.pem`:                              "-",
+		`sudo ln -fs /usr/share/ca-certificates/minikubeCA.pem /etc/ssl/certs/minikubeCA.pem`: "-",
 		`date -u +%d-%m-%y-%T`: time.Now().Format("02-01-06-15:04:05"),
 	}
 	f := command.NewFakeCommandRunner()
-	f.SetCommandToOutput(expected)
+	f.SetCommandToOutput(expectedToRun)
 
 	p := command.NewFakeCommandRunner()
 	p.SetCommandToOutput(map[string]string{})

pkg/minikube/command/ssh_runner.go

@@ -404,16 +404,17 @@ func (s *SSHRunner) Copy(f assets.CopyableFile) error {
 		return nil
 	})
 
-	scp := fmt.Sprintf("sudo mkdir -p %s && sudo scp -t %s", f.GetTargetDir(), f.GetTargetDir())
+	dir := f.GetTargetDir()
+	scpCmd := fmt.Sprintf("%s && %s", shellquote.Join("sudo", "mkdir", "-p", dir), shellquote.Join("sudo", "scp", "-t", dir))
 	mtime, err := f.GetModTime()
 	if err != nil {
 		klog.Infof("error getting modtime for %s: %v", dst, err)
 	} else if mtime != (time.Time{}) {
-		scp += fmt.Sprintf(" && sudo touch -d \"%s\" %s", mtime.Format(layout), dst)
+		scpCmd = fmt.Sprintf("%s && %s", scpCmd, shellquote.Join("sudo", "touch", "-d", mtime.Format(layout), dst))
 	}
-	out, err := sess.CombinedOutput(scp)
+	out, err := sess.CombinedOutput(scpCmd)
 	if err != nil {
-		return fmt.Errorf("%s: %s\noutput: %s", scp, err, out)
+		return fmt.Errorf("%s: %s\noutput: %s", scpCmd, err, out)
 	}
 	return g.Wait()
 }
@@ -489,7 +490,7 @@ func (s *SSHRunner) CopyFrom(f assets.CopyableFile) error {
 		return nil
 	})
 
-	scp := fmt.Sprintf("sudo scp -f %s", f.GetTargetPath())
+	scp := shellquote.Join("sudo", "scp", "-f", f.GetTargetPath())
 	err = sess.Start(scp)
 	if err != nil {
 		return fmt.Errorf("%s: %s", scp, err)