From 4a753e85abdbd64938cb86b3a69e6fb8d5a02892 Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Fri, 15 May 2020 18:10:53 +0200 Subject: [PATCH] config: Protect ctlpath from annotation attack This also adds annotation for ctlpath which were not present before. It's better to implement the code consistenly right now to make sure that we don't end up with a leaky implementation tacked on later. Fixes: #3004 Signed-off-by: Christophe de Dinechin --- cli/config/configuration-acrn.toml.in | 3 ++ pkg/katautils/config.go | 41 ++++++++++--------- virtcontainers/hypervisor.go | 3 ++ virtcontainers/persist.go | 2 + virtcontainers/persist/api/config.go | 4 ++ virtcontainers/pkg/annotations/annotations.go | 3 ++ virtcontainers/pkg/oci/utils.go | 7 ++++ 7 files changed, 43 insertions(+), 20 deletions(-) diff --git a/cli/config/configuration-acrn.toml.in b/cli/config/configuration-acrn.toml.in index 3c9075c9e9..0d174b0ba2 100644 --- a/cli/config/configuration-acrn.toml.in +++ b/cli/config/configuration-acrn.toml.in @@ -20,6 +20,9 @@ image = "@IMAGEPATH@" # Each member of the list can be a regular expression # path_list = [ "@ACRNPATH@.*" ] +# List of valid annotations values for ctlpath (default: empty) +# ctlpath_list = [ "@ACRNCTLPATH@.*" ] + # Optional space-separated list of options to pass to the guest kernel. # For example, use `kernel_params = "vsyscall=emulate"` if you are having # trouble running pre-2.15 glibc. diff --git a/pkg/katautils/config.go b/pkg/katautils/config.go index 632b89a5c1..498d564511 100644 --- a/pkg/katautils/config.go +++ b/pkg/katautils/config.go @@ -715,26 +715,27 @@ func newAcrnHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { } return vc.HypervisorConfig{ - HypervisorPath: hypervisor, - HypervisorPathList: h.HypervisorPathList, - KernelPath: kernel, - ImagePath: image, - HypervisorCtlPath: hypervisorctl, - FirmwarePath: firmware, - KernelParams: vc.DeserializeParams(strings.Fields(kernelParams)), - NumVCPUs: h.defaultVCPUs(), - DefaultMaxVCPUs: h.defaultMaxVCPUs(), - MemorySize: h.defaultMemSz(), - MemSlots: h.defaultMemSlots(), - EntropySource: h.GetEntropySource(), - DefaultBridges: h.defaultBridges(), - HugePages: h.HugePages, - Mlock: !h.Swap, - Debug: h.Debug, - DisableNestingChecks: h.DisableNestingChecks, - BlockDeviceDriver: blockDriver, - DisableVhostNet: h.DisableVhostNet, - GuestHookPath: h.guestHookPath(), + HypervisorPath: hypervisor, + HypervisorPathList: h.HypervisorPathList, + KernelPath: kernel, + ImagePath: image, + HypervisorCtlPath: hypervisorctl, + HypervisorCtlPathList: h.CtlPathList, + FirmwarePath: firmware, + KernelParams: vc.DeserializeParams(strings.Fields(kernelParams)), + NumVCPUs: h.defaultVCPUs(), + DefaultMaxVCPUs: h.defaultMaxVCPUs(), + MemorySize: h.defaultMemSz(), + MemSlots: h.defaultMemSlots(), + EntropySource: h.GetEntropySource(), + DefaultBridges: h.defaultBridges(), + HugePages: h.HugePages, + Mlock: !h.Swap, + Debug: h.Debug, + DisableNestingChecks: h.DisableNestingChecks, + BlockDeviceDriver: blockDriver, + DisableVhostNet: h.DisableVhostNet, + GuestHookPath: h.guestHookPath(), }, nil } diff --git a/virtcontainers/hypervisor.go b/virtcontainers/hypervisor.go index eb8f9b7c1c..95b2324444 100644 --- a/virtcontainers/hypervisor.go +++ b/virtcontainers/hypervisor.go @@ -278,6 +278,9 @@ type HypervisorConfig struct { // HypervisorPathList is the list of hypervisor paths names allowed in annotations HypervisorPathList []string + // HypervisorCtlPathList is the list of hypervisor control paths names allowed in annotations + HypervisorCtlPathList []string + // HypervisorCtlPath is the hypervisor ctl executable host path. HypervisorCtlPath string diff --git a/virtcontainers/persist.go b/virtcontainers/persist.go index b3c7fe4fcd..7b87289c9b 100644 --- a/virtcontainers/persist.go +++ b/virtcontainers/persist.go @@ -224,6 +224,7 @@ func (s *Sandbox) dumpConfig(ss *persistapi.SandboxState) { HypervisorPath: sconfig.HypervisorConfig.HypervisorPath, HypervisorPathList: sconfig.HypervisorConfig.HypervisorPathList, HypervisorCtlPath: sconfig.HypervisorConfig.HypervisorCtlPath, + HypervisorCtlPathList: sconfig.HypervisorConfig.HypervisorCtlPathList, JailerPath: sconfig.HypervisorConfig.JailerPath, JailerPathList: sconfig.HypervisorConfig.JailerPathList, BlockDeviceDriver: sconfig.HypervisorConfig.BlockDeviceDriver, @@ -516,6 +517,7 @@ func loadSandboxConfig(id string) (*SandboxConfig, error) { HypervisorPath: hconf.HypervisorPath, HypervisorPathList: hconf.HypervisorPathList, HypervisorCtlPath: hconf.HypervisorCtlPath, + HypervisorCtlPathList: hconf.HypervisorCtlPathList, JailerPath: hconf.JailerPath, JailerPathList: hconf.JailerPathList, BlockDeviceDriver: hconf.BlockDeviceDriver, diff --git a/virtcontainers/persist/api/config.go b/virtcontainers/persist/api/config.go index d15be7750c..2d126ddbc0 100644 --- a/virtcontainers/persist/api/config.go +++ b/virtcontainers/persist/api/config.go @@ -63,6 +63,10 @@ type HypervisorConfig struct { // HypervisorCtlPath is the hypervisor ctl executable host path. HypervisorCtlPath string + // HypervisorCtlPathList is the list of hypervisor control paths names allowed in annotations + HypervisorCtlPathList []string + + // HypervisorCtlPath is the hypervisor ctl executable host path. // JailerPath is the jailer executable host path. JailerPath string diff --git a/virtcontainers/pkg/annotations/annotations.go b/virtcontainers/pkg/annotations/annotations.go index 33eaaa0c2b..39109805d0 100644 --- a/virtcontainers/pkg/annotations/annotations.go +++ b/virtcontainers/pkg/annotations/annotations.go @@ -47,6 +47,9 @@ const ( // JailerPath is a sandbox annotation for passing a per container path pointing at the jailer that will constrain the container VM. JailerPath = kataAnnotHypervisorPrefix + "jailer_path" + // CtlPath is a sandbox annotation for passing a per container path pointing at the acrn ctl binary + CtlPath = kataAnnotHypervisorPrefix + "ctlpath" + // FirmwarePath is a sandbox annotation for passing a per container path pointing at the guest firmware that will run the container VM. FirmwarePath = kataAnnotHypervisorPrefix + "firmware" diff --git a/virtcontainers/pkg/oci/utils.go b/virtcontainers/pkg/oci/utils.go index 938d6d076d..504cca315d 100644 --- a/virtcontainers/pkg/oci/utils.go +++ b/virtcontainers/pkg/oci/utils.go @@ -411,6 +411,13 @@ func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig, config.HypervisorConfig.JailerPath = value } + if value, ok := ocispec.Annotations[vcAnnotations.CtlPath]; ok { + if !regexpContains(runtime.HypervisorConfig.HypervisorCtlPathList, value) { + return fmt.Errorf("hypervisor control %v required from annotation is not valid", value) + } + config.HypervisorConfig.HypervisorCtlPath = value + } + if value, ok := ocispec.Annotations[vcAnnotations.KernelParams]; ok { if value != "" { params := vc.DeserializeParams(strings.Fields(value))