From 92065d8b73415ec422a06cecbc418aba27d54449 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 880430ec08..d28e264d82 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 5d90c42179..51cd0b276a 100644 --- a/pkg/katautils/config.go +++ b/pkg/katautils/config.go @@ -752,26 +752,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 51afd051c1..2eb42c4a79 100644 --- a/virtcontainers/hypervisor.go +++ b/virtcontainers/hypervisor.go @@ -281,6 +281,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 cd5da2cd26..196cedac09 100644 --- a/virtcontainers/persist.go +++ b/virtcontainers/persist.go @@ -226,6 +226,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, @@ -520,6 +521,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 8c82be8df5..5bcc5a9658 100644 --- a/virtcontainers/persist/api/config.go +++ b/virtcontainers/persist/api/config.go @@ -66,6 +66,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 ca86b4f18e..c889da637b 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 81573c1f32..0a89d2eb33 100644 --- a/virtcontainers/pkg/oci/utils.go +++ b/virtcontainers/pkg/oci/utils.go @@ -414,6 +414,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))