From 22e89f6099e4af5a1633dddfede4976cf9abd2a8 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 e249bdd267..7a746b8665 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 75002abd50..9217c0f6ad 100644 --- a/cli/config/configuration-qemu-virtiofs.toml.in +++ b/cli/config/configuration-qemu-virtiofs.toml.in @@ -110,6 +110,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@ @@ -245,7 +248,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 80d7c5fc28..cf9fd68d93 100644 --- a/cli/config/configuration-qemu.toml.in +++ b/cli/config/configuration-qemu.toml.in @@ -102,10 +102,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@ @@ -117,6 +117,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@ @@ -181,7 +184,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 @@ -219,17 +222,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@ @@ -244,9 +247,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 @@ -259,7 +262,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 a6a8aac07e..df32e9770f 100644 --- a/pkg/katautils/config.go +++ b/pkg/katautils/config.go @@ -86,7 +86,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"` @@ -566,6 +566,7 @@ func newFirecrackerHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { return vc.HypervisorConfig{ HypervisorPath: hypervisor, + HypervisorPathList: h.HypervisorPathList, JailerPath: jailer, KernelPath: kernel, InitrdPath: initrd, @@ -662,6 +663,7 @@ func newQemuHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { return vc.HypervisorConfig{ HypervisorPath: hypervisor, + HypervisorPathList: h.HypervisorPathList, KernelPath: kernel, InitrdPath: initrd, ImagePath: image, @@ -750,6 +752,7 @@ func newAcrnHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { return vc.HypervisorConfig{ HypervisorPath: hypervisor, + HypervisorPathList: h.HypervisorPathList, KernelPath: kernel, ImagePath: image, HypervisorCtlPath: hypervisorctl, @@ -820,6 +823,7 @@ func newClhHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { return vc.HypervisorConfig{ HypervisorPath: hypervisor, + HypervisorPathList: h.HypervisorPathList, KernelPath: kernel, InitrdPath: initrd, ImagePath: image, @@ -838,6 +842,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 707eca6b83..e380b27cdc 100644 --- a/virtcontainers/hypervisor.go +++ b/virtcontainers/hypervisor.go @@ -278,6 +278,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 @@ -312,6 +315,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 9bac39106a..32602f8b07 100644 --- a/virtcontainers/persist.go +++ b/virtcontainers/persist.go @@ -224,6 +224,7 @@ func (s *Sandbox) dumpConfig(ss *persistapi.SandboxState) { MachineAccelerators: sconfig.HypervisorConfig.MachineAccelerators, CPUFeatures: sconfig.HypervisorConfig.CPUFeatures, HypervisorPath: sconfig.HypervisorConfig.HypervisorPath, + HypervisorPathList: sconfig.HypervisorConfig.HypervisorPathList, HypervisorCtlPath: sconfig.HypervisorConfig.HypervisorCtlPath, JailerPath: sconfig.HypervisorConfig.JailerPath, BlockDeviceDriver: sconfig.HypervisorConfig.BlockDeviceDriver, @@ -233,6 +234,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, @@ -515,6 +517,7 @@ func loadSandboxConfig(id string) (*SandboxConfig, error) { MachineAccelerators: hconf.MachineAccelerators, CPUFeatures: hconf.CPUFeatures, HypervisorPath: hconf.HypervisorPath, + HypervisorPathList: hconf.HypervisorPathList, HypervisorCtlPath: hconf.HypervisorCtlPath, JailerPath: hconf.JailerPath, BlockDeviceDriver: hconf.BlockDeviceDriver, @@ -524,6 +527,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 45534c1891..a14109e17c 100644 --- a/virtcontainers/persist/api/config.go +++ b/virtcontainers/persist/api/config.go @@ -60,6 +60,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 @@ -94,6 +97,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 13ea829f98..42c4caf838 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" @@ -205,6 +206,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"} @@ -337,17 +347,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 } @@ -375,7 +385,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 } @@ -388,7 +398,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 } @@ -676,7 +686,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 @@ -693,6 +703,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 } @@ -725,7 +738,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 { @@ -877,7 +890,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 133c3f5014..09f637ae16 100644 --- a/virtcontainers/pkg/oci/utils_test.go +++ b/virtcontainers/pkg/oci/utils_test.go @@ -696,7 +696,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) } @@ -720,9 +728,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) } @@ -742,8 +758,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) } @@ -772,8 +796,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" @@ -809,7 +841,7 @@ func TestAddHypervisorAnnotations(t *testing.T) { ocispec.Annotations[vcAnnotations.EntropySource] = "/dev/urandom" ocispec.Annotations[vcAnnotations.IOMMUPlatform] = "true" - 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)) @@ -845,16 +877,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" @@ -872,12 +904,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)