From 2e8f28d11d1b444f47e18ee070d6c97f392781d7 Mon Sep 17 00:00:00 2001 From: Tingmao Wang Date: Mon, 10 Nov 2025 10:23:04 +0000 Subject: [PATCH 1/5] Merged PR 12588213: Various enforcement fixes [Cherry-picked from ba849d8464ff019fe751872d458168bc9aec2256] While the entire PR contains several (perhaps unrelated) changes, the individual commits in this PR can be reviewed on their own, and there is some background in the commit messages. I've tested this with azcri-containerd as well as with the functional tests in hcsshim and some of the tests fail due to unrelated reasons (can't do VPMem, no GPU, WCOW) - I haven't noticed anything that could be caused by these changes. I also deployed these changes to a VM and can run some confidential workloads manually. Outstanding work that will not be done in this PR: - Deny unmounting of in-use layers (this should already be impossible due to directories being in use, but can result in rego metadata being inconsistent) Missing go tests that will not be part of this PR - I have local stash for these, but they are not fully working. Can do separately: - Functional test for checking that a bad OCISpec.Rootfs is rejected (tested manually) - Functional test for checking that a bad containerID is rejected (tested manually) Signed-off-by: Tingmao Wang --- internal/guest/runtime/hcsv2/uvm.go | 148 +++- internal/guestpath/paths.go | 14 +- internal/layers/lcow.go | 4 +- internal/uvm/start.go | 5 +- pkg/securitypolicy/api.rego | 38 +- pkg/securitypolicy/api_test.rego | 8 +- pkg/securitypolicy/framework.rego | 161 ++++- pkg/securitypolicy/open_door.rego | 2 + pkg/securitypolicy/policy.rego | 2 + .../policy_v0.10.0_api_test.rego | 71 ++ pkg/securitypolicy/rego_utils_test.go | 122 +++- pkg/securitypolicy/regopolicy_linux_test.go | 630 ++++++++++++++++-- pkg/securitypolicy/securitypolicyenforcer.go | 18 + .../securitypolicyenforcer_rego.go | 56 +- test/functional/lcow_policy_test.go | 22 +- 15 files changed, 1144 insertions(+), 157 deletions(-) create mode 100644 pkg/securitypolicy/policy_v0.10.0_api_test.rego diff --git a/internal/guest/runtime/hcsv2/uvm.go b/internal/guest/runtime/hcsv2/uvm.go index 4436c73c4f..c57079cfb0 100644 --- a/internal/guest/runtime/hcsv2/uvm.go +++ b/internal/guest/runtime/hcsv2/uvm.go @@ -14,6 +14,7 @@ import ( "os/exec" "path" "path/filepath" + "regexp" "strings" "sync" "syscall" @@ -40,6 +41,7 @@ import ( "github.com/Microsoft/hcsshim/internal/guest/storage/pmem" "github.com/Microsoft/hcsshim/internal/guest/storage/scsi" "github.com/Microsoft/hcsshim/internal/guest/transport" + "github.com/Microsoft/hcsshim/internal/guestpath" "github.com/Microsoft/hcsshim/internal/log" "github.com/Microsoft/hcsshim/internal/logfields" "github.com/Microsoft/hcsshim/internal/oci" @@ -54,6 +56,30 @@ import ( // for V2 where the specific message is targeted at the UVM itself. const UVMContainerID = "00000000-0000-0000-0000-000000000000" +// Prevent path traversal via malformed container / sandbox IDs. Container IDs +// can be either UVMContainerID, or a 64 character hex string. This is also used +// to check that sandbox IDs (which is also used in paths) are valid, which has +// the same format. +const validContainerIDRegexRaw = `[0-9a-fA-F]{64}` +var validContainerIDRegex = regexp.MustCompile("^" + validContainerIDRegexRaw + "$") + +// isSandboxId just changes the error message +func checkValidContainerID(id string, isSandboxId bool) error { + if id == UVMContainerID { + return nil + } + + if !validContainerIDRegex.MatchString(id) { + idtype := "container" + if isSandboxId { + idtype = "sandbox" + } + return errors.Errorf("invalid %s id: %s (must match %s)", idtype, id, validContainerIDRegex.String()) + } + + return nil +} + // VirtualPod represents a virtual pod that shares a UVM/Sandbox with other pods type VirtualPod struct { VirtualSandboxID string @@ -245,7 +271,54 @@ func setupSandboxLogDir(sandboxID, virtualSandboxID string) error { // TODO: unify workload and standalone logic for non-sandbox features (e.g., block devices, huge pages, uVM mounts) // TODO(go1.24): use [os.Root] instead of `!strings.HasPrefix(, )` +// Returns whether this host has a security policy set, i.e. if it's running +// confidential containers. +func (h *Host) HasSecurityPolicy() bool { + return len(h.securityOptions.PolicyEnforcer.EncodedSecurityPolicy()) > 0 +} + +// For confidential containers, make sure that the host can't use unexpected +// bundle paths / scratch dir / rootfs +func checkContainerSettings(sandboxID, containerID string, settings *prot.VMHostedContainerSettingsV2) error { + if settings.OCISpecification == nil { + return errors.Errorf("OCISpecification is nil") + } + if settings.OCISpecification.Root == nil { + return errors.Errorf("OCISpecification.Root is nil") + } + + // matches with CreateContainer / createLinuxContainerDocument in internal/hcsoci + containerRootInUVM := path.Join(guestpath.LCOWRootPrefixInUVM, containerID) + if settings.OCIBundlePath != containerRootInUVM { + return errors.Errorf("OCIBundlePath %q must equal expected %q", + settings.OCIBundlePath, containerRootInUVM) + } + expectedContainerRootfs := path.Join(containerRootInUVM, guestpath.RootfsPath) + if settings.OCISpecification.Root.Path != expectedContainerRootfs { + return errors.Errorf("OCISpecification.Root.Path %q must equal expected %q", + settings.OCISpecification.Root.Path, expectedContainerRootfs) + } + + // matches with MountLCOWLayers + scratchDirPath := settings.ScratchDirPath + expectedScratchDirPathNonShared := path.Join(containerRootInUVM, guestpath.ScratchDir, containerID) + expectedScratchDirPathShared := path.Join(guestpath.LCOWRootPrefixInUVM, sandboxID, guestpath.ScratchDir, containerID) + if scratchDirPath != expectedScratchDirPathNonShared && + scratchDirPath != expectedScratchDirPathShared { + return errors.Errorf("ScratchDirPath %q must be either %q or %q", + scratchDirPath, expectedScratchDirPathNonShared, expectedScratchDirPathShared) + } + + return nil +} + func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VMHostedContainerSettingsV2) (_ *Container, err error) { + if h.HasSecurityPolicy() { + if err = checkValidContainerID(id, false); err != nil { + return nil, err + } + } + criType, isCRI := settings.OCISpecification.Annotations[annotations.KubernetesContainerType] // Check for virtual pod annotation @@ -393,6 +466,11 @@ func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VM case "container": sid, ok := settings.OCISpecification.Annotations[annotations.KubernetesSandboxID] sandboxID = sid + if h.HasSecurityPolicy() { + if err = checkValidContainerID(sid, true); err != nil { + return nil, err + } + } if !ok || sid == "" { return nil, errors.Errorf("unsupported 'io.kubernetes.cri.sandbox-id': '%s'", sid) } @@ -402,7 +480,7 @@ func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VM // Add SEV device when security policy is not empty, except when privileged annotation is // set to "true", in which case all UVMs devices are added. - if len(h.securityOptions.PolicyEnforcer.EncodedSecurityPolicy()) > 0 && !oci.ParseAnnotationsBool(ctx, + if h.HasSecurityPolicy() && !oci.ParseAnnotationsBool(ctx, settings.OCISpecification.Annotations, annotations.LCOWPrivileged, false) { if err := specGuest.AddDevSev(ctx, settings.OCISpecification); err != nil { log.G(ctx).WithError(err).Debug("failed to add SEV device") @@ -448,6 +526,12 @@ func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VM }) } + if h.HasSecurityPolicy() { + if err = checkContainerSettings(sandboxID, id, settings); err != nil { + return nil, err + } + } + user, groups, umask, err := h.securityOptions.PolicyEnforcer.GetUserInfo(settings.OCISpecification.Process, settings.OCISpecification.Root.Path) if err != nil { return nil, err @@ -605,6 +689,12 @@ func writeSpecToFile(ctx context.Context, configFile string, spec *specs.Spec) e } func (h *Host) modifyHostSettings(ctx context.Context, containerID string, req *guestrequest.ModificationRequest) (retErr error) { + if h.HasSecurityPolicy() { + if err := checkValidContainerID(containerID, false); err != nil { + return err + } + } + switch req.ResourceType { case guestresource.ResourceTypeSCSIDevice: return modifySCSIDevice(ctx, req.RequestType, req.Settings.(*guestresource.SCSIDevice)) @@ -689,6 +779,12 @@ func (h *Host) modifyHostSettings(ctx context.Context, containerID string, req * } func (h *Host) modifyContainerSettings(ctx context.Context, containerID string, req *guestrequest.ModificationRequest) error { + if h.HasSecurityPolicy() { + if err := checkValidContainerID(containerID, false); err != nil { + return err + } + } + c, err := h.GetCreatedContainer(containerID) if err != nil { return err @@ -1060,6 +1156,9 @@ func modifyMappedVirtualDisk( if err != nil { return err } + if mvd.Filesystem != "" && mvd.Filesystem != "ext4" { + return errors.Errorf("filesystem must be ext4 for read-only scsi mounts") + } } } switch rt { @@ -1076,6 +1175,11 @@ func modifyMappedVirtualDisk( if err != nil { return errors.Wrapf(err, "mounting scsi device controller %d lun %d onto %s denied by policy", mvd.Controller, mvd.Lun, mvd.MountPath) } + } else { + err = securityPolicy.EnforceRWDeviceMountPolicy(ctx, mvd.MountPath, mvd.Encrypted, mvd.EnsureFilesystem, mvd.Filesystem) + if err != nil { + return errors.Wrapf(err, "mounting scsi device controller %d lun %d onto %s denied by policy", mvd.Controller, mvd.Lun, mvd.MountPath) + } } config := &scsi.Config{ Encrypted: mvd.Encrypted, @@ -1094,6 +1198,10 @@ func modifyMappedVirtualDisk( if err := securityPolicy.EnforceDeviceUnmountPolicy(ctx, mvd.MountPath); err != nil { return fmt.Errorf("unmounting scsi device at %s denied by policy: %w", mvd.MountPath, err) } + } else { + if err := securityPolicy.EnforceRWDeviceUnmountPolicy(ctx, mvd.MountPath); err != nil { + return fmt.Errorf("unmounting scsi device at %s denied by policy: %w", mvd.MountPath, err) + } } config := &scsi.Config{ Encrypted: mvd.Encrypted, @@ -1192,8 +1300,42 @@ func modifyCombinedLayers( scratchEncrypted bool, securityPolicy securitypolicy.SecurityPolicyEnforcer, ) (err error) { + isConfidential := len(securityPolicy.EncodedSecurityPolicy()) > 0 + containerID := cl.ContainerID + switch rt { case guestrequest.RequestTypeAdd: + if isConfidential { + if err := checkValidContainerID(containerID, false); err != nil { + return err + } + + // We check this regardless of what the policy says, as long as we're in + // confidential mode. This matches with checkContainerSettings called for + // container creation request. + expectedContainerRootfs := path.Join(guestpath.LCOWRootPrefixInUVM, containerID, guestpath.RootfsPath) + if cl.ContainerRootPath != expectedContainerRootfs { + return fmt.Errorf("combined layers target %q does not match expected path %q", + cl.ContainerRootPath, expectedContainerRootfs) + } + + if cl.ScratchPath != "" { + // At this point, we do not know what the sandbox ID would be yet, so we + // have to allow anything reasonable. + scratchDirRegexStr := fmt.Sprintf( + "^%s/%s/%s/%s$", + guestpath.LCOWRootPrefixInUVM, + validContainerIDRegexRaw, + guestpath.ScratchDir, + containerID, + ) + scratchDirRegex := regexp.MustCompile(scratchDirRegexStr) + if !scratchDirRegex.MatchString(cl.ScratchPath) { + return fmt.Errorf("scratch path %q must match regex %q", + cl.ScratchPath, scratchDirRegexStr) + } + } + } layerPaths := make([]string, len(cl.Layers)) for i, layer := range cl.Layers { layerPaths[i] = layer.Path @@ -1214,12 +1356,14 @@ func modifyCombinedLayers( } } - if err := securityPolicy.EnforceOverlayMountPolicy(ctx, cl.ContainerID, layerPaths, cl.ContainerRootPath); err != nil { + if err := securityPolicy.EnforceOverlayMountPolicy(ctx, containerID, layerPaths, cl.ContainerRootPath); err != nil { return fmt.Errorf("overlay creation denied by policy: %w", err) } return overlay.MountLayer(ctx, layerPaths, upperdirPath, workdirPath, cl.ContainerRootPath, readonly) case guestrequest.RequestTypeRemove: + // cl.ContainerID is not set on remove requests, but rego checks that we can + // only umount previously mounted targets anyway if err := securityPolicy.EnforceOverlayUnmountPolicy(ctx, cl.ContainerRootPath); err != nil { return errors.Wrap(err, "overlay removal denied by policy") } diff --git a/internal/guestpath/paths.go b/internal/guestpath/paths.go index aab9ed1053..1852bc2454 100644 --- a/internal/guestpath/paths.go +++ b/internal/guestpath/paths.go @@ -27,15 +27,17 @@ const ( // LCOWMountPathPrefixFmt is the path format in the LCOW UVM where // non-global mounts, such as Plan9 mounts are added LCOWMountPathPrefixFmt = "/mounts/m%d" - // LCOWGlobalMountPrefixFmt is the path format in the LCOW UVM where global - // mounts are added - LCOWGlobalMountPrefixFmt = "/run/mounts/m%d" + // LCOWGlobalScsiMountPrefixFmt is the path format in the LCOW UVM where + // global desk mounts are added + LCOWGlobalScsiMountPrefixFmt = "/run/mounts/scsi/m%d" // LCOWGlobalDriverPrefixFmt is the path format in the LCOW UVM where drivers // are mounted as read/write LCOWGlobalDriverPrefixFmt = "/run/drivers/%s" - // WCOWGlobalMountPrefixFmt is the path prefix format in the WCOW UVM where - // mounts are added - WCOWGlobalMountPrefixFmt = "C:\\mounts\\m%d" + // WCOWGlobalScsiMountPrefixFmt is the path prefix format in the WCOW UVM + // where global desk mounts are added + WCOWGlobalScsiMountPrefixFmt = `c:\mounts\scsi\m%d` // RootfsPath is part of the container's rootfs path RootfsPath = "rootfs" + // ScratchDir is the name of the directory used for overlay upper and work + ScratchDir = "scratch" ) diff --git a/internal/layers/lcow.go b/internal/layers/lcow.go index dccd994e87..b1385fa4ac 100644 --- a/internal/layers/lcow.go +++ b/internal/layers/lcow.go @@ -159,7 +159,9 @@ func MountLCOWLayers( // handles the case where we want to share a scratch disk for multiple containers instead // of mounting a new one. Pass a unique value for `ScratchPath` to avoid container upper and // work directories colliding in the UVM. - containerScratchPathInUVM := ospath.Join("linux", scsiMount.GuestPath(), "scratch", containerID) + // Note that in the shared scratch case, AddVirtualDisk above is a no-op and + // will return the existing mount. + containerScratchPathInUVM := ospath.Join("linux", scsiMount.GuestPath(), guestpath.ScratchDir, containerID) defer func() { if err != nil { diff --git a/internal/uvm/start.go b/internal/uvm/start.go index 781bc3c417..c921ace550 100644 --- a/internal/uvm/start.go +++ b/internal/uvm/start.go @@ -21,6 +21,7 @@ import ( "github.com/Microsoft/hcsshim/internal/gcs" "github.com/Microsoft/hcsshim/internal/gcs/prot" + "github.com/Microsoft/hcsshim/internal/guestpath" "github.com/Microsoft/hcsshim/internal/hcs" "github.com/Microsoft/hcsshim/internal/hcs/schema1" hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2" @@ -357,9 +358,9 @@ func (uvm *UtilityVM) Start(ctx context.Context) (err error) { } else { gb = scsi.NewHCSGuestBackend(uvm.hcsSystem, uvm.OS()) } - guestMountFmt := `c:\mounts\scsi\m%d` + guestMountFmt := guestpath.WCOWGlobalScsiMountPrefixFmt if uvm.OS() == "linux" { - guestMountFmt = "/run/mounts/scsi/m%d" + guestMountFmt = guestpath.LCOWGlobalScsiMountPrefixFmt } mgr, err := scsi.NewManager( scsi.NewHCSHostBackend(uvm.hcsSystem), diff --git a/pkg/securitypolicy/api.rego b/pkg/securitypolicy/api.rego index 36a197ebc2..e7bc653ac4 100644 --- a/pkg/securitypolicy/api.rego +++ b/pkg/securitypolicy/api.rego @@ -3,22 +3,24 @@ package api version := "@@API_VERSION@@" enforcement_points := { - "mount_device": {"introducedVersion": "0.1.0", "default_results": {"allowed": false}}, - "mount_overlay": {"introducedVersion": "0.1.0", "default_results": {"allowed": false}}, - "mount_cims": {"introducedVersion": "0.11.0", "default_results": {"allowed": false}}, - "create_container": {"introducedVersion": "0.1.0", "default_results": {"allowed": false, "env_list": null, "allow_stdio_access": false}}, - "unmount_device": {"introducedVersion": "0.2.0", "default_results": {"allowed": true}}, - "unmount_overlay": {"introducedVersion": "0.6.0", "default_results": {"allowed": true}}, - "exec_in_container": {"introducedVersion": "0.2.0", "default_results": {"allowed": true, "env_list": null}}, - "exec_external": {"introducedVersion": "0.3.0", "default_results": {"allowed": true, "env_list": null, "allow_stdio_access": false}}, - "shutdown_container": {"introducedVersion": "0.4.0", "default_results": {"allowed": true}}, - "signal_container_process": {"introducedVersion": "0.5.0", "default_results": {"allowed": true}}, - "plan9_mount": {"introducedVersion": "0.6.0", "default_results": {"allowed": true}}, - "plan9_unmount": {"introducedVersion": "0.6.0", "default_results": {"allowed": true}}, - "get_properties": {"introducedVersion": "0.7.0", "default_results": {"allowed": true}}, - "dump_stacks": {"introducedVersion": "0.7.0", "default_results": {"allowed": true}}, - "runtime_logging": {"introducedVersion": "0.8.0", "default_results": {"allowed": true}}, - "load_fragment": {"introducedVersion": "0.9.0", "default_results": {"allowed": false, "add_module": false}}, - "scratch_mount": {"introducedVersion": "0.10.0", "default_results": {"allowed": true}}, - "scratch_unmount": {"introducedVersion": "0.10.0", "default_results": {"allowed": true}}, + "mount_device": {"introducedVersion": "0.1.0", "default_results": {"allowed": false}, "use_framework": false}, + "rw_mount_device": {"introducedVersion": "0.11.0", "default_results": {}, "use_framework": true}, + "mount_overlay": {"introducedVersion": "0.1.0", "default_results": {"allowed": false}, "use_framework": false}, + "mount_cims": {"introducedVersion": "0.11.0", "default_results": {"allowed": false}, "use_framework": false}, + "create_container": {"introducedVersion": "0.1.0", "default_results": {"allowed": false, "env_list": null, "allow_stdio_access": false}, "use_framework": false}, + "unmount_device": {"introducedVersion": "0.2.0", "default_results": {"allowed": true}, "use_framework": false}, + "rw_unmount_device": {"introducedVersion": "0.11.0", "default_results": {}, "use_framework": true}, + "unmount_overlay": {"introducedVersion": "0.6.0", "default_results": {"allowed": true}, "use_framework": false}, + "exec_in_container": {"introducedVersion": "0.2.0", "default_results": {"allowed": true, "env_list": null}, "use_framework": false}, + "exec_external": {"introducedVersion": "0.3.0", "default_results": {"allowed": true, "env_list": null, "allow_stdio_access": false}, "use_framework": false}, + "shutdown_container": {"introducedVersion": "0.4.0", "default_results": {"allowed": true}, "use_framework": false}, + "signal_container_process": {"introducedVersion": "0.5.0", "default_results": {"allowed": true}, "use_framework": false}, + "plan9_mount": {"introducedVersion": "0.6.0", "default_results": {"allowed": true}, "use_framework": false}, + "plan9_unmount": {"introducedVersion": "0.6.0", "default_results": {"allowed": true}, "use_framework": false}, + "get_properties": {"introducedVersion": "0.7.0", "default_results": {"allowed": true}, "use_framework": false}, + "dump_stacks": {"introducedVersion": "0.7.0", "default_results": {"allowed": true}, "use_framework": false}, + "runtime_logging": {"introducedVersion": "0.8.0", "default_results": {"allowed": true}, "use_framework": false}, + "load_fragment": {"introducedVersion": "0.9.0", "default_results": {"allowed": false, "add_module": false}, "use_framework": false}, + "scratch_mount": {"introducedVersion": "0.10.0", "default_results": {"allowed": true}, "use_framework": false}, + "scratch_unmount": {"introducedVersion": "0.10.0", "default_results": {"allowed": true}, "use_framework": false}, } diff --git a/pkg/securitypolicy/api_test.rego b/pkg/securitypolicy/api_test.rego index 2d2de733c6..767c506e58 100644 --- a/pkg/securitypolicy/api_test.rego +++ b/pkg/securitypolicy/api_test.rego @@ -3,8 +3,8 @@ package api version := "0.0.2" enforcement_points := { - "__fixture_for_future_test__": {"introducedVersion": "100.0.0", "default_results": {"allowed": true}}, - "__fixture_for_allowed_test_true__": {"introducedVersion": "0.0.2", "default_results": {"allowed": true}}, - "__fixture_for_allowed_test_false__": {"introducedVersion": "0.0.2", "default_results": {"allowed": false}}, - "__fixture_for_allowed_extra__": {"introducedVersion": "0.0.1", "default_results": {"allowed": false, "__test__": "test"}} + "__fixture_for_future_test__": {"introducedVersion": "100.0.0", "default_results": {"allowed": true}, "use_framework": false}, + "__fixture_for_allowed_test_true__": {"introducedVersion": "0.0.2", "default_results": {"allowed": true}, "use_framework": false}, + "__fixture_for_allowed_test_false__": {"introducedVersion": "0.0.2", "default_results": {"allowed": false}, "use_framework": false}, + "__fixture_for_allowed_extra__": {"introducedVersion": "0.0.1", "default_results": {"allowed": false, "__test__": "test"}, "use_framework": false} } diff --git a/pkg/securitypolicy/framework.rego b/pkg/securitypolicy/framework.rego index 8a28f3e312..20e0e8b067 100644 --- a/pkg/securitypolicy/framework.rego +++ b/pkg/securitypolicy/framework.rego @@ -5,10 +5,28 @@ import future.keywords.in version := "@@FRAMEWORK_VERSION@@" +# Add ^ and $ to regex patterns that doesn't have them. +# This forces the regex to match the entire string, which is safer. +# Policies should include .* explicitly at the beginning or end if partial +# matches are to be allowed. + +anchor_pattern(p) := p { + startswith(p, "^") + endswith(p, "$") +} else := concat("", ["^", p]) { + endswith(p, "$") +} else := concat("", [p, "$"]) { + startswith(p, "^") +} else := concat("", ["^", p, "$"]) + device_mounted(target) { data.metadata.devices[target] } +device_mounted(target) { + data.metadata.rw_devices[target] +} + default deviceHash_ok := false # test if a device hash exists as a layer in a policy container @@ -27,9 +45,14 @@ deviceHash_ok { default mount_device := {"allowed": false} +mount_target_ok { + regex.match(anchor_pattern(input.mountPathRegex), input.target) +} + mount_device := {"metadata": [addDevice], "allowed": true} { not device_mounted(input.target) deviceHash_ok + mount_target_ok addDevice := { "name": "devices", "action": "add", @@ -38,10 +61,38 @@ mount_device := {"metadata": [addDevice], "allowed": true} { } } +allowed_scratch_fs("ext4") +allowed_scratch_fs("xfs") + +rwmount_device_encrypt_ok { + input.encrypted +} + +rwmount_device_encrypt_ok { + allow_unencrypted_scratch +} + +default rw_mount_device := {"allowed": false} + +rw_mount_device := {"metadata": [addDevice], "allowed": true} { + not device_mounted(input.target) + rwmount_device_encrypt_ok + input.ensureFilesystem + allowed_scratch_fs(input.filesystem) + mount_target_ok + addDevice := { + "name": "rw_devices", + "action": "add", + "key": input.target, + "value": true, + } +} + default unmount_device := {"allowed": false} unmount_device := {"metadata": [removeDevice], "allowed": true} { - device_mounted(input.unmountTarget) + data.metadata.devices[input.unmountTarget] + removeDevice := { "name": "devices", "action": "remove", @@ -49,6 +100,18 @@ unmount_device := {"metadata": [removeDevice], "allowed": true} { } } +default rw_unmount_device := {"allowed": false} + +rw_unmount_device := {"metadata": [removeRWDevice], "allowed": true} { + data.metadata.rw_devices[input.unmountTarget] + + removeRWDevice := { + "name": "rw_devices", + "action": "remove", + "key": input.unmountTarget, + } +} + layerPaths_ok(layers) { length := count(layers) count(input.layerPaths) == length @@ -127,6 +190,10 @@ default mount_overlay := {"allowed": false} mount_overlay := {"metadata": [addMatches, addOverlayTarget], "allowed": true} { not overlay_exists + # sanity check, but due to checks in the Go code, this should always pass if + # `not overlay_exists` passes. + not overlay_mounted(input.target) + containers := [container | container := candidate_containers[_] layerPaths_ok(container.layers) @@ -171,30 +238,7 @@ env_ok(pattern, "string", value) { } env_ok(pattern, "re2", value) { - anchored := anchor_pattern(pattern) - regex.match(anchored, value) -} - -anchor_pattern(p) := anchored { - startswith_leading := startswith(p, "^") - endswith_trailing := endswith(p, "$") - - anchored = sprintf("%s%s%s", [ - add_leading_trailing_chars(startswith_leading, "", "^"), # Add ^ only if missing - p, - add_leading_trailing_chars(endswith_trailing, "", "$") # Add $ only if missing - ]) -} - -# Function to return one of two values depending on a boolean condition -add_leading_trailing_chars(cond, ifTrue, ifFalse) := result { - cond - result = ifTrue -} - -add_leading_trailing_chars(cond, ifTrue, ifFalse) := result { - not cond - result = ifFalse + regex.match(anchor_pattern(pattern), value) } rule_ok(rule, env) { @@ -316,7 +360,7 @@ idName_ok(pattern, "name", value) { } idName_ok(pattern, "re2", value) { - regex.match(pattern, value.name) + regex.match(anchor_pattern(pattern), value.name) } user_ok(user) { @@ -682,13 +726,13 @@ security_ok(current_container) { mountSource_ok(constraint, source) { startswith(constraint, data.sandboxPrefix) newConstraint := replace(constraint, data.sandboxPrefix, input.sandboxDir) - regex.match(newConstraint, source) + regex.match(anchor_pattern(newConstraint), source) } mountSource_ok(constraint, source) { startswith(constraint, data.hugePagesPrefix) newConstraint := replace(constraint, data.hugePagesPrefix, input.hugePagesDir) - regex.match(newConstraint, source) + regex.match(anchor_pattern(newConstraint), source) } mountSource_ok(constraint, source) { @@ -918,7 +962,7 @@ default plan9_mount := {"allowed": false} plan9_mount := {"metadata": [addPlan9Target], "allowed": true} { not plan9_mounted(input.target) some containerID, _ in data.metadata.matches - pattern := concat("", [input.rootPrefix, "/", containerID, input.mountPathPrefix]) + pattern := concat("", ["^", input.rootPrefix, "/", containerID, input.mountPathPrefix, "$"]) regex.match(pattern, input.target) addPlan9Target := { "name": "p9mounts", @@ -940,20 +984,28 @@ plan9_unmount := {"metadata": [removePlan9Target], "allowed": true} { } -default enforcement_point_info := {"available": false, "default_results": {"allow": false}, "unknown": true, "invalid": false, "version_missing": false} +default enforcement_point_info := { + "available": false, + "default_results": {"allow": false}, + "unknown": true, + "invalid": false, + "version_missing": false, + "use_framework": false +} -enforcement_point_info := {"available": false, "default_results": {"allow": false}, "unknown": false, "invalid": false, "version_missing": true} { +enforcement_point_info := {"available": false, "default_results": {"allow": false}, "unknown": false, "invalid": false, "version_missing": true, "use_framework": false} { policy_api_version == null } -enforcement_point_info := {"available": available, "default_results": default_results, "unknown": false, "invalid": false, "version_missing": false} { +enforcement_point_info := {"available": available, "default_results": default_results, "unknown": false, "invalid": false, "version_missing": false, "use_framework": use_framework} { enforcement_point := data.api.enforcement_points[input.name] semver.compare(data.api.version, enforcement_point.introducedVersion) >= 0 available := semver.compare(policy_api_version, enforcement_point.introducedVersion) >= 0 default_results := enforcement_point.default_results + use_framework := enforcement_point.use_framework } -enforcement_point_info := {"available": false, "default_results": {"allow": false}, "unknown": false, "invalid": true, "version_missing": false} { +enforcement_point_info := {"available": false, "default_results": {"allow": false}, "unknown": false, "invalid": true, "version_missing": false, "use_framework": false} { enforcement_point := data.api.enforcement_points[input.name] semver.compare(data.api.version, enforcement_point.introducedVersion) < 0 } @@ -1250,9 +1302,50 @@ errors["device already mounted at path"] { device_mounted(input.target) } +errors["mountpoint invalid"] { + input.rule in ["mount_device", "rw_mount_device"] + not mount_target_ok +} + errors["no device at path to unmount"] { input.rule == "unmount_device" - not device_mounted(input.unmountTarget) + not data.metadata.devices[input.unmountTarget] + not data.metadata.rw_devices[input.unmountTarget] +} + +errors["received read-only unmount request, but device provided is read-write"] { + input.rule == "unmount_device" + not data.metadata.devices[input.unmountTarget] + data.metadata.rw_devices[input.unmountTarget] +} + +errors["no device at path to unmount"] { + input.rule == "rw_unmount_device" + not data.metadata.devices[input.unmountTarget] + not data.metadata.rw_devices[input.unmountTarget] +} + +errors["received read-write unmount request, but device provided is read-only"] { + input.rule == "rw_unmount_device" + not data.metadata.rw_devices[input.unmountTarget] + data.metadata.devices[input.unmountTarget] +} + +# Error string tested in azcri-containerd Test_RunPodSandboxNotAllowed_WithPolicy_EncryptedScratchPolicy +errors["unencrypted scratch not allowed, non-readonly mount request for SCSI disk must request encryption"] { + input.rule == "rw_mount_device" + not allow_unencrypted_scratch + not input.encrypted +} + +errors["ensureFilesystem must be set on rw device mounts"] { + input.rule == "rw_mount_device" + not input.ensureFilesystem +} + +errors["rw device mounts uses a filesystem that is not allowed"] { + input.rule == "rw_mount_device" + not allowed_scratch_fs(input.filesystem) } errors["container already started"] { diff --git a/pkg/securitypolicy/open_door.rego b/pkg/securitypolicy/open_door.rego index a8e283092d..23c35f9b04 100644 --- a/pkg/securitypolicy/open_door.rego +++ b/pkg/securitypolicy/open_door.rego @@ -3,10 +3,12 @@ package policy api_version := "@@API_VERSION@@" mount_device := {"allowed": true} +rw_mount_device := {"allowed": true} mount_overlay := {"allowed": true} create_container := {"allowed": true, "env_list": null, "allow_stdio_access": true} mount_cims := {"allowed": true} unmount_device := {"allowed": true} +rw_unmount_device := {"allowed": true} unmount_overlay := {"allowed": true} exec_in_container := {"allowed": true, "env_list": null} exec_external := {"allowed": true, "env_list": null, "allow_stdio_access": true} diff --git a/pkg/securitypolicy/policy.rego b/pkg/securitypolicy/policy.rego index 9414116c19..03a71094bd 100644 --- a/pkg/securitypolicy/policy.rego +++ b/pkg/securitypolicy/policy.rego @@ -6,7 +6,9 @@ framework_version := "@@FRAMEWORK_VERSION@@" @@OBJECTS@@ mount_device := data.framework.mount_device +rw_mount_device := data.framework.rw_mount_device unmount_device := data.framework.unmount_device +rw_unmount_device := data.framework.rw_unmount_device mount_overlay := data.framework.mount_overlay unmount_overlay := data.framework.unmount_overlay mount_cims:= data.framework.mount_cims diff --git a/pkg/securitypolicy/policy_v0.10.0_api_test.rego b/pkg/securitypolicy/policy_v0.10.0_api_test.rego new file mode 100644 index 0000000000..a5ea9c56d4 --- /dev/null +++ b/pkg/securitypolicy/policy_v0.10.0_api_test.rego @@ -0,0 +1,71 @@ +package policy + +api_version := "0.10.0" +framework_version := "0.3.0" + +containers := [ + { + "allow_elevated": false, + "allow_stdio_access": true, + "capabilities": { + "ambient": [], + "bounding": [], + "effective": [], + "inheritable": [], + "permitted": [] + }, + "command": [ "bash" ], + "env_rules": [], + "exec_processes": [], + "layers": [ + "@@CONTAINER_LAYER_HASH@@", + ], + "mounts": [], + "no_new_privileges": false, + "seccomp_profile_sha256": "", + "signals": [], + "user": { + "group_idnames": [ + { + "pattern": "", + "strategy": "any" + } + ], + "umask": "0022", + "user_idname": { + "pattern": "", + "strategy": "any" + } + }, + "working_dir": "/" + } +] + +allow_properties_access := true +allow_dump_stacks := false +allow_runtime_logging := false +allow_environment_variable_dropping := true +allow_unencrypted_scratch := false +allow_capability_dropping := true + +mount_device := data.framework.mount_device +unmount_device := data.framework.unmount_device +mount_overlay := data.framework.mount_overlay +unmount_overlay := data.framework.unmount_overlay +create_container := data.framework.create_container +exec_in_container := data.framework.exec_in_container +exec_external := {"allowed": true, + "allow_stdio_access": true, + "env_list": input.envList} +shutdown_container := data.framework.shutdown_container +signal_container_process := data.framework.signal_container_process +plan9_mount := data.framework.plan9_mount +plan9_unmount := data.framework.plan9_unmount +get_properties := data.framework.get_properties +dump_stacks := data.framework.dump_stacks +runtime_logging := data.framework.runtime_logging +load_fragment := data.framework.load_fragment +scratch_mount := data.framework.scratch_mount +scratch_unmount := data.framework.scratch_unmount + +reason := {"errors": data.framework.errors} diff --git a/pkg/securitypolicy/rego_utils_test.go b/pkg/securitypolicy/rego_utils_test.go index dbe016098d..2e357f071a 100644 --- a/pkg/securitypolicy/rego_utils_test.go +++ b/pkg/securitypolicy/rego_utils_test.go @@ -6,6 +6,7 @@ package securitypolicy import ( "context" _ "embed" + "encoding/hex" "encoding/json" "fmt" "math/rand" @@ -15,6 +16,7 @@ import ( "sort" "strconv" "strings" + "sync/atomic" "syscall" "testing" "time" @@ -34,7 +36,6 @@ const ( maxExternalProcessesInGeneratedConstraints = 16 maxFragmentsInGeneratedConstraints = 4 maxGeneratedExternalProcesses = 12 - maxGeneratedSandboxIDLength = 32 maxGeneratedEnforcementPointLength = 64 maxGeneratedPlan9Mounts = 8 maxGeneratedFragmentFeedLength = 256 @@ -46,7 +47,6 @@ const ( minStringLength = 10 maxContainersInGeneratedConstraints = 32 maxLayersInGeneratedContainer = 32 - maxGeneratedContainerID = 1000000 maxGeneratedCommandLength = 128 maxGeneratedCommandArgs = 12 maxGeneratedEnvironmentVariables = 16 @@ -355,8 +355,17 @@ func mountImageForContainer(policy *regoEnforcer, container *securityPolicyConta return "", fmt.Errorf("error creating valid overlay: %w", err) } + scratchDisk := getScratchDiskMountTarget(containerID) + err = policy.EnforceRWDeviceMountPolicy(ctx, scratchDisk, true, true, "xfs") + if err != nil { + return "", fmt.Errorf("error mounting scratch disk: %w", err) + } + + overlayTarget := getOverlayMountTarget(containerID) + // see NOTE_TESTCOPY - err = policy.EnforceOverlayMountPolicy(ctx, containerID, copyStrings(layerPaths), testDataGenerator.uniqueMountTarget()) + err = policy.EnforceOverlayMountPolicy( + ctx, containerID, copyStrings(layerPaths), overlayTarget) if err != nil { return "", fmt.Errorf("error mounting filesystem: %w", err) } @@ -1333,7 +1342,8 @@ func selectFragmentsFromConstraints(gc *generatedConstraints, numFragments int, } func generateSandboxID(r *rand.Rand) string { - return randVariableString(r, maxGeneratedSandboxIDLength) + // Sandbox IDs has the same format as container IDs + return generateContainerID(r) } func generateEnforcementPoint(r *rand.Rand) string { @@ -1615,6 +1625,13 @@ func copyStrings(values []string) []string { //go:embed api_test.rego var apiTestCode string +//go:embed policy_v0.10.0_api_test.rego +var policyWith_0_10_0_apiTestCode string + +func getPolicyCode_0_10_0(layerHash string) string { + return strings.Replace(policyWith_0_10_0_apiTestCode, "@@CONTAINER_LAYER_HASH@@", layerHash, 1) +} + func (p *regoEnforcer) injectTestAPI() error { p.rego.RemoveModule("api.rego") p.rego.AddModule("api.rego", &rpi.RegoModule{Namespace: "api", Code: apiTestCode}) @@ -2030,7 +2047,7 @@ func assertDecisionJSONContains(t *testing.T, err error, expectedValues ...strin for _, expected := range expectedValues { if !strings.Contains(policyDecision, expected) { - t.Errorf("expected error to contain %q", expected) + t.Errorf("expected error to contain %q, but got %q", expected, policyDecision) return false } } @@ -2492,7 +2509,6 @@ func buildEnvironmentVariablesFromEnvRules(rules []EnvRuleConfig, r *rand.Rand) // Build in all required rules, this isn't a setup method of "missing item" // tests for _, rule := range rules { - if rule.Required { if rule.Strategy != EnvVarRuleRegex { vars = append(vars, rule.Rule) @@ -2529,12 +2545,14 @@ func buildEnvironmentVariablesFromEnvRules(rules []EnvRuleConfig, r *rand.Rand) usedIndexes[anIndex] = struct{}{} } numberOfMatches-- - } return vars } +// Only used for random mount targets or for the standard enforcer. Rego policy +// enforces proper targets that are e.g. created from +// guestpath.LCOWGlobalScsiMountPrefixFmt func generateMountTarget(r *rand.Rand) string { return randVariableString(r, maxGeneratedMountTargetLength) } @@ -2563,8 +2581,12 @@ func selectRootHashFromConstraints(constraints *generatedConstraints, r *rand.Ra } func generateContainerID(r *rand.Rand) string { - id := atLeastOneAtMost(r, maxGeneratedContainerID) - return strconv.FormatInt(int64(id), 10) + idbytes := make([]byte, 32) + _, err := r.Read(idbytes) + if err != nil { + panic(fmt.Errorf("failed to generate random container ID: %w", err)) + } + return hex.EncodeToString(idbytes) } func generateMounts(r *rand.Rand) []mountInternal { @@ -2654,26 +2676,28 @@ func selectContainerFromContainerList(containers []*securityPolicyContainer, r * } type dataGenerator struct { - rng *rand.Rand - mountTargets stringSet - containerIDs stringSet - sandboxIDs stringSet - enforcementPoints stringSet - fragmentIssuers stringSet - fragmentFeeds stringSet - fragmentNamespaces stringSet + rng *rand.Rand + layerMountTarget stringSet + nextLayerMountTarget atomic.Uint64 + containerIDs stringSet + sandboxIDs stringSet + enforcementPoints stringSet + fragmentIssuers stringSet + fragmentFeeds stringSet + fragmentNamespaces stringSet } func newDataGenerator(rng *rand.Rand) *dataGenerator { return &dataGenerator{ - rng: rng, - mountTargets: make(stringSet), - containerIDs: make(stringSet), - sandboxIDs: make(stringSet), - enforcementPoints: make(stringSet), - fragmentIssuers: make(stringSet), - fragmentFeeds: make(stringSet), - fragmentNamespaces: make(stringSet), + rng: rng, + layerMountTarget: make(stringSet), + nextLayerMountTarget: atomic.Uint64{}, + containerIDs: make(stringSet), + sandboxIDs: make(stringSet), + enforcementPoints: make(stringSet), + fragmentIssuers: make(stringSet), + fragmentFeeds: make(stringSet), + fragmentNamespaces: make(stringSet), } } @@ -2687,21 +2711,36 @@ func (s *stringSet) randUnique(r *rand.Rand, generator func(*rand.Rand) string) } } -func (gen *dataGenerator) uniqueMountTarget() string { - return gen.mountTargets.randUnique(gen.rng, generateMountTarget) +// Generate a purely random mount target. This will be rejected by rego. +func (gen *dataGenerator) uniqueRandomMountTarget() string { + return gen.layerMountTarget.randUnique(gen.rng, generateMountTarget) } func (gen *dataGenerator) uniqueContainerID() string { return gen.containerIDs.randUnique(gen.rng, generateContainerID) } +func (gen *dataGenerator) uniqueLayerMountTarget() string { + idx := gen.nextLayerMountTarget.Add(1) + return fmt.Sprintf(guestpath.LCOWGlobalScsiMountPrefixFmt, idx) +} + +func getScratchDiskMountTarget(containerID string) string { + return path.Join(guestpath.LCOWRootPrefixInUVM, containerID) +} + +// Returns the roofs of a container. +func getOverlayMountTarget(containerID string) string { + return path.Join(guestpath.LCOWRootPrefixInUVM, containerID, guestpath.RootfsPath) +} + func (gen *dataGenerator) createValidOverlayForContainer(enforcer SecurityPolicyEnforcer, container *securityPolicyContainer) ([]string, error) { ctx := context.Background() // storage for our mount paths overlay := make([]string, len(container.Layers)) for i := 0; i < len(container.Layers); i++ { - mount := gen.uniqueMountTarget() + mount := gen.uniqueLayerMountTarget() err := enforcer.EnforceDeviceMountPolicy(ctx, mount, container.Layers[i]) if err != nil { return overlay, err @@ -2714,14 +2753,16 @@ func (gen *dataGenerator) createValidOverlayForContainer(enforcer SecurityPolicy } func (gen *dataGenerator) createInvalidOverlayForContainer(enforcer SecurityPolicyEnforcer, container *securityPolicyContainer) ([]string, error) { - method := gen.rng.Intn(3) + method := gen.rng.Intn(4) switch method { case 0: return gen.invalidOverlaySameSizeWrongMounts(enforcer, container) case 1: return gen.invalidOverlayCorrectDevicesWrongOrderSomeMissing(enforcer, container) - default: + case 2: return gen.invalidOverlayRandomJunk(enforcer, container) + default: + return gen.invalidOverlayRandomNoMount(enforcer, container) } } @@ -2731,14 +2772,14 @@ func (gen *dataGenerator) invalidOverlaySameSizeWrongMounts(enforcer SecurityPol overlay := make([]string, len(container.Layers)) for i := 0; i < len(container.Layers); i++ { - mount := gen.uniqueMountTarget() + mount := gen.uniqueLayerMountTarget() err := enforcer.EnforceDeviceMountPolicy(ctx, mount, container.Layers[i]) if err != nil { return overlay, err } // generate a random new mount point to cause an error - overlay[len(overlay)-i-1] = gen.uniqueMountTarget() + overlay[len(overlay)-i-1] = gen.uniqueLayerMountTarget() } return overlay, nil @@ -2754,7 +2795,7 @@ func (gen *dataGenerator) invalidOverlayCorrectDevicesWrongOrderSomeMissing(enfo var overlay []string for i := 0; i < len(container.Layers); i++ { - mount := gen.uniqueMountTarget() + mount := gen.uniqueLayerMountTarget() err := enforcer.EnforceDeviceMountPolicy(ctx, mount, container.Layers[i]) if err != nil { return overlay, err @@ -2775,12 +2816,12 @@ func (gen *dataGenerator) invalidOverlayRandomJunk(enforcer SecurityPolicyEnforc overlay := make([]string, layersToCreate) for i := 0; i < int(layersToCreate); i++ { - overlay[i] = gen.uniqueMountTarget() + overlay[i] = generateMountTarget(gen.rng) } // setup entirely different and "correct" expected mounting for i := 0; i < len(container.Layers); i++ { - mount := gen.uniqueMountTarget() + mount := gen.uniqueLayerMountTarget() err := enforcer.EnforceDeviceMountPolicy(ctx, mount, container.Layers[i]) if err != nil { return overlay, err @@ -2790,6 +2831,17 @@ func (gen *dataGenerator) invalidOverlayRandomJunk(enforcer SecurityPolicyEnforc return overlay, nil } +func (gen *dataGenerator) invalidOverlayRandomNoMount(enforcer SecurityPolicyEnforcer, container *securityPolicyContainer) ([]string, error) { + layersToCreate := gen.rng.Int31n(maxLayersInGeneratedContainer) + overlay := make([]string, layersToCreate) + + for i := 0; i < int(layersToCreate); i++ { + overlay[i] = gen.uniqueLayerMountTarget() + } + + return overlay, nil +} + func randVariableString(r *rand.Rand, maxLen int32) string { return randString(r, atLeastOneAtMost(r, maxLen)) } diff --git a/pkg/securitypolicy/regopolicy_linux_test.go b/pkg/securitypolicy/regopolicy_linux_test.go index 94cfd8d031..bec4f27f07 100644 --- a/pkg/securitypolicy/regopolicy_linux_test.go +++ b/pkg/securitypolicy/regopolicy_linux_test.go @@ -9,6 +9,7 @@ import ( "fmt" "math/rand" "os" + "path" "path/filepath" "slices" "strconv" @@ -106,7 +107,7 @@ func Test_MarshalRego_Policy(t *testing.T) { _, err = newRegoPolicy(expected, defaultMounts, privilegedMounts, testOSType) if err != nil { - t.Errorf("unable to convert policy to rego: %v", err) + t.Errorf("cannot make rego policy from constraints: %v", err) return false } @@ -193,11 +194,11 @@ func Test_Rego_EnforceDeviceMountPolicy_No_Matches(t *testing.T) { policy, err := newRegoPolicy(securityPolicy.marshalRego(), []oci.Mount{}, []oci.Mount{}, testOSType) if err != nil { - t.Errorf("unable to convert policy to rego: %v", err) + t.Errorf("cannot make rego policy from constraints: %v", err) return false } - target := testDataGenerator.uniqueMountTarget() + target := testDataGenerator.uniqueLayerMountTarget() rootHash := generateInvalidRootHash(testRand) err = policy.EnforceDeviceMountPolicy(p.ctx, target, rootHash) @@ -219,11 +220,11 @@ func Test_Rego_EnforceDeviceMountPolicy_Matches(t *testing.T) { policy, err := newRegoPolicy(securityPolicy.marshalRego(), []oci.Mount{}, []oci.Mount{}, testOSType) if err != nil { - t.Errorf("unable to convert policy to rego: %v", err) + t.Errorf("cannot make rego policy from constraints: %v", err) return false } - target := testDataGenerator.uniqueMountTarget() + target := testDataGenerator.uniqueLayerMountTarget() rootHash := selectRootHashFromConstraints(p, testRand) err = policy.EnforceDeviceMountPolicy(p.ctx, target, rootHash) @@ -237,7 +238,7 @@ func Test_Rego_EnforceDeviceMountPolicy_Matches(t *testing.T) { } } -func Test_Rego_EnforceDeviceUmountPolicy_Removes_Device_Entries(t *testing.T) { +func Test_Rego_EnforceDeviceUnmountPolicy_Removes_Device_Entries(t *testing.T) { f := func(p *generatedConstraints) bool { securityPolicy := p.toPolicy() policy, err := newRegoPolicy(securityPolicy.marshalRego(), []oci.Mount{}, []oci.Mount{}, testOSType) @@ -247,7 +248,7 @@ func Test_Rego_EnforceDeviceUmountPolicy_Removes_Device_Entries(t *testing.T) { return false } - target := testDataGenerator.uniqueMountTarget() + target := testDataGenerator.uniqueLayerMountTarget() rootHash := selectRootHashFromConstraints(p, testRand) err = policy.EnforceDeviceMountPolicy(p.ctx, target, rootHash) @@ -272,7 +273,36 @@ func Test_Rego_EnforceDeviceUmountPolicy_Removes_Device_Entries(t *testing.T) { } if err := quick.Check(f, &quick.Config{MaxCount: 50, Rand: testRand}); err != nil { - t.Errorf("Test_Rego_EnforceDeviceUmountPolicy_Removes_Device_Entries failed: %v", err) + t.Errorf("Test_Rego_EnforceDeviceUnmountPolicy_Removes_Device_Entries failed: %v", err) + } +} + +func Test_Rego_EnforceDeviceUnmountPolicy_No_Matches(t *testing.T) { + f := func(p *generatedConstraints) bool { + securityPolicy := p.toPolicy() + policy, err := newRegoPolicy(securityPolicy.marshalRego(), []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Error(err) + return false + } + + target := testDataGenerator.uniqueLayerMountTarget() + err = policy.EnforceDeviceUnmountPolicy(p.ctx, target) + if !assertDecisionJSONContains(t, err, "no device at path to unmount") { + return false + } + + target = getScratchDiskMountTarget(testDataGenerator.uniqueContainerID()) + err = policy.EnforceRWDeviceUnmountPolicy(p.ctx, target) + if !assertDecisionJSONContains(t, err, "no device at path to unmount") { + return false + } + + return true + } + + if err := quick.Check(f, &quick.Config{MaxCount: 50, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_EnforceDeviceUnmountPolicy_No_Matches failed: %v", err) } } @@ -282,11 +312,11 @@ func Test_Rego_EnforceDeviceMountPolicy_Duplicate_Device_Target(t *testing.T) { policy, err := newRegoPolicy(securityPolicy.marshalRego(), []oci.Mount{}, []oci.Mount{}, testOSType) if err != nil { - t.Errorf("unable to convert policy to rego: %v", err) + t.Errorf("cannot make rego policy from constraints: %v", err) return false } - target := testDataGenerator.uniqueMountTarget() + target := testDataGenerator.uniqueLayerMountTarget() rootHash := selectRootHashFromConstraints(p, testRand) err = policy.EnforceDeviceMountPolicy(p.ctx, target, rootHash) if err != nil { @@ -309,6 +339,342 @@ func Test_Rego_EnforceDeviceMountPolicy_Duplicate_Device_Target(t *testing.T) { } } +func Test_Rego_EnforceDeviceMountPolicy_InvalidMountTarget(t *testing.T) { + f := func(p *generatedConstraints) bool { + securityPolicy := p.toPolicy() + policy, err := newRegoPolicy(securityPolicy.marshalRego(), []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Errorf("cannot make rego policy from constraints: %v", err) + return false + } + + target := testDataGenerator.uniqueRandomMountTarget() + rootHash := selectRootHashFromConstraints(p, testRand) + + err = policy.EnforceDeviceMountPolicy(p.ctx, target, rootHash) + + return assertDecisionJSONContains(t, err, "mountpoint invalid") + } + + if err := quick.Check(f, &quick.Config{MaxCount: 50, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_EnforceDeviceMountPolicy_InvalidMountTarget failed: %v", err) + } +} + +func Test_Rego_EnforceDeviceMountPolicy_InvalidMountTarget_PathTraversal(t *testing.T) { + p := generateConstraints(testRand, 1) + securityPolicy := p.toPolicy() + policy, err := newRegoPolicy(securityPolicy.marshalRego(), []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Errorf("cannot make rego policy from constraints: %v", err) + return + } + + target := testDataGenerator.uniqueLayerMountTarget() + "/../../../../.." + rootHash := selectRootHashFromConstraints(p, testRand) + + err = policy.EnforceDeviceMountPolicy(p.ctx, target, rootHash) + + assertDecisionJSONContains(t, err, "mountpoint invalid") +} + +func deviceMountUnmountTest(t *testing.T, p *generatedConstraints, policy *regoEnforcer, mountScratchFirst, unmountScratchFirst, testInvalidUnmount bool) bool { + container := selectContainerFromContainerList(p.containers, testRand) + containerID := testDataGenerator.uniqueContainerID() + rotarget := testDataGenerator.uniqueLayerMountTarget() + rwtarget := getScratchDiskMountTarget(containerID) + + var err error + + mountScratch := func() bool { + err = policy.EnforceRWDeviceMountPolicy(p.ctx, rwtarget, true, true, "xfs") + if err != nil { + t.Errorf("unable to mount rw device: %v", err) + return false + } + return true + } + + mountLayer := func() bool { + err = policy.EnforceDeviceMountPolicy(p.ctx, rotarget, container.Layers[0]) + if err != nil { + t.Errorf("unable to mount ro device: %v", err) + return false + } + return true + } + + if mountScratchFirst { + if !mountScratch() || !mountLayer() { + return false + } + } else { + if !mountLayer() || !mountScratch() { + return false + } + } + + unmountScratch := func() bool { + err = policy.EnforceRWDeviceUnmountPolicy(p.ctx, rwtarget) + if err != nil { + t.Errorf("unable to unmount rw device: %v", err) + return false + } + return true + } + + unmountLayer := func() bool { + err = policy.EnforceDeviceUnmountPolicy(p.ctx, rotarget) + if err != nil { + t.Errorf("unable to unmount ro device: %v", err) + return false + } + return true + } + + if unmountScratchFirst { + if !unmountScratch() || !unmountLayer() { + return false + } + } else { + if !unmountLayer() || !unmountScratch() { + return false + } + } + + if testInvalidUnmount { + err = policy.EnforceDeviceUnmountPolicy(p.ctx, rotarget) + if !assertDecisionJSONContains(t, err, "no device at path to unmount") { + return false + } + + err = policy.EnforceRWDeviceUnmountPolicy(p.ctx, rwtarget) + if !assertDecisionJSONContains(t, err, "no device at path to unmount") { + return false + } + } + + return true +} + +func Test_Rego_EnforceRWDeviceMountPolicy_MountAndUnmount(t *testing.T) { + f := func(p *generatedConstraints, mountScratchFirst, unmountScratchFirst bool) bool { + securityPolicy := p.toPolicy() + policy, err := newRegoPolicy(securityPolicy.marshalRego(), []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Errorf("cannot make rego policy from constraints: %v", err) + return false + } + + return deviceMountUnmountTest(t, p, policy, mountScratchFirst, unmountScratchFirst, true) + } + if err := quick.Check(f, &quick.Config{MaxCount: 50, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_EnforceRWDeviceMountPolicy_MountAndUnmount failed: %v", err) + } +} + +func Test_Rego_EnforceRWDeviceMountPolicy_InvalidTarget(t *testing.T) { + f := func(p *generatedConstraints, encrypted bool, ensureFileSystem bool) bool { + securityPolicy := p.toPolicy() + policy, err := newRegoPolicy(securityPolicy.marshalRego(), []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Errorf("cannot make rego policy from constraints: %v", err) + return false + } + + target := testDataGenerator.uniqueRandomMountTarget() + filesystem := "xfs" + + err = policy.EnforceRWDeviceMountPolicy(p.ctx, target, encrypted, ensureFileSystem, filesystem) + + return assertDecisionJSONContains(t, err, "mountpoint invalid") + } + + if err := quick.Check(f, &quick.Config{MaxCount: 50, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_EnforceRWDeviceMountPolicy_Matches failed: %v", err) + } +} + +func Test_Rego_EnforceRWDeviceMountPolicy_MissingEnsureFilesystem(t *testing.T) { + f := func(p *generatedConstraints, encrypted bool) bool { + p.allowUnencryptedScratch = !encrypted + securityPolicy := p.toPolicy() + policy, err := newRegoPolicy(securityPolicy.marshalRego(), []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Errorf("cannot make rego policy from constraints: %v", err) + return false + } + + target := getScratchDiskMountTarget(testDataGenerator.uniqueContainerID()) + filesystem := "xfs" + + err = policy.EnforceRWDeviceMountPolicy(p.ctx, target, encrypted, false, filesystem) + + return assertDecisionJSONContains(t, err, "ensureFilesystem must be set on rw device mounts") + } + + if err := quick.Check(f, &quick.Config{MaxCount: 10, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_EnforceRWDeviceMountPolicy_Matches failed: %v", err) + } +} + +func Test_Rego_EnforceRWDeviceMountPolicy_DontAllowUnencrypted(t *testing.T) { + p := generateConstraints(testRand, 1) + p.allowUnencryptedScratch = false + securityPolicy := p.toPolicy() + policy, err := newRegoPolicy(securityPolicy.marshalRego(), []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Errorf("cannot make rego policy from constraints: %v", err) + return + } + + target := getScratchDiskMountTarget(testDataGenerator.uniqueContainerID()) + filesystem := "xfs" + + err = policy.EnforceRWDeviceMountPolicy(p.ctx, target, false, true, filesystem) + + assertDecisionJSONContains(t, err, "unencrypted scratch not allowed, non-readonly mount request for SCSI disk must request encryption") +} + +func Test_Rego_EnforceRWDeviceMountPolicy_InvalidFilesystem(t *testing.T) { + p := generateConstraints(testRand, 1) + securityPolicy := p.toPolicy() + policy, err := newRegoPolicy(securityPolicy.marshalRego(), []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Errorf("cannot make rego policy from constraints: %v", err) + return + } + + target := getScratchDiskMountTarget(testDataGenerator.uniqueContainerID()) + dangerousFilesystems := []string{ + "9p", + "overlay", + "nfs", + "cifs", + } + + for _, filesystem := range dangerousFilesystems { + err = policy.EnforceRWDeviceMountPolicy(p.ctx, target, true, true, filesystem) + assertDecisionJSONContains(t, err, "rw device mounts uses a filesystem that is not allowed") + } +} + +// Test that for an older allow all policy (api version < 0.11.0) that does not +// have rw_mount_device, the use_framework passthrough is done correctly, +// allowing enforcing rw mounts. +func Test_Rego_EnforceRWDeviceMountPolicy_Compat_0_10_0_allow_all(t *testing.T) { + p := generateConstraints(testRand, 1) + regoPolicy := ` +package policy + +api_version := "0.10.0" +framework_version := "0.3.0" + +mount_device := {"allowed": true} +mount_overlay := {"allowed": true} +create_container := {"allowed": true, "env_list": null, "allow_stdio_access": true} +unmount_device := {"allowed": true} +unmount_overlay := {"allowed": true} +exec_in_container := {"allowed": true, "env_list": null} +exec_external := {"allowed": true, "env_list": null, "allow_stdio_access": true} +shutdown_container := {"allowed": true} +signal_container_process := {"allowed": true} +plan9_mount := {"allowed": true} +plan9_unmount := {"allowed": true} +get_properties := {"allowed": true} +dump_stacks := {"allowed": true} +runtime_logging := {"allowed": true} +load_fragment := {"allowed": true} +scratch_mount := {"allowed": true} +scratch_unmount := {"allowed": true} +` + for _, b1 := range []bool{true, false} { + for _, b2 := range []bool{true, false} { + policy, err := newRegoPolicy(regoPolicy, []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Errorf("cannot compile rego policy: %v", err) + return + } + + t.Run(fmt.Sprintf("mountScratchFirst=%t, unmountScratchFirst=%t", b1, b2), func(t *testing.T) { + deviceMountUnmountTest(t, p, policy, b1, b2, false) + }) + } + } +} + +// Test that for an older policy (api version < 0.11.0) that does not have +// rw_mount_device, the use_framework passthrough is done correctly, allowing +// enforcing rw mounts. +func Test_Rego_EnforceRWDeviceMountPolicy_Compat_0_10_0(t *testing.T) { + p := generateConstraints(testRand, 1) + regoPolicy := getPolicyCode_0_10_0(p.containers[0].Layers[0]) + for _, b1 := range []bool{true, false} { + for _, b2 := range []bool{true, false} { + policy, err := newRegoPolicy(regoPolicy, []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Errorf("cannot compile rego policy: %v", err) + return + } + + t.Run(fmt.Sprintf("mountScratchFirst=%t, unmountScratchFirst=%t", b1, b2), func(t *testing.T) { + deviceMountUnmountTest(t, p, policy, b1, b2, true) + }) + } + } + + policy, err := newRegoPolicy(regoPolicy, []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Errorf("cannot compile rego policy: %v", err) + return + } + + // Invalid mount target + target := testDataGenerator.uniqueRandomMountTarget() + filesystem := "xfs" + encrypted := true + ensureFileSystem := true + err = policy.EnforceRWDeviceMountPolicy(p.ctx, target, encrypted, ensureFileSystem, filesystem) + assertDecisionJSONContains(t, err, "mountpoint invalid") + + // Missing ensureFilesystem + ensureFileSystem = false + target = getScratchDiskMountTarget(testDataGenerator.uniqueContainerID()) + err = policy.EnforceRWDeviceMountPolicy(p.ctx, target, encrypted, ensureFileSystem, filesystem) + assertDecisionJSONContains(t, err, "ensureFilesystem must be set on rw device mounts") + + // Unencrypted scratch not allowed + ensureFileSystem = true + encrypted = false + err = policy.EnforceRWDeviceMountPolicy(p.ctx, target, encrypted, ensureFileSystem, filesystem) + assertDecisionJSONContains(t, err, "unencrypted scratch not allowed, non-readonly mount request for SCSI disk must request encryption") +} + +func Test_Rego_EnforceRWDeviceMountPolicy_OpenDoor(t *testing.T) { + p := generateConstraints(testRand, 1) + policy, err := newRegoPolicy(openDoorRego, []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Errorf("cannot compile open door rego policy: %v", err) + return + } + + deviceMountUnmountTest(t, p, policy, true, true, false) + + ensureFileSystem := false + encrypted := false + filesystem := "zfs" + target := "/bin" + err = policy.EnforceRWDeviceMountPolicy(p.ctx, target, encrypted, ensureFileSystem, filesystem) + if err != nil { + t.Errorf("unexpected error mounting rw device: %v", err) + } + + err = policy.EnforceRWDeviceUnmountPolicy(p.ctx, target) + if err != nil { + t.Errorf("unexpected error unmounting rw device: %v", err) + } +} + // Verify that RegoSecurityPolicyEnforcer.EnforceOverlayMountPolicy will // return an error when there's no matching overlay targets. func Test_Rego_EnforceOverlayMountPolicy_No_Matches(t *testing.T) { @@ -319,7 +685,8 @@ func Test_Rego_EnforceOverlayMountPolicy_No_Matches(t *testing.T) { return false } - err = tc.policy.EnforceOverlayMountPolicy(p.ctx, tc.containerID, tc.layers, testDataGenerator.uniqueMountTarget()) + err = tc.policy.EnforceOverlayMountPolicy( + p.ctx, tc.containerID, tc.layers, getOverlayMountTarget(tc.containerID)) if err == nil { return false @@ -348,7 +715,8 @@ func Test_Rego_EnforceOverlayMountPolicy_Matches(t *testing.T) { return false } - err = tc.policy.EnforceOverlayMountPolicy(p.ctx, tc.containerID, tc.layers, testDataGenerator.uniqueMountTarget()) + err = tc.policy.EnforceOverlayMountPolicy( + p.ctx, tc.containerID, tc.layers, getOverlayMountTarget(tc.containerID)) // getting an error means something is broken return err == nil @@ -388,7 +756,8 @@ func Test_Rego_EnforceOverlayMountPolicy_Layers_With_Same_Root_Hash(t *testing.T t.Fatalf("error creating valid overlay: %v", err) } - err = policy.EnforceOverlayMountPolicy(constraints.ctx, containerID, layers, testDataGenerator.uniqueMountTarget()) + err = policy.EnforceOverlayMountPolicy( + constraints.ctx, containerID, layers, getOverlayMountTarget(containerID)) if err != nil { t.Fatalf("Unable to create an overlay where root hashes are the same") } @@ -428,7 +797,7 @@ func Test_Rego_EnforceOverlayMountPolicy_Layers_Shared_Layers(t *testing.T) { sharedMount := "" for i := 0; i < len(containerOne.Layers); i++ { - mount := testDataGenerator.uniqueMountTarget() + mount := testDataGenerator.uniqueLayerMountTarget() err := policy.EnforceDeviceMountPolicy(constraints.ctx, mount, containerOne.Layers[i]) if err != nil { t.Fatalf("Unexpected error mounting overlay device: %v", err) @@ -440,13 +809,14 @@ func Test_Rego_EnforceOverlayMountPolicy_Layers_Shared_Layers(t *testing.T) { containerOneOverlay[len(containerOneOverlay)-i-1] = mount } - err = policy.EnforceOverlayMountPolicy(constraints.ctx, containerID, containerOneOverlay, testDataGenerator.uniqueMountTarget()) + err = policy.EnforceOverlayMountPolicy( + constraints.ctx, containerID, containerOneOverlay, getOverlayMountTarget(containerID)) if err != nil { t.Fatalf("Unexpected error mounting overlay: %v", err) } // - // Mount our second contaniers overlay. This should all work. + // Mount our second container overlay. This should all work. // containerID = testDataGenerator.uniqueContainerID() @@ -456,7 +826,7 @@ func Test_Rego_EnforceOverlayMountPolicy_Layers_Shared_Layers(t *testing.T) { for i := 0; i < len(containerTwo.Layers); i++ { var mount string if i != sharedLayerIndex { - mount = testDataGenerator.uniqueMountTarget() + mount = testDataGenerator.uniqueLayerMountTarget() err := policy.EnforceDeviceMountPolicy(constraints.ctx, mount, containerTwo.Layers[i]) if err != nil { @@ -469,7 +839,8 @@ func Test_Rego_EnforceOverlayMountPolicy_Layers_Shared_Layers(t *testing.T) { containerTwoOverlay[len(containerTwoOverlay)-i-1] = mount } - err = policy.EnforceOverlayMountPolicy(constraints.ctx, containerID, containerTwoOverlay, testDataGenerator.uniqueMountTarget()) + err = policy.EnforceOverlayMountPolicy( + constraints.ctx, containerID, containerTwoOverlay, getOverlayMountTarget(containerID)) if err != nil { t.Fatalf("Unexpected error mounting overlay: %v", err) } @@ -490,12 +861,16 @@ func Test_Rego_EnforceOverlayMountPolicy_Overlay_Single_Container_Twice(t *testi return false } - if err := tc.policy.EnforceOverlayMountPolicy(p.ctx, tc.containerID, tc.layers, testDataGenerator.uniqueMountTarget()); err != nil { + overlayTarget := getOverlayMountTarget(tc.containerID) + + if err := tc.policy.EnforceOverlayMountPolicy( + p.ctx, tc.containerID, tc.layers, overlayTarget); err != nil { t.Errorf("expected nil error got: %v", err) return false } - if err := tc.policy.EnforceOverlayMountPolicy(p.ctx, tc.containerID, tc.layers, testDataGenerator.uniqueMountTarget()); err == nil { + if err := tc.policy.EnforceOverlayMountPolicy( + p.ctx, tc.containerID, tc.layers, overlayTarget); err == nil { t.Errorf("able to create overlay for the same container twice") return false } else { @@ -536,7 +911,8 @@ func Test_Rego_EnforceOverlayMountPolicy_Reusing_ID_Across_Overlays(t *testing.T t.Fatalf("Unexpected error creating valid overlay: %v", err) } - err = policy.EnforceOverlayMountPolicy(constraints.ctx, containerID, layerPaths, testDataGenerator.uniqueMountTarget()) + err = policy.EnforceOverlayMountPolicy( + constraints.ctx, containerID, layerPaths, getOverlayMountTarget(containerID)) if err != nil { t.Fatalf("Unexpected error mounting overlay filesystem: %v", err) } @@ -547,7 +923,8 @@ func Test_Rego_EnforceOverlayMountPolicy_Reusing_ID_Across_Overlays(t *testing.T t.Fatalf("Unexpected error creating valid overlay: %v", err) } - err = policy.EnforceOverlayMountPolicy(constraints.ctx, containerID, layerPaths, testDataGenerator.uniqueMountTarget()) + err = policy.EnforceOverlayMountPolicy( + constraints.ctx, containerID, layerPaths, getOverlayMountTarget(containerID)) if err == nil { t.Fatalf("Unexpected success mounting overlay filesystem") } @@ -588,7 +965,8 @@ func Test_Rego_EnforceOverlayMountPolicy_Multiple_Instances_Same_Container(t *te } id := testDataGenerator.uniqueContainerID() - err = policy.EnforceOverlayMountPolicy(constraints.ctx, id, layerPaths, testDataGenerator.uniqueMountTarget()) + err = policy.EnforceOverlayMountPolicy( + constraints.ctx, id, layerPaths, getOverlayMountTarget(id)) if err != nil { t.Fatalf("failed with %d containers", containersToCreate) } @@ -604,7 +982,7 @@ func Test_Rego_EnforceOverlayUnmountPolicy(t *testing.T) { return false } - target := testDataGenerator.uniqueMountTarget() + target := getOverlayMountTarget(tc.containerID) err = tc.policy.EnforceOverlayMountPolicy(p.ctx, tc.containerID, tc.layers, target) if err != nil { t.Errorf("Failure setting up overlay for testing: %v", err) @@ -633,14 +1011,14 @@ func Test_Rego_EnforceOverlayUnmountPolicy_No_Matches(t *testing.T) { return false } - target := testDataGenerator.uniqueMountTarget() + target := getOverlayMountTarget(tc.containerID) err = tc.policy.EnforceOverlayMountPolicy(p.ctx, tc.containerID, tc.layers, target) if err != nil { t.Errorf("Failure setting up overlay for testing: %v", err) return false } - badTarget := testDataGenerator.uniqueMountTarget() + badTarget := getOverlayMountTarget(generateContainerID(testRand)) err = tc.policy.EnforceOverlayUnmountPolicy(p.ctx, badTarget) if err == nil { t.Errorf("Unexpected policy enforcement success: %v", err) @@ -774,6 +1152,125 @@ func Test_Rego_EnforceEnvironmentVariablePolicy_NotAllMatches(t *testing.T) { } } +func Test_Rego_EnforceEnvironmentVariablePolicy_RegexPatterns(t *testing.T) { + testCases := []struct { + rule string + expectMatches []string + expectNotMatches []string + skipAddAnchors bool + }{ + { + rule: "PREFIX_.+=.+", + expectMatches: []string{"PREFIX_FOO=BAR"}, + expectNotMatches: []string{"PREFIX_FOO=", "SOMETHING=ELSE", "SOMETHING_PREFIX_FOO=BAR"}, + }, + { + rule: "PREFIX_.+=.+BAR", + expectMatches: []string{"PREFIX_FOO=FOO_BAR"}, + expectNotMatches: []string{"PREFIX_FOO=BAR_FOO"}, + }, + { + rule: "SIMPLE_VAR=.+", + expectMatches: []string{"SIMPLE_VAR=FOO"}, + expectNotMatches: []string{"SIMPLE_VAR=", "SOMETHING=ELSE", "SOMETHING=ELSE:SIMPLE_VAR=FOO", "SIMPLE_VAR_FOO=BAR", "SIMPLE_VAR"}, + }, + { + rule: "SIMPLE_VAR=.*", + expectMatches: []string{"SIMPLE_VAR=FOO", "SIMPLE_VAR="}, + expectNotMatches: []string{"SIMPLE_VAR"}, + }, + { + rule: "SIMPLE_VAR=", + expectMatches: []string{"SIMPLE_VAR="}, + expectNotMatches: []string{"SIMPLE_VAR", "SIMPLE_VAR=FOO"}, + }, + { + rule: "", + expectMatches: []string{}, + expectNotMatches: []string{"ANYTHING", "ANYTHING=ELSE"}, + }, + { + rule: "(^PREFIX1|^PREFIX2)=.+$", + expectMatches: []string{"PREFIX1=FOO", "PREFIX2=BAR"}, + expectNotMatches: []string{"PREFIX3_FOO=BAR", "PREFIX1=", "SOMETHING=ELSE", ""}, + skipAddAnchors: true, + }, + } + + testRule := func(rule string, expectMatches, expectNotMatches []string) { + testName := rule + if testName == "" { + testName = "(empty)" + } + t.Run(testName, func(t *testing.T) { + gc := generateConstraints(testRand, 1) + container := selectContainerFromContainerList(gc.containers, testRand) + container.EnvRules = append(container.EnvRules, EnvRuleConfig{ + Strategy: EnvVarRuleRegex, + Rule: rule, + }) + gc.allowEnvironmentVariableDropping = false + + for _, env := range expectMatches { + tc, err := setupRegoCreateContainerTest(gc, container, false) + if err != nil { + t.Error(err) + return + } + + tc.envList = append(tc.envList, env) + envsToKeep, _, _, err := tc.policy.EnforceCreateContainerPolicy(gc.ctx, tc.sandboxID, tc.containerID, tc.argList, tc.envList, tc.workingDir, tc.mounts, false, tc.noNewPrivileges, tc.user, tc.groups, tc.umask, tc.capabilities, tc.seccomp) + + // getting an error means something is broken + if err != nil { + t.Errorf("Expected container creation to be allowed for env %s. It wasn't: %v", env, err) + return + } + + if !areStringArraysEqual(envsToKeep, tc.envList) { + t.Errorf("Expected env %s to be kept, but it was not in the returned envs: %v", env, envsToKeep) + return + } + } + + for _, env := range expectNotMatches { + tc, err := setupRegoCreateContainerTest(gc, container, false) + if err != nil { + t.Error(err) + return + } + + tc.envList = append(tc.envList, env) + _, _, _, err = tc.policy.EnforceCreateContainerPolicy(gc.ctx, tc.sandboxID, tc.containerID, tc.argList, tc.envList, tc.workingDir, tc.mounts, false, tc.noNewPrivileges, tc.user, tc.groups, tc.umask, tc.capabilities, tc.seccomp) + + // not getting an error means something is broken + if err == nil { + t.Errorf("Expected container creation not to be allowed for env %s. It was allowed: %v", env, err) + return + } + + envName := strings.Split(env, "=")[0] + assertDecisionJSONContains(t, err, "invalid env list", envName) + } + }) + } + + for _, testCase := range testCases { + if !testCase.skipAddAnchors { + for _, rule := range []string{ + testCase.rule, + "^" + testCase.rule, + testCase.rule + "$", + "^" + testCase.rule + "$", + } { + testRule(rule, testCase.expectMatches, testCase.expectNotMatches) + } + } else { + testRule(testCase.rule, testCase.expectMatches, testCase.expectNotMatches) + } + } +} + func Test_Rego_EnforceEnvironmentVariablePolicy_DropEnvs(t *testing.T) { testFunc := func(gc *generatedConstraints) bool { gc.allowEnvironmentVariableDropping = true @@ -1880,8 +2377,8 @@ func Test_Rego_Enforcement_Point_Allowed(t *testing.T) { t.Fatal(err) } - input := make(map[string]interface{}) - results, err := policy.applyDefaults("__fixture_for_allowed_test_false__", input) + results := make(rpi.RegoQueryResult) + results, err = policy.applyDefaults("__fixture_for_allowed_test_false__", nil, results) if err != nil { t.Fatalf("applied defaults for an enforcement point receieved an error: %v", err) } @@ -1896,8 +2393,8 @@ func Test_Rego_Enforcement_Point_Allowed(t *testing.T) { t.Fatal("result of allowed for an available enforcement point was not the specified default (false)") } - input = make(map[string]interface{}) - results, err = policy.applyDefaults("__fixture_for_allowed_test_true__", input) + results = make(rpi.RegoQueryResult) + results, err = policy.applyDefaults("__fixture_for_allowed_test_true__", nil, results) if err != nil { t.Fatalf("applied defaults for an enforcement point receieved an error: %v", err) } @@ -3326,9 +3823,7 @@ func Test_Rego_Plan9MountPolicy_No_Matches(t *testing.T) { tc.seccomp, ) - if err == nil { - t.Fatal("Policy enforcement unexpectedly was allowed") - } + assertDecisionJSONContains(t, err, "invalid mount list") } func Test_Rego_Plan9MountPolicy_Invalid(t *testing.T) { @@ -3346,6 +3841,21 @@ func Test_Rego_Plan9MountPolicy_Invalid(t *testing.T) { } } +func Test_Rego_Plan9MountPolicy_Invalid_PathTraversal(t *testing.T) { + gc := generateConstraints(testRand, maxContainersInGeneratedConstraints) + + tc, err := setupPlan9MountTest(gc) + if err != nil { + t.Fatalf("unable to setup test: %v", err) + } + + mount := tc.uvmPathForShare + "/../../bin" + err = tc.policy.EnforcePlan9MountPolicy(gc.ctx, mount) + if err == nil { + t.Fatal("Policy enforcement unexpectedly was allowed", err) + } +} + func Test_Rego_Plan9UnmountPolicy(t *testing.T) { gc := generateConstraints(testRand, maxContainersInGeneratedConstraints) @@ -3381,9 +3891,7 @@ func Test_Rego_Plan9UnmountPolicy(t *testing.T) { tc.seccomp, ) - if err == nil { - t.Fatal("Policy enforcement unexpectedly was allowed") - } + assertDecisionJSONContains(t, err, "invalid mount list") } func Test_Rego_Plan9UnmountPolicy_No_Matches(t *testing.T) { @@ -3402,9 +3910,7 @@ func Test_Rego_Plan9UnmountPolicy_No_Matches(t *testing.T) { badMount := randString(testRand, maxPlan9MountTargetLength) err = tc.policy.EnforcePlan9UnmountPolicy(gc.ctx, badMount) - if err == nil { - t.Fatalf("Policy enforcement unexpectedly was allowed") - } + assertDecisionJSONContains(t, err, "no device at path to unmount") } func Test_Rego_GetPropertiesPolicy_On(t *testing.T) { @@ -4226,6 +4732,9 @@ func Test_Rego_Scratch_Mount_Policy(t *testing.T) { failureExpected: false, }, } { + + filesystem := "xfs" + t.Run(fmt.Sprintf("UnencryptedAllowed_%t_And_Encrypted_%t", tc.unencryptedAllowed, tc.encrypted), func(t *testing.T) { gc := generateConstraints(testRand, maxContainersInGeneratedConstraints) smConfig, err := setupRegoScratchMountTest(gc, tc.unencryptedAllowed) @@ -4233,15 +4742,29 @@ func Test_Rego_Scratch_Mount_Policy(t *testing.T) { t.Fatalf("unable to setup test: %s", err) } - scratchPath := generateMountTarget(testRand) + containerId := testDataGenerator.uniqueContainerID() + scratchDiskMount := getScratchDiskMountTarget(containerId) + + err = smConfig.policy.EnforceRWDeviceMountPolicy(gc.ctx, scratchDiskMount, tc.encrypted, true, filesystem) + if tc.failureExpected { + if err == nil { + t.Fatal("mounting should've been denied") + } + } else { + if err != nil { + t.Fatalf("mounting unexpectedly was denied: %s", err) + } + } + + scratchPath := path.Join(scratchDiskMount, guestpath.ScratchDir, containerId) err = smConfig.policy.EnforceScratchMountPolicy(gc.ctx, scratchPath, tc.encrypted) if tc.failureExpected { if err == nil { - t.Fatal("policy enforcement should've been denied") + t.Fatal("scratch mount should've been denied") } } else { if err != nil { - t.Fatalf("policy enforcement unexpectedly was denied: %s", err) + t.Fatalf("scratch mount unexpectedly was denied: %s", err) } } }) @@ -4280,7 +4803,15 @@ func Test_Rego_Scratch_Unmount_Policy(t *testing.T) { t.Fatalf("unable to setup test: %s", err) } - scratchPath := generateMountTarget(testRand) + containerId := testDataGenerator.uniqueContainerID() + scratchDiskMount := getScratchDiskMountTarget(containerId) + + err = smConfig.policy.EnforceRWDeviceMountPolicy(gc.ctx, scratchDiskMount, tc.encrypted, true, "xfs") + if err != nil { + t.Fatalf("mounting unexpectedly was denied: %s", err) + } + + scratchPath := path.Join(scratchDiskMount, guestpath.ScratchDir, containerId) err = smConfig.policy.EnforceScratchMountPolicy(gc.ctx, scratchPath, tc.encrypted) if err != nil { t.Fatalf("scratch_mount policy enforcement unexpectedly was denied: %s", err) @@ -4290,6 +4821,11 @@ func Test_Rego_Scratch_Unmount_Policy(t *testing.T) { if err != nil { t.Fatalf("scratch_unmount policy enforcement unexpectedly was denied: %s", err) } + + err = smConfig.policy.EnforceRWDeviceUnmountPolicy(gc.ctx, scratchDiskMount) + if err != nil { + t.Fatalf("device_unmount policy enforcement unexpectedly was denied: %s", err) + } }) } } @@ -4798,7 +5334,7 @@ func Test_FrameworkVersion_Missing(t *testing.T) { layerPaths, err := testDataGenerator.createValidOverlayForContainer(tc.policy, c) - err = tc.policy.EnforceOverlayMountPolicy(gc.ctx, containerID, layerPaths, testDataGenerator.uniqueMountTarget()) + err = tc.policy.EnforceOverlayMountPolicy(gc.ctx, containerID, layerPaths, testDataGenerator.uniqueLayerMountTarget()) if err == nil { t.Error("unexpected success. Missing framework_version should trigger an error.") } @@ -4834,7 +5370,8 @@ func Test_FrameworkVersion_In_Future(t *testing.T) { layerPaths, err := testDataGenerator.createValidOverlayForContainer(tc.policy, c) - err = tc.policy.EnforceOverlayMountPolicy(gc.ctx, containerID, layerPaths, testDataGenerator.uniqueMountTarget()) + err = tc.policy.EnforceOverlayMountPolicy( + gc.ctx, containerID, layerPaths, getOverlayMountTarget(containerID)) if err == nil { t.Error("unexpected success. Future framework_version should trigger an error.") } @@ -5799,7 +6336,8 @@ func Test_Rego_ErrorTruncation_Unable(t *testing.T) { maxErrorMessageLength := 32 tc.policy.maxErrorMessageLength = maxErrorMessageLength - err = tc.policy.EnforceOverlayMountPolicy(gc.ctx, tc.containerID, tc.layers, testDataGenerator.uniqueMountTarget()) + err = tc.policy.EnforceOverlayMountPolicy( + gc.ctx, tc.containerID, tc.layers, getOverlayMountTarget(tc.containerID)) if err == nil { t.Fatal("Policy did not throw the expected error") diff --git a/pkg/securitypolicy/securitypolicyenforcer.go b/pkg/securitypolicy/securitypolicyenforcer.go index 59c3780638..c9782e0d63 100644 --- a/pkg/securitypolicy/securitypolicyenforcer.go +++ b/pkg/securitypolicy/securitypolicyenforcer.go @@ -59,7 +59,9 @@ func init() { type SecurityPolicyEnforcer interface { EnforceDeviceMountPolicy(ctx context.Context, target string, deviceHash string) (err error) + EnforceRWDeviceMountPolicy(ctx context.Context, target string, encrypted, ensureFilesystem bool, filesystem string) (err error) EnforceDeviceUnmountPolicy(ctx context.Context, unmountTarget string) (err error) + EnforceRWDeviceUnmountPolicy(ctx context.Context, unmountTarget string) (err error) EnforceOverlayMountPolicy(ctx context.Context, containerID string, layerPaths []string, target string) (err error) EnforceOverlayUnmountPolicy(ctx context.Context, target string) (err error) EnforceCreateContainerPolicy( @@ -200,10 +202,18 @@ func (OpenDoorSecurityPolicyEnforcer) EnforceDeviceMountPolicy(context.Context, return nil } +func (OpenDoorSecurityPolicyEnforcer) EnforceRWDeviceMountPolicy(context.Context, string, bool, bool, string) error { + return nil +} + func (OpenDoorSecurityPolicyEnforcer) EnforceDeviceUnmountPolicy(context.Context, string) error { return nil } +func (OpenDoorSecurityPolicyEnforcer) EnforceRWDeviceUnmountPolicy(context.Context, string) error { + return nil +} + func (OpenDoorSecurityPolicyEnforcer) EnforceOverlayMountPolicy(context.Context, string, []string, string) error { return nil } @@ -317,10 +327,18 @@ func (ClosedDoorSecurityPolicyEnforcer) EnforceDeviceMountPolicy(context.Context return errors.New("mounting is denied by policy") } +func (ClosedDoorSecurityPolicyEnforcer) EnforceRWDeviceMountPolicy(context.Context, string, bool, bool, string) error { + return errors.New("Read-write device mounting is denied by policy") +} + func (ClosedDoorSecurityPolicyEnforcer) EnforceDeviceUnmountPolicy(context.Context, string) error { return errors.New("unmounting is denied by policy") } +func (ClosedDoorSecurityPolicyEnforcer) EnforceRWDeviceUnmountPolicy(context.Context, string) error { + return errors.New("Read-write device unmounting is denied by policy") +} + func (ClosedDoorSecurityPolicyEnforcer) EnforceOverlayMountPolicy(context.Context, string, []string, string) error { return errors.New("creating an overlay fs is denied by policy") } diff --git a/pkg/securitypolicy/securitypolicyenforcer_rego.go b/pkg/securitypolicy/securitypolicyenforcer_rego.go index bb2fc27530..276fb7b9bf 100644 --- a/pkg/securitypolicy/securitypolicyenforcer_rego.go +++ b/pkg/securitypolicy/securitypolicyenforcer_rego.go @@ -170,7 +170,7 @@ func newRegoPolicy(code string, defaultMounts []oci.Mount, privilegedMounts []oc return policy, nil } -func (policy *regoEnforcer) applyDefaults(enforcementPoint string, results rpi.RegoQueryResult) (rpi.RegoQueryResult, error) { +func (policy *regoEnforcer) applyDefaults(enforcementPoint string, input inputData, results rpi.RegoQueryResult) (rpi.RegoQueryResult, error) { deny := rpi.RegoQueryResult{"allowed": false} info, err := policy.queryEnforcementPoint(enforcementPoint) if err != nil { @@ -182,12 +182,22 @@ func (policy *regoEnforcer) applyDefaults(enforcementPoint string, results rpi.R return deny, fmt.Errorf("rule for %s is missing from policy", enforcementPoint) } + if results.IsEmpty() && info.useFramework { + rule := "data.framework." + enforcementPoint + result, err := policy.rego.Query(rule, input) + if err != nil { + result = nil + } + return result, err + } + return info.defaultResults.Union(results), nil } type enforcementPointInfo struct { availableByPolicyVersion bool defaultResults rpi.RegoQueryResult + useFramework bool } func (policy *regoEnforcer) queryEnforcementPoint(enforcementPoint string) (*enforcementPointInfo, error) { @@ -230,17 +240,23 @@ func (policy *regoEnforcer) queryEnforcementPoint(enforcementPoint string) (*enf defaultResults, err := result.Object("default_results") if err != nil { - return nil, errors.New("enforcement point result missing defaults") + return nil, fmt.Errorf("enforcement point %s result missing defaults", enforcementPoint) } availableByPolicyVersion, err := result.Bool("available") if err != nil { - return nil, errors.New("enforcement point result missing availability info") + return nil, fmt.Errorf("enforcement point %s result missing availability info", enforcementPoint) + } + + useFramework, err := result.Bool("use_framework") + if err != nil { + return nil, fmt.Errorf("enforcement point %s result missing use_framework info", enforcementPoint) } return &enforcementPointInfo{ availableByPolicyVersion: availableByPolicyVersion, defaultResults: defaultResults, + useFramework: useFramework, }, nil } @@ -251,7 +267,7 @@ func (policy *regoEnforcer) enforce(ctx context.Context, enforcementPoint string return nil, policy.denyWithError(ctx, err, input) } - result, err = policy.applyDefaults(enforcementPoint, result) + result, err = policy.applyDefaults(enforcementPoint, input, result) if err != nil { return result, policy.denyWithError(ctx, err, input) } @@ -486,15 +502,34 @@ func (policy *regoEnforcer) redactSensitiveData(input inputData) inputData { } func (policy *regoEnforcer) EnforceDeviceMountPolicy(ctx context.Context, target string, deviceHash string) error { + mountPathRegex := strings.Replace(guestpath.LCOWGlobalScsiMountPrefixFmt, "%d", "[0-9]+", 1) input := inputData{ - "target": target, - "deviceHash": deviceHash, + "target": target, + "deviceHash": deviceHash, + "mountPathRegex": mountPathRegex, } _, err := policy.enforce(ctx, "mount_device", input) return err } +func (policy *regoEnforcer) EnforceRWDeviceMountPolicy(ctx context.Context, target string, encrypted, ensureFilesystem bool, filesystem string) error { + // At this point we do not know what the container ID would be, so we allow + // any valid IDs. + containerIdRegex := "[0-9a-fA-F]{64}" + mountPathRegex := guestpath.LCOWRootPrefixInUVM + "/" + containerIdRegex + input := inputData{ + "target": target, + "encrypted": encrypted, + "ensureFilesystem": ensureFilesystem, + "filesystem": filesystem, + "mountPathRegex": mountPathRegex, + } + + _, err := policy.enforce(ctx, "rw_mount_device", input) + return err +} + func (policy *regoEnforcer) EnforceOverlayMountPolicy(ctx context.Context, containerID string, layerPaths []string, target string) error { input := inputData{ "containerID": containerID, @@ -768,6 +803,15 @@ func (policy *regoEnforcer) EnforceDeviceUnmountPolicy(ctx context.Context, unmo return err } +func (policy *regoEnforcer) EnforceRWDeviceUnmountPolicy(ctx context.Context, unmountTarget string) error { + input := inputData{ + "unmountTarget": unmountTarget, + } + + _, err := policy.enforce(ctx, "rw_unmount_device", input) + return err +} + func appendMountData(mountData []interface{}, mounts []oci.Mount) []interface{} { for _, mount := range mounts { mountData = append(mountData, inputData{ diff --git a/test/functional/lcow_policy_test.go b/test/functional/lcow_policy_test.go index 43c5fd2090..cf0c857e08 100644 --- a/test/functional/lcow_policy_test.go +++ b/test/functional/lcow_policy_test.go @@ -4,7 +4,9 @@ package functional import ( "context" + "encoding/hex" "fmt" + "math/rand" "testing" ctrdoci "github.com/containerd/containerd/v2/pkg/oci" @@ -22,6 +24,15 @@ import ( testuvm "github.com/Microsoft/hcsshim/test/pkg/uvm" ) +func genValidContainerID(t *testing.T, rng *rand.Rand) string { + t.Helper() + randBytes := make([]byte, 32) + if _, err := rng.Read(randBytes); err != nil { + t.Fatalf("failed to generate random bytes for container ID: %v", err) + } + return hex.EncodeToString(randBytes) +} + func setupScratchTemplate(ctx context.Context, tb testing.TB) string { tb.Helper() opts := defaultLCOWOptions(ctx, tb) @@ -43,6 +54,8 @@ func TestGetProperties_WithPolicy(t *testing.T) { ctx := util.Context(namespacedContext(context.Background()), t) scratchPath := setupScratchTemplate(ctx, t) + rng := rand.New(rand.NewSource(0)) + ls := linuxImageLayers(ctx, t) for _, allowProperties := range []bool{true, false} { t.Run(fmt.Sprintf("AllowPropertiesAccess_%t", allowProperties), func(t *testing.T) { @@ -61,21 +74,24 @@ func TestGetProperties_WithPolicy(t *testing.T) { ) opts.SecurityPolicyEnforcer = "rego" opts.SecurityPolicy = policy + // VPMem is not currently supported for C-LCOW. + opts.VPMemDeviceCount = 0 - cleanName := util.CleanName(t) + containerID := genValidContainerID(t, rng) vm := testuvm.CreateAndStartLCOWFromOpts(ctx, t, opts) spec := testoci.CreateLinuxSpec( ctx, t, - cleanName, + containerID, testoci.DefaultLinuxSpecOpts( "", ctrdoci.WithProcessArgs("/bin/sh", "-c", testoci.TailNullArgs), + ctrdoci.WithEnv(testoci.DefaultUnixEnv), testoci.WithWindowsLayerFolders(append(ls, scratchPath)), )..., ) - c, _, cleanup := testcontainer.Create(ctx, t, vm, spec, cleanName, hcsOwner) + c, _, cleanup := testcontainer.Create(ctx, t, vm, spec, containerID, hcsOwner) t.Cleanup(cleanup) init := testcontainer.Start(ctx, t, c, nil) From e8d92fd0192d0c8c5c66b822c0a6675ea8fc78ba Mon Sep 17 00:00:00 2001 From: Tingmao Wang Date: Mon, 10 Nov 2025 14:51:27 +0000 Subject: [PATCH 2/5] Extend and apply checkValidContainerID to virtual pod IDs as well Since the virtual pod ID is used to construct paths, it needs to be validated in the same way we validate container/sandbox IDs. This only applies to confidential. Signed-off-by: Tingmao Wang --- internal/guest/runtime/hcsv2/uvm.go | 36 +++++++++++++++-------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/internal/guest/runtime/hcsv2/uvm.go b/internal/guest/runtime/hcsv2/uvm.go index c57079cfb0..45386582f4 100644 --- a/internal/guest/runtime/hcsv2/uvm.go +++ b/internal/guest/runtime/hcsv2/uvm.go @@ -61,20 +61,17 @@ const UVMContainerID = "00000000-0000-0000-0000-000000000000" // to check that sandbox IDs (which is also used in paths) are valid, which has // the same format. const validContainerIDRegexRaw = `[0-9a-fA-F]{64}` + var validContainerIDRegex = regexp.MustCompile("^" + validContainerIDRegexRaw + "$") -// isSandboxId just changes the error message -func checkValidContainerID(id string, isSandboxId bool) error { +// idType just changes the error message +func checkValidContainerID(id string, idType string) error { if id == UVMContainerID { return nil } if !validContainerIDRegex.MatchString(id) { - idtype := "container" - if isSandboxId { - idtype = "sandbox" - } - return errors.Errorf("invalid %s id: %s (must match %s)", idtype, id, validContainerIDRegex.String()) + return errors.Errorf("invalid %s id: %s (must match %s)", idType, id, validContainerIDRegex.String()) } return nil @@ -313,17 +310,22 @@ func checkContainerSettings(sandboxID, containerID string, settings *prot.VMHost } func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VMHostedContainerSettingsV2) (_ *Container, err error) { - if h.HasSecurityPolicy() { - if err = checkValidContainerID(id, false); err != nil { - return nil, err - } - } - criType, isCRI := settings.OCISpecification.Annotations[annotations.KubernetesContainerType] // Check for virtual pod annotation virtualPodID, isVirtualPod := settings.OCISpecification.Annotations[annotations.VirtualPodID] + if h.HasSecurityPolicy() { + if err = checkValidContainerID(id, "container"); err != nil { + return nil, err + } + if virtualPodID != "" { + if err = checkValidContainerID(virtualPodID, "virtual pod"); err != nil { + return nil, err + } + } + } + // Special handling for virtual pod sandbox containers: // The first container in a virtual pod (containerID == virtualPodID) should be treated as a sandbox // even if the CRI annotation might indicate otherwise due to host-side UVM setup differences @@ -467,7 +469,7 @@ func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VM sid, ok := settings.OCISpecification.Annotations[annotations.KubernetesSandboxID] sandboxID = sid if h.HasSecurityPolicy() { - if err = checkValidContainerID(sid, true); err != nil { + if err = checkValidContainerID(sid, "sandbox"); err != nil { return nil, err } } @@ -690,7 +692,7 @@ func writeSpecToFile(ctx context.Context, configFile string, spec *specs.Spec) e func (h *Host) modifyHostSettings(ctx context.Context, containerID string, req *guestrequest.ModificationRequest) (retErr error) { if h.HasSecurityPolicy() { - if err := checkValidContainerID(containerID, false); err != nil { + if err := checkValidContainerID(containerID, "container"); err != nil { return err } } @@ -780,7 +782,7 @@ func (h *Host) modifyHostSettings(ctx context.Context, containerID string, req * func (h *Host) modifyContainerSettings(ctx context.Context, containerID string, req *guestrequest.ModificationRequest) error { if h.HasSecurityPolicy() { - if err := checkValidContainerID(containerID, false); err != nil { + if err := checkValidContainerID(containerID, "container"); err != nil { return err } } @@ -1306,7 +1308,7 @@ func modifyCombinedLayers( switch rt { case guestrequest.RequestTypeAdd: if isConfidential { - if err := checkValidContainerID(containerID, false); err != nil { + if err := checkValidContainerID(containerID, "container"); err != nil { return err } From 0c78a298839f040723141fee81e54040c425da15 Mon Sep 17 00:00:00 2001 From: Tingmao Wang Date: Mon, 14 Jul 2025 21:13:39 +0100 Subject: [PATCH 3/5] Merged PR 12916199: hcsv2/uvm: Check that OCISpecification.Hooks is nil for confidential [cherry-picked from 73a7151ad8f15aad9f0e4eef6b86ed1a1a5a0953] This fixes a vulnerability where the host can provide arbitrary commands as hooks in the OCISpecification, which will get executed when passed to runc. Normally, ociSpec.Hooks is cleared explicitly by the host, and so we should never get passed hooks from a genuine host. The only case where OCI hooks would be set is if we are running a GPU container, in which case guest-side code in GCS will add hooks to set up the device - see addNvidiaDeviceHook. This restriction will also apply to the hook set this way by ourselves, however this is fine for now as we do not support Confidential GPUs. When we do support GPU confidential containers, we will need to pull this enforcement into Rego, and allow the policy to determine whether the hook should be allowed (along with enforcing which devices are allowed to be exposed to the container). Even after this fix, we still have unenforced fields in the OCISpecification that we receive from the host, that may allow the host to, for example, add device nodes, and we should harden this further in a future release. Tested using the following host-side reproduction patch: ```diff diff --git a/internal/hcsoci/hcsdoc_lcow.go b/internal/hcsoci/hcsdoc_lcow.go index db94d73df..a270c13ac 100644 --- a/internal/hcsoci/hcsdoc_lcow.go +++ b/internal/hcsoci/hcsdoc_lcow.go @@ -94,6 +94,22 @@ func createLinuxContainerDocument(ctx context.Context, coi *createOptionsInterna return nil, err } + isSandbox := spec.Annotations[annotations.KubernetesContainerType] == "sandbox" + + if !isSandbox { + if spec.Hooks == nil { + spec.Hooks = &specs.Hooks{} + } + timeout := 5000000 + spec.Hooks.StartContainer = append(spec.Hooks.StartContainer, specs.Hook{ + Path: "/bin/sh", + Args: []string{"/bin/sh", "-c", "echo hacked by hooks > /hacked.txt"}, + Env: []string{"PATH=/usr/local/bin:/usr/bin:/bin:/sbin"}, + Timeout: &timeout, + }) + log.G(ctx).Info("Injected hook into OCISpec") + } + log.G(ctx).WithField("guestRoot", guestRoot).Debug("hcsshim::createLinuxContainerDoc") return &linuxHostedSystem{ SchemaVersion: schemaversion.SchemaV21(), ``` Non-confidential scenarios should not be affected. Signed-off-by: Tingmao Wang --- internal/guest/runtime/hcsv2/uvm.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/guest/runtime/hcsv2/uvm.go b/internal/guest/runtime/hcsv2/uvm.go index 45386582f4..e748260650 100644 --- a/internal/guest/runtime/hcsv2/uvm.go +++ b/internal/guest/runtime/hcsv2/uvm.go @@ -306,6 +306,10 @@ func checkContainerSettings(sandboxID, containerID string, settings *prot.VMHost scratchDirPath, expectedScratchDirPathNonShared, expectedScratchDirPathShared) } + if settings.OCISpecification.Hooks != nil { + return errors.Errorf("OCISpecification.Hooks must be nil.") + } + return nil } From 2766a367f6ba4b3e153ba877fa2a96e893633ac0 Mon Sep 17 00:00:00 2001 From: Tingmao Wang Date: Mon, 10 Nov 2025 15:10:23 +0000 Subject: [PATCH 4/5] Merged PR 12936349: rego: Harden fragments loading [cherry-picked from 5469e5c013e08dfca8aaf31dae8f617831fd5ac7] For an arbitrary fragment offered by the host, currently we need to interpret the fragment code as a new Rego module in order for the framework to check it. However, we did not check that the namespace of the fragment, which is used as the namespace of the module, does not conflict with any of our own namespaces (e.g. framework, policy, or api). This means that as soon as we load it for policy checking, a malicious fragment could override enforcement points defined in the framework, including "load_fragment", and allow itself in, even if we would otherwise have denied it because of wrong issuer, etc. This can be tested with the below fragment: package framework svn := "1" framework_version := "0.3.1" load_fragment := {"allowed": true, "add_module": true} enforcement_point_info := { "available": true, "unknown": false, "invalid": false, "version_missing": false, "default_results": {"allowed": true}, "use_framework": true } mount_device := {"allowed": true} rw_mount_device := {"allowed": true} mount_overlay := {"allowed": true} create_container := {"allowed": true, "env_list": null, "allow_stdio_access": true} unmount_device := {"allowed": true} rw_unmount_device := {"allowed": true} unmount_overlay := {"allowed": true} exec_in_container := {"allowed": true, "env_list": null} exec_external := {"allowed": true, "env_list": null, "allow_stdio_access": true} shutdown_container := {"allowed": true} signal_container_process := {"allowed": true} plan9_mount := {"allowed": true} plan9_unmount := {"allowed": true} get_properties := {"allowed": true} dump_stacks := {"allowed": true} runtime_logging := {"allowed": true} scratch_mount := {"allowed": true} scratch_unmount := {"allowed": true} However, our namespace parsing was also suspectable to attacks, and we might end up concluding that the fragment namespace is safe even if when the Rego engine parses it, it sees a package of "framework". This can be demonstrated with the following fragment: package #aa framework svn := "0" framework_version := "0.3.1" load_fragment := {"allowed": true, "add_module": true} And so we also ensure that the `package ...` line can't contain anything unusual. Furthermore, since it can still be risky to inject arbitrary, untrusted Rego code into our execution context, we add another check prior to loading the fragment as a module, where we make sure that the fragment has to first pass the issuer and feed check, which do not require loading it. Signed-off-by: Tingmao Wang --- pkg/securitypolicy/framework.rego | 34 +- .../policy_v0.10.0_api_test.rego | 13 + .../policy_v0.10.0_api_test_allow_all.rego | 22 + pkg/securitypolicy/rego_utils_test.go | 11 +- pkg/securitypolicy/regopolicy_linux_test.go | 535 +++++++++++++++++- .../securitypolicyenforcer_rego.go | 75 ++- 6 files changed, 644 insertions(+), 46 deletions(-) create mode 100644 pkg/securitypolicy/policy_v0.10.0_api_test_allow_all.rego diff --git a/pkg/securitypolicy/framework.rego b/pkg/securitypolicy/framework.rego index 20e0e8b067..1be2625048 100644 --- a/pkg/securitypolicy/framework.rego +++ b/pkg/securitypolicy/framework.rego @@ -1209,8 +1209,6 @@ candidate_fragments := fragments { fragments := array.concat(policy_fragments, fragment_fragments) } -default load_fragment := {"allowed": false} - svn_ok(svn, minimum_svn) { # deprecated semver.is_valid(svn) @@ -1222,15 +1220,32 @@ svn_ok(svn, minimum_svn) { to_number(svn) >= to_number(minimum_svn) } -fragment_ok(fragment) { +fragment_issuer_feed_ok(fragment) { input.issuer == fragment.issuer input.feed == fragment.feed - svn_ok(data[input.namespace].svn, fragment.minimum_svn) +} + +default load_fragment := {"allowed": false} + +# load_fragment gets called twice - first before loading the fragment as a Rego +# module, with input.fragment_loaded set to false, in which case we do not yet +# have access to anything under data[fragment.namespace] yet, and so we only +# check that the fragment issuer and feed is valid, but does not actually load +# the fragment into metadata. It will then be called a second time, at which +# point we can check the SVN defined in the fragment is valid, and if +# successful, add the fragment to the metadata. + +load_fragment := {"allowed": true} { + not input.fragment_loaded + some fragment in candidate_fragments + fragment_issuer_feed_ok(fragment) } load_fragment := {"metadata": [updateIssuer], "add_module": add_module, "allowed": true} { + input.fragment_loaded some fragment in candidate_fragments - fragment_ok(fragment) + fragment_issuer_feed_ok(fragment) + svn_ok(data[input.namespace].svn, fragment.minimum_svn) issuer := update_issuer(fragment.includes) updateIssuer := { @@ -1641,6 +1656,7 @@ default fragment_version_is_valid := false fragment_version_is_valid { some fragment in candidate_fragments + input.fragment_loaded fragment.issuer == input.issuer fragment.feed == input.feed svn_ok(data[input.namespace].svn, fragment.minimum_svn) @@ -1652,6 +1668,7 @@ svn_mismatch { some fragment in candidate_fragments fragment.issuer == input.issuer fragment.feed == input.feed + input.fragment_loaded to_number(data[input.namespace].svn) semver.is_valid(fragment.minimum_svn) } @@ -1660,6 +1677,7 @@ svn_mismatch { some fragment in candidate_fragments fragment.issuer == input.issuer fragment.feed == input.feed + input.fragment_loaded semver.is_valid(data[input.namespace].svn) to_number(fragment.minimum_svn) } @@ -1667,6 +1685,7 @@ svn_mismatch { errors["fragment svn is below the specified minimum"] { input.rule == "load_fragment" fragment_feed_matches + input.fragment_loaded not svn_mismatch not fragment_version_is_valid } @@ -1674,6 +1693,7 @@ errors["fragment svn is below the specified minimum"] { errors["fragment svn and the specified minimum are different types"] { input.rule == "load_fragment" fragment_feed_matches + input.fragment_loaded svn_mismatch } @@ -1704,12 +1724,16 @@ errors[framework_version_error] { } errors[fragment_framework_version_error] { + input.rule == "load_fragment" + input.fragment_loaded input.namespace fragment_framework_version == null fragment_framework_version_error := concat(" ", ["fragment framework_version is missing. Current version:", version]) } errors[fragment_framework_version_error] { + input.rule == "load_fragment" + input.fragment_loaded input.namespace semver.compare(fragment_framework_version, version) > 0 fragment_framework_version_error := concat(" ", ["fragment framework_version is ahead of the current version:", fragment_framework_version, "is greater than", version]) diff --git a/pkg/securitypolicy/policy_v0.10.0_api_test.rego b/pkg/securitypolicy/policy_v0.10.0_api_test.rego index a5ea9c56d4..407c3ee8ff 100644 --- a/pkg/securitypolicy/policy_v0.10.0_api_test.rego +++ b/pkg/securitypolicy/policy_v0.10.0_api_test.rego @@ -3,6 +3,19 @@ package policy api_version := "0.10.0" framework_version := "0.3.0" +fragments := [ + { + "feed": "@@FRAGMENT_FEED@@", + "includes": [ + "containers", + "fragments" + ], + "issuer": "@@FRAGMENT_ISSUER@@", + "minimum_svn": "0" + } +] + + containers := [ { "allow_elevated": false, diff --git a/pkg/securitypolicy/policy_v0.10.0_api_test_allow_all.rego b/pkg/securitypolicy/policy_v0.10.0_api_test_allow_all.rego new file mode 100644 index 0000000000..dccdba0dec --- /dev/null +++ b/pkg/securitypolicy/policy_v0.10.0_api_test_allow_all.rego @@ -0,0 +1,22 @@ +package policy + +api_version := "0.10.0" +framework_version := "0.3.0" + +mount_device := {"allowed": true} +mount_overlay := {"allowed": true} +create_container := {"allowed": true, "env_list": null, "allow_stdio_access": true} +unmount_device := {"allowed": true} +unmount_overlay := {"allowed": true} +exec_in_container := {"allowed": true, "env_list": null} +exec_external := {"allowed": true, "env_list": null, "allow_stdio_access": true} +shutdown_container := {"allowed": true} +signal_container_process := {"allowed": true} +plan9_mount := {"allowed": true} +plan9_unmount := {"allowed": true} +get_properties := {"allowed": true} +dump_stacks := {"allowed": true} +runtime_logging := {"allowed": true} +load_fragment := {"allowed": true} +scratch_mount := {"allowed": true} +scratch_unmount := {"allowed": true} diff --git a/pkg/securitypolicy/rego_utils_test.go b/pkg/securitypolicy/rego_utils_test.go index 2e357f071a..5ac12a5a0a 100644 --- a/pkg/securitypolicy/rego_utils_test.go +++ b/pkg/securitypolicy/rego_utils_test.go @@ -1628,8 +1628,15 @@ var apiTestCode string //go:embed policy_v0.10.0_api_test.rego var policyWith_0_10_0_apiTestCode string -func getPolicyCode_0_10_0(layerHash string) string { - return strings.Replace(policyWith_0_10_0_apiTestCode, "@@CONTAINER_LAYER_HASH@@", layerHash, 1) +//go:embed policy_v0.10.0_api_test_allow_all.rego +var policyWith_0_10_0_apiTestAllowAllCode string + +func getPolicyCode_0_10_0(layerHash, fragmentIssuer, fragmentFeed string) string { + s := policyWith_0_10_0_apiTestCode + s = strings.Replace(s, "@@CONTAINER_LAYER_HASH@@", layerHash, 1) + s = strings.Replace(s, "@@FRAGMENT_ISSUER@@", fragmentIssuer, 1) + s = strings.Replace(s, "@@FRAGMENT_FEED@@", fragmentFeed, 1) + return s } func (p *regoEnforcer) injectTestAPI() error { diff --git a/pkg/securitypolicy/regopolicy_linux_test.go b/pkg/securitypolicy/regopolicy_linux_test.go index bec4f27f07..e83a6d30e4 100644 --- a/pkg/securitypolicy/regopolicy_linux_test.go +++ b/pkg/securitypolicy/regopolicy_linux_test.go @@ -564,30 +564,7 @@ func Test_Rego_EnforceRWDeviceMountPolicy_InvalidFilesystem(t *testing.T) { // allowing enforcing rw mounts. func Test_Rego_EnforceRWDeviceMountPolicy_Compat_0_10_0_allow_all(t *testing.T) { p := generateConstraints(testRand, 1) - regoPolicy := ` -package policy - -api_version := "0.10.0" -framework_version := "0.3.0" - -mount_device := {"allowed": true} -mount_overlay := {"allowed": true} -create_container := {"allowed": true, "env_list": null, "allow_stdio_access": true} -unmount_device := {"allowed": true} -unmount_overlay := {"allowed": true} -exec_in_container := {"allowed": true, "env_list": null} -exec_external := {"allowed": true, "env_list": null, "allow_stdio_access": true} -shutdown_container := {"allowed": true} -signal_container_process := {"allowed": true} -plan9_mount := {"allowed": true} -plan9_unmount := {"allowed": true} -get_properties := {"allowed": true} -dump_stacks := {"allowed": true} -runtime_logging := {"allowed": true} -load_fragment := {"allowed": true} -scratch_mount := {"allowed": true} -scratch_unmount := {"allowed": true} -` + regoPolicy := policyWith_0_10_0_apiTestAllowAllCode for _, b1 := range []bool{true, false} { for _, b2 := range []bool{true, false} { policy, err := newRegoPolicy(regoPolicy, []oci.Mount{}, []oci.Mount{}, testOSType) @@ -608,7 +585,7 @@ scratch_unmount := {"allowed": true} // enforcing rw mounts. func Test_Rego_EnforceRWDeviceMountPolicy_Compat_0_10_0(t *testing.T) { p := generateConstraints(testRand, 1) - regoPolicy := getPolicyCode_0_10_0(p.containers[0].Layers[0]) + regoPolicy := getPolicyCode_0_10_0(p.containers[0].Layers[0], testDataGenerator.uniqueFragmentIssuer(), testDataGenerator.uniqueFragmentFeed()) for _, b1 := range []bool{true, false} { for _, b2 := range []bool{true, false} { policy, err := newRegoPolicy(regoPolicy, []oci.Mount{}, []oci.Mount{}, testOSType) @@ -4088,6 +4065,134 @@ func Test_Rego_LoadFragment_Container(t *testing.T) { } } +// Make sure we don't break fragment loading for old policies +func Test_Rego_LoadFragment_Container_Compat_0_10_0(t *testing.T) { + f := func(p *generatedConstraints) bool { + tc, err := setupRegoFragmentTestConfigWithIncludes(p, []string{"containers"}) + if err != nil { + t.Error(err) + return false + } + + fragment := tc.fragments[0] + container := tc.containers[0] + rego := getPolicyCode_0_10_0(container.container.Layers[0], fragment.info.issuer, fragment.info.feed) + policy, err := newRegoPolicy(rego, []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Fatalf("unable to create Rego policy: %v", err) + } + tc.policy = policy + + err = tc.policy.LoadFragment(p.ctx, fragment.info.issuer, fragment.info.feed, fragment.code) + if err != nil { + t.Error("unable to load fragment: %w", err) + return false + } + + containerID, err := mountImageForContainer(tc.policy, container.container) + if err != nil { + t.Error("unable to mount image for fragment container: %w", err) + return false + } + + _, _, _, err = tc.policy.EnforceCreateContainerPolicy(p.ctx, + container.sandboxID, + containerID, + copyStrings(container.container.Command), + copyStrings(container.envList), + container.container.WorkingDir, + copyMounts(container.mounts), + false, + container.container.NoNewPrivileges, + container.user, + container.groups, + container.container.User.Umask, + container.capabilities, + container.seccomp, + ) + + if err != nil { + t.Error("unable to create container from fragment: %w", err) + return false + } + + if tc.policy.rego.IsModuleActive(rpi.ModuleID(fragment.info.issuer, fragment.info.feed)) { + t.Error("module not removed after load") + return false + } + + return true + } + + if err := quick.Check(f, &quick.Config{MaxCount: 25, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_LoadFragment_Container_Compat_0_10_0: %v", err) + } +} + +// Make sure we don't break fragment loading for old allow all policies +func Test_Rego_LoadFragment_Container_Compat_0_10_0_allow_all(t *testing.T) { + f := func(p *generatedConstraints) bool { + tc, err := setupRegoFragmentTestConfigWithIncludes(p, []string{"containers"}) + if err != nil { + t.Error(err) + return false + } + + rego := policyWith_0_10_0_apiTestAllowAllCode + policy, err := newRegoPolicy(rego, []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Fatalf("unable to create Rego policy: %v", err) + } + tc.policy = policy + + fragment := tc.fragments[0] + container := tc.containers[0] + err = tc.policy.LoadFragment(p.ctx, fragment.info.issuer, fragment.info.feed, fragment.code) + if err != nil { + t.Error("unable to load fragment: %w", err) + return false + } + + containerID, err := mountImageForContainer(tc.policy, container.container) + if err != nil { + t.Error("unable to mount image for fragment container: %w", err) + return false + } + + _, _, _, err = tc.policy.EnforceCreateContainerPolicy(p.ctx, + container.sandboxID, + containerID, + copyStrings(container.container.Command), + copyStrings(container.envList), + container.container.WorkingDir, + copyMounts(container.mounts), + false, + container.container.NoNewPrivileges, + container.user, + container.groups, + container.container.User.Umask, + container.capabilities, + container.seccomp, + ) + + if err != nil { + t.Error("unable to create container from fragment: %w", err) + return false + } + + if tc.policy.rego.IsModuleActive(rpi.ModuleID(fragment.info.issuer, fragment.info.feed)) { + t.Error("module not removed after load") + return false + } + + return true + } + + if err := quick.Check(f, &quick.Config{MaxCount: 25, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_LoadFragment_Container_Compat_0_10_0: %v", err) + } +} + func Test_Rego_LoadFragment_Fragment(t *testing.T) { f := func(p *generatedConstraints) bool { tc, err := setupRegoFragmentTestConfigWithIncludes(p, []string{"fragments"}) @@ -4236,6 +4341,173 @@ func Test_Rego_LoadFragment_BadFeed(t *testing.T) { } } +func Test_Rego_parseNamespace(t *testing.T) { + type testCase struct { + inputs []string + expected string + expectFail bool + } + testCases := []testCase{ + { + inputs: []string{ + "package a\nanything-else", + "package a \n\n", + "package a ", + }, + expected: "a", + }, + { + inputs: []string{ + "package aaa", + "package aaa ", + "package aaa\n# anything", + }, + expected: "aaa", + }, + { + inputs: []string{ + "package", + "package\n", + "package ", + "package ", + "package$", + "package aa#bb\nframework", + "package\naa\n", + }, + expectFail: true, + }, + { + inputs: []string{ + "package framework", + "package api", + }, + expectFail: true, + }, + } + + for _, tc := range testCases { + for _, input := range tc.inputs { + result, err := parseNamespace(input) + if tc.expectFail && err == nil { + t.Errorf("Expected failure for input %q, but got success", input) + } else if !tc.expectFail && err != nil { + t.Errorf("Unexpected error for input %q: %v", input, err) + } else if !tc.expectFail && result != tc.expected { + t.Errorf("Expected to parse namespace %q for input %q, but got %q", tc.expected, input, result) + } + } + } +} + +func expectFragmentNotLoaded(t *testing.T, policy *regoEnforcer, issuer, feed string) bool { + if policy.rego.IsModuleActive(rpi.ModuleID(issuer, feed)) { + t.Errorf("fragment module is present") + return false + } + mtdIssuer, err := policy.rego.GetMetadata("issuers", issuer) + if err != nil && !strings.Contains(err.Error(), "value not found") && + !strings.Contains(err.Error(), "metadata not found for name issuers") { + t.Errorf("unexpected error when checking issuer metadata: %v", err) + return false + } + if mtdIssuer != nil || err == nil { + t.Errorf("fragment issuer metadata is present") + return false + } + return true +} + +func Test_Rego_LoadFragment_BadNamespace(t *testing.T) { + f := func(p *generatedConstraints) bool { + tc, err := setupSimpleRegoFragmentTestConfig(p) + if err != nil { + t.Error(err) + return false + } + + fragment := tc.fragments[0] + code := fmt.Sprintf(`package framework + +svn := "%s" +framework_version := "%s" + +load_fragment := {"allowed": true, "add_module": true} +enforcement_point_info := { + "available": true, + "unknown": false, + "invalid": false, + "version_missing": false, + "default_results": {"allowed": true}, + "use_framework": true +} +`, fragment.info.minimumSVN, frameworkVersion) + + err = tc.policy.LoadFragment(p.ctx, fragment.info.issuer, fragment.info.feed, code) + + if err == nil { + t.Error("expected to be unable to load fragment due to bad namespace") + return false + } + + if !strings.Contains(err.Error(), "namespace \"framework\" is reserved") { + t.Errorf("expected error string to contain 'namespace \"framework\" is reserved', but got %q", err.Error()) + return false + } + + if !expectFragmentNotLoaded(t, tc.policy, fragment.info.issuer, fragment.info.feed) { + return false + } + + return true + } + + if err := quick.Check(f, &quick.Config{MaxCount: 25, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_LoadFragment_BadNamespace: %v", err) + } +} + +func Test_Rego_LoadFragment_BadNamespace2(t *testing.T) { + f := func(p *generatedConstraints) bool { + tc, err := setupSimpleRegoFragmentTestConfig(p) + if err != nil { + t.Error(err) + return false + } + + fragment := tc.fragments[0] + code := fmt.Sprintf(`package #aa +framework + +svn := "%s" +framework_version := "%s" + +load_fragment := {"allowed": true, "add_module": true} +`, fragment.info.minimumSVN, frameworkVersion) + + err = tc.policy.LoadFragment(p.ctx, fragment.info.issuer, fragment.info.feed, code) + + if err == nil { + t.Error("expected to be unable to load fragment due to invalid namespace") + return false + } + + if !strings.Contains(err.Error(), "valid package definition required on first line") { + t.Errorf("expected error string to contain 'valid package definition required on first line', but got %q", err.Error()) + return false + } + + if !expectFragmentNotLoaded(t, tc.policy, fragment.info.issuer, fragment.info.feed) { + return false + } + + return true + } + + if err := quick.Check(f, &quick.Config{MaxCount: 25, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_LoadFragment_BadNamespace: %v", err) + } +} + func Test_Rego_LoadFragment_InvalidSVN(t *testing.T) { f := func(p *generatedConstraints) bool { tc, err := setupRegoFragmentSVNErrorTestConfig(p) @@ -4256,7 +4528,7 @@ func Test_Rego_LoadFragment_InvalidSVN(t *testing.T) { return false } - if tc.policy.rego.IsModuleActive(rpi.ModuleID(fragment.info.issuer, fragment.info.feed)) { + if !expectFragmentNotLoaded(t, tc.policy, fragment.info.issuer, fragment.info.feed) { t.Error("module not removed upon failure") return false } @@ -4369,7 +4641,7 @@ func Test_Rego_LoadFragment_SVNMismatch(t *testing.T) { return false } - if tc.policy.rego.IsModuleActive(rpi.ModuleID(fragment.info.issuer, fragment.info.feed)) { + if !expectFragmentNotLoaded(t, tc.policy, fragment.info.issuer, fragment.info.feed) { t.Error("module not removed upon failure") return false } @@ -4671,10 +4943,16 @@ framework_version := "%s" default load_fragment := {"allowed": false} +check_svn_if_loaded { + not input.fragment_loaded +} else { + data[input.namespace].svn >= 1 +} + load_fragment := {"allowed": true, "add_module": true} { input.issuer == "%s" input.feed == "%s" - data[input.namespace].svn >= 1 + check_svn_if_loaded } mount_device := data.fragment.mount_device @@ -4705,6 +4983,207 @@ mount_device := data.fragment.mount_device } } +func Test_Rego_LoadFragment_BadIssuer_AttemptOverrideFrameworkItems(t *testing.T) { + f := func(p *generatedConstraints) bool { + tc, err := setupSimpleRegoFragmentTestConfig(p) + if err != nil { + t.Error(err) + return false + } + + fragment := tc.fragments[0] + expectedIssuer := fragment.info.issuer + actualIssuer := testDataGenerator.uniqueFragmentIssuer() + code := fmt.Sprintf(`package fragment + +svn := "%s" +framework_version := "%s" + +load_fragment := {"allowed": true, "add_module": true} +data.framework.load_fragment := {"allowed": true, "add_module": true} +input.issuer := "%s" +data.framework.input.issuer := "%s" +`, fragment.info.minimumSVN, frameworkVersion, expectedIssuer, expectedIssuer) + + err = tc.policy.LoadFragment(p.ctx, actualIssuer, fragment.info.feed, code) + + if !assertDecisionJSONContains(t, err, "invalid fragment issuer") { + return false + } + + if !expectFragmentNotLoaded(t, tc.policy, fragment.info.issuer, fragment.info.feed) { + return false + } + + return true + } + + if err := quick.Check(f, &quick.Config{MaxCount: 25, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_LoadFragment_BadIssuer_AttemptOverrideFrameworkItems: %v", err) + } +} + +// The intent of this test is really to check that Rego module names are +// case-sensitive, since we do not deny a fragment from having a namespace +// "Framework" or the like. We use svn mismatch here since otherwise the +// enforcer will not even try to load the fragment module at all if issuer or +// feed is wrong. But in reality, if an attacker can sign fragments with the +// correct issuer, they can make the fragment have any SVN they want. +func Test_Rego_LoadFragment_BadSvn_FrameworkNamespaceCaseConfusion(t *testing.T) { + f := func(p *generatedConstraints) bool { + tc, err := setupRegoFragmentSVNErrorTestConfig(p) + if err != nil { + t.Error(err) + return false + } + + fragment := tc.fragments[0] + code := fmt.Sprintf(`package Framework + +svn := "%s" +framework_version := "%s" + +load_fragment := {"allowed": true, "add_module": true} +enforcement_point_info := { + "available": true, + "unknown": false, + "invalid": false, + "version_missing": false, + "default_results": {"allowed": true}, + "use_framework": true +} +data.framework.load_fragment := load_fragment +`, fragment.constraints.svn, frameworkVersion) + + err = tc.policy.LoadFragment(p.ctx, fragment.info.issuer, fragment.info.feed, code) + + if !assertDecisionJSONContains(t, err, "fragment svn is below the specified minimum") { + return false + } + + if !expectFragmentNotLoaded(t, tc.policy, fragment.info.issuer, fragment.info.feed) { + return false + } + + return true + } + + if err := quick.Check(f, &quick.Config{MaxCount: 25, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_LoadFragment_BadSvn_FrameworkNamespaceCaseConfusion: %v", err) + } +} + +func Test_Rego_LoadFragment_BadIssuer_MustNotTryToLoadRego(t *testing.T) { + f := func(p *generatedConstraints) bool { + tc, err := setupSimpleRegoFragmentTestConfig(p) + if err != nil { + t.Error(err) + return false + } + + fragment := tc.fragments[0] + actualIssuer := testDataGenerator.uniqueFragmentIssuer() + code := "package fragment\n!invalid!rego" + + err = tc.policy.LoadFragment(p.ctx, actualIssuer, fragment.info.feed, code) + + if strings.Contains(err.Error(), "error when compiling module") || + !assertDecisionJSONDoesNotContain(t, err, "error when compiling module") { + t.Errorf("expected error to not contain 'error when compiling module', got: %s", err.Error()) + return false + } + if !assertDecisionJSONDoesNotContain(t, err, "fragment framework_version is missing") { + return false + } + if !assertDecisionJSONContains(t, err, "invalid fragment issuer") { + return false + } + if !expectFragmentNotLoaded(t, tc.policy, actualIssuer, fragment.info.feed) { + return false + } + return true + } + + if err := quick.Check(f, &quick.Config{MaxCount: 25, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_LoadFragment_BadIssuer_MustNotTryToLoadRego: %v", err) + } +} + +func Test_Rego_LoadFragment_BadFeed_MustNotTryToLoadRego(t *testing.T) { + f := func(p *generatedConstraints) bool { + tc, err := setupSimpleRegoFragmentTestConfig(p) + if err != nil { + t.Error(err) + return false + } + + fragment := tc.fragments[0] + actualFeed := testDataGenerator.uniqueFragmentFeed() + code := "package fragment\n!invalid!rego" + + err = tc.policy.LoadFragment(p.ctx, fragment.info.issuer, actualFeed, code) + + if strings.Contains(err.Error(), "error when compiling module") || + !assertDecisionJSONDoesNotContain(t, err, "error when compiling module") { + t.Errorf("expected error to not contain 'error when compiling module', got: %s", err.Error()) + return false + } + if !assertDecisionJSONDoesNotContain(t, err, "fragment framework_version is missing") { + return false + } + if !assertDecisionJSONContains(t, err, "invalid fragment feed") { + return false + } + if !expectFragmentNotLoaded(t, tc.policy, fragment.info.issuer, actualFeed) { + return false + } + return true + } + + if err := quick.Check(f, &quick.Config{MaxCount: 25, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_LoadFragment_BadFeed_MustNotTryToLoadRego: %v", err) + } +} + +func Test_Rego_LoadFragment_BadIssuer_MustNotTryToLoadRego_Compat_0_10_0(t *testing.T) { + f := func(p *generatedConstraints) bool { + tc, err := setupSimpleRegoFragmentTestConfig(p) + if err != nil { + t.Error(err) + return false + } + rego := getPolicyCode_0_10_0(tc.containers[0].container.Layers[0], tc.fragments[0].info.issuer, tc.fragments[0].info.feed) + policy, err := newRegoPolicy(rego, []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Fatalf("unable to create Rego policy: %v", err) + } + tc.policy = policy + + fragment := tc.fragments[0] + actualIssuer := testDataGenerator.uniqueFragmentIssuer() + code := "package fragment\n!invalid!rego" + + err = tc.policy.LoadFragment(p.ctx, actualIssuer, fragment.info.feed, code) + + if strings.Contains(err.Error(), "error when compiling module") || + !assertDecisionJSONDoesNotContain(t, err, "error when compiling module") { + t.Errorf("expected error to not contain 'error when compiling module', got: %s", err.Error()) + return false + } + if !assertDecisionJSONDoesNotContain(t, err, "fragment framework_version is missing") { + return false + } + if !assertDecisionJSONContains(t, err, "invalid fragment issuer") { + return false + } + return true + } + + if err := quick.Check(f, &quick.Config{MaxCount: 25, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_LoadFragment_BadIssuer_MustNotTryToLoadRego_Compat_0_10_0: %v", err) + } +} + func Test_Rego_Scratch_Mount_Policy(t *testing.T) { for _, tc := range []struct { unencryptedAllowed bool diff --git a/pkg/securitypolicy/securitypolicyenforcer_rego.go b/pkg/securitypolicy/securitypolicyenforcer_rego.go index 276fb7b9bf..075d5450c5 100644 --- a/pkg/securitypolicy/securitypolicyenforcer_rego.go +++ b/pkg/securitypolicy/securitypolicyenforcer_rego.go @@ -9,6 +9,8 @@ import ( "encoding/base64" "encoding/json" "fmt" + "regexp" + "slices" "strings" "syscall" @@ -1044,14 +1046,45 @@ func (policy *regoEnforcer) EnforceRuntimeLoggingPolicy(ctx context.Context) err return err } +// Rego identifier is a letter or underscore, followed by any number of letters, +// underscores, or digits. See open-policy-agent/opa +// ast/internal/scanner/scanner.go :: scanIdentifier, isLetter +// Technically it also allows other unicode digit characters (but not letters) +// but we do not allow those, for simplicity. +var validNamespaceRegex = `[a-zA-Z_][a-zA-Z0-9_]*` + +// First line of the fragment Rego source code must be a package definition +// without any potential for confusion attacks. We thus limit it to exactly +// "package" followed by one or more spaces, then a valid Rego identifier, then +// optionally more spaces. We do not check if the namespace is a Rego keyword +// (e.g. "in", "every" etc) but it would fail Rego compilation anyway. +var validFirstLine = regexp.MustCompile(`^package +(` + validNamespaceRegex + `)\s*$`) + +// These namespaces must not be overridden by a fragment +var reservedNamespaces []string = []string{ + // Built-in modules + "framework", + "api", + "policy", + // This is not a module, but to prevent confusion since framework uses + // data.metadata to access those, we block it as well. + "metadata", +} + func parseNamespace(rego string) (string, error) { lines := strings.Split(rego, "\n") - parts := strings.Split(lines[0], " ") - if parts[0] != "package" { - return "", errors.New("package definition required on first line") + if lines[0] == "" { + return "", errors.New("Fragment Rego is empty") } - - return strings.TrimSpace(parts[1]), nil + match := validFirstLine.FindStringSubmatch(lines[0]) + if match == nil { + return "", errors.Errorf("valid package definition required on first line, got %q", lines[0]) + } + namespace := match[1] + if slices.Contains(reservedNamespaces, namespace) { + return "", errors.Errorf("namespace %q is reserved and cannot be used for fragments", namespace) + } + return namespace, nil } func (policy *regoEnforcer) LoadFragment(ctx context.Context, issuer string, feed string, rego string) error { @@ -1067,22 +1100,42 @@ func (policy *regoEnforcer) LoadFragment(ctx context.Context, issuer string, fee Namespace: namespace, } - policy.rego.AddModule(fragment.ID(), fragment) - input := inputData{ - "issuer": issuer, - "feed": feed, - "namespace": namespace, + "issuer": issuer, + "feed": feed, + "namespace": namespace, + "fragment_loaded": false, } + // Check that the fragment is signed by the expected issuer before loading + // its Rego code. + _, err = policy.enforce(ctx, "load_fragment", input) + if err != nil { + return err + } + + // At this point we need to add the fragment code as a new Rego module in + // order for the framework (or any user defined policies) to check the SVN, + // and potentially other information defined by its Rego code. We've already + // checked that the fragment is signed correctly, and the namespace is safe + // to load (won't override framework or other built-in modules). Once we + // added the module, we must make sure the module is removed if we return + // with error (or if add_module returned from Rego is false). + policy.rego.AddModule(fragment.ID(), fragment) + input["fragment_loaded"] = true + results, err := policy.enforce(ctx, "load_fragment", input) + if err != nil { + policy.rego.RemoveModule(fragment.ID()) + return err + } addModule, _ := results.Bool("add_module") if !addModule { policy.rego.RemoveModule(fragment.ID()) } - return err + return nil } func (policy *regoEnforcer) EnforceScratchMountPolicy(ctx context.Context, scratchPath string, encrypted bool) error { From 506dd6c16220b8349fc400e94491d4bda7a40eb9 Mon Sep 17 00:00:00 2001 From: Tingmao Wang Date: Mon, 10 Nov 2025 15:14:07 +0000 Subject: [PATCH 5/5] Merged PR 12878106: Small rego fixes [cherry-picked from 0ca40bb4f130b3508f4a130011463070328d40d0] - rego: Fix missing error reason when mounting a rw device to an existing mount point. This fixes a missing error message introduced in the last round of security fixes. It's not hugely important, but eases debugging if we get policy denials on mounting the scratch, for whatever reason. Also adds test for it. - Remove a no-op from rego Checked with @ earlier that this basically does nothing and is just something left over. However I will not actually add a remove op for `metadata.started` for now. This PR is targeting the conf-aci branch on ADO because the commit being fixed is not on main yet. This should be backported to main together with the fixes from last month. Signed-off-by: Tingmao Wang --- pkg/securitypolicy/framework.rego | 4 ++-- pkg/securitypolicy/regopolicy_linux_test.go | 16 ++++++++++++++-- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/pkg/securitypolicy/framework.rego b/pkg/securitypolicy/framework.rego index 1be2625048..ca6721c5dc 100644 --- a/pkg/securitypolicy/framework.rego +++ b/pkg/securitypolicy/framework.rego @@ -901,7 +901,7 @@ exec_in_container := {"metadata": [updateMatches], default shutdown_container := {"allowed": false} -shutdown_container := {"started": remove, "metadata": [remove], "allowed": true} { +shutdown_container := {"metadata": [remove], "allowed": true} { container_started remove := { "name": "matches", @@ -1313,7 +1313,7 @@ errors["deviceHash not found"] { } errors["device already mounted at path"] { - input.rule == "mount_device" + input.rule in ["mount_device", "rw_mount_device"] device_mounted(input.target) } diff --git a/pkg/securitypolicy/regopolicy_linux_test.go b/pkg/securitypolicy/regopolicy_linux_test.go index e83a6d30e4..8dd409fccf 100644 --- a/pkg/securitypolicy/regopolicy_linux_test.go +++ b/pkg/securitypolicy/regopolicy_linux_test.go @@ -378,7 +378,7 @@ func Test_Rego_EnforceDeviceMountPolicy_InvalidMountTarget_PathTraversal(t *test assertDecisionJSONContains(t, err, "mountpoint invalid") } -func deviceMountUnmountTest(t *testing.T, p *generatedConstraints, policy *regoEnforcer, mountScratchFirst, unmountScratchFirst, testInvalidUnmount bool) bool { +func deviceMountUnmountTest(t *testing.T, p *generatedConstraints, policy *regoEnforcer, mountScratchFirst, unmountScratchFirst, testDenials bool) bool { container := selectContainerFromContainerList(p.containers, testRand) containerID := testDataGenerator.uniqueContainerID() rotarget := testDataGenerator.uniqueLayerMountTarget() @@ -414,6 +414,18 @@ func deviceMountUnmountTest(t *testing.T, p *generatedConstraints, policy *regoE } } + if testDenials { + err = policy.EnforceRWDeviceMountPolicy(p.ctx, rwtarget, true, true, "xfs") + if !assertDecisionJSONContains(t, err, "device already mounted at path") { + return false + } + + err = policy.EnforceDeviceMountPolicy(p.ctx, rotarget, container.Layers[0]) + if !assertDecisionJSONContains(t, err, "device already mounted at path") { + return false + } + } + unmountScratch := func() bool { err = policy.EnforceRWDeviceUnmountPolicy(p.ctx, rwtarget) if err != nil { @@ -442,7 +454,7 @@ func deviceMountUnmountTest(t *testing.T, p *generatedConstraints, policy *regoE } } - if testInvalidUnmount { + if testDenials { err = policy.EnforceDeviceUnmountPolicy(p.ctx, rotarget) if !assertDecisionJSONContains(t, err, "no device at path to unmount") { return false