From b21a829c61a687555c12b82691cb5267fd671f8c 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: #901 Signed-off-by: Christophe de Dinechin --- .../cli/config/configuration-acrn.toml.in | 3 ++ src/runtime/pkg/katautils/config.go | 41 ++++++++++--------- src/runtime/virtcontainers/hypervisor.go | 3 ++ src/runtime/virtcontainers/persist.go | 2 + .../virtcontainers/persist/api/config.go | 4 ++ .../pkg/annotations/annotations.go | 3 ++ src/runtime/virtcontainers/pkg/oci/utils.go | 7 ++++ 7 files changed, 43 insertions(+), 20 deletions(-) diff --git a/src/runtime/cli/config/configuration-acrn.toml.in b/src/runtime/cli/config/configuration-acrn.toml.in index 7bc8d3172e..f6e9b7849e 100644 --- a/src/runtime/cli/config/configuration-acrn.toml.in +++ b/src/runtime/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/src/runtime/pkg/katautils/config.go b/src/runtime/pkg/katautils/config.go index a71d4064e1..5b739a682d 100644 --- a/src/runtime/pkg/katautils/config.go +++ b/src/runtime/pkg/katautils/config.go @@ -725,26 +725,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/src/runtime/virtcontainers/hypervisor.go b/src/runtime/virtcontainers/hypervisor.go index b5a7122258..e006ed7582 100644 --- a/src/runtime/virtcontainers/hypervisor.go +++ b/src/runtime/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/src/runtime/virtcontainers/persist.go b/src/runtime/virtcontainers/persist.go index 2b8a0ee436..7630bc2f83 100644 --- a/src/runtime/virtcontainers/persist.go +++ b/src/runtime/virtcontainers/persist.go @@ -214,6 +214,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, @@ -479,6 +480,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/src/runtime/virtcontainers/persist/api/config.go b/src/runtime/virtcontainers/persist/api/config.go index 4dc7849b7c..a4e815f94f 100644 --- a/src/runtime/virtcontainers/persist/api/config.go +++ b/src/runtime/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/src/runtime/virtcontainers/pkg/annotations/annotations.go b/src/runtime/virtcontainers/pkg/annotations/annotations.go index ff9bd13e47..5e8f52c503 100644 --- a/src/runtime/virtcontainers/pkg/annotations/annotations.go +++ b/src/runtime/virtcontainers/pkg/annotations/annotations.go @@ -44,6 +44,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/src/runtime/virtcontainers/pkg/oci/utils.go b/src/runtime/virtcontainers/pkg/oci/utils.go index 5b4edb35b6..c3b66433f1 100644 --- a/src/runtime/virtcontainers/pkg/oci/utils.go +++ b/src/runtime/virtcontainers/pkg/oci/utils.go @@ -405,6 +405,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))