From 461156732d26960d90e303c4eca7a6d4ee8d92ed Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Thu, 14 May 2020 20:18:18 +0200 Subject: [PATCH] config: Protect virtio_fs_daemon annotation Sending the virtio_fs_daemon annotation can be used to execute arbitrary code on the host. In order to prevent this, restrict the values of the annotation to a list provided by the configuration file. Fixes: #3004 Signed-off-by: Christophe de Dinechin --- cli/config/configuration-clh.toml.in | 3 + .../configuration-qemu-virtiofs.toml.in | 5 +- cli/config/configuration-qemu.toml.in | 23 ++++---- pkg/katautils/config.go | 7 ++- virtcontainers/hypervisor.go | 6 ++ virtcontainers/persist.go | 4 ++ virtcontainers/persist/api/config.go | 6 ++ virtcontainers/pkg/oci/utils.go | 29 +++++++--- virtcontainers/pkg/oci/utils_test.go | 58 ++++++++++++++++--- 9 files changed, 112 insertions(+), 29 deletions(-) diff --git a/cli/config/configuration-clh.toml.in b/cli/config/configuration-clh.toml.in index 00185ea2e4..1ae10bd90a 100644 --- a/cli/config/configuration-clh.toml.in +++ b/cli/config/configuration-clh.toml.in @@ -62,6 +62,9 @@ default_memory = @DEFMEMSZ@ # Path to vhost-user-fs daemon. virtio_fs_daemon = "@DEFVIRTIOFSDAEMON@" +# List of valid annotations values for the virtiofs daemon (default: empty) +# virtio_fs_daemon_list = [ "/opt/kata/bin/virtiofsd", "/usr/.*/virtiofsd" ] + # Default size of DAX cache in MiB virtio_fs_cache_size = @DEFVIRTIOFSCACHESIZE@ diff --git a/cli/config/configuration-qemu-virtiofs.toml.in b/cli/config/configuration-qemu-virtiofs.toml.in index 46a5ff0cab..9d46eedd47 100644 --- a/cli/config/configuration-qemu-virtiofs.toml.in +++ b/cli/config/configuration-qemu-virtiofs.toml.in @@ -104,6 +104,9 @@ shared_fs = "@DEFSHAREDFS_QEMU_VIRTIOFS@" # Path to vhost-user-fs daemon. virtio_fs_daemon = "@DEFVIRTIOFSDAEMON@" +# List of valid annotations values for the virtiofs daemon (default: empty) +# virtio_fs_daemon_list = [ "/opt/kata/bin/virtiofsd", "/usr/.*/virtiofsd" ] + # Default size of DAX cache in MiB virtio_fs_cache_size = @DEFVIRTIOFSCACHESIZE@ @@ -229,7 +232,7 @@ vhost_user_store_path = "@DEFVHOSTUSERSTOREPATH@" #hotplug_vfio_on_root_bus = true # If vhost-net backend for virtio-net is not desired, set to true. Default is false, which trades off -# security (vhost-net runs ring0) for network I/O performance. +# security (vhost-net runs ring0) for network I/O performance. #disable_vhost_net = true # diff --git a/cli/config/configuration-qemu.toml.in b/cli/config/configuration-qemu.toml.in index 46ce7d9b53..0b4a01c34c 100644 --- a/cli/config/configuration-qemu.toml.in +++ b/cli/config/configuration-qemu.toml.in @@ -96,10 +96,10 @@ default_memory = @DEFMEMSZ@ #enable_virtio_mem = true # Disable block device from being used for a container's rootfs. -# In case of a storage driver like devicemapper where a container's +# In case of a storage driver like devicemapper where a container's # root file system is backed by a block device, the block device is passed -# directly to the hypervisor for performance reasons. -# This flag prevents the block device from being passed to the hypervisor, +# directly to the hypervisor for performance reasons. +# This flag prevents the block device from being passed to the hypervisor, # 9pfs is used instead to pass the rootfs. disable_block_device_use = @DEFDISABLEBLOCK@ @@ -111,6 +111,9 @@ shared_fs = "@DEFSHAREDFS@" # Path to vhost-user-fs daemon. virtio_fs_daemon = "@DEFVIRTIOFSDAEMON@" +# List of valid annotations values for the virtiofs daemon (default: empty) +# virtio_fs_daemon_list = [ "/opt/kata/bin/virtiofsd", "/usr/.*/virtiofsd" ] + # Default size of DAX cache in MiB virtio_fs_cache_size = @DEFVIRTIOFSCACHESIZE@ @@ -175,7 +178,7 @@ enable_iothreads = @DEFENABLEIOTHREADS@ # Enabling this will result in the VM memory # being allocated using huge pages. # This is useful when you want to use vhost-user network -# stacks within the container. This will automatically +# stacks within the container. This will automatically # result in memory pre allocation #enable_hugepages = true @@ -203,17 +206,17 @@ vhost_user_store_path = "@DEFVHOSTUSERSTOREPATH@" # This option changes the default hypervisor and kernel parameters # to enable debug output where available. This extra output is added # to the proxy logs, but only when proxy debug is also enabled. -# +# # Default false #enable_debug = true # Disable the customizations done in the runtime when it detects # that it is running on top a VMM. This will result in the runtime # behaving as it would when running on bare metal. -# +# #disable_nesting_checks = true -# This is the msize used for 9p shares. It is the number of bytes +# This is the msize used for 9p shares. It is the number of bytes # used for 9p packet payload. #msize_9p = @DEFMSIZE9P@ @@ -228,9 +231,9 @@ vhost_user_store_path = "@DEFVHOSTUSERSTOREPATH@" # Default is false #disable_image_nvdimm = true -# VFIO devices are hotplugged on a bridge by default. +# VFIO devices are hotplugged on a bridge by default. # Enable hotplugging on root bus. This may be required for devices with -# a large PCI bar, as this is a current limitation with hotplugging on +# a large PCI bar, as this is a current limitation with hotplugging on # a bridge. This value is valid for "pc" machine type. # Default false #hotplug_vfio_on_root_bus = true @@ -243,7 +246,7 @@ vhost_user_store_path = "@DEFVHOSTUSERSTOREPATH@" #pcie_root_port = 2 # If vhost-net backend for virtio-net is not desired, set to true. Default is false, which trades off -# security (vhost-net runs ring0) for network I/O performance. +# security (vhost-net runs ring0) for network I/O performance. #disable_vhost_net = true # diff --git a/pkg/katautils/config.go b/pkg/katautils/config.go index dd028c9e1e..8d5098ea40 100644 --- a/pkg/katautils/config.go +++ b/pkg/katautils/config.go @@ -85,7 +85,7 @@ type factory struct { type hypervisor struct { Path string `toml:"path"` - PathList []string `toml:"path_list"` + HypervisorPathList []string `toml:"path_list"` JailerPath string `toml:"jailer_path"` JailerPathList []string `toml:"jailer_path_list"` Kernel string `toml:"kernel"` @@ -541,6 +541,7 @@ func newFirecrackerHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { return vc.HypervisorConfig{ HypervisorPath: hypervisor, + HypervisorPathList: h.HypervisorPathList, JailerPath: jailer, KernelPath: kernel, InitrdPath: initrd, @@ -628,6 +629,7 @@ func newQemuHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { return vc.HypervisorConfig{ HypervisorPath: hypervisor, + HypervisorPathList: h.HypervisorPathList, KernelPath: kernel, InitrdPath: initrd, ImagePath: image, @@ -713,6 +715,7 @@ func newAcrnHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { return vc.HypervisorConfig{ HypervisorPath: hypervisor, + HypervisorPathList: h.HypervisorPathList, KernelPath: kernel, ImagePath: image, HypervisorCtlPath: hypervisorctl, @@ -783,6 +786,7 @@ func newClhHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { return vc.HypervisorConfig{ HypervisorPath: hypervisor, + HypervisorPathList: h.HypervisorPathList, KernelPath: kernel, InitrdPath: initrd, ImagePath: image, @@ -801,6 +805,7 @@ func newClhHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { DisableBlockDeviceUse: h.DisableBlockDeviceUse, SharedFS: sharedFS, VirtioFSDaemon: h.VirtioFSDaemon, + VirtioFSDaemonList: h.VirtioFSDaemonList, VirtioFSCacheSize: h.VirtioFSCacheSize, VirtioFSCache: h.VirtioFSCache, MemPrealloc: h.MemPrealloc, diff --git a/virtcontainers/hypervisor.go b/virtcontainers/hypervisor.go index 95b4d9282b..0a677049f9 100644 --- a/virtcontainers/hypervisor.go +++ b/virtcontainers/hypervisor.go @@ -275,6 +275,9 @@ type HypervisorConfig struct { // HypervisorPath is the hypervisor executable host path. HypervisorPath string + // HypervisorPathList is the list of hypervisor paths names allowed in annotations + HypervisorPathList []string + // HypervisorCtlPath is the hypervisor ctl executable host path. HypervisorCtlPath string @@ -309,6 +312,9 @@ type HypervisorConfig struct { // VirtioFSDaemon is the virtio-fs vhost-user daemon path VirtioFSDaemon string + // VirtioFSDaemonList is the list of valid virtiofs names for annotations + VirtioFSDaemonList []string + // VirtioFSCache cache mode for fs version cache or "none" VirtioFSCache string diff --git a/virtcontainers/persist.go b/virtcontainers/persist.go index d96a3890ea..1322304537 100644 --- a/virtcontainers/persist.go +++ b/virtcontainers/persist.go @@ -222,6 +222,7 @@ func (s *Sandbox) dumpConfig(ss *persistapi.SandboxState) { FirmwarePath: sconfig.HypervisorConfig.FirmwarePath, MachineAccelerators: sconfig.HypervisorConfig.MachineAccelerators, HypervisorPath: sconfig.HypervisorConfig.HypervisorPath, + HypervisorPathList: sconfig.HypervisorConfig.HypervisorPathList, HypervisorCtlPath: sconfig.HypervisorConfig.HypervisorCtlPath, JailerPath: sconfig.HypervisorConfig.JailerPath, BlockDeviceDriver: sconfig.HypervisorConfig.BlockDeviceDriver, @@ -231,6 +232,7 @@ func (s *Sandbox) dumpConfig(ss *persistapi.SandboxState) { EntropySource: sconfig.HypervisorConfig.EntropySource, SharedFS: sconfig.HypervisorConfig.SharedFS, VirtioFSDaemon: sconfig.HypervisorConfig.VirtioFSDaemon, + VirtioFSDaemonList: sconfig.HypervisorConfig.VirtioFSDaemonList, VirtioFSCache: sconfig.HypervisorConfig.VirtioFSCache, VirtioFSExtraArgs: sconfig.HypervisorConfig.VirtioFSExtraArgs[:], BlockDeviceCacheSet: sconfig.HypervisorConfig.BlockDeviceCacheSet, @@ -511,6 +513,7 @@ func loadSandboxConfig(id string) (*SandboxConfig, error) { FirmwarePath: hconf.FirmwarePath, MachineAccelerators: hconf.MachineAccelerators, HypervisorPath: hconf.HypervisorPath, + HypervisorPathList: hconf.HypervisorPathList, HypervisorCtlPath: hconf.HypervisorCtlPath, JailerPath: hconf.JailerPath, BlockDeviceDriver: hconf.BlockDeviceDriver, @@ -520,6 +523,7 @@ func loadSandboxConfig(id string) (*SandboxConfig, error) { EntropySource: hconf.EntropySource, SharedFS: hconf.SharedFS, VirtioFSDaemon: hconf.VirtioFSDaemon, + VirtioFSDaemonList: hconf.VirtioFSDaemonList, VirtioFSCache: hconf.VirtioFSCache, VirtioFSExtraArgs: hconf.VirtioFSExtraArgs[:], BlockDeviceCacheSet: hconf.BlockDeviceCacheSet, diff --git a/virtcontainers/persist/api/config.go b/virtcontainers/persist/api/config.go index 34a5fd0fbf..489d2d6dbf 100644 --- a/virtcontainers/persist/api/config.go +++ b/virtcontainers/persist/api/config.go @@ -57,6 +57,9 @@ type HypervisorConfig struct { // HypervisorPath is the hypervisor executable host path. HypervisorPath string + // HypervisorPathList is the list of hypervisor paths names allowed in annotations + HypervisorPathList []string + // HypervisorCtlPath is the hypervisor ctl executable host path. HypervisorCtlPath string @@ -91,6 +94,9 @@ type HypervisorConfig struct { // VirtioFSDaemon is the virtio-fs vhost-user daemon path VirtioFSDaemon string + // VirtioFSDaemonList is the list of valid virtiofs names for annotations + VirtioFSDaemonList []string + // VirtioFSCache cache mode for fs version cache or "none" VirtioFSCache string diff --git a/virtcontainers/pkg/oci/utils.go b/virtcontainers/pkg/oci/utils.go index 527c483c3f..1df336890f 100644 --- a/virtcontainers/pkg/oci/utils.go +++ b/virtcontainers/pkg/oci/utils.go @@ -10,6 +10,7 @@ import ( "errors" "fmt" "path/filepath" + "regexp" goruntime "runtime" "strconv" "strings" @@ -202,6 +203,15 @@ func contains(s []string, e string) bool { return false } +func regexpContains(s []string, e string) bool { + for _, a := range s { + if matched, _ := regexp.MatchString(a, e); matched { + return true + } + } + return false +} + func newLinuxDeviceInfo(d specs.LinuxDevice) (*config.DeviceInfo, error) { allowedDeviceTypes := []string{"c", "b", "u", "p"} @@ -334,17 +344,17 @@ func SandboxID(spec specs.Spec) (string, error) { return "", fmt.Errorf("Could not find sandbox ID") } -func addAnnotations(ocispec specs.Spec, config *vc.SandboxConfig) error { +func addAnnotations(ocispec specs.Spec, config *vc.SandboxConfig, runtime RuntimeConfig) error { err := addAssetAnnotations(ocispec, config) if err != nil { return err } - if err := addHypervisorConfigOverrides(ocispec, config); err != nil { + if err := addHypervisorConfigOverrides(ocispec, config, runtime); err != nil { return err } - if err := addRuntimeConfigOverrides(ocispec, config); err != nil { + if err := addRuntimeConfigOverrides(ocispec, config, runtime); err != nil { return err } @@ -372,7 +382,7 @@ func addAssetAnnotations(ocispec specs.Spec, config *vc.SandboxConfig) error { return nil } -func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig) error { +func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig, runtime RuntimeConfig) error { if err := addHypervisorCPUOverrides(ocispec, config); err != nil { return err } @@ -385,7 +395,7 @@ func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig) return err } - if err := addHypervisporVirtioFsOverrides(ocispec, config); err != nil { + if err := addHypervisporVirtioFsOverrides(ocispec, config, runtime); err != nil { return err } @@ -649,7 +659,7 @@ func addHypervisorBlockOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConfig) return nil } -func addHypervisporVirtioFsOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConfig) error { +func addHypervisporVirtioFsOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConfig, runtime RuntimeConfig) error { if value, ok := ocispec.Annotations[vcAnnotations.SharedFS]; ok { supportedSharedFS := []string{config.Virtio9P, config.VirtioFS} valid := false @@ -666,6 +676,9 @@ func addHypervisporVirtioFsOverrides(ocispec specs.Spec, sbConfig *vc.SandboxCon } if value, ok := ocispec.Annotations[vcAnnotations.VirtioFSDaemon]; ok { + if !regexpContains(runtime.HypervisorConfig.VirtioFSDaemonList, value) { + return fmt.Errorf("virtiofs daemon %v required from annotation is not valid", value) + } sbConfig.HypervisorConfig.VirtioFSDaemon = value } @@ -698,7 +711,7 @@ func addHypervisporVirtioFsOverrides(ocispec specs.Spec, sbConfig *vc.SandboxCon return nil } -func addRuntimeConfigOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConfig) error { +func addRuntimeConfigOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConfig, runtime RuntimeConfig) error { if value, ok := ocispec.Annotations[vcAnnotations.DisableGuestSeccomp]; ok { disableGuestSeccomp, err := strconv.ParseBool(value) if err != nil { @@ -848,7 +861,7 @@ func SandboxConfig(ocispec specs.Spec, runtime RuntimeConfig, bundlePath, cid, c Experimental: runtime.Experimental, } - if err := addAnnotations(ocispec, &sandboxConfig); err != nil { + if err := addAnnotations(ocispec, &sandboxConfig, runtime); err != nil { return vc.SandboxConfig{}, err } diff --git a/virtcontainers/pkg/oci/utils_test.go b/virtcontainers/pkg/oci/utils_test.go index 851abfba7c..33c6026cde 100644 --- a/virtcontainers/pkg/oci/utils_test.go +++ b/virtcontainers/pkg/oci/utils_test.go @@ -695,7 +695,15 @@ func TestAddAssetAnnotations(t *testing.T) { Annotations: expectedAnnotations, } - addAnnotations(ocispec, &config) + runtimeConfig := RuntimeConfig{ + HypervisorType: vc.QemuHypervisor, + AgentType: vc.KataContainersAgent, + ProxyType: vc.KataProxyType, + ShimType: vc.KataShimType, + Console: consolePath, + } + + addAnnotations(ocispec, &config, runtimeConfig) assert.Exactly(expectedAnnotations, config.Annotations) } @@ -719,9 +727,17 @@ func TestAddAgentAnnotations(t *testing.T) { ContainerPipeSize: 1024, } + runtimeConfig := RuntimeConfig{ + HypervisorType: vc.QemuHypervisor, + AgentType: vc.KataContainersAgent, + ProxyType: vc.KataProxyType, + ShimType: vc.KataShimType, + Console: consolePath, + } + ocispec.Annotations[vcAnnotations.KernelModules] = strings.Join(expectedAgentConfig.KernelModules, KernelModulesSeparator) ocispec.Annotations[vcAnnotations.AgentContainerPipeSize] = "1024" - addAnnotations(ocispec, &config) + addAnnotations(ocispec, &config, runtimeConfig) assert.Exactly(expectedAgentConfig, config.AgentConfig) } @@ -741,8 +757,16 @@ func TestContainerPipeSizeAnnotation(t *testing.T) { ContainerPipeSize: 0, } + runtimeConfig := RuntimeConfig{ + HypervisorType: vc.QemuHypervisor, + AgentType: vc.KataContainersAgent, + ProxyType: vc.KataProxyType, + ShimType: vc.KataShimType, + Console: consolePath, + } + ocispec.Annotations[vcAnnotations.AgentContainerPipeSize] = "foo" - err := addAnnotations(ocispec, &config) + err := addAnnotations(ocispec, &config, runtimeConfig) assert.Error(err) assert.Exactly(expectedAgentConfig, config.AgentConfig) } @@ -771,8 +795,16 @@ func TestAddHypervisorAnnotations(t *testing.T) { }, } + runtimeConfig := RuntimeConfig{ + HypervisorType: vc.QemuHypervisor, + AgentType: vc.KataContainersAgent, + ProxyType: vc.KataProxyType, + ShimType: vc.KataShimType, + Console: consolePath, + } + ocispec.Annotations[vcAnnotations.KernelParams] = "vsyscall=emulate iommu=on" - addHypervisorConfigOverrides(ocispec, &config) + addHypervisorConfigOverrides(ocispec, &config, runtimeConfig) assert.Exactly(expectedHyperConfig, config.HypervisorConfig) ocispec.Annotations[vcAnnotations.DefaultVCPUs] = "1" @@ -805,7 +837,7 @@ func TestAddHypervisorAnnotations(t *testing.T) { ocispec.Annotations[vcAnnotations.PCIeRootPort] = "2" ocispec.Annotations[vcAnnotations.EntropySource] = "/dev/urandom" - addAnnotations(ocispec, &config) + addAnnotations(ocispec, &config, runtimeConfig) assert.Equal(config.HypervisorConfig.NumVCPUs, uint32(1)) assert.Equal(config.HypervisorConfig.DefaultMaxVCPUs, uint32(1)) assert.Equal(config.HypervisorConfig.MemorySize, uint32(1024)) @@ -838,16 +870,16 @@ func TestAddHypervisorAnnotations(t *testing.T) { // In case an absurd large value is provided, the config value if not over-ridden ocispec.Annotations[vcAnnotations.DefaultVCPUs] = "655536" - err := addAnnotations(ocispec, &config) + err := addAnnotations(ocispec, &config, runtimeConfig) assert.Error(err) ocispec.Annotations[vcAnnotations.DefaultVCPUs] = "-1" - err = addAnnotations(ocispec, &config) + err = addAnnotations(ocispec, &config, runtimeConfig) assert.Error(err) ocispec.Annotations[vcAnnotations.DefaultVCPUs] = "1" ocispec.Annotations[vcAnnotations.DefaultMaxVCPUs] = "-1" - err = addAnnotations(ocispec, &config) + err = addAnnotations(ocispec, &config, runtimeConfig) assert.Error(err) ocispec.Annotations[vcAnnotations.DefaultMaxVCPUs] = "1" @@ -865,12 +897,20 @@ func TestAddRuntimeAnnotations(t *testing.T) { Annotations: make(map[string]string), } + runtimeConfig := RuntimeConfig{ + HypervisorType: vc.QemuHypervisor, + AgentType: vc.KataContainersAgent, + ProxyType: vc.KataProxyType, + ShimType: vc.KataShimType, + Console: consolePath, + } + ocispec.Annotations[vcAnnotations.DisableGuestSeccomp] = "true" ocispec.Annotations[vcAnnotations.SandboxCgroupOnly] = "true" ocispec.Annotations[vcAnnotations.DisableNewNetNs] = "true" ocispec.Annotations[vcAnnotations.InterNetworkModel] = "macvtap" - addAnnotations(ocispec, &config) + addAnnotations(ocispec, &config, runtimeConfig) assert.Equal(config.DisableGuestSeccomp, true) assert.Equal(config.SandboxCgroupOnly, true) assert.Equal(config.NetworkConfig.DisableNewNetNs, true)