From f265533855f961b24f7322937290a2d7068291e6 Mon Sep 17 00:00:00 2001 From: Takuro Sato Date: Thu, 3 Sep 2026 16:55:46 +0100 Subject: [PATCH] CWCOW: enforce security policy in gcs-sidecar Apply policy decisions to container creation, exec, environment, stdio, storage, mounts, registry changes, and CIM lifecycle operations. Validate forwarded requests and unsupported fields, maintain policy state consistently, and fail closed when host operations fail. Co-authored-by: Mahati Chamarthy Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Takuro Sato --- internal/gcs-sidecar/bridge.go | 78 ++ internal/gcs-sidecar/handlers.go | 919 +++++++++++++++--- internal/gcs-sidecar/handlers_test.go | 906 +++++++++++++++++ internal/gcs-sidecar/host.go | 140 +++ internal/guest/runtime/hcsv2/uvm.go | 4 +- internal/hcs/schema2/virtual_machine.go | 22 +- internal/tools/securitypolicy/main.go | 2 + pkg/securitypolicy/api.rego | 3 + pkg/securitypolicy/framework.rego | 396 +++++++- pkg/securitypolicy/open_door.rego | 3 + pkg/securitypolicy/opts.go | 7 + pkg/securitypolicy/policy.rego | 3 + pkg/securitypolicy/rego_utils_test.go | 64 +- pkg/securitypolicy/regopolicy_linux_test.go | 1 + pkg/securitypolicy/regopolicy_windows_test.go | 834 +++++++++++++++- pkg/securitypolicy/securitypolicy.go | 46 +- pkg/securitypolicy/securitypolicy_internal.go | 68 ++ pkg/securitypolicy/securitypolicy_marshal.go | 90 +- pkg/securitypolicy/securitypolicyenforcer.go | 41 +- .../securitypolicyenforcer_rego.go | 69 +- pkg/securitypolicy/windows_tooling_test.go | 8 +- test/pkg/securitypolicy/policy.go | 2 + 22 files changed, 3521 insertions(+), 185 deletions(-) diff --git a/internal/gcs-sidecar/bridge.go b/internal/gcs-sidecar/bridge.go index 3f73765679..dd22b15322 100644 --- a/internal/gcs-sidecar/bridge.go +++ b/internal/gcs-sidecar/bridge.go @@ -35,6 +35,16 @@ type Bridge struct { pendingMu sync.Mutex pending map[sequenceID]chan *prot.ContainerExecuteProcessResponse + // monitoredMu guards monitoredIDs. + monitoredMu sync.Mutex + // monitoredIDs holds request IDs of forwarded combined-layers / + // mapped-directory mount/unmount operations whose inbox GCS response must be + // watched. The sidecar forwards those operations rather than performing them, + // so it cannot revert the policy state it staged; if the inbox reports a + // failure the UVM is failed closed (see monitorInboxResponse and + // Host.setUVMInconsistent). + monitoredIDs map[sequenceID]struct{} + hostState *Host // List of handlers for handling different rpc message requests. rpcHandlerList map[prot.RPCProc]HandlerFunc @@ -81,6 +91,7 @@ func NewBridge(shimConn io.ReadWriteCloser, inboxGCSConn io.ReadWriteCloser, ini hostState := NewHost(initialEnforcer, logWriter) return &Bridge{ pending: make(map[sequenceID]chan *prot.ContainerExecuteProcessResponse), + monitoredIDs: make(map[sequenceID]struct{}), rpcHandlerList: make(map[prot.RPCProc]HandlerFunc), hostState: hostState, shimConn: shimConn, @@ -220,6 +231,36 @@ func (b *Bridge) forwardRequestToGcs(req *request) { b.sendToGCSCh <- *req } +// monitorInboxResponse records that the inbox GCS response for the given +// request ID must be watched. It is used for forwarded combined-layers and +// mapped-directory mount/unmount operations, whose real work happens in the +// inbox GCS: because the sidecar cannot revert the policy state it staged for +// them, a failure response fails the UVM closed instead (see the receive loop +// and Host.setUVMInconsistent). +func (b *Bridge) monitorInboxResponse(id sequenceID) { + b.monitoredMu.Lock() + b.monitoredIDs[id] = struct{}{} + b.monitoredMu.Unlock() +} + +// responseFailure returns a non-nil error if the inbox GCS response message +// reports the operation failed (non-zero HResult). A response that cannot be +// parsed is treated as success (nil) so a malformed message does not by itself +// fail the UVM closed. +func responseFailure(message []byte) error { + var base prot.ResponseBase + if err := json.Unmarshal(message, &base); err != nil { + return nil + } + if base.Result != 0 { + if base.ErrorMessage != "" { + return errors.New(base.ErrorMessage) + } + return fmt.Errorf("inbox GCS returned HResult 0x%x", uint32(base.Result)) + } + return nil +} + func getContextAndSpan(baseSpanCtx prot.Otelspancontext) (context.Context, trace.Span) { var ctx context.Context var span trace.Span @@ -447,6 +488,43 @@ func (b *Bridge) ListenAndServeShimRequests() error { b.pendingMu.Unlock() } + // If this is a container-exit notification, mark the container + // terminated so a later combined-layers unmount isn't blocked as + // in-use. + const MsgNotifyContainer prot.MsgType = prot.MsgTypeNotify | prot.ComputeSystem | prot.NotifyContainer + + if header.Type == MsgNotifyContainer { + var ntf prot.ContainerNotification + ntf.ResultInfo.Value = &json.RawMessage{} + if uerr := json.Unmarshal(message, &ntf); uerr != nil { + log.G(ctx).WithError(uerr).Error("failed to unmarshal container notification") + } else if c, cerr := b.hostState.GetCreatedContainer(ctx, ntf.ContainerID); cerr == nil { + // A not-found error just means the notification is for + // something we don't track (the UVM itself, or a container + // already deleted). + c.terminated.Store(true) + } + } + + // If this response correlates to a forwarded mount/unmount + // operation we are monitoring (combined-layers or mapped + // directory) and it reports a failure, the sidecar's policy state + // may now be out of sync with what is actually mounted. Since we + // forwarded rather than performed the operation, we cannot safely + // revert; fail the UVM closed instead so no further container or + // mount operations proceed on possibly-desynced state. + b.monitoredMu.Lock() + _, monitored := b.monitoredIDs[header.ID] + if monitored { + delete(b.monitoredIDs, header.ID) + } + b.monitoredMu.Unlock() + if monitored { + if respErr := responseFailure(message); respErr != nil { + b.hostState.setUVMInconsistent(fmt.Errorf("forwarded mount/unmount operation (request %d) failed in inbox GCS: %w", header.ID, respErr)) + } + } + // Forward to shim resp := bridgeResponse{ ctx: context.Background(), diff --git a/internal/gcs-sidecar/handlers.go b/internal/gcs-sidecar/handlers.go index 33709a9278..06ec8ad6b2 100644 --- a/internal/gcs-sidecar/handlers.go +++ b/internal/gcs-sidecar/handlers.go @@ -10,6 +10,7 @@ import ( "fmt" "os" "path/filepath" + "regexp" "strings" "time" @@ -22,15 +23,14 @@ import ( "github.com/Microsoft/hcsshim/internal/guestpath" hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2" "github.com/Microsoft/hcsshim/internal/log" - oci "github.com/Microsoft/hcsshim/internal/oci" "github.com/Microsoft/hcsshim/internal/ot" "github.com/Microsoft/hcsshim/internal/protocol/guestrequest" "github.com/Microsoft/hcsshim/internal/protocol/guestresource" "github.com/Microsoft/hcsshim/internal/vm/vmutils/etw" "github.com/Microsoft/hcsshim/internal/windevice" - "github.com/Microsoft/hcsshim/pkg/annotations" "github.com/Microsoft/hcsshim/pkg/cimfs" "github.com/Microsoft/hcsshim/pkg/securitypolicy" + oci "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" "golang.org/x/sys/windows" ) @@ -58,6 +58,12 @@ func (b *Bridge) createContainer(req *request) (err error) { defer span.End() defer func() { ot.SetSpanStatus(span, err) }() + // Refuse to create containers once the UVM has been marked inconsistent by a + // failed forwarded mount/unmount (cf. LCOW Host.checkState). + if err := b.hostState.checkState(); err != nil { + return fmt.Errorf("CreateContainer denied: %w", err) + } + var createContainerRequest prot.ContainerCreate var containerConfig json.RawMessage createContainerRequest.ContainerConfig.Value = &containerConfig @@ -82,56 +88,160 @@ func (b *Bridge) createContainer(req *request) (err error) { container := cwcowHostedSystem.Container spec := cwcowHostedSystemConfig.Spec containerID := createContainerRequest.ContainerID - log.G(ctx).Tracef("rpcCreate: CWCOWHostedSystemConfig {spec: %v, schemaVersion: %v, container: %v}}", string(req.message), schemaVersion, container) - - // Enforce registry changes policy - if container != nil && container.RegistryChanges != nil { - log.G(ctx).Trace("Container has registry changes, validating against policy") - - // First, separate default values from non-default values - var defaultValues []hcsschema.RegistryValue - var nonDefaultValues []hcsschema.RegistryValue + if err := validateContainerID(containerID); err != nil { + return fmt.Errorf("CreateContainer operation is denied by policy: %w", err) + } + containerJSON, _ := json.Marshal(container) + log.G(ctx).Tracef("rpcCreate: CWCOWHostedSystemConfig {spec: %v, schemaVersion: %v, container: %s}}", string(req.message), schemaVersion, containerJSON) + + // The block below is a reference example (not executed): a sample CRI + // container.json and the HostedSystem.Container the host derives from it. + // It documents the shapes this handler enforces and forwards. + /* + Test container.json: + + { + "metadata": { + "name": "wcow-test" + }, + "image": { + "image": "takurosatodevacr.azurecr.io/payload-demo:250929" + }, + "command": [ + "python", + "hello.py" + ], + "envs": [ + { + "key": "APP_FOO", + "value": "BAR" + } + ], + "mounts": [ + { + "host_path": "C:\\share-ro", + "container_path": "C:\\mnt\\ro", + "readonly": true + }, + { + "host_path": "\\\\.\\pipe\\hostedsystem-demo", + "container_path": "\\\\.\\pipe\\hostedsystem-demo" + } + ], + "windows": { + "security_context": { + "credential_spec": "{\"CmsPlugins\":[\"ActiveDirectory\"],\"DomainJoinConfig\":{\"Sid\":\"S-1-5-21-1111111111-2222222222-3333333333\",\"MachineAccountName\":\"WebApp01\",\"Guid\":\"244818ae-87ac-4fcd-92ec-e79e5252348a\",\"DnsTreeName\":\"contoso.com\",\"DnsName\":\"contoso.com\",\"NetBiosName\":\"CONTOSO\"},\"ActiveDirectoryConfig\":{\"GroupManagedServiceAccounts\":[{\"Name\":\"WebApp01\",\"Scope\":\"contoso.com\"},{\"Name\":\"WebApp01\",\"Scope\":\"CONTOSO\"}]}}" + }, + "resources": { + "rootfs_size_in_bytes": 42949672960 + } + } + } - if container.RegistryChanges.AddValues != nil { - for _, value := range container.RegistryChanges.AddValues { - if isDefaultRegistryValue(value) { - defaultValues = append(defaultValues, value) - log.G(ctx).WithField("name", value.Name).Trace("Registry value matches default, accepting without policy check") - } else { - nonDefaultValues = append(nonDefaultValues, value) + HostedSystem.Container: + { + "Storage": { + "Layers": [ + { + "Id": "6e2349b7-8215-4325-a88a-38a8e1f67e18", + "Path": "\\\\?\\Volume{6e2349b7-8215-4325-a88a-38a8e1f67e18}\\" } + ], + "Path": "c:\\mounts\\scsi\\m0" + }, + "MappedDirectories": [ + { + "HostPath": "\\\\?\\VMSMB\\VSMB-{dcc079ae-60ba-4d07-847c-3493609c0870}\\s1", + "ContainerPath": "C:\\mnt\\ro", + "ReadOnly": true + } + ], + "MappedPipes": [ + { + "ContainerPipeName": "hostedsystem-demo", + "HostPath": "\\\\?\\VMSMB\\VSMB-{dcc079ae-60ba-4d07-847c-3493609c0870}\\IPC$\\hostedsystem-demo" + } + ], + "Processor": {}, + "Networking": { + "Namespace": "644da769-7f9a-41c7-820b-8ef9e66d747b" + }, + "ContainerCredentialGuard": { + "Cookie": "01000000740069000CEBF50D32C0CF80BE559BE206B4EAF9", + "RpcEndpoint": "91571621-3782-9EC0-3C5C-C0EC10E6E763", + "Transport": "HvSocket", + "CredentialSpec": "{\"CmsPlugins\":[\"ActiveDirectory\"],\"DomainJoinConfig\":{\"Sid\":\"S-1-5-21-1111111111-2222222222-3333333333\",\"MachineAccountName\":\"WebApp01\",\"Guid\":\"244818ae-87ac-4fcd-92ec-e79e5252348a\",\"DnsTreeName\":\"contoso.com\",\"DnsName\":\"contoso.com\",\"NetBiosName\":\"CONTOSO\"},\"ActiveDirectoryConfig\":{\"GroupManagedServiceAccounts\":[{\"Name\":\"WebApp01\",\"Scope\":\"contoso.com\"},{\"Name\":\"WebApp01\",\"Scope\":\"CONTOSO\"}]}}" + }, + "RegistryChanges": { + "AddValues": [ + { + "Key": { + "Hive": "System", + "Name": "ControlSet001\\Control" + }, + "Name": "WaitToKillServiceTimeout", + "Type": "String", + "StringValue": "2147483647" + } + ] } } + */ - // If there are non-default values, validate them against policy - if len(nonDefaultValues) > 0 { - log.G(ctx).Tracef("Validating %d registry values against policy", len(nonDefaultValues)) + // Reject HostedSystem Container fields we don't yet support. + if err := denyUnsupportedContainerFields(container); err != nil { + return fmt.Errorf("CreateContainer operation rejected: %w", err) + } - nonDefaultChanges := &hcsschema.RegistryChanges{ - AddValues: nonDefaultValues, - } + // Enforce registry changes policy. This may drop unauthorized + // non-default registry values from the container before forwarding. + if container != nil && container.RegistryChanges != nil { + log.G(ctx).Trace("Container has registry changes, validating against policy") + + // Separate the pre-approved defaults from the changes that must be + // validated against policy (non-default add values plus all delete + // keys). + defaultValues, nonDefaultChanges := splitRegistryChanges(container.RegistryChanges) - err := b.hostState.securityOptions.PolicyEnforcer.EnforceRegistryChangesPolicy(ctx, containerID, nonDefaultChanges) + // If there are non-default values or any delete keys, validate them + // against policy. + if len(nonDefaultChanges.AddValues) > 0 || len(nonDefaultChanges.DeleteKeys) > 0 { + log.G(ctx).Tracef("Validating %d registry values and %d delete keys against policy", len(nonDefaultChanges.AddValues), len(nonDefaultChanges.DeleteKeys)) + + keptRaw, err := b.hostState.securityOptions.PolicyEnforcer.EnforceRegistryChangesPolicy(ctx, containerID, nonDefaultChanges) if err != nil { log.G(ctx).WithError(err).Warn("Registry changes validation failed - rejecting") return fmt.Errorf("registry entry operation is denied by policy: %w", err) } - log.G(ctx).Tracef("All container registry values validated successfully") + + // The policy uses dropping semantics: it may authorize only a + // subset of the requested non-default values and delete keys. + // Rebuild the container's registry changes as the pre-approved + // defaults plus the policy-kept non-default values, and the + // policy-kept delete keys, so the guest only applies what policy + // sanctioned. + container.RegistryChanges.AddValues, container.RegistryChanges.DeleteKeys = mergeKeptRegistryChanges(defaultValues, keptRaw) } - log.G(ctx).Infof("Registry validation complete: %d total values (%d defaults + %d validated)", - len(container.RegistryChanges.AddValues), len(defaultValues), len(nonDefaultValues)) + log.G(ctx).Infof("Registry validation complete: %d total values now applied (%d defaults), %d delete keys", + len(container.RegistryChanges.AddValues), len(defaultValues), len(container.RegistryChanges.DeleteKeys)) } + // We enforce `spec`, which is not passed to inbox gcs within this createContainer. + // The result of enforcement is stored in memory and used for executeProcess. user := securitypolicy.IDName{ Name: spec.Process.User.Username, } - _, _, _, err := b.hostState.securityOptions.PolicyEnforcer.EnforceCreateContainerPolicyV2(req.ctx, containerID, spec.Process.Args, spec.Process.Env, spec.Process.Cwd, spec.Mounts, user, nil) + envToKeep, _, allowStdio, err := b.hostState.securityOptions.PolicyEnforcer.EnforceCreateContainerPolicyV2(req.ctx, containerID, spec.Process.Args, spec.Process.Env, spec.Process.Cwd, spec.Mounts, user, nil) if err != nil { return fmt.Errorf("CreateContainer operation is denied by policy: %w", err) } + if envToKeep != nil { + spec.Process.Env = []string(envToKeep) + } + // Create the source directory for each mapped directory if it does not // already exist. In non-confidential WCOW the host does this for // sandbox:// mounts by exec'ing `cmd /c mkdir ... & dir ...` inside the @@ -148,11 +258,12 @@ func (b *Bridge) createContainer(req *request) (err error) { processes: make(map[uint32]*containerProcess), commandLine: commandLine, commandLineExec: false, + allowStdio: allowStdio, } log.G(ctx).Tracef("Adding ContainerID: %v", containerID) if err := b.hostState.AddContainer(req.ctx, containerID, c); err != nil { - log.G(ctx).Tracef("Container exists in the map.") + log.G(ctx).Tracef("Container exists in the map. containerID: %v", containerID) return err } defer func() { @@ -163,25 +274,43 @@ func (b *Bridge) createContainer(req *request) (err error) { } }() - if oci.ParseAnnotationsBool(ctx, spec.Annotations, annotations.WCOWSecurityPolicyEnv, true) { - securityContextDir, err := b.hostState.securityOptions.WriteSecurityContextDir(&spec) - if err != nil { - return fmt.Errorf("failed to write security context dir: %w", err) - } + // The security-context dir must always be written; it must not be gated + // by a host-controlled annotation. + securityContextDir, err := b.hostState.securityOptions.WriteSecurityContextDir(&spec) + if err != nil { + return fmt.Errorf("failed to write security context dir: %w", err) + } - // Stage the AMD SNP PSP API DLL into the container's security-context - // directory so the workload can fetch SNP attestation reports. This - // happens after security policy enforcement, consistent with the - // UVM_SECURITY_CONTEXT_DIR env injection done by WriteSecurityContextDir. - if securityContextDir != "" { - if err := stageSnpPspDLL(ctx, securityContextDir); err != nil { - return fmt.Errorf("failed to stage %s: %w", amdSnpPspDLLName, err) - } + // Stage the AMD SNP PSP API DLL into the container's security-context + // directory so the workload can fetch SNP attestation reports. This + // happens after security policy enforcement, consistent with the + // UVM_SECURITY_CONTEXT_DIR env injection done by WriteSecurityContextDir. + if securityContextDir != "" { + if err := stageSnpPspDLL(ctx, securityContextDir); err != nil { + return fmt.Errorf("failed to stage %s: %w", amdSnpPspDLLName, err) } - cwcowHostedSystemConfig.Spec = spec + } + cwcowHostedSystemConfig.Spec = spec + + // Reconcile the host-provided HostedSystem mounts against the enforced + // spec. spec.Mounts has already been validated against policy by + // EnforceCreateContainerPolicyV2 above. Here we make sure the host is + // not forwarding any MappedDirectories or MappedPipes that don't map to + // an enforced spec mount, so the host can't smuggle in a mount the + // policy never saw. + if err := reconcileHostedSystemMounts(spec.Mounts, container); err != nil { + return fmt.Errorf("CreateContainer operation is denied by policy: %w", err) + } + + // Cross-check the forwarded Container.Storage against the root path and + // block-CIM volume the sidecar recorded for this container during layer setup. + if err := reconcileHostedSystemStorage(b.hostState, containerID, container); err != nil { + return fmt.Errorf("CreateContainer operation is denied by policy: %w", err) } - // Strip the spec field + // Marshal the original cwcowHostedSystem from the request. That's safe + // because we've enforced `spec` above and reconciled the forwarded + // MappedDirectories/MappedPipes against it. hostedSystemBytes, err := json.Marshal(cwcowHostedSystem) if err != nil { @@ -223,6 +352,204 @@ func (b *Bridge) createContainer(req *request) (err error) { return nil } +// splitRegistryChanges separates a container's requested registry changes into +// the pre-approved default add values (which bypass policy) and the changes +// that must be validated against policy: the non-default add values plus all +// delete keys, which have no default allowance. +func splitRegistryChanges(changes *hcsschema.RegistryChanges) (defaultValues []hcsschema.RegistryValue, nonDefaultChanges *hcsschema.RegistryChanges) { + var nonDefaultValues []hcsschema.RegistryValue + for _, value := range changes.AddValues { + if isDefaultRegistryValue(value) { + defaultValues = append(defaultValues, value) + } else { + nonDefaultValues = append(nonDefaultValues, value) + } + } + return defaultValues, &hcsschema.RegistryChanges{ + AddValues: nonDefaultValues, + DeleteKeys: changes.DeleteKeys, + } +} + +// mergeKeptRegistryChanges combines the pre-approved default registry values +// with the policy-kept subset returned by EnforceRegistryChangesPolicy. Because +// the policy uses dropping semantics, it may authorize only a subset of the +// requested non-default values and delete keys; the returned slices are what +// the guest should apply (defaults plus the kept non-default values, and the +// kept delete keys). +func mergeKeptRegistryChanges(defaultValues []hcsschema.RegistryValue, kept interface{}) ([]hcsschema.RegistryValue, []hcsschema.RegistryKey) { + var keptNonDefault []hcsschema.RegistryValue + var keptDeleteKeys []hcsschema.RegistryKey + if k, ok := kept.(*hcsschema.RegistryChanges); ok && k != nil { + keptNonDefault = k.AddValues + keptDeleteKeys = k.DeleteKeys + } + + newValues := make([]hcsschema.RegistryValue, 0, len(defaultValues)+len(keptNonDefault)) + newValues = append(newValues, defaultValues...) + newValues = append(newValues, keptNonDefault...) + return newValues, keptDeleteKeys +} + +// namedPipePrefix is the prefix used for Windows named pipe paths. A mount +// whose OCI destination starts with this prefix becomes a MappedPipe in the +// HostedSystem, with ContainerPipeName set to the destination minus this +// prefix (see internal/uvm.ParseNamedPipe and internal/hcsoci/hcsdoc_wcow.go). +const namedPipePrefix = `\\.\pipe\` + +// isPipeDestination reports whether an OCI mount destination refers to a named +// pipe (and would therefore become a MappedPipe rather than a MappedDirectory). +func isPipeDestination(dest string) bool { + return strings.HasPrefix(dest, namedPipePrefix) +} + +// pipeNameFromDestination derives the ContainerPipeName that the host sets for +// a pipe mount from its OCI destination, mirroring ParseNamedPipe. +func pipeNameFromDestination(dest string) string { + return strings.TrimPrefix(dest, namedPipePrefix) +} + +// mountReadOnly reports whether an OCI mount's options request a read-only +// mount, mirroring how the host derives MappedDirectory.ReadOnly in +// internal/hcsoci/hcsdoc_wcow.go (an "ro" option, case-insensitive). +func mountReadOnly(options []string) bool { + for _, o := range options { + if strings.EqualFold(o, "ro") { + return true + } + } + return false +} + +// reconcileHostedSystemMounts verifies that every MappedDirectory and +// MappedPipe the host forwards in the HostedSystem corresponds to an enforced +// spec mount. The spec mounts have already been validated against policy, so +// this binds the forwarded HostedSystem to that enforced view and rejects any +// host-added mount the policy never saw. Note that HostPath is intentionally +// not compared: the spec source is a host-side path while the HostedSystem +// HostPath is the path the host resolved the mount to for the UVM. +// So it legitimately differs from the spec source, +// and the host controls both regardless. +func reconcileHostedSystemMounts(mounts []oci.Mount, container *hcsschema.Container) error { + if container == nil { + return nil + } + + // Every MappedDirectory must correspond to a (non-pipe) spec mount that + // targets the same container path with the same read-only flag. + for _, md := range container.MappedDirectories { + matched := false + for _, m := range mounts { + // Pipe mounts are reconciled against MappedPipes below, not here. + if isPipeDestination(m.Destination) { + continue + } + // Bind on container path (spec destination) + read-only. + if m.Destination == md.ContainerPath && mountReadOnly(m.Options) == md.ReadOnly { + matched = true + break + } + } + if !matched { + return fmt.Errorf("mapped directory %q (readOnly=%v) does not match any enforced spec mount", md.ContainerPath, md.ReadOnly) + } + } + + // Every MappedPipe must correspond to a pipe spec mount that yields the same + // pipe name. We match on the pipe name (derived from the spec destination), + // not the source. + // + // NB: for a pipe, the spec mount and the HostedSystem entry hold *different* + // values for the "same" pipe, which is easy to trip over: + // - spec mount source: "\\.\pipe\" (pure name, NO guid) + // - MappedPipe.HostPath: "\\?\VMSMB\VSMB-{guid}\IPC$\" (host VSMB transport, has guid) + // The spec source stays the clean "\\.\pipe\"; only the host-side + // transport path (HostPath) carries the VSMB guid. HostPath is host-controlled + // and not comparable to the spec source, so we don't compare it here; instead + // we bind on the pipe name. The clean spec source is enforced separately by + // policy (windows_mountConstraint_ok in framework.rego). + for _, mp := range container.MappedPipes { + matched := false + for _, m := range mounts { + // Non-pipe mounts are reconciled against MappedDirectories above. + if !isPipeDestination(m.Destination) { + continue + } + // Bind on the pipe name (destination minus the \\.\pipe\ prefix). + if pipeNameFromDestination(m.Destination) == mp.ContainerPipeName { + matched = true + break + } + } + if !matched { + return fmt.Errorf("mapped pipe %q does not match any enforced spec mount", mp.ContainerPipeName) + } + } + + return nil +} + +// volumeGUIDFromStoragePath extracts the volume GUID from a Container.Storage +// layer path of the form `\\?\Volume{}\` (the volume root, as the host +// writes it in the createContainer document). This differs from +// volumeGUIDFromLayerPath, which parses the `...}\Files` form used in the +// CWCOWCombinedLayers modify request. +func volumeGUIDFromStoragePath(path string) (string, bool) { + if p, ok := strings.CutPrefix(path, `\\?\Volume{`); ok { + if q, ok := strings.CutSuffix(p, `}\`); ok { + return q, true + } + } + return "", false +} + +// reconcileHostedSystemStorage checks that the host-forwarded Container.Storage +// matches the verified handles the sidecar recorded for this container during +// layer setup: +// - Storage.Path must equal the combined-layers root that CWCOWCombinedLayers +// mounted for this container (the scratch that becomes the container root). +// - Storage.Layers must be the single block-CIM volume whose hashes mount_cims +// verified for this container. +// +// The bytes at that volume are already verity-verified, so this does not re-check +// content. It closes a cross-wiring gap: without it a host could forward a create +// document that points the container root at a different (even if separately +// verified) volume than the one enforced for this container. +func reconcileHostedSystemStorage(host *Host, containerID string, container *hcsschema.Container) error { + if container == nil || container.Storage == nil { + return fmt.Errorf("container storage is missing") + } + storage := container.Storage + + wantRootPath, ok := host.containerRootPaths[containerID] + if !ok { + return fmt.Errorf("no container root path recorded for container %s", containerID) + } + if !strings.EqualFold(storage.Path, wantRootPath) { + return fmt.Errorf("storage path %q does not match the enforced container root path %q", storage.Path, wantRootPath) + } + + if len(storage.Layers) != 1 { + return fmt.Errorf("expected exactly one storage layer, got %d", len(storage.Layers)) + } + guidStr, ok := volumeGUIDFromStoragePath(storage.Layers[0].Path) + if !ok { + return fmt.Errorf("storage layer path %q is not a volume path", storage.Layers[0].Path) + } + volGUID, err := guid.FromString(guidStr) + if err != nil { + return fmt.Errorf("invalid storage layer volume GUID %q: %w", guidStr, err) + } + containers, ok := host.blockCIMVolumeContainers[volGUID] + if !ok { + return fmt.Errorf("storage layer volume %s was not verified", volGUID) + } + if _, ok := containers[containerID]; !ok { + return fmt.Errorf("storage layer volume %s was not verified for container %s", volGUID, containerID) + } + return nil +} + // stageSnpPspDLL copies the AMD SNP PSP API DLL from the UVM's System32 into the // container's security-context directory so the workload can fetch SNP // attestation reports. The directory is exposed to the container via the @@ -247,6 +574,57 @@ func stageSnpPspDLL(ctx context.Context, securityContextDir string) error { return nil } +// containerIDRegex matches the identifier format used for container IDs: one +// or more alphanumeric segments joined by single '.', '_' or '-' separators +// (the same shape containerd enforces for identifiers). GUIDs and hex digests +// both satisfy it. It rejects empty strings, path separators, ".." and +// absolute paths, so a host-supplied container ID cannot be used to escape an +// intended directory if it is later joined into a filesystem path. +var containerIDRegex = regexp.MustCompile(`^[a-zA-Z0-9]+(?:[._-][a-zA-Z0-9]+)*$`) + +func validateContainerID(id string) error { + if !containerIDRegex.MatchString(id) { + return fmt.Errorf("invalid container ID %q", id) + } + return nil +} + +// denyUnsupportedContainerFields rejects HostedSystem Container fields that the +// sidecar does not yet enforce a policy over. They may be needed in the future, +// but until we have enforcement for them we block them rather than forward +// host-controlled values unchecked. +// +// Memory, Processor and Networking are deliberately not checked: the host +// controls the UVM's resources and networking regardless, so there is nothing +// we can meaningfully enforce over them here. +// GuestOs is not checked as it just sets hostname string. +func denyUnsupportedContainerFields(container *hcsschema.Container) error { + if container == nil { + return nil + } + + // In case we get any error here, we include entire container JSON + // in the error message for debugging so that we know all the fields + // that need to be enforced by policy. + + // Error is ignored as it's a best-effort debug string. + containerJSON, _ := json.Marshal(container) + + if container.HvSocket != nil { + return fmt.Errorf("HvSocket is not supported. Container: %s", containerJSON) + } + if container.ContainerCredentialGuard != nil { + return fmt.Errorf("ContainerCredentialGuard is not supported. Container: %s", containerJSON) + } + if len(container.AssignedDevices) > 0 { + return fmt.Errorf("AssignedDevices is not supported. Container: %s", containerJSON) + } + if container.AdditionalDeviceNamespace != nil { + return fmt.Errorf("AdditionalDeviceNamespace is not supported. Container: %s", containerJSON) + } + return nil +} + // stageDLL copies the DLL at srcPath into dstDir. If the source DLL does not // exist it is a no-op and returns false without error, so callers can tolerate // environments where the DLL is not present. @@ -306,11 +684,83 @@ func processParamEnvToOCIEnv(environment map[string]string) []string { return environmentList } +// ociEnvToProcessParamEnv is the inverse of processParamEnvToOCIEnv. It converts +// an OCI-style env list (["KEY=VALUE", ...]) back to a ProcessParameters +// Environment map. +func ociEnvToProcessParamEnv(envs []string) map[string]string { + paramEnv := make(map[string]string, len(envs)) + for _, env := range envs { + parts := strings.SplitN(env, "=", 2) + if len(parts) == 2 { + paramEnv[parts[0]] = parts[1] + } + } + return paramEnv +} + +// escapeArgs builds a Windows-style escaped command line from a set of OCI +// process args. This mirrors how the host shim constructs the init process' +// ProcessParameters.CommandLine (internal/cmd.escapeArgs), so the sidecar can +// reconstruct the expected command line from the enforced spec and compare it +// against what the host actually sends in executeProcess. +func escapeArgs(args []string) string { + escaped := make([]string, len(args)) + for i, a := range args { + escaped[i] = windows.EscapeArg(a) + } + return strings.Join(escaped, " ") +} + +// rewriteExecRequest re-marshals an execute process request with updated +// ProcessParameters (e.g., after env filtering by policy). +func rewriteExecRequest(req *request, r prot.ContainerExecuteProcess, params hcsschema.ProcessParameters) (*request, error) { + r.Settings.ProcessParameters.Value = ¶ms + + buf, err := json.Marshal(&r) + if err != nil { + return nil, fmt.Errorf("failed to marshal updated exec request: %w", err) + } + + newReq := &request{ + ctx: req.ctx, + header: req.header, + message: buf, + } + newReq.header.Size = uint32(len(buf)) + prot.HdrSize + return newReq, nil +} + +// enforceStdioParams applies a stdio-access policy decision. When denied, a +// process that requires a console is rejected (there is no console without +// stdio); otherwise the stdio pipe flags are cleared. Returns whether params +// changed so callers can skip an unnecessary rewrite. +func enforceStdioParams(allowStdio bool, params *hcsschema.ProcessParameters) (bool, error) { + if allowStdio { + return false, nil + } + + // A console can't be honored without stdio, so reject rather than silently + // dropping EmulateConsole and running a non-interactive process the caller + // didn't ask for. + if params.EmulateConsole { + return false, errors.New("process that requires console access denied due to policy not allowing stdio access") + } + + changed := params.CreateStdInPipe || params.CreateStdOutPipe || params.CreateStdErrPipe + params.CreateStdInPipe = false + params.CreateStdOutPipe = false + params.CreateStdErrPipe = false + return changed, nil +} + func (b *Bridge) startContainer(req *request) (err error) { _, span := ot.StartSpan(req.ctx, "sidecar::startContainer") defer span.End() defer func() { ot.SetSpanStatus(span, err) }() + // We don't need any enforcement here because the container has already been created and + // this request is just to start the container. + var r prot.RequestBase if err := commonutils.UnmarshalJSONWithHresult(req.message, &r); err != nil { return fmt.Errorf("failed to unmarshal startContainer: %w", err) @@ -377,7 +827,7 @@ func (b *Bridge) executeProcess(req *request) (err error) { if containerID == UVMContainerID { log.G(req.ctx).Tracef("Enforcing policy on external exec process") - _, _, err := b.hostState.securityOptions.PolicyEnforcer.EnforceExecExternalProcessPolicy( + envToKeep, stdioAllowed, err := b.hostState.securityOptions.PolicyEnforcer.EnforceExecExternalProcessPolicy( req.ctx, commandLine, processParamEnvToOCIEnv(processParams.Environment), @@ -386,6 +836,22 @@ func (b *Bridge) executeProcess(req *request) (err error) { if err != nil { return errors.Wrapf(err, "exec is denied due to policy") } + needsRewrite := false + if envToKeep != nil { + processParams.Environment = ociEnvToProcessParamEnv(envToKeep) + needsRewrite = true + } + stdioChanged, err := enforceStdioParams(stdioAllowed, &processParams) + if err != nil { + return errors.Wrapf(err, "exec is denied due to policy") + } + needsRewrite = needsRewrite || stdioChanged + if needsRewrite { + req, err = rewriteExecRequest(req, r, processParams) + if err != nil { + return fmt.Errorf("failed to rewrite exec request: %w", err) + } + } b.forwardRequestToGcs(req) } else { // fetch the container command line @@ -399,7 +865,10 @@ func (b *Bridge) executeProcess(req *request) (err error) { isCreateExec := c.commandLine && !c.commandLineExec if isCreateExec { // if this is an exec of Container command line, then it's already enforced - // during container creation, hence skip it here + // during container creation. + // We use the result of enforcement from container creation to + // validate the exec command line and drop environment variable if necessary. + c.commandLineExec = true } @@ -409,7 +878,7 @@ func (b *Bridge) executeProcess(req *request) (err error) { Name: processParams.User, } log.G(req.ctx).Tracef("Enforcing policy on exec in container") - _, _, _, err = b.hostState.securityOptions.PolicyEnforcer. + envToKeep, _, stdioAllowed, err := b.hostState.securityOptions.PolicyEnforcer. EnforceExecInContainerPolicyV2( req.ctx, containerID, @@ -422,6 +891,64 @@ func (b *Bridge) executeProcess(req *request) (err error) { if err != nil { return errors.Wrapf(err, "exec in container denied due to policy") } + needsRewrite := false + if envToKeep != nil { + processParams.Environment = ociEnvToProcessParamEnv(envToKeep) + needsRewrite = true + } + stdioChanged, err := enforceStdioParams(stdioAllowed, &processParams) + if err != nil { + return errors.Wrapf(err, "exec in container denied due to policy") + } + needsRewrite = needsRewrite || stdioChanged + if needsRewrite { + req, err = rewriteExecRequest(req, r, processParams) + if err != nil { + return fmt.Errorf("failed to rewrite exec request: %w", err) + } + } + } else { + // This is the container's init process. Its command line, working + // directory, user and environment were already validated against + // policy in createContainer, and the result is stored in c.spec. + // The host fully controls this executeProcess request though, so we + // cross-check it against the enforced spec instead of trusting it: + // otherwise a host could pass policy with a benign spec at create + // time and then launch a different init command (e.g. + // "cmd.exe /c ") or smuggle back environment variables that + // create-time enforcement dropped. + if c.spec.Process == nil { + return errors.New("exec in container denied due to policy: enforced spec has no process") + } + enforced := c.spec.Process + + expectedCmdLine := enforced.CommandLine + if expectedCmdLine == "" { + expectedCmdLine = escapeArgs(enforced.Args) + } + if processParams.CommandLine != expectedCmdLine { + return fmt.Errorf("exec in container denied due to policy: init command line %q does not match enforced %q", processParams.CommandLine, expectedCmdLine) + } + if enforced.Cwd != "" && processParams.WorkingDirectory != enforced.Cwd { + return fmt.Errorf("exec in container denied due to policy: init working directory %q does not match enforced %q", processParams.WorkingDirectory, enforced.Cwd) + } + if enforced.User.Username != "" && processParams.User != enforced.User.Username { + return fmt.Errorf("exec in container denied due to policy: init user %q does not match enforced %q", processParams.User, enforced.User.Username) + } + + // Re-apply the environment that createContainer enforcement + // produced (dropped variables removed, nothing injected) so the + // init process runs with exactly the enforced environment. + processParams.Environment = ociEnvToProcessParamEnv(enforced.Env) + + if _, err = enforceStdioParams(c.allowStdio, &processParams); err != nil { + return errors.Wrapf(err, "exec in container denied due to policy") + } + + req, err = rewriteExecRequest(req, r, processParams) + if err != nil { + return fmt.Errorf("failed to rewrite init exec request: %w", err) + } } headerID := req.header.ID @@ -591,15 +1118,36 @@ func (b *Bridge) deleteContainerState(req *request) (err error) { defer span.End() defer func() { ot.SetSpanStatus(span, err) }() + // Refuse to delete container state once the UVM has been marked inconsistent + // by a failed forwarded mount/unmount (cf. LCOW Host.checkState). + if err := b.hostState.checkState(); err != nil { + return fmt.Errorf("deleteContainerState denied: %w", err) + } + var r prot.DeleteContainerStateRequest if err := commonutils.UnmarshalJSONWithHresult(req.message, &r); err != nil { return fmt.Errorf("failed to unmarshal deleteContainerState: %w", err) } - err = b.hostState.RemoveContainer(req.ctx, r.ContainerID) + + // Refuse to delete the state of a container that is still running, or whose + // combined-layers root is still mounted, so the host can't wipe a live + // container's rootfs (cf. LCOW Host.DeleteContainerState). + c, err := b.hostState.GetCreatedContainer(req.ctx, r.ContainerID) if err != nil { log.G(req.ctx).Tracef("Container not found during deleteContainerState: %v", r.ContainerID) return fmt.Errorf("container not found: %w", err) } + if !c.terminated.Load() { + return fmt.Errorf("deleteContainerState denied: container %s is still running", r.ContainerID) + } + if b.hostState.IsContainerRootMountedForContainer(r.ContainerID) { + return fmt.Errorf("deleteContainerState denied: container %s combined-layers root is still mounted", r.ContainerID) + } + + if err = b.hostState.RemoveContainer(req.ctx, r.ContainerID); err != nil { + log.G(req.ctx).Tracef("Container not found during deleteContainerState: %v", r.ContainerID) + return fmt.Errorf("container not found: %w", err) + } b.forwardRequestToGcs(req) return nil @@ -835,31 +1383,94 @@ func (b *Bridge) modifySettings(req *request) (err error) { return fmt.Errorf("invald guestRequestType %v", guestRequestType) } + // If a previously forwarded mount/unmount operation failed in the inbox GCS, + // the sidecar's policy state may be out of sync with what is actually mounted + // and cannot be safely recovered, so refuse all further settings changes + // (cf. LCOW checkState gating in internal/guest/runtime/hcsv2/uvm.go). + if err := b.hostState.checkState(); err != nil { + return fmt.Errorf("modifySettings denied: %w", err) + } + + // monitorResponse is set for forwarded combined-layers / mapped-directory + // operations whose real work happens in the inbox GCS. Their inbox response + // is watched (see monitorInboxResponse) so a failure fails the UVM closed, + // since the sidecar cannot revert the policy state it staged for them. + monitorResponse := false + + // Question: should we enforce policy for each type? Maybe just reject if we don't implement policy? if guestResourceType != "" { switch guestResourceType { case guestresource.ResourceTypeCombinedLayers: + // This is for non-confidential WCOW. + // Ideally gcs-sidecar supports it with policy enforcement, + // but for now we just reject it because + // we don't have a policy enforcer for it. settings := modifyGuestSettingsRequest.Settings.(*guestresource.WCOWCombinedLayers) log.G(ctx).Tracef("WCOWCombinedLayers: {%v}", settings) + return fmt.Errorf("WCOWCombinedLayers is not supported") case guestresource.ResourceTypeNetworkNamespace: + // Forwarded to inbox GCS without enforcement, by design: the host + // controls the UVM's networking regardless of what is configured here, + // so there is nothing meaningful for the guest to enforce. + // LCOW does the same (see modifyNetwork in internal\guest\runtime\hcsv2\uvm.go). settings := modifyGuestSettingsRequest.Settings.(*hcn.HostComputeNamespace) log.G(ctx).Tracef("HostComputeNamespaces { %v}", settings) case guestresource.ResourceTypeNetwork: + // Forwarded without enforcement for the same reason as + // ResourceTypeNetworkNamespace above: networking is host-controlled. settings := modifyGuestSettingsRequest.Settings.(*guestrequest.NetworkModifyRequest) log.G(ctx).Tracef("NetworkModifyRequest { %v}", settings) case guestresource.ResourceTypeMappedVirtualDisk: - wcowMappedVirtualDisk := modifyGuestSettingsRequest.Settings.(*guestresource.WCOWMappedVirtualDisk) - log.G(ctx).Tracef("wcowMappedVirtualDisk { %v}", wcowMappedVirtualDisk) + settings := modifyGuestSettingsRequest.Settings.(*guestresource.WCOWMappedVirtualDisk) + log.G(ctx).Tracef("WCOWMappedVirtualDisk: {%v}", settings) + // The container scratch disk is *added* via + // ResourceTypeMappedVirtualDiskForContainerScratch (which formats it + // and rewrites the request to MappedVirtualDisk before forwarding), + // but it is *removed* as a plain MappedVirtualDisk. So a Remove here + // is the scratch (or other disk) detach on teardown and must be + // forwarded to the inbox GCS: rejecting it leaves the scratch + // attached, which breaks a later re-mount of the same container root. + // Detaching a disk grants no access, so forwarding Remove is safe. A + // raw Add, on the other hand, is the host trying to attach an + // arbitrary disk we don't enforce over, so it stays rejected. + if modifyGuestSettingsRequest.RequestType != guestrequest.RequestTypeRemove { + // Error is ignored as it's a best-effort debug string. + settingsJSON, _ := json.Marshal(settings) + return fmt.Errorf("MappedVirtualDisk Add is not supported. Settings: %s", settingsJSON) + } + // Remove falls through to forwardRequestToGcs below. case guestresource.ResourceTypeHvSocket: - hvSocketAddress := modifyGuestSettingsRequest.Settings.(*hcsschema.HvSocketAddress) - log.G(ctx).Tracef("hvSocketAddress { %v }", hvSocketAddress) + // Forwarded without enforcement: this is just for configuration + // to help guest to resolve hvsocket targets. + settings := modifyGuestSettingsRequest.Settings.(*hcsschema.HvSocketAddress) + log.G(ctx).Tracef("HvSocketAddress { %v }", settings) case guestresource.ResourceTypeMappedDirectory: + // We don't have hostpath enforcement because anyway contents of the dir can be changed by the host. settings := modifyGuestSettingsRequest.Settings.(*hcsschema.MappedDirectory) log.G(ctx).Tracef("hcsschema.MappedDirectory { %v }", settings) + switch modifyGuestSettingsRequest.RequestType { + case guestrequest.RequestTypeAdd: + if err := b.hostState.securityOptions.PolicyEnforcer.EnforceMappedDirectoryMountPolicy( + ctx, settings.ContainerPath, settings.ReadOnly); err != nil { + return fmt.Errorf("mapped directory mount is denied by policy: %w", err) + } + case guestrequest.RequestTypeRemove: + if err := b.hostState.securityOptions.PolicyEnforcer.EnforceMappedDirectoryUnmountPolicy( + ctx, settings.ContainerPath); err != nil { + return fmt.Errorf("mapped directory unmount is denied by policy: %w", err) + } + default: + return fmt.Errorf("unsupported request type %v for MappedDirectory", modifyGuestSettingsRequest.RequestType) + } + // The sidecar enforced policy here but the actual VSMB mount/unmount + // happens in the inbox GCS, so watch its response and fail closed on + // failure (the staged policy metadata cannot be reverted). + monitorResponse = true case guestresource.ResourceTypeSecurityPolicy: securityPolicyRequest := modifyGuestSettingsRequest.Settings.(*guestresource.ConfidentialOptions) @@ -963,46 +1574,69 @@ func (b *Bridge) modifySettings(req *request) (err error) { hashesToVerify = layerHashes[1:] } - err := b.hostState.securityOptions.PolicyEnforcer.EnforceVerifiedCIMsPolicy(req.ctx, containerID, hashesToVerify, mountedCim) - if err != nil { - return errors.Wrap(err, "CIM mount is denied by policy") - } - - // Volume GUID from request + // Volume GUID from request. volGUID := wcowBlockCimMounts.VolumeGUID - // Cache hashes along with volGUID - b.hostState.blockCIMVolumeHashes[volGUID] = layerHashes - - // Store the containerID (associated with volGUID) to mark that hashes are verified for this container - if _, ok := b.hostState.blockCIMVolumeContainers[volGUID]; !ok { - b.hostState.blockCIMVolumeContainers[volGUID] = make(map[string]struct{}) - } - b.hostState.blockCIMVolumeContainers[volGUID][containerID] = struct{}{} - - log.G(ctx).Tracef("Cached %d verified CIM layer hashes for volume %s (container %s)", len(hashesToVerify), volGUID, containerID) + // Enforce policy, mount, then record the verified state as a single + // transaction: if the real mount fails after the policy check, + // WithMetadataRollback reverts the policy metadata and we skip the + // sidecar caches, so policy state can't desync from what is mounted. + if rberr := b.hostState.securityOptions.PolicyEnforcer.WithMetadataRollback(func() error { + if err := b.hostState.securityOptions.PolicyEnforcer.EnforceVerifiedCIMsPolicy(req.ctx, containerID, hashesToVerify, mountedCim, volGUID.String()); err != nil { + return errors.Wrap(err, "CIM mount is denied by policy") + } - if len(layerCIMs) > 1 { - _, err = cimfs.MountMergedVerifiedBlockCIMs(layerCIMs[0], layerCIMs[1:], wcowBlockCimMounts.MountFlags, wcowBlockCimMounts.VolumeGUID, layerDigests[0]) - if err != nil { - return fmt.Errorf("error mounting multilayer block cims: %w", err) + if len(layerCIMs) > 1 { + if _, merr := cimfs.MountMergedVerifiedBlockCIMs(layerCIMs[0], layerCIMs[1:], wcowBlockCimMounts.MountFlags, wcowBlockCimMounts.VolumeGUID, layerDigests[0]); merr != nil { + return fmt.Errorf("error mounting multilayer block cims: %w", merr) + } + } else { + if _, merr := cimfs.MountVerifiedBlockCIM(layerCIMs[0], wcowBlockCimMounts.MountFlags, wcowBlockCimMounts.VolumeGUID, layerDigests[0]); merr != nil { + return fmt.Errorf("error mounting verified block cim: %w", merr) + } } - } else { - _, err = cimfs.MountVerifiedBlockCIM(layerCIMs[0], wcowBlockCimMounts.MountFlags, wcowBlockCimMounts.VolumeGUID, layerDigests[0]) - if err != nil { - return fmt.Errorf("error mounting verified block cim: %w", err) + + // Real mount succeeded: record the verified state. + b.hostState.blockCIMVolumeHashes[volGUID] = layerHashes + if _, ok := b.hostState.blockCIMVolumeContainers[volGUID]; !ok { + b.hostState.blockCIMVolumeContainers[volGUID] = make(map[string]struct{}) } + b.hostState.blockCIMVolumeContainers[volGUID][containerID] = struct{}{} + log.G(ctx).Tracef("Cached %d verified CIM layer hashes for volume %s (container %s)", len(hashesToVerify), volGUID, containerID) + return nil + }); rberr != nil { + return rberr } case guestrequest.RequestTypeRemove: log.G(ctx).Tracef("WCOWBlockCIMMounts: Remove") wcowBlockCimMounts := modifyGuestSettingsRequest.Settings.(*guestresource.CWCOWBlockCIMMounts) - volumePath := fmt.Sprintf(cimfs.VolumePathFormat, wcowBlockCimMounts.VolumeGUID.String()) - err := cimfs.Unmount(volumePath) + volGUID := wcowBlockCimMounts.VolumeGUID - if err != nil { - return fmt.Errorf("error unmounting block cim: %w", err) + // Enforce policy, unmount, then drop the cached state as a single + // transaction: unmount_cims removes the mountedCimVolumes record, + // so if the real unmount fails after the policy check, + // WithMetadataRollback restores that record and we skip the cache + // deletes, keeping policy state in sync with what is mounted. + if rberr := b.hostState.securityOptions.PolicyEnforcer.WithMetadataRollback(func() error { + if err := b.hostState.securityOptions.PolicyEnforcer.EnforceCIMUnmountPolicy(req.ctx, volGUID.String()); err != nil { + return fmt.Errorf("CIM unmount is denied by policy: %w", err) + } + + volumePath := fmt.Sprintf(cimfs.VolumePathFormat, volGUID.String()) + if err := cimfs.Unmount(volumePath); err != nil { + return fmt.Errorf("error unmounting block cim: %w", err) + } + + // Real unmount succeeded: drop the cached mount state. + delete(b.hostState.blockCIMVolumeHashes, volGUID) + delete(b.hostState.blockCIMVolumeContainers, volGUID) + return nil + }); rberr != nil { + return rberr } + default: + return fmt.Errorf("unsupported request type %v for WCOWBlockCims", modifyGuestSettingsRequest.RequestType) } // Send response back to shim resp := &prot.ResponseBase{ @@ -1016,9 +1650,20 @@ func (b *Bridge) modifySettings(req *request) (err error) { return nil case guestresource.ResourceTypeMappedVirtualDiskForContainerScratch: + // It doesn't have an enforcement point within this case block, but it has EnforceScratchMountPolicy + // in ResourceTypeCWCOWCombinedLayers. wcowMappedVirtualDisk := modifyGuestSettingsRequest.Settings.(*guestresource.WCOWMappedVirtualDisk) log.G(ctx).Tracef("ResourceTypeMappedVirtualDiskForContainerScratch: { %v }", wcowMappedVirtualDisk) + // Validate the scratch disk mount path matches the expected pattern + if wcowMappedVirtualDisk.ContainerPath != "" { + matched, merr := regexp.MatchString(`(?i)^[Cc]:\\mounts\\scsi\\m[0-9]+$`, wcowMappedVirtualDisk.ContainerPath) + if merr != nil || !matched { + return fmt.Errorf("scratch disk mount path %q does not match expected pattern c:\\mounts\\scsi\\m", + wcowMappedVirtualDisk.ContainerPath) + } + } + // This will return the volume path of the mounted scratch. // Scratch disk should be >= 30 GB for refs formatter to work. // fsFormatter understands only virtualDevObjectPathFormat. Therefore fetch the @@ -1075,6 +1720,19 @@ func (b *Bridge) modifySettings(req *request) (err error) { log.G(ctx).Tracef("CWCOWCombinedLayers:: ContainerID: %v, ContainerRootPath: %v, Layers: %v, ScratchPath: %v", containerID, settings.CombinedLayers.ContainerRootPath, settings.CombinedLayers.Layers, settings.CombinedLayers.ScratchPath) + // Combined layers are set up once per container. Reject a repeated + // Add for the same container: otherwise a second Add with a + // different root would overwrite containerRootPaths[containerID] + // and leak the previous root's mounted-root entry. + if b.hostState.HasContainerRoot(containerID) { + return fmt.Errorf("combined layers already set up for container %q", containerID) + } + + if matched, merr := regexp.MatchString(`(?i)^[Cc]:\\mounts\\scsi\\m[0-9]+$`, settings.CombinedLayers.ContainerRootPath); merr != nil || !matched { + return fmt.Errorf("combined-layers container root path %q does not match expected pattern c:\\mounts\\scsi\\m", + settings.CombinedLayers.ContainerRootPath) + } + // The layers size is only one, as this is the volume path if len(settings.CombinedLayers.Layers) != 1 { return fmt.Errorf("expected exactly one layer in CWCOWCombinedLayers, got %d", len(settings.CombinedLayers.Layers)) @@ -1088,52 +1746,82 @@ func (b *Bridge) modifySettings(req *request) (err error) { if err != nil { return fmt.Errorf("failed to parse volume GUID %s: %w", guidStr, err) } - hashes, haveHashes := b.hostState.blockCIMVolumeHashes[volGUID] - if haveHashes { - // Only do this if the ContainerID is not already seen for this volume - containers := b.hostState.blockCIMVolumeContainers[volGUID] - if _, seen := containers[containerID]; !seen { - // This is a container with similar layers as an existing container, hence already mounted. - // Call EnforceVerifiedCIMsPolicy on this new container. - hashesToVerify := hashes - mountedCim := []string{hashes[0]} - if len(hashes) > 1 { - hashesToVerify = hashes[1:] - } - if err := b.hostState.securityOptions.PolicyEnforcer.EnforceVerifiedCIMsPolicy(ctx, containerID, hashesToVerify, mountedCim); err != nil { - return fmt.Errorf("CIM mount is denied by policy for this container: %w", err) + + // Enforce policy and set up the scratch as a single transaction: if a + // later step (e.g. mkdir) fails, WithMetadataRollback reverts the + // policy metadata and we skip the sidecar caches, so policy state + // can't desync from reality. + if rberr := b.hostState.securityOptions.PolicyEnforcer.WithMetadataRollback(func() error { + hashes, haveHashes := b.hostState.blockCIMVolumeHashes[volGUID] + markVolumeContainer := false + if haveHashes { + // Only re-verify if this container hasn't been seen for this volume. + containers := b.hostState.blockCIMVolumeContainers[volGUID] + if _, seen := containers[containerID]; !seen { + hashesToVerify := hashes + mountedCim := []string{hashes[0]} + if len(hashes) > 1 { + hashesToVerify = hashes[1:] + } + if err := b.hostState.securityOptions.PolicyEnforcer.EnforceVerifiedCIMsPolicy(ctx, containerID, hashesToVerify, mountedCim, volGUID.String()); err != nil { + return fmt.Errorf("CIM mount is denied by policy for this container: %w", err) + } + log.G(ctx).Tracef("Verified CIM hashes for reused mount volume %s (container %s)", volGUID.String(), containerID) + markVolumeContainer = true } - log.G(ctx).Tracef("Verified CIM hashes for reused mount volume %s (container %s)", volGUID.String(), containerID) - containers[containerID] = struct{}{} } - } - //Since unencrypted scratch is not an option, always pass true - if err := b.hostState.securityOptions.PolicyEnforcer.EnforceScratchMountPolicy(ctx, settings.CombinedLayers.ContainerRootPath, true); err != nil { - return fmt.Errorf("scratch mounting denied by policy: %w", err) - } - // The following two folders are expected to be present in the scratch. - // But since we have just formatted the scratch we would need to - // create them manually. - sandboxStateDirectory := filepath.Join(settings.CombinedLayers.ContainerRootPath, sandboxStateDirName) - err = os.Mkdir(sandboxStateDirectory, 0777) - if err != nil { - return fmt.Errorf("failed to create sandboxStateDirectory: %w", err) - } + if err := b.hostState.securityOptions.PolicyEnforcer.EnforceScratchMountPolicy(ctx, settings.CombinedLayers.ContainerRootPath, true); err != nil { + return fmt.Errorf("scratch mounting denied by policy: %w", err) + } - hivesDirectory := filepath.Join(settings.CombinedLayers.ContainerRootPath, hivesDirName) - err = os.Mkdir(hivesDirectory, 0777) - if err != nil { - return fmt.Errorf("failed to create hivesDirectory: %w", err) + // The following two folders are expected to be present in the + // scratch. Since we just formatted it, create them manually. + sandboxStateDirectory := filepath.Join(settings.CombinedLayers.ContainerRootPath, sandboxStateDirName) + if err := os.Mkdir(sandboxStateDirectory, 0777); err != nil { + return fmt.Errorf("failed to create sandboxStateDirectory: %w", err) + } + hivesDirectory := filepath.Join(settings.CombinedLayers.ContainerRootPath, hivesDirName) + if err := os.Mkdir(hivesDirectory, 0777); err != nil { + return fmt.Errorf("failed to create hivesDirectory: %w", err) + } + + // Everything succeeded: record the sidecar state. containerRootPaths + // lets createContainer cross-check the forwarded Storage.Path, and + // the mounted-root flag lets deleteContainerState refuse deletion + // until the root is unmounted. + if markVolumeContainer { + b.hostState.blockCIMVolumeContainers[volGUID][containerID] = struct{}{} + } + b.hostState.containerRootPaths[containerID] = settings.CombinedLayers.ContainerRootPath + b.hostState.SetContainerRootMounted(settings.CombinedLayers.ContainerRootPath, true) + return nil + }); rberr != nil { + return rberr } case guestrequest.RequestTypeRemove: log.G(ctx).Tracef("CWCOWCombinedLayers: Remove") + // Refuse to unmount the combined-layers root while a running + // container still uses it as its rootfs, so the host can't swap a + // live container's rootfs (cf. LCOW Host.IsOverlayInUse). + if b.hostState.IsContainerRootInUse(settings.CombinedLayers.ContainerRootPath) { + return fmt.Errorf("combined-layers unmount denied: container root %q is in use by a running container", settings.CombinedLayers.ContainerRootPath) + } if err := b.hostState.securityOptions.PolicyEnforcer.EnforceScratchUnmountPolicy(ctx, settings.CombinedLayers.ContainerRootPath); err != nil { return fmt.Errorf("scratch unmounting denied by policy: %w", err) } + b.hostState.SetContainerRootMounted(settings.CombinedLayers.ContainerRootPath, false) + default: + return fmt.Errorf("unsupported request type %v for CWCOWCombinedLayers", modifyGuestSettingsRequest.RequestType) } + // The sidecar enforced policy and staged the scratch here, but the + // actual union mount/unmount happens in the inbox GCS, so watch its + // response and fail closed on failure (the staged policy metadata and + // sidecar caches cannot be reverted). + monitorResponse = true + // Reconstruct WCOWCombinedLayers{} req before forwarding to GCS // as GCS does not understand ResourceTypeCWCOWCombinedLayers modifyGuestSettingsRequest.ResourceType = guestresource.ResourceTypeCombinedLayers @@ -1156,6 +1844,9 @@ func (b *Bridge) modifySettings(req *request) (err error) { } } + if monitorResponse { + b.monitorInboxResponse(req.header.ID) + } b.forwardRequestToGcs(req) return nil } diff --git a/internal/gcs-sidecar/handlers_test.go b/internal/gcs-sidecar/handlers_test.go index b30f9b5b5a..89f9f89e11 100644 --- a/internal/gcs-sidecar/handlers_test.go +++ b/internal/gcs-sidecar/handlers_test.go @@ -7,9 +7,11 @@ import ( "context" "encoding/base64" "encoding/json" + "errors" "io" "os" "path/filepath" + "reflect" "strings" "testing" "time" @@ -23,6 +25,7 @@ import ( "github.com/Microsoft/hcsshim/internal/protocol/guestresource" "github.com/Microsoft/hcsshim/internal/vm/vmutils/etw" "github.com/Microsoft/hcsshim/pkg/securitypolicy" + oci "github.com/opencontainers/runtime-spec/specs-go" "github.com/sirupsen/logrus" ) @@ -96,6 +99,7 @@ func newTestBridge(enforcer securitypolicy.SecurityPolicyEnforcer) *Bridge { host := NewHost(enforcer, io.Discard) return &Bridge{ pending: make(map[sequenceID]chan *prot.ContainerExecuteProcessResponse), + monitoredIDs: make(map[sequenceID]struct{}), rpcHandlerList: make(map[prot.RPCProc]HandlerFunc), hostState: host, sendToGCSCh: make(chan request, 10), @@ -138,6 +142,132 @@ func TestExecuteProcess_ApplicationNameDenied(t *testing.T) { } } +// TestResponseFailure verifies responseFailure classifies inbox GCS responses: +// a zero Result is success, a non-zero Result is a failure, and an unparseable +// message is treated as success so a malformed message cannot by itself fail +// the UVM closed. +func TestResponseFailure(t *testing.T) { + mustMarshal := func(v interface{}) []byte { + b, err := json.Marshal(v) + if err != nil { + t.Fatalf("marshal: %v", err) + } + return b + } + + tests := []struct { + name string + message []byte + wantErr bool + }{ + {name: "success", message: mustMarshal(prot.ResponseBase{Result: 0}), wantErr: false}, + {name: "failure with message", message: mustMarshal(prot.ResponseBase{Result: 1, ErrorMessage: "boom"}), wantErr: true}, + {name: "failure without message", message: mustMarshal(prot.ResponseBase{Result: 1}), wantErr: true}, + {name: "unparseable", message: []byte("not json"), wantErr: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := responseFailure(tt.message) + if (err != nil) != tt.wantErr { + t.Fatalf("responseFailure() err = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + +// TestCheckState_BlocksHandlers verifies that once the UVM is marked +// inconsistent, container creation/deletion and settings changes are refused +// (fail-closed), matching the LCOW behavior. +func TestCheckState_BlocksHandlers(t *testing.T) { + b := newTestBridge(&securitypolicy.OpenDoorSecurityPolicyEnforcer{}) + + // Before failing closed, checkState is clear. + if err := b.hostState.checkState(); err != nil { + t.Fatalf("checkState should be nil before setUVMInconsistent, got %v", err) + } + + b.hostState.setUVMInconsistent(errors.New("inbox mount failed")) + + if err := b.hostState.checkState(); err == nil { + t.Fatal("checkState should be non-nil after setUVMInconsistent") + } + + // createContainer refuses before it even parses the request (gate is at the top). + createReq := &request{ + ctx: context.Background(), + header: messageHeader{Type: prot.MsgTypeRequest | prot.MsgType(prot.RPCCreate), ID: 1}, + } + if err := b.createContainer(createReq); err == nil { + t.Error("createContainer should be denied when UVM is inconsistent") + } + + // deleteContainerState refuses similarly. + deleteReq := &request{ + ctx: context.Background(), + header: messageHeader{Type: prot.MsgTypeRequest | prot.MsgType(prot.RPCDeleteContainerState), ID: 2}, + } + if err := b.deleteContainerState(deleteReq); err == nil { + t.Error("deleteContainerState should be denied when UVM is inconsistent") + } + + // modifySettings refuses too (checkState runs after unmarshalling a valid request). + msg := buildModifySettingsRequest(t, + guestresource.ResourceTypeSecurityPolicy, + guestrequest.RequestTypeAdd, + guestresource.ConfidentialOptions{EnforcerType: "rego"}, + ) + modifyReq := &request{ + ctx: context.Background(), + header: messageHeader{Type: prot.MsgTypeRequest | prot.MsgType(prot.RPCModifySettings), Size: uint32(len(msg)) + prot.HdrSize, ID: 3}, + message: msg, + } + if err := b.modifySettings(modifyReq); err == nil { + t.Error("modifySettings should be denied when UVM is inconsistent") + } +} + +// TestModifySettings_MappedDirectory_TagsInboxResponse verifies that a forwarded +// mapped-directory operation registers its request ID for inbox-response +// monitoring and is forwarded to the inbox GCS, so a later failure response can +// fail the UVM closed. +func TestModifySettings_MappedDirectory_TagsInboxResponse(t *testing.T) { + b := newTestBridge(&securitypolicy.OpenDoorSecurityPolicyEnforcer{}) + + msg := buildModifySettingsRequest(t, + guestresource.ResourceTypeMappedDirectory, + guestrequest.RequestTypeAdd, + hcsschema.MappedDirectory{ContainerPath: `C:\mnt\ro`, ReadOnly: true}, + ) + const id sequenceID = 77 + req := &request{ + ctx: context.Background(), + header: messageHeader{Type: prot.MsgTypeRequest | prot.MsgType(prot.RPCModifySettings), Size: uint32(len(msg)) + prot.HdrSize, ID: id}, + message: msg, + } + + if err := b.modifySettings(req); err != nil { + t.Fatalf("modifySettings returned error: %v", err) + } + + // The request ID must be registered for monitoring. + b.monitoredMu.Lock() + _, monitored := b.monitoredIDs[id] + b.monitoredMu.Unlock() + if !monitored { + t.Errorf("mapped-directory request ID %d was not registered for inbox-response monitoring", id) + } + + // And the request must have been forwarded to the inbox GCS. + select { + case got := <-b.sendToGCSCh: + if got.header.ID != id { + t.Errorf("forwarded request ID = %d, want %d", got.header.ID, id) + } + default: + t.Error("mapped-directory request was not forwarded to inbox GCS") + } +} + // TestModifySettings_PolicyFragment_InvalidFragment tests that a PolicyFragment // request with an invalid (non-base64, non-COSE) fragment value returns an error // from the handler. The bridge's main loop converts handler errors into error @@ -411,6 +541,782 @@ func TestModifySettings_PolicyFragment_TypeAssertionFailure(t *testing.T) { } } +// Tests for environment variable filtering helpers (envlist persistence) + +func TestOciEnvToProcessParamEnv_Basic(t *testing.T) { + input := []string{"FOO=bar", `PATH=C:\Windows\System32`, "EMPTY="} + result := ociEnvToProcessParamEnv(input) + + if result["FOO"] != "bar" { + t.Errorf("FOO = %q, want %q", result["FOO"], "bar") + } + if result["PATH"] != `C:\Windows\System32` { + t.Errorf("PATH = %q, want %q", result["PATH"], `C:\Windows\System32`) + } + if result["EMPTY"] != "" { + t.Errorf("EMPTY = %q, want %q", result["EMPTY"], "") + } + if len(result) != 3 { + t.Errorf("len = %d, want 3", len(result)) + } +} + +func TestOciEnvToProcessParamEnv_ValueWithEquals(t *testing.T) { + input := []string{"CONN=host=db;port=5432"} + result := ociEnvToProcessParamEnv(input) + + if result["CONN"] != "host=db;port=5432" { + t.Errorf("CONN = %q, want %q", result["CONN"], "host=db;port=5432") + } +} + +func TestOciEnvToProcessParamEnv_MalformedSkipped(t *testing.T) { + input := []string{"GOOD=value", "NOEQUALS", "ALSO_GOOD=yes"} + result := ociEnvToProcessParamEnv(input) + + if len(result) != 2 { + t.Errorf("len = %d, want 2 (malformed entry should be skipped)", len(result)) + } + if result["GOOD"] != "value" { + t.Errorf("GOOD = %q, want %q", result["GOOD"], "value") + } + if result["ALSO_GOOD"] != "yes" { + t.Errorf("ALSO_GOOD = %q, want %q", result["ALSO_GOOD"], "yes") + } +} + +func TestOciEnvToProcessParamEnv_Empty(t *testing.T) { + result := ociEnvToProcessParamEnv([]string{}) + if len(result) != 0 { + t.Errorf("len = %d, want 0", len(result)) + } +} + +func TestOciEnvToProcessParamEnv_Nil(t *testing.T) { + result := ociEnvToProcessParamEnv(nil) + if result == nil { + t.Error("result should be non-nil empty map, got nil") + } + if len(result) != 0 { + t.Errorf("len = %d, want 0", len(result)) + } +} + +func TestProcessParamEnvToOCIEnv_Roundtrip(t *testing.T) { + original := map[string]string{ + "FOO": "bar", + "PATH": `C:\Windows\System32`, + } + + ociEnv := processParamEnvToOCIEnv(original) + roundtripped := ociEnvToProcessParamEnv(ociEnv) + + if len(roundtripped) != len(original) { + t.Fatalf("roundtrip len = %d, want %d", len(roundtripped), len(original)) + } + for k, v := range original { + if roundtripped[k] != v { + t.Errorf("roundtrip[%q] = %q, want %q", k, roundtripped[k], v) + } + } +} + +// envFilterEnforcer wraps OpenDoorSecurityPolicyEnforcer and overrides the +// external-exec env-filtering hook to return a caller-specified subset. +// Embedding OpenDoor satisfies the rest of the SecurityPolicyEnforcer +// interface (all return-allow / no-op behaviour), so a single overridden +// method is enough to drive the env-filter code path in executeProcess. +type envFilterEnforcer struct { + securitypolicy.OpenDoorSecurityPolicyEnforcer + keep []string +} + +func (e *envFilterEnforcer) EnforceExecExternalProcessPolicy( + _ context.Context, _ []string, _ []string, _ string, +) (securitypolicy.EnvList, bool, error) { + return securitypolicy.EnvList(e.keep), true, nil +} + +// TestExecuteProcess_External_AppliesFilteredEnv exercises the env-filter +// rewrite path of the external-exec (UVMContainerID) branch of +// executeProcess. The fake enforcer returns a strict subset of the input +// env; the test asserts the request forwarded to GCS carries exactly that +// subset in ProcessParameters.Environment. +func TestExecuteProcess_External_AppliesFilteredEnv(t *testing.T) { + enf := &envFilterEnforcer{ + keep: []string{`PATH=C:\Windows\System32`, "KEEP=1"}, + } + b := newTestBridge(enf) + + params := hcsschema.ProcessParameters{ + CommandLine: "cmd.exe /c exit", + Environment: map[string]string{ + "PATH": `C:\Windows\System32`, + "KEEP": "1", + "DROP": "secret", + }, + } + r := prot.ContainerExecuteProcess{ + RequestBase: prot.RequestBase{ + ContainerID: UVMContainerID, + ActivityID: guid.GUID{}, + }, + Settings: prot.ExecuteProcessSettings{ + ProcessParameters: prot.AnyInString{Value: ¶ms}, + }, + } + msg, err := json.Marshal(&r) + if err != nil { + t.Fatalf("marshal request: %v", err) + } + + req := &request{ + ctx: context.Background(), + header: messageHeader{ + Type: prot.MsgTypeRequest | prot.MsgType(prot.RPCExecuteProcess), + Size: uint32(len(msg)) + prot.HdrSize, + ID: 1, + }, + message: msg, + } + + if err := b.executeProcess(req); err != nil { + t.Fatalf("executeProcess: %v", err) + } + + var got request + select { + case got = <-b.sendToGCSCh: + case <-time.After(time.Second): + t.Fatal("nothing forwarded to GCS") + } + + // Unwrap the re-marshalled request and pull the inner ProcessParameters + // JSON back out via the same *json.RawMessage trick that the handler + // uses, then decode it as hcsschema.ProcessParameters. + var outer prot.ContainerExecuteProcess + var paramsRaw json.RawMessage + outer.Settings.ProcessParameters.Value = ¶msRaw + if err := json.Unmarshal(got.message, &outer); err != nil { + t.Fatalf("unmarshal forwarded outer: %v", err) + } + var gotParams hcsschema.ProcessParameters + if err := json.Unmarshal(paramsRaw, &gotParams); err != nil { + t.Fatalf("unmarshal forwarded ProcessParameters: %v", err) + } + + want := map[string]string{ + "PATH": `C:\Windows\System32`, + "KEEP": "1", + } + if !reflect.DeepEqual(gotParams.Environment, want) { + t.Errorf("forwarded Environment = %v, want %v", gotParams.Environment, want) + } +} + +// addInitContainer registers a container in the "init process not yet exec'd" +// state (commandLine=true, commandLineExec=false) with the given enforced +// process spec, so executeProcess takes the create-exec cross-check branch. +func addInitContainer(t *testing.T, b *Bridge, id string, proc *oci.Process) { + t.Helper() + c := &Container{ + id: id, + spec: oci.Spec{Process: proc}, + processes: make(map[uint32]*containerProcess), + commandLine: true, + commandLineExec: false, + } + if err := b.hostState.AddContainer(context.Background(), id, c); err != nil { + t.Fatalf("AddContainer: %v", err) + } +} + +// buildExecRequest serializes an executeProcess request for the given container +// and process parameters. +func buildExecRequest(t *testing.T, containerID string, params hcsschema.ProcessParameters) *request { + t.Helper() + r := prot.ContainerExecuteProcess{ + RequestBase: prot.RequestBase{ + ContainerID: containerID, + ActivityID: guid.GUID{}, + }, + Settings: prot.ExecuteProcessSettings{ + ProcessParameters: prot.AnyInString{Value: ¶ms}, + }, + } + msg, err := json.Marshal(&r) + if err != nil { + t.Fatalf("marshal request: %v", err) + } + return &request{ + ctx: context.Background(), + header: messageHeader{ + Type: prot.MsgTypeRequest | prot.MsgType(prot.RPCExecuteProcess), + Size: uint32(len(msg)) + prot.HdrSize, + ID: 7, + }, + message: msg, + } +} + +// unwrapExecParams pulls the inner ProcessParameters back out of a forwarded +// executeProcess request message. +func unwrapExecParams(t *testing.T, message []byte) hcsschema.ProcessParameters { + t.Helper() + var outer prot.ContainerExecuteProcess + var paramsRaw json.RawMessage + outer.Settings.ProcessParameters.Value = ¶msRaw + if err := json.Unmarshal(message, &outer); err != nil { + t.Fatalf("unmarshal forwarded outer: %v", err) + } + var params hcsschema.ProcessParameters + if err := json.Unmarshal(paramsRaw, ¶ms); err != nil { + t.Fatalf("unmarshal forwarded ProcessParameters: %v", err) + } + return params +} + +func assertNothingForwarded(t *testing.T, b *Bridge) { + t.Helper() + select { + case got := <-b.sendToGCSCh: + t.Fatalf("unexpected request forwarded to GCS: %+v", got) + default: + } +} + +// enforcedInitProcess is the process spec used by the create-exec tests: the +// command line, working directory, user and environment that createContainer +// enforcement would have produced. +func enforcedInitProcess() *oci.Process { + return &oci.Process{ + Args: []string{"python", "hello.py"}, + Cwd: `C:\app`, + User: oci.User{Username: "ContainerUser"}, + Env: []string{"APP_FOO=BAR"}, + } +} + +// TestExecuteProcess_InitExec_DeniesCommandLineMismatch verifies that an init +// exec whose command line does not match the enforced spec (the +// "cmd.exe /c " tamper) is denied before anything is forwarded to GCS. +func TestExecuteProcess_InitExec_DeniesCommandLineMismatch(t *testing.T) { + b := newTestBridge(&securitypolicy.OpenDoorSecurityPolicyEnforcer{}) + const cid = "container-1" + addInitContainer(t, b, cid, enforcedInitProcess()) + + req := buildExecRequest(t, cid, hcsschema.ProcessParameters{ + CommandLine: "cmd.exe /c whoami", + WorkingDirectory: `C:\app`, + User: "ContainerUser", + }) + + err := b.executeProcess(req) + if err == nil || !strings.Contains(err.Error(), "command line") { + t.Fatalf("expected command-line denial, got %v", err) + } + assertNothingForwarded(t, b) +} + +// TestExecuteProcess_InitExec_DeniesWorkingDirMismatch verifies a tampered +// working directory is denied. +func TestExecuteProcess_InitExec_DeniesWorkingDirMismatch(t *testing.T) { + b := newTestBridge(&securitypolicy.OpenDoorSecurityPolicyEnforcer{}) + const cid = "container-1" + addInitContainer(t, b, cid, enforcedInitProcess()) + + req := buildExecRequest(t, cid, hcsschema.ProcessParameters{ + CommandLine: "python hello.py", + WorkingDirectory: `C:\Windows`, + User: "ContainerUser", + }) + + err := b.executeProcess(req) + if err == nil || !strings.Contains(err.Error(), "working directory") { + t.Fatalf("expected working-directory denial, got %v", err) + } + assertNothingForwarded(t, b) +} + +// TestExecuteProcess_InitExec_DeniesUserMismatch verifies a tampered user is +// denied. +func TestExecuteProcess_InitExec_DeniesUserMismatch(t *testing.T) { + b := newTestBridge(&securitypolicy.OpenDoorSecurityPolicyEnforcer{}) + const cid = "container-1" + addInitContainer(t, b, cid, enforcedInitProcess()) + + req := buildExecRequest(t, cid, hcsschema.ProcessParameters{ + CommandLine: "python hello.py", + WorkingDirectory: `C:\app`, + User: "ContainerAdministrator", + }) + + err := b.executeProcess(req) + if err == nil || !strings.Contains(err.Error(), "user") { + t.Fatalf("expected user denial, got %v", err) + } + assertNothingForwarded(t, b) +} + +// TestExecuteProcess_InitExec_AllowsAndAppliesEnv verifies that an init exec +// matching the enforced command line/cwd/user is allowed, and that the +// environment forwarded to GCS is reduced to exactly the enforced set (extra +// host-supplied variables are dropped). +func TestExecuteProcess_InitExec_AllowsAndAppliesEnv(t *testing.T) { + b := newTestBridge(&securitypolicy.OpenDoorSecurityPolicyEnforcer{}) + const cid = "container-1" + addInitContainer(t, b, cid, enforcedInitProcess()) + + req := buildExecRequest(t, cid, hcsschema.ProcessParameters{ + CommandLine: "python hello.py", + WorkingDirectory: `C:\app`, + User: "ContainerUser", + Environment: map[string]string{ + "APP_FOO": "BAR", + "DROP": "secret", + }, + }) + + // The container path forwards to GCS and then blocks waiting for the exec + // response keyed by header ID, so run the handler in a goroutine and feed + // it a response once we've captured the forwarded request. + done := make(chan error, 1) + go func() { done <- b.executeProcess(req) }() + + var got request + select { + case got = <-b.sendToGCSCh: + case <-time.After(time.Second): + t.Fatal("nothing forwarded to GCS") + } + + // Stand in for GCS: deliver an exec response on the channel the handler + // registered under this request's header ID, which unblocks its select. + b.pendingMu.Lock() + ch := b.pending[got.header.ID] + b.pendingMu.Unlock() + if ch == nil { + t.Fatal("no pending response channel registered for forwarded request") + } + ch <- &prot.ContainerExecuteProcessResponse{ProcessID: 42} + + if err := <-done; err != nil { + t.Fatalf("executeProcess: %v", err) + } + + gotParams := unwrapExecParams(t, got.message) + want := map[string]string{"APP_FOO": "BAR"} + if !reflect.DeepEqual(gotParams.Environment, want) { + t.Errorf("forwarded Environment = %v, want %v", gotParams.Environment, want) + } +} + +// TestEnforceStdioParams covers the stdio-access decision helper: allowed +// leaves params untouched, denied clears the stdio pipe flags, denied with no +// pipes reports no change, and denied for a console process is rejected. +func TestEnforceStdioParams(t *testing.T) { + tests := []struct { + name string + allowStdio bool + params hcsschema.ProcessParameters + wantErr bool + wantChanged bool + wantPipes bool + }{ + { + name: "allowed leaves params untouched", + allowStdio: true, + params: hcsschema.ProcessParameters{CreateStdInPipe: true, CreateStdOutPipe: true, CreateStdErrPipe: true}, + wantChanged: false, + wantPipes: true, + }, + { + name: "denied with console is rejected", + allowStdio: false, + params: hcsschema.ProcessParameters{EmulateConsole: true}, + wantErr: true, + }, + { + name: "denied clears stdio pipes", + allowStdio: false, + params: hcsschema.ProcessParameters{CreateStdInPipe: true, CreateStdOutPipe: true, CreateStdErrPipe: true}, + wantChanged: true, + wantPipes: false, + }, + { + name: "denied with no pipes reports no change", + allowStdio: false, + params: hcsschema.ProcessParameters{}, + wantChanged: false, + wantPipes: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + params := tt.params + changed, err := enforceStdioParams(tt.allowStdio, ¶ms) + if tt.wantErr { + if err == nil { + t.Fatal("expected error, got nil") + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if changed != tt.wantChanged { + t.Errorf("changed = %v, want %v", changed, tt.wantChanged) + } + if params.CreateStdInPipe != tt.wantPipes || + params.CreateStdOutPipe != tt.wantPipes || + params.CreateStdErrPipe != tt.wantPipes { + t.Errorf("pipe flags = (%v,%v,%v), want all %v", + params.CreateStdInPipe, params.CreateStdOutPipe, params.CreateStdErrPipe, tt.wantPipes) + } + }) + } +} + +// stdioDenyExternalEnforcer denies stdio access on the external-exec path while +// allowing everything else via the embedded open-door enforcer. +type stdioDenyExternalEnforcer struct { + securitypolicy.OpenDoorSecurityPolicyEnforcer +} + +func (stdioDenyExternalEnforcer) EnforceExecExternalProcessPolicy( + _ context.Context, _ []string, _ []string, _ string, +) (securitypolicy.EnvList, bool, error) { + return nil, false, nil +} + +// TestExecuteProcess_External_DeniedStdioClearsPipes verifies the external-exec +// branch clears the stdio pipe flags before forwarding when policy denies stdio. +func TestExecuteProcess_External_DeniedStdioClearsPipes(t *testing.T) { + b := newTestBridge(&stdioDenyExternalEnforcer{}) + + params := hcsschema.ProcessParameters{ + CommandLine: "cmd.exe /c exit", + CreateStdInPipe: true, + CreateStdOutPipe: true, + CreateStdErrPipe: true, + } + r := prot.ContainerExecuteProcess{ + RequestBase: prot.RequestBase{ContainerID: UVMContainerID, ActivityID: guid.GUID{}}, + Settings: prot.ExecuteProcessSettings{ProcessParameters: prot.AnyInString{Value: ¶ms}}, + } + msg, err := json.Marshal(&r) + if err != nil { + t.Fatalf("marshal request: %v", err) + } + req := &request{ + ctx: context.Background(), + header: messageHeader{ + Type: prot.MsgTypeRequest | prot.MsgType(prot.RPCExecuteProcess), + Size: uint32(len(msg)) + prot.HdrSize, + ID: 1, + }, + message: msg, + } + + if err := b.executeProcess(req); err != nil { + t.Fatalf("executeProcess: %v", err) + } + + var got request + select { + case got = <-b.sendToGCSCh: + case <-time.After(time.Second): + t.Fatal("nothing forwarded to GCS") + } + + gotParams := unwrapExecParams(t, got.message) + if gotParams.CreateStdInPipe || gotParams.CreateStdOutPipe || gotParams.CreateStdErrPipe { + t.Errorf("stdio pipes not cleared: %+v", gotParams) + } +} + +// TestExecuteProcess_External_DeniedStdioWithConsoleRejected verifies that a +// console-requesting external process is rejected (not forwarded) when policy +// denies stdio. +func TestExecuteProcess_External_DeniedStdioWithConsoleRejected(t *testing.T) { + b := newTestBridge(&stdioDenyExternalEnforcer{}) + + params := hcsschema.ProcessParameters{CommandLine: "cmd.exe", EmulateConsole: true} + r := prot.ContainerExecuteProcess{ + RequestBase: prot.RequestBase{ContainerID: UVMContainerID, ActivityID: guid.GUID{}}, + Settings: prot.ExecuteProcessSettings{ProcessParameters: prot.AnyInString{Value: ¶ms}}, + } + msg, err := json.Marshal(&r) + if err != nil { + t.Fatalf("marshal request: %v", err) + } + req := &request{ + ctx: context.Background(), + header: messageHeader{ + Type: prot.MsgTypeRequest | prot.MsgType(prot.RPCExecuteProcess), + Size: uint32(len(msg)) + prot.HdrSize, + ID: 1, + }, + message: msg, + } + + err = b.executeProcess(req) + if err == nil || !strings.Contains(err.Error(), "console") { + t.Fatalf("expected console denial, got %v", err) + } + assertNothingForwarded(t, b) +} + +// TestExecuteProcess_InitExec_DeniedStdioClearsPipes verifies the init-process +// branch applies the create-time stdio decision (c.allowStdio=false) by +// clearing the stdio pipe flags before forwarding. +func TestExecuteProcess_InitExec_DeniedStdioClearsPipes(t *testing.T) { + b := newTestBridge(&securitypolicy.OpenDoorSecurityPolicyEnforcer{}) + const cid = "container-1" + c := &Container{ + id: cid, + spec: oci.Spec{Process: enforcedInitProcess()}, + processes: make(map[uint32]*containerProcess), + commandLine: true, + commandLineExec: false, + allowStdio: false, + } + if err := b.hostState.AddContainer(context.Background(), cid, c); err != nil { + t.Fatalf("AddContainer: %v", err) + } + + req := buildExecRequest(t, cid, hcsschema.ProcessParameters{ + CommandLine: "python hello.py", + WorkingDirectory: `C:\app`, + User: "ContainerUser", + CreateStdInPipe: true, + CreateStdOutPipe: true, + CreateStdErrPipe: true, + }) + + done := make(chan error, 1) + go func() { done <- b.executeProcess(req) }() + + var got request + select { + case got = <-b.sendToGCSCh: + case <-time.After(time.Second): + t.Fatal("nothing forwarded to GCS") + } + + b.pendingMu.Lock() + ch := b.pending[got.header.ID] + b.pendingMu.Unlock() + if ch == nil { + t.Fatal("no pending response channel registered for forwarded request") + } + ch <- &prot.ContainerExecuteProcessResponse{ProcessID: 42} + + if err := <-done; err != nil { + t.Fatalf("executeProcess: %v", err) + } + + gotParams := unwrapExecParams(t, got.message) + if gotParams.CreateStdInPipe || gotParams.CreateStdOutPipe || gotParams.CreateStdErrPipe { + t.Errorf("stdio pipes not cleared: %+v", gotParams) + } +} + +// TestExecuteProcess_InitExec_AllowsStdioKeepsPipes verifies the init-process +// branch leaves the stdio pipe flags intact when the create-time decision +// allows stdio (c.allowStdio=true). +func TestExecuteProcess_InitExec_AllowsStdioKeepsPipes(t *testing.T) { + b := newTestBridge(&securitypolicy.OpenDoorSecurityPolicyEnforcer{}) + const cid = "container-1" + c := &Container{ + id: cid, + spec: oci.Spec{Process: enforcedInitProcess()}, + processes: make(map[uint32]*containerProcess), + commandLine: true, + commandLineExec: false, + allowStdio: true, + } + if err := b.hostState.AddContainer(context.Background(), cid, c); err != nil { + t.Fatalf("AddContainer: %v", err) + } + + req := buildExecRequest(t, cid, hcsschema.ProcessParameters{ + CommandLine: "python hello.py", + WorkingDirectory: `C:\app`, + User: "ContainerUser", + CreateStdInPipe: true, + CreateStdOutPipe: true, + CreateStdErrPipe: true, + }) + + done := make(chan error, 1) + go func() { done <- b.executeProcess(req) }() + + var got request + select { + case got = <-b.sendToGCSCh: + case <-time.After(time.Second): + t.Fatal("nothing forwarded to GCS") + } + + b.pendingMu.Lock() + ch := b.pending[got.header.ID] + b.pendingMu.Unlock() + if ch == nil { + t.Fatal("no pending response channel registered for forwarded request") + } + ch <- &prot.ContainerExecuteProcessResponse{ProcessID: 42} + + if err := <-done; err != nil { + t.Fatalf("executeProcess: %v", err) + } + + gotParams := unwrapExecParams(t, got.message) + if !gotParams.CreateStdInPipe || !gotParams.CreateStdOutPipe || !gotParams.CreateStdErrPipe { + t.Errorf("stdio pipes should be preserved when allowed: %+v", gotParams) + } +} + +// TestIsContainerRootInUse verifies that a container's combined-layers root is +// treated as in-use only while the container is running (not terminated), and +// only for the matching root path (case-insensitive). +func TestIsContainerRootInUse(t *testing.T) { + b := newTestBridge(&securitypolicy.OpenDoorSecurityPolicyEnforcer{}) + host := b.hostState + + const cid = "container-1" + const rootPath = `C:\mounts\scsi\m0` + + c := &Container{id: cid, processes: make(map[uint32]*containerProcess)} + if err := host.AddContainer(context.Background(), cid, c); err != nil { + t.Fatalf("AddContainer: %v", err) + } + host.containerRootPaths[cid] = rootPath + + // Running container: its root is in use. + if !host.IsContainerRootInUse(rootPath) { + t.Errorf("expected root %q to be in use for a running container", rootPath) + } + // Paths compare with EqualFold, so a differently-cased path still matches. + if !host.IsContainerRootInUse(`c:\mounts\scsi\m0`) { + t.Errorf("expected case-insensitive match for %q", rootPath) + } + // An unrelated path is not in use. + if host.IsContainerRootInUse(`C:\mounts\scsi\m1`) { + t.Errorf("did not expect unrelated path to be in use") + } + + // Once the container has exited, its root is no longer in use. + c.terminated.Store(true) + if host.IsContainerRootInUse(rootPath) { + t.Errorf("expected root %q to be free after container terminated", rootPath) + } +} + +// TestModifySettings_CombinedLayers_RejectsDuplicateAdd verifies that a second +// CWCOWCombinedLayers Add for a container that already has combined layers set +// up is rejected, so a repeated Add can't overwrite the recorded root path or +// leak the previous root's mounted-root entry. +func TestModifySettings_CombinedLayers_RejectsDuplicateAdd(t *testing.T) { + b := newTestBridge(&securitypolicy.OpenDoorSecurityPolicyEnforcer{}) + + const cid = "container-1" + const rootPath = `C:\mounts\scsi\m0` + + // Pretend combined layers were already set up for this container. + b.hostState.containerRootPaths[cid] = rootPath + b.hostState.SetContainerRootMounted(rootPath, true) + + msg := buildModifySettingsRequest(t, + guestresource.ResourceTypeCWCOWCombinedLayers, + guestrequest.RequestTypeAdd, + guestresource.CWCOWCombinedLayers{ + ContainerID: cid, + CombinedLayers: guestresource.WCOWCombinedLayers{ + ContainerRootPath: `C:\mounts\scsi\m1`, + Layers: []hcsschema.Layer{{Path: rootPath}}, + }, + }, + ) + req := &request{ + ctx: context.Background(), + header: messageHeader{Type: prot.MsgTypeRequest | prot.MsgType(prot.RPCModifySettings), Size: uint32(len(msg)) + prot.HdrSize, ID: 1}, + message: msg, + } + + err := b.modifySettings(req) + if err == nil || !strings.Contains(err.Error(), "already set up") { + t.Fatalf("expected duplicate-add denial, got %v", err) + } + + // The recorded root path must be unchanged and nothing forwarded to GCS. + if got := b.hostState.containerRootPaths[cid]; got != rootPath { + t.Errorf("containerRootPaths[%q] = %q, want %q (unchanged)", cid, got, rootPath) + } + select { + case <-b.sendToGCSCh: + t.Error("duplicate CombinedLayers Add must not be forwarded to inbox GCS") + default: + } +} + +// TestDeleteContainerState_DeniesRunningOrMounted verifies deleteContainerState +// refuses to delete the state of a container that is still running or whose +// combined-layers root is still mounted, and allows it once terminated and +// unmounted. +func TestDeleteContainerState_DeniesRunningOrMounted(t *testing.T) { + const cid = "container-1" + const rootPath = `C:\mounts\scsi\m0` + + newReq := func() *request { + msg, err := json.Marshal(prot.DeleteContainerStateRequest{ + RequestBase: prot.RequestBase{ContainerID: cid}, + }) + if err != nil { + t.Fatalf("marshal: %v", err) + } + return &request{ + ctx: context.Background(), + header: messageHeader{ + Type: prot.MsgTypeRequest | prot.MsgType(prot.RPCDeleteContainerState), + Size: uint32(len(msg)) + prot.HdrSize, + ID: 1, + }, + message: msg, + } + } + + b := newTestBridge(&securitypolicy.OpenDoorSecurityPolicyEnforcer{}) + c := &Container{id: cid, processes: make(map[uint32]*containerProcess)} + if err := b.hostState.AddContainer(context.Background(), cid, c); err != nil { + t.Fatalf("AddContainer: %v", err) + } + b.hostState.containerRootPaths[cid] = rootPath + b.hostState.SetContainerRootMounted(rootPath, true) + + // Still running -> denied. + if err := b.deleteContainerState(newReq()); err == nil || !strings.Contains(err.Error(), "still running") { + t.Fatalf("expected running denial, got %v", err) + } + + // Terminated but root still mounted -> denied. + c.terminated.Store(true) + if err := b.deleteContainerState(newReq()); err == nil || !strings.Contains(err.Error(), "still mounted") { + t.Fatalf("expected mounted denial, got %v", err) + } + + // Terminated and unmounted -> allowed and forwarded to GCS. + b.hostState.SetContainerRootMounted(rootPath, false) + if err := b.deleteContainerState(newReq()); err != nil { + t.Fatalf("expected allow, got %v", err) + } + select { + case <-b.sendToGCSCh: + case <-time.After(time.Second): + t.Fatal("expected request forwarded to GCS") + } +} + // buildLogForwardServiceRequest builds a serialized ServiceModificationRequest // for the LogForwardService with the given provider names baked into a // base64-encoded LogSourcesInfo payload. diff --git a/internal/gcs-sidecar/host.go b/internal/gcs-sidecar/host.go index ebe4f5687e..90d573d632 100644 --- a/internal/gcs-sidecar/host.go +++ b/internal/gcs-sidecar/host.go @@ -5,8 +5,11 @@ package bridge import ( "context" + "fmt" "io" + "strings" "sync" + "sync/atomic" "github.com/Microsoft/go-winio/pkg/guid" "github.com/Microsoft/hcsshim/internal/bridgeutils/gcserr" @@ -27,6 +30,55 @@ type Host struct { blockCIMVolumeHashes map[guid.GUID][]string // mapping of volumeGUID to container IDs blockCIMVolumeContainers map[guid.GUID]map[string]struct{} + // mapping of containerID to the ContainerRootPath recorded when + // CWCOWCombinedLayers mounted it, used to validate the createContainer + // Storage.Path. + containerRootPaths map[string]string + // mountedRoots holds the combined-layers container roots that are currently + // mounted (set on CWCOWCombinedLayers Add, cleared on Remove), keyed by the + // lower-cased root path. Used to refuse deleting a container whose root is + // still mounted. + mountedRoots map[string]struct{} + + // uvmError is set when the UVM has entered an inconsistent state from which + // the sidecar cannot safely recover. Once set, checkState makes all further + // container creation/deletion and mount/unmount operations fail (cf. LCOW + // hcsv2 Host.uvmError in internal/guest/runtime/hcsv2/uvm.go). See the + // setUVMInconsistent call sites for the conditions that trigger it. + uvmError uvmConsistencyError +} + +// uvmConsistencyError records that the UVM has entered an inconsistent state +// from which the sidecar cannot safely recover, so it must fail closed. See the +// setUVMInconsistent call sites for what can cause this. +type uvmConsistencyError struct { + mu sync.Mutex + // cause is the error describing why the UVM entered an inconsistent state. + // If nil, Check returns nil. + cause error +} + +// Set records the cause of the inconsistency, keeping the first cause if one is +// already set. +func (u *uvmConsistencyError) Set(cause error) { + u.mu.Lock() + defer u.mu.Unlock() + if u.cause == nil { + u.cause = cause + } +} + +// Check returns a non-nil error if the UVM has been marked inconsistent. +func (u *uvmConsistencyError) Check() error { + u.mu.Lock() + defer u.mu.Unlock() + if u.cause == nil { + return nil + } + return fmt.Errorf( + "mount, unmount, container creation and deletion have been disabled in this UVM due to a previous error: %w", + u.cause, + ) } type Container struct { @@ -36,6 +88,11 @@ type Container struct { processes map[uint32]*containerProcess commandLine bool commandLineExec bool + // allowStdio is the create-time stdio-access policy decision. + allowStdio bool + // terminated is set once the container's init process has exited (via the + // guest container-exit notification). + terminated atomic.Bool } // Process is a struct that defines the lifetime and operations associated with @@ -59,10 +116,28 @@ func NewHost(initialEnforcer securitypolicy.SecurityPolicyEnforcer, logWriter io containers: make(map[string]*Container), blockCIMVolumeHashes: make(map[guid.GUID][]string), blockCIMVolumeContainers: make(map[guid.GUID]map[string]struct{}), + containerRootPaths: make(map[string]string), + mountedRoots: make(map[string]struct{}), securityOptions: securityPolicyOptions, } } +// checkState returns an error if the UVM has entered an inconsistent state from +// which the sidecar cannot safely recover, in which case further mount/unmount, +// container creation and deletion must be refused. +func (h *Host) checkState() error { + return h.uvmError.Check() +} + +// setUVMInconsistent records that the UVM has entered an inconsistent state and +// logs the cause. After this, checkState refuses further operations. The caller +// passes the specific cause; see its call sites for the conditions that trigger +// it. +func (h *Host) setUVMInconsistent(cause error) { + h.uvmError.Set(cause) + log.G(context.Background()).WithError(cause).Error("Host marked inconsistent. All further mounts/unmounts, container creation and deletion will fail.") +} + func (h *Host) AddContainer(ctx context.Context, id string, c *Container) error { h.containersMutex.Lock() defer h.containersMutex.Unlock() @@ -86,7 +161,11 @@ func (h *Host) RemoveContainer(ctx context.Context, id string) error { return gcserr.NewHresultError(gcserr.HrVmcomputeSystemNotFound) } + if rootPath, ok := h.containerRootPaths[id]; ok { + delete(h.mountedRoots, strings.ToLower(rootPath)) + } delete(h.containers, id) + delete(h.containerRootPaths, id) return nil } @@ -102,6 +181,67 @@ func (h *Host) GetCreatedContainer(ctx context.Context, id string) (*Container, return c, nil } +// IsContainerRootInUse reports whether a container that has not exited is still +// using the combined-layers root mounted at rootPath as its rootfs, so the +// sidecar can refuse to unmount it. (cf. LCOW Host.IsOverlayInUse in +// internal/guest/runtime/hcsv2/uvm.go; WCOW uses a filesystem filter / combined +// layers rather than an overlayfs.) +func (h *Host) IsContainerRootInUse(rootPath string) bool { + h.containersMutex.Lock() + defer h.containersMutex.Unlock() + + for id, c := range h.containers { + if c.terminated.Load() { + continue + } + if strings.EqualFold(h.containerRootPaths[id], rootPath) { + return true + } + } + return false +} + +// SetContainerRootMounted records (mounted=true) or clears (mounted=false) +// whether the combined-layers root at rootPath is currently mounted. +func (h *Host) SetContainerRootMounted(rootPath string, mounted bool) { + h.containersMutex.Lock() + defer h.containersMutex.Unlock() + + key := strings.ToLower(rootPath) + if mounted { + h.mountedRoots[key] = struct{}{} + } else { + delete(h.mountedRoots, key) + } +} + +// HasContainerRoot reports whether a combined-layers root has already been +// recorded for the given container. It lets the CWCOWCombinedLayers Add handler +// stay idempotent: a second Add for the same container would otherwise overwrite +// containerRootPaths[cid] and leak the previous root's mounted-root entry. +func (h *Host) HasContainerRoot(cid string) bool { + h.containersMutex.Lock() + defer h.containersMutex.Unlock() + + _, ok := h.containerRootPaths[cid] + return ok +} + +// IsContainerRootMountedForContainer reports whether the combined-layers root +// recorded for the given container is still mounted. +// (cf. LCOW hostMounts.HasOverlayMountedAt) +func (h *Host) IsContainerRootMountedForContainer(cid string) bool { + h.containersMutex.Lock() + defer h.containersMutex.Unlock() + + rootPath, ok := h.containerRootPaths[cid] + if !ok { + return false + } + _, mounted := h.mountedRoots[strings.ToLower(rootPath)] + return mounted +} + // GetProcess returns the Process with the matching 'pid'. If the 'pid' does // not exit returns error. func (c *Container) GetProcess(pid uint32) (*containerProcess, error) { diff --git a/internal/guest/runtime/hcsv2/uvm.go b/internal/guest/runtime/hcsv2/uvm.go index 5df004c03d..f3c07d04bb 100644 --- a/internal/guest/runtime/hcsv2/uvm.go +++ b/internal/guest/runtime/hcsv2/uvm.go @@ -825,7 +825,9 @@ func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VM settings.OCISpecification.Process.Capabilities = capsToKeep } - if oci.ParseAnnotationsBool(ctx, settings.OCISpecification.Annotations, annotations.LCOWSecurityPolicyEnv, true) { + if h.HasSecurityPolicy() { + // The security-context dir must always be written for confidential containers; + // it must not be gated by a host-controlled annotation. if _, err := h.securityOptions.WriteSecurityContextDir(settings.OCISpecification); err != nil { return nil, fmt.Errorf("failed to write security context dir: %w", err) } diff --git a/internal/hcs/schema2/virtual_machine.go b/internal/hcs/schema2/virtual_machine.go index f76aa5b288..48ce8418cb 100644 --- a/internal/hcs/schema2/virtual_machine.go +++ b/internal/hcs/schema2/virtual_machine.go @@ -15,17 +15,17 @@ package hcsschema type VirtualMachine struct { Version *Version `json:"Version,omitempty"` // When set to true, the virtual machine will treat a reset as a stop, releasing resources and cleaning up state. - StopOnReset bool `json:"StopOnReset,omitempty"` - Chipset *Chipset `json:"Chipset,omitempty"` - ComputeTopology *Topology `json:"ComputeTopology,omitempty"` - Devices *Devices `json:"Devices,omitempty"` - GuestState *GuestState `json:"GuestState,omitempty"` - RestoreState *RestoreState `json:"RestoreState,omitempty"` - RegistryChanges *RegistryChanges `json:"RegistryChanges,omitempty"` - StorageQoS *StorageQoS `json:"StorageQoS,omitempty"` - DebugOptions *DebugOptions `json:"DebugOptions,omitempty"` - GuestConnection *GuestConnection `json:"GuestConnection,omitempty"` - SecuritySettings *SecuritySettings `json:"SecuritySettings,omitempty"` + StopOnReset bool `json:"StopOnReset,omitempty"` + Chipset *Chipset `json:"Chipset,omitempty"` + ComputeTopology *Topology `json:"ComputeTopology,omitempty"` + Devices *Devices `json:"Devices,omitempty"` + GuestState *GuestState `json:"GuestState,omitempty"` + RestoreState *RestoreState `json:"RestoreState,omitempty"` + RegistryChanges *RegistryChanges `json:"RegistryChanges,omitempty"` + StorageQoS *StorageQoS `json:"StorageQoS,omitempty"` + DebugOptions *DebugOptions `json:"DebugOptions,omitempty"` + GuestConnection *GuestConnection `json:"GuestConnection,omitempty"` + SecuritySettings *SecuritySettings `json:"SecuritySettings,omitempty"` ResourcePartitionId string `json:"ResourcePartitionId,omitempty"` // Live migration options to be used on destination. MigrationOptions *MigrationInitializeOptions `json:"MigrationOptions,omitempty"` diff --git a/internal/tools/securitypolicy/main.go b/internal/tools/securitypolicy/main.go index f05d0e4231..b8c6ae509f 100644 --- a/internal/tools/securitypolicy/main.go +++ b/internal/tools/securitypolicy/main.go @@ -95,6 +95,7 @@ func main() { config.AllowEnvironmentVariableDropping, config.AllowUnencryptedScratch, config.AllowCapabilityDropping, + config.AllowRegistryChangesDropping, config.AllowLogProviderDropping, ) case "windows": @@ -116,6 +117,7 @@ func main() { config.AllowEnvironmentVariableDropping, config.AllowUnencryptedScratch, config.AllowCapabilityDropping, + config.AllowRegistryChangesDropping, config.AllowLogProviderDropping, ) default: diff --git a/pkg/securitypolicy/api.rego b/pkg/securitypolicy/api.rego index 4e9f339df2..70620e27cb 100644 --- a/pkg/securitypolicy/api.rego +++ b/pkg/securitypolicy/api.rego @@ -26,6 +26,9 @@ enforcement_points := { "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}, + "mapped_directory_mount": {"introducedVersion": "0.11.0", "default_results": {"allowed": true}, "use_framework": false}, + "mapped_directory_unmount": {"introducedVersion": "0.11.0", "default_results": {"allowed": true}, "use_framework": false}, + "unmount_cims": {"introducedVersion": "0.11.0", "default_results": {"allowed": true}, "use_framework": false}, "log_provider": {"introducedVersion": "0.11.0", "default_results": {"allowed": true, "providers_to_keep": null}, "use_framework": false}, "load_transparency_trust_list": {"introducedVersion": "0.12.0", "default_results": {"allowed": false}, "use_framework": false}, "host_network": {"introducedVersion": "0.12.0", "default_results": {"allowed": false}, "use_framework": false}, diff --git a/pkg/securitypolicy/framework.rego b/pkg/securitypolicy/framework.rego index e462f4f34e..2cf3a5a475 100644 --- a/pkg/securitypolicy/framework.rego +++ b/pkg/securitypolicy/framework.rego @@ -184,7 +184,7 @@ candidate_containers := containers if { default mount_cims := {"allowed": false} -mount_cims := {"metadata": [addMatches], "allowed": true} if { +mount_cims := {"metadata": [addMatches, addCimVolume], "allowed": true} if { not overlay_exists containers := [container | @@ -200,6 +200,36 @@ mount_cims := {"metadata": [addMatches], "allowed": true} if { "key": input.containerID, "value": containers, } + + # Record the host-minted volume GUID (a runtime handle, not a policy-authored + # value) so unmount_cims can require it later. A single block CIM volume is + # shared by every container from the same image and is mounted/unmounted once + # for its whole lifetime: the host physically mounts it once, then re-drives + # this rule per container that reuses it, and unmounts it once when the last + # reference is gone (per-container teardown does not touch the CIM). "update" + # keeps those repeated mounts of the same volume idempotent (one record), so + # the matching single unmount stays symmetric. + addCimVolume := { + "name": "mountedCimVolumes", + "action": "update", + "key": input.volumeGUID, + "value": true, + } +} + +cim_volume_mounted(volumeGUID) if { + data.metadata.mountedCimVolumes[volumeGUID] +} + +default unmount_cims := {"allowed": false} + +unmount_cims := {"metadata": [removeCimVolume], "allowed": true} if { + cim_volume_mounted(input.volumeGUID) + removeCimVolume := { + "name": "mountedCimVolumes", + "action": "remove", + "key": input.volumeGUID, + } } default mount_overlay := {"allowed": false} @@ -747,6 +777,7 @@ create_container := {"metadata": [updateMatches, addStarted], user_ok(container.user) workingDirectory_ok(container.working_dir) command_ok(container.command) + mountList_ok(container.mounts, false) ] count(possible_after_initial_containers) > 0 @@ -825,15 +856,11 @@ mountSource_ok(constraint, source) if { constraint == source } -mountConstraint_ok(constraint, mount) if { - mount.type == constraint.type - mountSource_ok(constraint.source, mount.source) - mount.destination != "" - mount.destination == constraint.destination - - # the following check is not required (as the following tests will prove this - # condition as well), however it will check whether those more expensive - # tests need to be performed. +# mountOptions_ok holds when a mount's options are exactly the constraint's +# option set: every requested option is allowed and every allowed option is +# present (no missing, no extras). The count check is a cheap pre-filter for the +# two set-containment checks that follow. +mountOptions_ok(constraint, mount) if { count(mount.options) == count(constraint.options) every option in mount.options { some constraintOption in constraint.options @@ -846,6 +873,82 @@ mountConstraint_ok(constraint, mount) if { } } +# is_named_pipe reports whether an OCI mount destination refers to a Windows +# named pipe. This matches how the host decides to turn a mount into a +# MappedPipe rather than a MappedDirectory (see internal/gcs-sidecar handlers' +# isPipeDestination and internal/hcsoci/hcsdoc_wcow.go). +is_named_pipe(path) if { + startswith(path, `\\.\pipe\`) +} + +# windows_mount_type_ok gates which OCI mount `type` values are acceptable on +# Windows. We only handle "plain" mounts - mapped directories and named pipes - +# which carry an empty type or an explicit +# "bind". Disk/device mount types (virtual-disk / physical-disk / +# extensible-virtual-disk) are not supported and rejected. +windows_mount_type_ok(mount) if { + mount.type == "" +} + +windows_mount_type_ok(mount) if { + mount.type == "bind" +} + +mountConstraint_ok(constraint, mount) if { + is_linux + mount.type == constraint.type + mountSource_ok(constraint.source, mount.source) + mount.destination != "" + mount.destination == constraint.destination + mountOptions_ok(constraint, mount) +} + +# Windows named pipe: the source is a stable "\\.\pipe\" path that the +# policy author can predict, so we require it to match the constraint exactly. +# This stops the host from wiring a container's expected pipe destination up to a +# different host pipe. We don't match mount.type against a policy value (it's +# empty/"bind" for real mounts), but we do reject non-plain types via +# windows_mount_type_ok so a disk/device mount can't pass as a pipe. +mountConstraint_ok(constraint, mount) if { + is_windows + windows_mount_type_ok(mount) + is_named_pipe(mount.destination) + mount.destination != "" + mount.destination == constraint.destination + constraint.source == mount.source + mountOptions_ok(constraint, mount) +} + +# Windows mapped directory (anything that is not a named pipe): by the time the +# request reaches the UVM the host has rewritten the user's host_path (e.g. +# "C:\share-host") into a host-generated volume path such as +# "\\?\Volume{}\share-host". That GUID is picked by the host and +# cannot be predicted by the policy author, so we do not enforce the source and +# rely on the destination + options (mirroring the top-level mapped_directories +# rule, which matches on container_path + read_only). windows_mount_type_ok +# rejects disk/device mount types so they can't pass as a directory. +# +# Note on state: this create-time match (reached via mountList_ok in the Windows +# create_container) records NO per-container mount state - unlike LCOW, where a +# container's mounts are established by separate, independently-unmountable +# modifySettings ops (plan9_mount / scsi / overlay) whose metadata +# create_container then reads via mountSource_ok. We deliberately track nothing +# here, resting on the assumption that there is no operation to "remove mount X +# from container Y" independently of the container: a Windows container's +# create-time mounts live and die with the container (torn down wholesale when +# its combined layers are removed), so no independent unmount could ever consume +# such state - hence there is nothing to track. (The UVM-level mapped directory +# added/removed via mapped_directory_mount / mapped_directory_unmount has a +# separate mechanism that keeps its own state.) +mountConstraint_ok(constraint, mount) if { + is_windows + windows_mount_type_ok(mount) + not is_named_pipe(mount.destination) + mount.destination != "" + mount.destination == constraint.destination + mountOptions_ok(constraint, mount) +} + mount_ok(mounts, allow_elevated, mount) if { some constraint in mounts mountConstraint_ok(constraint, mount) @@ -896,16 +999,13 @@ mount_ok(mounts, allow_elevated, mount) if { "rw" in mount.options } +# mountList_ok is OS-agnostic here: the per-mount OS differences are handled by +# the is_linux/is_windows bodies of mountConstraint_ok. mountList_ok(mounts, allow_elevated) if { - is_linux every mount in input.mounts { mount_ok(mounts, allow_elevated, mount) } } -mountList_ok(mounts, allow_elevated) if { - # no-op for windows - is_windows -} is_linux if { data.metadata.operatingsystem[ostype] == "linux" @@ -1691,6 +1791,62 @@ scratch_unmount := {"metadata": [remove_scratch_mount], "allowed": true} if { } } +# Mapped directory (VSMB share) validation for Windows containers +default mapped_directory_mount := {"allowed": false} + +mapped_directory_mounted(target) if { + data.metadata.mapped_directories[target] +} + +default mapped_directory_ok := false + +# A mapped directory is matched on container_path + read_only only; we do not +# enforce its host-side source. This mirrors the reasoning in +# windows_mountSource_ok for directory (non-pipe) mounts: by the time the +# request reaches the UVM the host has already rewritten the user's host_path +# (e.g. "C:\share-host") into a host-generated volume path such as +# "\\?\Volume{}\share-host". That GUID is picked by the host and +# cannot be predicted by the policy author, so matching on it carries no +# security value. + +# allowed by an entry in the base policy +mapped_directory_ok if { + mapped_directory := data.policy.mapped_directories[_] + input.containerPath == mapped_directory.container_path + input.readOnly == mapped_directory.read_only +} + +# allowed by an entry loaded from a fragment +mapped_directory_ok if { + feed := data.metadata.issuers[_].feeds[_] + some fragment in feed + mapped_directory := fragment.mapped_directories[_] + input.containerPath == mapped_directory.container_path + input.readOnly == mapped_directory.read_only +} + +mapped_directory_mount := {"metadata": [add_mapped_dir], "allowed": true} if { + not mapped_directory_mounted(input.containerPath) + mapped_directory_ok + add_mapped_dir := { + "name": "mapped_directories", + "action": "add", + "key": input.containerPath, + "value": {"readOnly": input.readOnly}, + } +} + +default mapped_directory_unmount := {"allowed": false} + +mapped_directory_unmount := {"metadata": [remove_mapped_dir], "allowed": true} if { + mapped_directory_mounted(input.unmountTarget) + remove_mapped_dir := { + "name": "mapped_directories", + "action": "remove", + "key": input.unmountTarget, + } +} + # Log provider validation for Windows containers. # # Two modes (mirrors allow_environment_variable_dropping): @@ -1818,27 +1974,123 @@ registry_value_matches(policy_value, input_value) if { policy_value.type == "None" } -# Filter input registry values to only include those that match policy -filtered_registry_values(input_values, policy_values) := [input_val | - input_val := input_values[_] - some policy_val in policy_values - registry_value_matches(policy_val, input_val) -] +# requested_registry_changes is the tagged set of all requested registry +# changes: each requested add value and each requested delete key, tagged by +# kind so the add and delete cases share the same dropping/narrowing machinery. +requested_registry_changes := changes if { + adds := {{"kind": "add", "value": input_value} | + some input_value in input.registryChanges.AddValues + } + deletes := {{"kind": "delete", "key": input_key} | + some input_key in input.registryChanges.DeleteKeys + } + changes := adds | deletes +} -registry_changes := {"allowed": true} if { - containers := data.metadata.matches[input.containerID] - container := containers[_] +# registry_change_authorized holds when the container's registry_changes policy +# authorizes the requested change (an add value or a delete key). +registry_change_authorized(container, change) if { + change.kind == "add" + some policy_value in container.registry_changes.add_values + registry_value_matches(policy_value, change.value) +} - # Check if container has registry_changes defined in policy - container.registry_changes +registry_change_authorized(container, change) if { + change.kind == "delete" + some policy_key in container.registry_changes.delete_keys + registry_keys_match(policy_key, change.key) +} + +# valid_registry_subset is the set of requested registry changes that the +# container's policy authorizes. +valid_registry_subset(container) := changes if { + changes := {change | + some change in requested_registry_changes + registry_change_authorized(container, change) + } +} + +# valid_registry_for_all selects the registry changes to keep across the +# candidate containers, mirroring valid_envs_for_all. With +# allow_registry_changes_dropping it keeps the most specific (largest) +# authorized subset, dropping the rest; if several containers tie for the +# largest subset they must authorize the same set (intersection == union) for +# the result to be decidable. Without dropping it keeps every requested change, +# so a container must authorize all of them for the request to be allowed. +valid_registry_for_all(containers) := changes if { + allow_registry_changes_dropping - # If input has registry changes, filter to only matching ones - input.registryChanges.AddValues - matched_values := filtered_registry_values(input.registryChanges.AddValues, container.registry_changes.add_values) + valid := [subset | + some container in containers + subset := valid_registry_subset(container) + ] + + counts := [count(subset) | subset := valid[_]] + max_count := max(counts) - # Build result with filtered AddValues - result := { - "AddValues": matched_values + largest_change_sets := {subset | + some i + counts[i] == max_count + subset := valid[i] + } + + changes_i := intersection(largest_change_sets) + changes_u := union(largest_change_sets) + changes_i == changes_u + changes := changes_i +} + +valid_registry_for_all(containers) := changes if { + not allow_registry_changes_dropping + + # no dropping: keep every requested change, so a container must authorize all + changes := requested_registry_changes +} + +# registryChanges_ok holds when the container authorizes every change in +# `changes`. +registryChanges_ok(container, changes) if { + every change in changes { + registry_change_authorized(container, change) + } +} + +# registry_changes decides whether the requested registry changes are allowed, +# returning the add values and delete keys to keep (add_values_to_keep / +# delete_keys_to_keep). It also narrows the matched containers so the decision +# stays consistent with create_container whichever order the two run in. +registry_changes := { + "metadata": [updateMatches], + "add_values_to_keep": add_values, + "delete_keys_to_keep": delete_keys, + "allowed": true, +} if { + matches := data.metadata.matches[input.containerID] + + # honors allow_registry_changes_dropping + kept := valid_registry_for_all(matches) + + containers := [container | + container := matches[_] + registryChanges_ok(container, kept) + ] + + count(containers) > 0 + + add_values := [change.value | + some change in kept + change.kind == "add" + ] + delete_keys := [change.key | + some change in kept + change.kind == "delete" + ] + + updateMatches := { + "name": "matches", + "action": "update", + "key": input.containerID, + "value": containers, } } @@ -2012,6 +2264,37 @@ errors contains "no matching containers for overlay" if { not overlay_matches } +default cim_matches := false + +cim_matches if { + some container in candidate_containers + layerHashes_ok(container.layers) + input.mountedCim == container.mounted_cim +} + +errors contains "the container image layers have already been matched by a prior mount_cims" if { + input.rule == "mount_cims" + overlay_exists +} + +errors contains "no matching containers for CIM mount" if { + input.rule == "mount_cims" + not overlay_exists + not cim_matches +} + +# Actionable hint for the common misconfiguration: a policy that uses CIM +# mounts (mounted_cim) but declares a framework_version older than when CIM +# support was added. In that case check_container reconstructs the container +# without mounted_cim, so cim_matches can never be true and the mount is denied. +errors contains cimVersionError if { + input.rule == "mount_cims" + not overlay_exists + not cim_matches + semver.compare(policy_framework_version, "0.5.0") < 0 + cimVersionError := concat(" ", ["policy framework_version", policy_framework_version, "predates CIM mount support (mounted_cim added in 0.5.0); set it to the UVM framework version:", version]) +} + default privileged_matches := false privileged_matches if { @@ -2216,12 +2499,18 @@ errors contains "invalid working directory" if { } mount_matches(mount) if { + is_linux some container in data.metadata.matches[input.containerID] mount_ok(container.mounts, container.allow_elevated, mount) } +mount_matches(mount) if { + is_windows + some container in data.metadata.matches[input.containerID] + mount_ok(container.mounts, false, mount) +} + errors contains mountError if { - is_linux input.rule == "create_container" bad_mounts := [mount.destination | mount := input.mounts[_] @@ -2445,6 +2734,31 @@ errors contains "no scratch at path to unmount" if { not scratch_mounted(input.unmountTarget) } +errors contains "no CIM volume at GUID to unmount" if { + input.rule == "unmount_cims" + not cim_volume_mounted(input.volumeGUID) +} + +errors contains "mapped directory already mounted at path" if { + input.rule == "mapped_directory_mount" + mapped_directory_mounted(input.containerPath) +} + +errors contains "no matching mapped directory in policy" if { + input.rule == "mapped_directory_mount" + not mapped_directory_ok +} + +errors contains "no mapped directory at path to unmount" if { + input.rule == "mapped_directory_unmount" + not mapped_directory_mounted(input.unmountTarget) +} + +errors contains "invalid registry changes" if { + input.rule == "registry_changes" + not registry_changes.allowed +} + errors contains "log provider not allowed by policy" if { input.rule == "log_provider" not log_provider.allowed @@ -2541,6 +2855,7 @@ errors contains "containers only distinguishable by allow_stdio_access" if { user_ok(container.user) workingDirectory_ok(container.working_dir) command_ok(container.command) + mountList_ok(container.mounts, false) ] count(possible_after_initial_containers) > 0 @@ -2827,6 +3142,7 @@ check_container(raw_container, framework_version) := container if { "user": check_user(raw_container, framework_version), "capabilities": check_capabilities(raw_container, framework_version), "seccomp_profile_sha256": check_seccomp_profile_sha256(raw_container, framework_version), + "mounted_cim": check_mounted_cim(raw_container, framework_version), } } @@ -2896,6 +3212,16 @@ check_signals(raw_container, framework_version) := signals if { signals := array.concat(raw_container.signals, [9, 15]) } +check_mounted_cim(raw_container, framework_version) := mounted_cim if { + semver.compare(framework_version, "0.5.0") >= 0 + mounted_cim := object.get(raw_container, "mounted_cim", []) +} + +check_mounted_cim(raw_container, framework_version) := mounted_cim if { + semver.compare(framework_version, "0.5.0") < 0 + mounted_cim := [] +} + check_external_process(raw_process, framework_version) := process if { semver.compare(framework_version, version) == 0 process := raw_process @@ -2963,6 +3289,10 @@ allow_capability_dropping := flag if { flag := data.policy.allow_capability_dropping } +default allow_registry_changes_dropping := false + +allow_registry_changes_dropping := data.policy.allow_registry_changes_dropping + default policy_framework_version := null default policy_api_version := null diff --git a/pkg/securitypolicy/open_door.rego b/pkg/securitypolicy/open_door.rego index 0a5e90a663..7a786b912b 100644 --- a/pkg/securitypolicy/open_door.rego +++ b/pkg/securitypolicy/open_door.rego @@ -25,6 +25,9 @@ runtime_logging := {"allowed": true} load_fragment := {"allowed": true} scratch_mount := {"allowed": true} scratch_unmount := {"allowed": true} +mapped_directory_mount := {"allowed": true} +mapped_directory_unmount := {"allowed": true} +unmount_cims := {"allowed": true} log_provider := {"allowed": true} load_transparency_trust_list := {"allowed": true} host_network := {"allowed": true} diff --git a/pkg/securitypolicy/opts.go b/pkg/securitypolicy/opts.go index f3f9a24517..5475c610a9 100644 --- a/pkg/securitypolicy/opts.go +++ b/pkg/securitypolicy/opts.go @@ -206,6 +206,13 @@ func WithAllowCapabilityDropping(allow bool) PolicyConfigOpt { } } +func WithAllowRegistryChangesDropping(allow bool) PolicyConfigOpt { + return func(config *PolicyConfig) error { + config.AllowRegistryChangesDropping = allow + return nil + } +} + func WithAllowRuntimeLogging(allow bool) PolicyConfigOpt { return func(config *PolicyConfig) error { config.AllowRuntimeLogging = allow diff --git a/pkg/securitypolicy/policy.rego b/pkg/securitypolicy/policy.rego index 706fdac666..cf114d59ef 100644 --- a/pkg/securitypolicy/policy.rego +++ b/pkg/securitypolicy/policy.rego @@ -28,6 +28,9 @@ runtime_logging := data.framework.runtime_logging load_fragment := data.framework.load_fragment scratch_mount := data.framework.scratch_mount scratch_unmount := data.framework.scratch_unmount +mapped_directory_mount := data.framework.mapped_directory_mount +mapped_directory_unmount := data.framework.mapped_directory_unmount +unmount_cims := data.framework.unmount_cims log_provider := data.framework.log_provider load_transparency_trust_list := data.framework.load_transparency_trust_list host_network := data.framework.host_network diff --git a/pkg/securitypolicy/rego_utils_test.go b/pkg/securitypolicy/rego_utils_test.go index edcd14c97f..7727ee481a 100644 --- a/pkg/securitypolicy/rego_utils_test.go +++ b/pkg/securitypolicy/rego_utils_test.go @@ -64,6 +64,8 @@ const ( maxGeneratedMountOptionLength = 32 maxGeneratedExecProcesses = 4 maxGeneratedWorkingDirLength = 128 + maxGeneratedMappedDirectories = 8 + maxGeneratedMappedDirectoryPathLength = 64 maxSignalNumber = 64 maxGeneratedNameLength = 8 maxGeneratedGroupNames = 4 @@ -720,6 +722,27 @@ type regoExternalPolicyTestConfig struct { policy *regoEnforcer } +func setupWindowsMappedDirectoriesTest(gc *generatedWindowsConstraints) (tc *regoMappedDirectoriesTestConfig, err error) { + gc.mappedDirectories = generateMappedDirectories(testRand) + securityPolicy := gc.toPolicy() + + policy, err := newRegoPolicy(securityPolicy.marshalWindowsRego(), + []oci.Mount{}, + []oci.Mount{}, + testOSType) + if err != nil { + return nil, err + } + + return ®oMappedDirectoriesTestConfig{ + policy: policy, + }, nil +} + +type regoMappedDirectoriesTestConfig struct { + policy *regoEnforcer +} + func setupGetPropertiesTest(gc *generatedConstraints, allowPropertiesAccess bool) (tc *regoGetPropertiesTestConfig, err error) { gc.allowGetProperties = allowPropertiesAccess @@ -1875,6 +1898,7 @@ func (c *securityPolicyWindowsContainer) toWindowsContainer() *WindowsContainer Layers: Layers(stringArrayToStringMap(c.Layers)), MountedCim: c.MountedCim, WorkingDir: c.WorkingDir, + Mounts: mountArrayToMounts(c.Mounts), ExecProcesses: execProcesses, Signals: c.Signals, User: c.User, @@ -2014,6 +2038,10 @@ func selectWindowsExternalProcessFromConstraints(constraints *generatedWindowsCo return constraints.externalProcesses[r.Intn(numberOfProcessesInConstraints)] } +func selectMappedDirectoryFromConstraints(constraints *generatedWindowsConstraints, r *rand.Rand) WindowsMappedDirectoryRule { + return constraints.mappedDirectories[r.Intn(len(constraints.mappedDirectories))] +} + func (constraints *generatedConstraints) toPolicy() *securityPolicyInternal { return &securityPolicyInternal{ Containers: constraints.containers, @@ -2026,6 +2054,7 @@ func (constraints *generatedConstraints) toPolicy() *securityPolicyInternal { AllowEnvironmentVariableDropping: constraints.allowEnvironmentVariableDropping, AllowUnencryptedScratch: constraints.allowUnencryptedScratch, AllowCapabilityDropping: constraints.allowCapabilityDropping, + AllowRegistryChangesDropping: constraints.allowRegistryChangesDropping, AllowLogProviderDropping: constraints.allowLogProviderDropping, } } @@ -2168,6 +2197,10 @@ func setupRegoCreateContainerTestWindows(gc *generatedWindowsConstraints, testCo }, nil } +// testCIMVolumeGUID is a placeholder volume GUID for tests that mount a CIM but +// don't exercise unmount; the unmount tests use their own GUIDs. +const testCIMVolumeGUID = "test-cim-volume-guid" + //nolint:unused func mountImageForWindowsContainer(policy *regoEnforcer, container *securityPolicyWindowsContainer) (string, error) { ctx := context.Background() @@ -2183,7 +2216,7 @@ func mountImageForWindowsContainer(policy *regoEnforcer, container *securityPoli // Mount the CIMFS for the Windows container // layerHashes are the individual layer hashes, mountedCim is the merged CIM from the policy - err := policy.EnforceVerifiedCIMsPolicy(ctx, containerID, layerHashes, container.MountedCim) + err := policy.EnforceVerifiedCIMsPolicy(ctx, containerID, layerHashes, container.MountedCim, testCIMVolumeGUID) if err != nil { return "", fmt.Errorf("error mounting CIMFS: %w", err) } @@ -2289,6 +2322,7 @@ func generateConstraints(r *rand.Rand, maxContainers int32) *generatedConstraint namespace: generateFragmentNamespace(testRand), svn: generateSVN(testRand), allowCapabilityDropping: false, + allowRegistryChangesDropping: false, allowLogProviderDropping: false, ctx: context.Background(), } @@ -2404,6 +2438,28 @@ func generateWorkingDir(r *rand.Rand) string { return randVariableString(r, maxGeneratedWorkingDirLength) } +func generateMappedDirectory(r *rand.Rand) WindowsMappedDirectoryRule { + return WindowsMappedDirectoryRule{ + ContainerPath: `C:\` + randVariableString(r, maxGeneratedMappedDirectoryPathLength), + ReadOnly: randBool(r), + } +} + +func generateMappedDirectories(r *rand.Rand) []WindowsMappedDirectoryRule { + numRules := atLeastOneAtMost(r, maxGeneratedMappedDirectories) + rules := make([]WindowsMappedDirectoryRule, 0, numRules) + seen := make(map[string]struct{}, numRules) + for int32(len(rules)) < numRules { + rule := generateMappedDirectory(r) + if _, dup := seen[rule.ContainerPath]; dup { + continue + } + seen[rule.ContainerPath] = struct{}{} + rules = append(rules, rule) + } + return rules +} + func generateWindowsUser(r *rand.Rand) string { return randVariableString(r, maxGeneratedWorkingDirLength) } @@ -2955,6 +3011,7 @@ type generatedConstraints struct { namespace string svn string allowCapabilityDropping bool + allowRegistryChangesDropping bool allowLogProviderDropping bool ctx context.Context } @@ -2963,6 +3020,7 @@ type generatedWindowsConstraints struct { containers []*securityPolicyWindowsContainer externalProcesses []*externalProcess fragments []*fragment + mappedDirectories []WindowsMappedDirectoryRule allowGetProperties bool allowDumpStacks bool allowRuntimeLogging bool @@ -2972,6 +3030,7 @@ type generatedWindowsConstraints struct { namespace string svn string allowCapabilityDropping bool + allowRegistryChangesDropping bool allowLogProviderDropping bool ctx context.Context } @@ -2981,6 +3040,7 @@ func (constraints *generatedWindowsConstraints) toPolicy() *securityPolicyWindow Containers: constraints.containers, ExternalProcesses: constraints.externalProcesses, Fragments: constraints.fragments, + MappedDirectories: constraints.mappedDirectories, AllowPropertiesAccess: constraints.allowGetProperties, AllowDumpStacks: constraints.allowDumpStacks, AllowRuntimeLogging: constraints.allowRuntimeLogging, @@ -2988,6 +3048,7 @@ func (constraints *generatedWindowsConstraints) toPolicy() *securityPolicyWindow AllowEnvironmentVariableDropping: constraints.allowEnvironmentVariableDropping, AllowUnencryptedScratch: constraints.allowUnencryptedScratch, AllowCapabilityDropping: constraints.allowCapabilityDropping, + AllowRegistryChangesDropping: constraints.allowRegistryChangesDropping, AllowLogProviderDropping: constraints.allowLogProviderDropping, } } @@ -3034,6 +3095,7 @@ func generateWindowsConstraints(r *rand.Rand, maxContainers int32) *generatedWin allowEnvironmentVariableDropping: false, allowUnencryptedScratch: false, allowCapabilityDropping: false, + allowRegistryChangesDropping: false, allowLogProviderDropping: false, namespace: generateFragmentNamespace(r), svn: generateSVN(r), diff --git a/pkg/securitypolicy/regopolicy_linux_test.go b/pkg/securitypolicy/regopolicy_linux_test.go index 52500dfcc6..401bbee4ec 100644 --- a/pkg/securitypolicy/regopolicy_linux_test.go +++ b/pkg/securitypolicy/regopolicy_linux_test.go @@ -83,6 +83,7 @@ func Test_MarshalRego_Policy(t *testing.T) { p.allowEnvironmentVariableDropping, p.allowUnencryptedScratch, p.allowCapabilityDropping, + p.allowRegistryChangesDropping, p.allowLogProviderDropping, ) if err != nil { diff --git a/pkg/securitypolicy/regopolicy_windows_test.go b/pkg/securitypolicy/regopolicy_windows_test.go index e9eeff622d..ec0d84c1bb 100644 --- a/pkg/securitypolicy/regopolicy_windows_test.go +++ b/pkg/securitypolicy/regopolicy_windows_test.go @@ -229,6 +229,235 @@ func Test_Rego_EnforceCreateContainer_Windows(t *testing.T) { } } +func Test_Rego_MountPolicy_Matches_Windows(t *testing.T) { + f := func(p *generatedWindowsConstraints) bool { + c := selectWindowsContainerFromContainerList(p.containers, testRand) + mnt := mountInternal{ + Source: "C:\\host\\share", + Destination: "C:\\container\\share", + Options: []string{"ro"}, + } + c.Mounts = append(c.Mounts, mnt) + + tc, err := setupRegoCreateContainerTestWindows(p, c, false) + if err != nil { + t.Error(err) + return false + } + + requestMounts := []oci.Mount{ + { + Source: mnt.Source, + Destination: mnt.Destination, + Options: mnt.Options, + }, + } + + _, _, _, err = tc.policy.EnforceCreateContainerPolicyV2(p.ctx, tc.containerID, tc.argList, tc.envList, tc.workingDir, requestMounts, tc.user, nil) + if err != nil { + t.Errorf("a mount matching the policy was denied: %v", err) + return false + } + return true + } + + if err := quick.Check(f, &quick.Config{MaxCount: 10, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_MountPolicy_Matches_Windows: %v", err) + } +} + +func Test_Rego_MountPolicy_DiskTypeRejected_Windows(t *testing.T) { + f := func(p *generatedWindowsConstraints) bool { + c := selectWindowsContainerFromContainerList(p.containers, testRand) + mnt := mountInternal{ + Source: "C:\\host\\share", + Destination: "C:\\container\\share", + Options: []string{"ro"}, + } + c.Mounts = append(c.Mounts, mnt) + + tc, err := setupRegoCreateContainerTestWindows(p, c, false) + if err != nil { + t.Error(err) + return false + } + + // Same destination + options the policy allows, but tagged as a disk + // mount type. A disk/device type must be rejected regardless of the + // destination match, so it can't ride in on a directory allowance. + requestMounts := []oci.Mount{ + { + Source: mnt.Source, + Destination: mnt.Destination, + Options: mnt.Options, + Type: "virtual-disk", + }, + } + + _, _, _, err = tc.policy.EnforceCreateContainerPolicyV2(p.ctx, tc.containerID, tc.argList, tc.envList, tc.workingDir, requestMounts, tc.user, nil) + if err == nil { + t.Error("a disk-type mount was allowed by policy") + return false + } + + return assertDecisionJSONContains(t, err, "invalid mount list") + } + + if err := quick.Check(f, &quick.Config{MaxCount: 10, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_MountPolicy_DiskTypeRejected_Windows: %v", err) + } +} + +func Test_Rego_MountPolicy_BindTypeAllowed_Windows(t *testing.T) { + f := func(p *generatedWindowsConstraints) bool { + c := selectWindowsContainerFromContainerList(p.containers, testRand) + mnt := mountInternal{ + Source: "C:\\host\\share", + Destination: "C:\\container\\share", + Options: []string{"ro"}, + } + c.Mounts = append(c.Mounts, mnt) + + tc, err := setupRegoCreateContainerTestWindows(p, c, false) + if err != nil { + t.Error(err) + return false + } + + // An explicit "bind" type is a plain mount and must still be allowed. + requestMounts := []oci.Mount{ + { + Source: mnt.Source, + Destination: mnt.Destination, + Options: mnt.Options, + Type: "bind", + }, + } + + _, _, _, err = tc.policy.EnforceCreateContainerPolicyV2(p.ctx, tc.containerID, tc.argList, tc.envList, tc.workingDir, requestMounts, tc.user, nil) + if err != nil { + t.Errorf("a bind-type mount matching the policy was denied: %v", err) + return false + } + return true + } + + if err := quick.Check(f, &quick.Config{MaxCount: 10, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_MountPolicy_BindTypeAllowed_Windows: %v", err) + } +} + +func Test_Rego_MountPolicy_NoMatches_Windows(t *testing.T) { + f := func(p *generatedWindowsConstraints) bool { + c := selectWindowsContainerFromContainerList(p.containers, testRand) + tc, err := setupRegoCreateContainerTestWindows(p, c, false) + if err != nil { + t.Error(err) + return false + } + + // The container declares no matching mount constraint, so any + // requested mount must be rejected. + requestMounts := []oci.Mount{ + { + Source: "C:\\host\\not-in-policy", + Destination: "C:\\container\\not-in-policy", + Options: []string{"rw"}, + }, + } + + _, _, _, err = tc.policy.EnforceCreateContainerPolicyV2(p.ctx, tc.containerID, tc.argList, tc.envList, tc.workingDir, requestMounts, tc.user, nil) + if err == nil { + t.Error("a mount not present in the policy did not result in an error") + return false + } + + return assertDecisionJSONContains(t, err, "invalid mount list") + } + + if err := quick.Check(f, &quick.Config{MaxCount: 10, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_MountPolicy_NoMatches_Windows: %v", err) + } +} + +func Test_Rego_MountPolicy_Pipe_Matches_Windows(t *testing.T) { + f := func(p *generatedWindowsConstraints) bool { + c := selectWindowsContainerFromContainerList(p.containers, testRand) + pipe := mountInternal{ + Source: "\\\\.\\pipe\\host-pipe", + Destination: "\\\\.\\pipe\\container-pipe", + Options: []string{}, + } + c.Mounts = append(c.Mounts, pipe) + + tc, err := setupRegoCreateContainerTestWindows(p, c, false) + if err != nil { + t.Error(err) + return false + } + + requestMounts := []oci.Mount{ + { + Source: pipe.Source, + Destination: pipe.Destination, + Options: pipe.Options, + }, + } + + _, _, _, err = tc.policy.EnforceCreateContainerPolicyV2(p.ctx, tc.containerID, tc.argList, tc.envList, tc.workingDir, requestMounts, tc.user, nil) + if err != nil { + t.Errorf("a pipe mount matching the policy was denied: %v", err) + return false + } + return true + } + + if err := quick.Check(f, &quick.Config{MaxCount: 10, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_MountPolicy_Pipe_Matches_Windows: %v", err) + } +} + +func Test_Rego_MountPolicy_Pipe_BadSource_Windows(t *testing.T) { + f := func(p *generatedWindowsConstraints) bool { + c := selectWindowsContainerFromContainerList(p.containers, testRand) + pipe := mountInternal{ + Source: "\\\\.\\pipe\\host-pipe", + Destination: "\\\\.\\pipe\\container-pipe", + Options: []string{}, + } + c.Mounts = append(c.Mounts, pipe) + + tc, err := setupRegoCreateContainerTestWindows(p, c, false) + if err != nil { + t.Error(err) + return false + } + + // Same (policy-matching) pipe destination, but a different host pipe + // source. Unlike a mapped directory, a pipe source is enforced, so this + // must be rejected. + requestMounts := []oci.Mount{ + { + Source: "\\\\.\\pipe\\attacker-pipe", + Destination: pipe.Destination, + Options: pipe.Options, + }, + } + + _, _, _, err = tc.policy.EnforceCreateContainerPolicyV2(p.ctx, tc.containerID, tc.argList, tc.envList, tc.workingDir, requestMounts, tc.user, nil) + if err == nil { + t.Error("a pipe mount with a non-matching source did not result in an error") + return false + } + + return assertDecisionJSONContains(t, err, "invalid mount list") + } + + if err := quick.Check(f, &quick.Config{MaxCount: 10, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_MountPolicy_Pipe_BadSource_Windows: %v", err) + } +} + func Test_Rego_EnforceCreateContainer_Start_All_Containers(t *testing.T) { f := func(p *generatedWindowsConstraints) bool { securityPolicy := p.toPolicy() @@ -353,7 +582,7 @@ func Test_Rego_EnforceVerifiedCIMSPolicy_Multiple_Instances_Same_Container(t *te // The runtime sends individual layers as hashesToVerify // and the merged CIM hash separately id := testDataGenerator.uniqueContainerID() - err = policy.EnforceVerifiedCIMsPolicy(constraints.ctx, id, layerHashes, container.MountedCim) + err = policy.EnforceVerifiedCIMsPolicy(constraints.ctx, id, layerHashes, container.MountedCim, testCIMVolumeGUID) if err != nil { t.Fatalf("failed with %d containers", containersToCreate) } @@ -361,6 +590,99 @@ func Test_Rego_EnforceVerifiedCIMSPolicy_Multiple_Instances_Same_Container(t *te } } +// setupMountedCIMVolume builds a generated Windows policy, mounts container[0]'s +// CIM under the given volume GUID (so mount_cims records it), and returns the +// enforcer ready for an unmount_cims call. +func setupMountedCIMVolume(t *testing.T, p *generatedWindowsConstraints, volumeGUID string) *regoEnforcer { + t.Helper() + securityPolicy := p.toPolicy() + policy, err := newRegoPolicy(securityPolicy.marshalWindowsRego(), []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Fatalf("failed to create policy: %v", err) + } + + container := p.containers[0] + layerHashes := make([]string, len(container.Layers)) + for i, layer := range container.Layers { + layerHashes[len(container.Layers)-1-i] = layer + } + + id := testDataGenerator.uniqueContainerID() + if err := policy.EnforceVerifiedCIMsPolicy(context.Background(), id, layerHashes, container.MountedCim, volumeGUID); err != nil { + t.Fatalf("mount should succeed: %v", err) + } + return policy +} + +// Unmounting a CIM volume that was recorded at mount time is allowed. +func Test_Rego_EnforceCIMUnmountPolicy_Windows(t *testing.T) { + f := func(p *generatedWindowsConstraints) bool { + if len(p.containers) == 0 { + return true + } + volumeGUID := "12345678-1234-1234-1234-123456789abc" + policy := setupMountedCIMVolume(t, p, volumeGUID) + + if err := policy.EnforceCIMUnmountPolicy(context.Background(), volumeGUID); err != nil { + t.Errorf("unmount of a mounted CIM volume should succeed: %v", err) + return false + } + return true + } + + if err := quick.Check(f, &quick.Config{MaxCount: 5, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_EnforceCIMUnmountPolicy_Windows: %v", err) + } +} + +// Unmounting a CIM volume GUID that was never mounted is denied (no symmetry). +func Test_Rego_EnforceCIMUnmountPolicy_NotMounted_Denied_Windows(t *testing.T) { + f := func(p *generatedWindowsConstraints) bool { + securityPolicy := p.toPolicy() + policy, err := newRegoPolicy(securityPolicy.marshalWindowsRego(), []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Errorf("failed to create policy: %v", err) + return false + } + + if err := policy.EnforceCIMUnmountPolicy(context.Background(), "never-mounted-guid"); err == nil { + t.Error("unmount of a never-mounted CIM volume should be denied") + return false + } + return true + } + + if err := quick.Check(f, &quick.Config{MaxCount: 5, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_EnforceCIMUnmountPolicy_NotMounted_Denied_Windows: %v", err) + } +} + +// Unmounting the same CIM volume twice is denied: the first unmount removes the +// record, so the second has nothing to match. +func Test_Rego_EnforceCIMUnmountPolicy_DoubleUnmount_Denied_Windows(t *testing.T) { + f := func(p *generatedWindowsConstraints) bool { + if len(p.containers) == 0 { + return true + } + volumeGUID := "aaaabbbb-cccc-dddd-eeee-ffffffffffff" + policy := setupMountedCIMVolume(t, p, volumeGUID) + + if err := policy.EnforceCIMUnmountPolicy(context.Background(), volumeGUID); err != nil { + t.Errorf("first unmount should succeed: %v", err) + return false + } + if err := policy.EnforceCIMUnmountPolicy(context.Background(), volumeGUID); err == nil { + t.Error("second unmount of the same CIM volume should be denied") + return false + } + return true + } + + if err := quick.Check(f, &quick.Config{MaxCount: 5, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_EnforceCIMUnmountPolicy_DoubleUnmount_Denied_Windows: %v", err) + } +} + // -- Capabilities/Mount/Rego version tests are removed -- Add back Rego versions test// func Test_Rego_ExecInContainerPolicy_Windows(t *testing.T) { f := func(p *generatedWindowsConstraints) bool { @@ -1398,7 +1720,7 @@ func Test_Rego_EnforceRegistryChangesPolicy_Matches_Windows(t *testing.T) { }, } - err = tc.policy.EnforceRegistryChangesPolicy(p.ctx, container.containerID, registryChanges) + _, err = tc.policy.EnforceRegistryChangesPolicy(p.ctx, container.containerID, registryChanges) // With default values, this should be allowed if err != nil { t.Logf("Registry enforcement returned: %v", err) @@ -1438,7 +1760,7 @@ func Test_Rego_EnforceRegistryChangesPolicy_Invalid_ContainerID_Windows(t *testi }, } - err = tc.policy.EnforceRegistryChangesPolicy(p.ctx, invalidContainerID, registryChanges) + _, err = tc.policy.EnforceRegistryChangesPolicy(p.ctx, invalidContainerID, registryChanges) if err == nil { t.Error("Expected registry changes to be denied with invalid container ID") return false @@ -1487,7 +1809,7 @@ func Test_Rego_EnforceRegistryChangesPolicy_Default_Values_Allowed_Windows(t *te }, } - err = tc.policy.EnforceRegistryChangesPolicy(p.ctx, container.containerID, registryChanges) + _, err = tc.policy.EnforceRegistryChangesPolicy(p.ctx, container.containerID, registryChanges) // Default values should be allowed if err != nil { t.Logf("Default registry values enforcement returned: %v", err) @@ -1501,6 +1823,306 @@ func Test_Rego_EnforceRegistryChangesPolicy_Default_Values_Allowed_Windows(t *te } } +func twoContainersSharedLayersRegistryRego(dropping bool) string { + constraints := &generatedWindowsConstraints{ + allowRegistryChangesDropping: dropping, + containers: []*securityPolicyWindowsContainer{ + { + Command: []string{"cmd"}, + Layers: []string{"layerA", "layerB"}, + MountedCim: []string{"merged"}, + WorkingDir: `C:\app`, + User: "ContainerUser", + AllowStdioAccess: true, + }, + { + Command: []string{"ping"}, + Layers: []string{"layerA", "layerB"}, + MountedCim: []string{"merged"}, + WorkingDir: `C:\app`, + User: "ContainerUser", + AllowStdioAccess: true, + RegistryChanges: registryChangesInternal{ + AddValues: []registryValueInternal{ + { + Key: registryKeyInternal{Hive: "System", Name: "TestControl"}, + Name: "Danger", + Type: "String", + StringValue: "danger", + }, + }, + DeleteKeys: []registryKeyInternal{ + {Hive: "System", Name: "TestControl\\Obsolete"}, + }, + }, + }, + }, + } + return constraints.toPolicy().marshalWindowsRego() +} + +// Test_Rego_RegistryChanges_NarrowsMatches_Windows verifies that the registry +// enforcement point narrows data.metadata.matches so it composes with +// create_container in either order, under both allow_registry_changes_dropping +// settings. Containers A (command "cmd", no registry rule) and B (command +// "ping", authorizes a dangerous registry value) share layers, so both survive +// mount_cims; the danger is that a request could pass the dangerous value while +// running A's command. With dropping on, "cmd" never runs with the dangerous +// value (either create(cmd) is denied when registry narrows to B first, or the +// value is dropped when create(cmd) narrows to A first). With dropping off, a +// request is only allowed if a matched container authorizes every requested +// value, so registry(dangerous) against A is denied outright. +// TODO: maybe delete it if it's too much. +func Test_Rego_RegistryChanges_NarrowsMatches_Windows(t *testing.T) { + dangerous := &hcsschema.RegistryChanges{ + AddValues: []hcsschema.RegistryValue{ + { + Key: &hcsschema.RegistryKey{Hive: "System", Name: "TestControl"}, + Name: "Danger", + Type_: hcsschema.RegistryValueType_STRING, + StringValue: "danger", + }, + }, + } + + keptCount := func(t *testing.T, keptRaw interface{}) int { + t.Helper() + kept, ok := keptRaw.(*hcsschema.RegistryChanges) + if !ok || kept == nil { + t.Fatalf("expected *hcsschema.RegistryChanges, got %T", keptRaw) + } + return len(kept.AddValues) + } + + // mount_cims reverses the layer order, so pass layers reversed. + layerHashes := []string{"layerB", "layerA"} + mountedCim := []string{"merged"} + ctx := context.Background() + user := IDName{Name: "ContainerUser"} + + newPolicy := func(dropping bool) *regoEnforcer { + policy, err := newRegoPolicy(twoContainersSharedLayersRegistryRego(dropping), []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Fatalf("failed to create policy: %v", err) + } + return policy + } + + // Order 1: registry (kept via B) then create with A's command. Registry + // narrows matches to [B], so create with "cmd" must be denied. + t.Run("registry_then_create_denied", func(t *testing.T) { + policy := newPolicy(true) + cid := testDataGenerator.uniqueContainerID() + if err := policy.EnforceVerifiedCIMsPolicy(ctx, cid, layerHashes, mountedCim, testCIMVolumeGUID); err != nil { + t.Fatalf("mount_cims: %v", err) + } + kept, err := policy.EnforceRegistryChangesPolicy(ctx, cid, dangerous) + if err != nil { + t.Fatalf("registry should be allowed (kept via B): %v", err) + } + if n := keptCount(t, kept); n != 1 { + t.Errorf("expected the dangerous value kept via B, got %d kept values", n) + } + _, _, _, err = policy.EnforceCreateContainerPolicyV2(ctx, cid, []string{"cmd"}, []string{}, `C:\app`, []oci.Mount{}, user, nil) + if err == nil { + t.Error("create(cmd) after registry(dangerous) should be denied: registry narrowed matches to B (command ping)") + } + }) + + // Order 2: create with A's command (narrows to [A]) then registry. A has no + // registry rule, so the dangerous value must be dropped (kept empty), but + // the request is still allowed since dropping is permissive. + t.Run("create_then_registry_drops", func(t *testing.T) { + policy := newPolicy(true) + cid := testDataGenerator.uniqueContainerID() + if err := policy.EnforceVerifiedCIMsPolicy(ctx, cid, layerHashes, mountedCim, testCIMVolumeGUID); err != nil { + t.Fatalf("mount_cims: %v", err) + } + if _, _, _, err := policy.EnforceCreateContainerPolicyV2(ctx, cid, []string{"cmd"}, []string{}, `C:\app`, []oci.Mount{}, user, nil); err != nil { + t.Fatalf("create(cmd) should be allowed as A: %v", err) + } + kept, err := policy.EnforceRegistryChangesPolicy(ctx, cid, dangerous) + if err != nil { + t.Fatalf("registry(dangerous) after create(cmd) should be allowed with dropping: %v", err) + } + if n := keptCount(t, kept); n != 0 { + t.Errorf("dangerous value should be dropped for A, got %d kept values", n) + } + }) + + // Legit path: B's command with B's registry value keeps the value. + t.Run("create_ping_then_registry_keeps", func(t *testing.T) { + policy := newPolicy(true) + cid := testDataGenerator.uniqueContainerID() + if err := policy.EnforceVerifiedCIMsPolicy(ctx, cid, layerHashes, mountedCim, testCIMVolumeGUID); err != nil { + t.Fatalf("mount_cims: %v", err) + } + if _, _, _, err := policy.EnforceCreateContainerPolicyV2(ctx, cid, []string{"ping"}, []string{}, `C:\app`, []oci.Mount{}, user, nil); err != nil { + t.Fatalf("create(ping) should be allowed as B: %v", err) + } + kept, err := policy.EnforceRegistryChangesPolicy(ctx, cid, dangerous) + if err != nil { + t.Fatalf("registry(dangerous) after create(ping) should be allowed via B: %v", err) + } + if n := keptCount(t, kept); n != 1 { + t.Errorf("dangerous value should be kept for B, got %d kept values", n) + } + }) + + // With dropping disabled, a request is only allowed if a matched container + // authorizes every requested value. After create(cmd) narrows to A (no + // registry rule), registry(dangerous) must be denied rather than dropped. + t.Run("no_dropping_create_then_registry_denied", func(t *testing.T) { + policy := newPolicy(false) + cid := testDataGenerator.uniqueContainerID() + if err := policy.EnforceVerifiedCIMsPolicy(ctx, cid, layerHashes, mountedCim, testCIMVolumeGUID); err != nil { + t.Fatalf("mount_cims: %v", err) + } + if _, _, _, err := policy.EnforceCreateContainerPolicyV2(ctx, cid, []string{"cmd"}, []string{}, `C:\app`, []oci.Mount{}, user, nil); err != nil { + t.Fatalf("create(cmd) should be allowed as A: %v", err) + } + if _, err := policy.EnforceRegistryChangesPolicy(ctx, cid, dangerous); err == nil { + t.Error("registry(dangerous) after create(cmd) should be denied without dropping (A authorizes nothing)") + } + }) + + // With dropping disabled, the legit path (B authorizes the value) is still + // allowed and keeps the value. + t.Run("no_dropping_create_ping_then_registry_keeps", func(t *testing.T) { + policy := newPolicy(false) + cid := testDataGenerator.uniqueContainerID() + if err := policy.EnforceVerifiedCIMsPolicy(ctx, cid, layerHashes, mountedCim, testCIMVolumeGUID); err != nil { + t.Fatalf("mount_cims: %v", err) + } + if _, _, _, err := policy.EnforceCreateContainerPolicyV2(ctx, cid, []string{"ping"}, []string{}, `C:\app`, []oci.Mount{}, user, nil); err != nil { + t.Fatalf("create(ping) should be allowed as B: %v", err) + } + kept, err := policy.EnforceRegistryChangesPolicy(ctx, cid, dangerous) + if err != nil { + t.Fatalf("registry(dangerous) after create(ping) should be allowed via B: %v", err) + } + if n := keptCount(t, kept); n != 1 { + t.Errorf("dangerous value should be kept for B, got %d kept values", n) + } + }) +} + +// Test_Rego_RegistryChanges_DeleteKeys_Windows verifies that delete keys flow +// through the same narrowing/dropping machinery as add values. Container B +// authorizes deleting a specific key; container A authorizes nothing. With +// dropping on, deleting B's authorized key narrows matches to B (so create(cmd) +// is then denied) or, if create(cmd) narrows to A first, the delete is dropped. +// With dropping off, the delete is only allowed against a container (B) that +// authorizes it. +// TODO: maybe delete it if it's too much. +func Test_Rego_RegistryChanges_DeleteKeys_Windows(t *testing.T) { + deleteRequest := &hcsschema.RegistryChanges{ + DeleteKeys: []hcsschema.RegistryKey{ + {Hive: "System", Name: "TestControl\\Obsolete"}, + }, + } + + keptDeleteCount := func(t *testing.T, keptRaw interface{}) int { + t.Helper() + kept, ok := keptRaw.(*hcsschema.RegistryChanges) + if !ok || kept == nil { + t.Fatalf("expected *hcsschema.RegistryChanges, got %T", keptRaw) + } + return len(kept.DeleteKeys) + } + + // mount_cims reverses the layer order, so pass layers reversed. + layerHashes := []string{"layerB", "layerA"} + mountedCim := []string{"merged"} + ctx := context.Background() + user := IDName{Name: "ContainerUser"} + + newPolicy := func(dropping bool) *regoEnforcer { + policy, err := newRegoPolicy(twoContainersSharedLayersRegistryRego(dropping), []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Fatalf("failed to create policy: %v", err) + } + return policy + } + + // Order 1: delete (kept via B) then create with A's command. The delete + // narrows matches to [B], so create with "cmd" must be denied. + t.Run("registry_delete_then_create_denied", func(t *testing.T) { + policy := newPolicy(true) + cid := testDataGenerator.uniqueContainerID() + if err := policy.EnforceVerifiedCIMsPolicy(ctx, cid, layerHashes, mountedCim, testCIMVolumeGUID); err != nil { + t.Fatalf("mount_cims: %v", err) + } + kept, err := policy.EnforceRegistryChangesPolicy(ctx, cid, deleteRequest) + if err != nil { + t.Fatalf("registry delete should be allowed (kept via B): %v", err) + } + if n := keptDeleteCount(t, kept); n != 1 { + t.Errorf("expected the delete key kept via B, got %d kept keys", n) + } + _, _, _, err = policy.EnforceCreateContainerPolicyV2(ctx, cid, []string{"cmd"}, []string{}, `C:\app`, []oci.Mount{}, user, nil) + if err == nil { + t.Error("create(cmd) after registry(delete) should be denied: registry narrowed matches to B (command ping)") + } + }) + + // Order 2: create with A's command (narrows to [A]) then delete. A has no + // registry rule, so the delete must be dropped (kept empty), but the request + // is still allowed since dropping is permissive. + t.Run("create_then_registry_delete_drops", func(t *testing.T) { + policy := newPolicy(true) + cid := testDataGenerator.uniqueContainerID() + if err := policy.EnforceVerifiedCIMsPolicy(ctx, cid, layerHashes, mountedCim, testCIMVolumeGUID); err != nil { + t.Fatalf("mount_cims: %v", err) + } + if _, _, _, err := policy.EnforceCreateContainerPolicyV2(ctx, cid, []string{"cmd"}, []string{}, `C:\app`, []oci.Mount{}, user, nil); err != nil { + t.Fatalf("create(cmd) should be allowed as A: %v", err) + } + kept, err := policy.EnforceRegistryChangesPolicy(ctx, cid, deleteRequest) + if err != nil { + t.Fatalf("registry delete after create(cmd) should be allowed with dropping: %v", err) + } + if n := keptDeleteCount(t, kept); n != 0 { + t.Errorf("delete key should be dropped for A, got %d kept keys", n) + } + }) + + // With dropping disabled, the delete is only allowed against a container + // that authorizes it. After create(cmd) narrows to A, the delete is denied; + // the legit path via B keeps it. + t.Run("no_dropping_create_then_delete_denied", func(t *testing.T) { + policy := newPolicy(false) + cid := testDataGenerator.uniqueContainerID() + if err := policy.EnforceVerifiedCIMsPolicy(ctx, cid, layerHashes, mountedCim, testCIMVolumeGUID); err != nil { + t.Fatalf("mount_cims: %v", err) + } + if _, _, _, err := policy.EnforceCreateContainerPolicyV2(ctx, cid, []string{"cmd"}, []string{}, `C:\app`, []oci.Mount{}, user, nil); err != nil { + t.Fatalf("create(cmd) should be allowed as A: %v", err) + } + if _, err := policy.EnforceRegistryChangesPolicy(ctx, cid, deleteRequest); err == nil { + t.Error("registry(delete) after create(cmd) should be denied without dropping (A authorizes nothing)") + } + }) + + t.Run("no_dropping_create_ping_then_delete_keeps", func(t *testing.T) { + policy := newPolicy(false) + cid := testDataGenerator.uniqueContainerID() + if err := policy.EnforceVerifiedCIMsPolicy(ctx, cid, layerHashes, mountedCim, testCIMVolumeGUID); err != nil { + t.Fatalf("mount_cims: %v", err) + } + if _, _, _, err := policy.EnforceCreateContainerPolicyV2(ctx, cid, []string{"ping"}, []string{}, `C:\app`, []oci.Mount{}, user, nil); err != nil { + t.Fatalf("create(ping) should be allowed as B: %v", err) + } + kept, err := policy.EnforceRegistryChangesPolicy(ctx, cid, deleteRequest) + if err != nil { + t.Fatalf("registry(delete) after create(ping) should be allowed via B: %v", err) + } + if n := keptDeleteCount(t, kept); n != 1 { + t.Errorf("delete key should be kept for B, got %d kept keys", n) + } + }) +} + // This is a no-op for windows. // substituteUVMPath substitutes mount prefix to an appropriate path inside // UVM. At policy generation time, it's impossible to tell what the sandboxID @@ -1511,6 +2133,210 @@ func substituteUVMPath(sandboxID string, m mountInternal) mountInternal { return m } +// Tests for MappedDirectory enforcement + +func Test_Rego_EnforceMappedDirectoryPolicy_OpenDoor_AllowsAll_Windows(t *testing.T) { + policy, err := newRegoPolicy( + openDoorRego, + []oci.Mount{}, + []oci.Mount{}, + testOSType, + ) + if err != nil { + t.Fatalf("failed to create policy: %v", err) + } + + ctx := context.Background() + + // Open door should allow both readonly and writable mounts, regardless of + // path, and an unmount of a never-mounted path. + if err := policy.EnforceMappedDirectoryMountPolicy(ctx, `C:\readonly`, true); err != nil { + t.Errorf("open door should allow readonly mount: %v", err) + } + if err := policy.EnforceMappedDirectoryMountPolicy(ctx, `C:\writable`, false); err != nil { + t.Errorf("open door should allow writable mount: %v", err) + } + if err := policy.EnforceMappedDirectoryUnmountPolicy(ctx, `C:\never_mounted`); err != nil { + t.Errorf("open door should allow unmount of any path: %v", err) + } +} + +func Test_Rego_EnforceMappedDirectoryPolicy_ClosedDoor_DeniesAll_Windows(t *testing.T) { + // Mirror of the open-door case: a hand-rolled policy that explicitly + // returns {"allowed": false} for both mapped-directory rules. This + // verifies the Go-side enforcer surfaces the deny decision regardless + // of input. + closedDoorRego := fmt.Sprintf(`package policy +api_version := "%s" + +mapped_directory_mount := {"allowed": false} +mapped_directory_unmount := {"allowed": false} +`, apiVersion) + + policy, err := newRegoPolicy( + closedDoorRego, + []oci.Mount{}, + []oci.Mount{}, + testOSType, + ) + if err != nil { + t.Fatalf("failed to create policy: %v", err) + } + + ctx := context.Background() + + if err := policy.EnforceMappedDirectoryMountPolicy(ctx, `C:\readonly`, true); err == nil { + t.Error("closed door should deny readonly mount") + } + if err := policy.EnforceMappedDirectoryMountPolicy(ctx, `C:\writable`, false); err == nil { + t.Error("closed door should deny writable mount") + } + if err := policy.EnforceMappedDirectoryUnmountPolicy(ctx, `C:\any_path`); err == nil { + t.Error("closed door should deny unmount of any path") + } +} + +func Test_Rego_EnforceMappedDirectoryMountPolicy_Matches_Windows(t *testing.T) { + f := func(gc *generatedWindowsConstraints) bool { + tc, err := setupWindowsMappedDirectoriesTest(gc) + if err != nil { + t.Error(err) + return false + } + + rule := selectMappedDirectoryFromConstraints(gc, testRand) + + err = tc.policy.EnforceMappedDirectoryMountPolicy(gc.ctx, rule.ContainerPath, rule.ReadOnly) + + // getting an error means something is broken + return err == nil + } + + if err := quick.Check(f, &quick.Config{MaxCount: 50, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_EnforceMappedDirectoryMountPolicy_Matches_Windows failed: %v", err) + } +} + +func Test_Rego_EnforceMappedDirectoryMountPolicy_No_Matches_Windows(t *testing.T) { + f := func(gc *generatedWindowsConstraints) bool { + tc, err := setupWindowsMappedDirectoriesTest(gc) + if err != nil { + t.Error(err) + return false + } + + fresh := generateMappedDirectory(testRand) + + err = tc.policy.EnforceMappedDirectoryMountPolicy(gc.ctx, fresh.ContainerPath, fresh.ReadOnly) + + return assertDecisionJSONContains(t, err, "no matching mapped directory in policy") + } + + if err := quick.Check(f, &quick.Config{MaxCount: 50, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_EnforceMappedDirectoryMountPolicy_No_Matches_Windows failed: %v", err) + } +} + +func Test_Rego_EnforceMappedDirectoryMountPolicy_Wrong_ReadOnly_Windows(t *testing.T) { + f := func(gc *generatedWindowsConstraints) bool { + tc, err := setupWindowsMappedDirectoriesTest(gc) + if err != nil { + t.Error(err) + return false + } + + rule := selectMappedDirectoryFromConstraints(gc, testRand) + + err = tc.policy.EnforceMappedDirectoryMountPolicy(gc.ctx, rule.ContainerPath, !rule.ReadOnly) + + return assertDecisionJSONContains(t, err, "no matching mapped directory in policy") + } + + if err := quick.Check(f, &quick.Config{MaxCount: 50, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_EnforceMappedDirectoryMountPolicy_Wrong_ReadOnly_Windows failed: %v", err) + } +} + +func Test_Rego_EnforceMappedDirectoryMountPolicy_Duplicate_Container_Path_Windows(t *testing.T) { + f := func(gc *generatedWindowsConstraints) bool { + tc, err := setupWindowsMappedDirectoriesTest(gc) + if err != nil { + t.Error(err) + return false + } + + rule := selectMappedDirectoryFromConstraints(gc, testRand) + + if err := tc.policy.EnforceMappedDirectoryMountPolicy(gc.ctx, rule.ContainerPath, rule.ReadOnly); err != nil { + t.Error("Valid mapped directory mount failed. It shouldn't have.") + return false + } + + err = tc.policy.EnforceMappedDirectoryMountPolicy(gc.ctx, rule.ContainerPath, rule.ReadOnly) + if err == nil { + t.Error("Duplicate mapped directory mount target was allowed. It shouldn't have been.") + return false + } + + return assertDecisionJSONContains(t, err, "mapped directory already mounted at path") + } + + if err := quick.Check(f, &quick.Config{MaxCount: 50, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_EnforceMappedDirectoryMountPolicy_Duplicate_Container_Path_Windows failed: %v", err) + } +} + +func Test_Rego_EnforceMappedDirectoryUnmountPolicy_Removes_Mapped_Directory_Entries_Windows(t *testing.T) { + f := func(gc *generatedWindowsConstraints) bool { + tc, err := setupWindowsMappedDirectoriesTest(gc) + if err != nil { + t.Error(err) + return false + } + + rule := selectMappedDirectoryFromConstraints(gc, testRand) + + if err := tc.policy.EnforceMappedDirectoryMountPolicy(gc.ctx, rule.ContainerPath, rule.ReadOnly); err != nil { + t.Errorf("unable to mount mapped directory: %v", err) + return false + } + if err := tc.policy.EnforceMappedDirectoryUnmountPolicy(gc.ctx, rule.ContainerPath); err != nil { + t.Errorf("unable to unmount mapped directory: %v", err) + return false + } + if err := tc.policy.EnforceMappedDirectoryMountPolicy(gc.ctx, rule.ContainerPath, rule.ReadOnly); err != nil { + t.Errorf("unable to re-mount mapped directory: %v", err) + return false + } + + return true + } + + if err := quick.Check(f, &quick.Config{MaxCount: 50, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_EnforceMappedDirectoryUnmountPolicy_Removes_Mapped_Directory_Entries_Windows failed: %v", err) + } +} + +func Test_Rego_EnforceMappedDirectoryUnmountPolicy_No_Matches_Windows(t *testing.T) { + f := func(gc *generatedWindowsConstraints) bool { + tc, err := setupWindowsMappedDirectoriesTest(gc) + if err != nil { + t.Error(err) + return false + } + + fresh := generateMappedDirectory(testRand) + + err = tc.policy.EnforceMappedDirectoryUnmountPolicy(gc.ctx, fresh.ContainerPath) + + return assertDecisionJSONContains(t, err, "no mapped directory at path to unmount") + } + + if err := quick.Check(f, &quick.Config{MaxCount: 50, Rand: testRand}); err != nil { + t.Errorf("Test_Rego_EnforceMappedDirectoryUnmountPolicy_No_Matches_Windows failed: %v", err) + } +} + // Tests for log provider enforcement // newLogProviderTestPolicy builds a Rego policy whose allowed_log_providers diff --git a/pkg/securitypolicy/securitypolicy.go b/pkg/securitypolicy/securitypolicy.go index 358b7746bd..32f9ce097b 100644 --- a/pkg/securitypolicy/securitypolicy.go +++ b/pkg/securitypolicy/securitypolicy.go @@ -70,8 +70,9 @@ type PolicyConfig struct { AllowEnvironmentVariableDropping bool `json:"allow_environment_variable_dropping" toml:"allow_environment_variable_dropping"` // AllowUnencryptedScratch is a global policy configuration that allows // all containers within a pod to be run without scratch encryption. - AllowUnencryptedScratch bool `json:"allow_unencrypted_scratch" toml:"allow_unencrypted_scratch"` - AllowCapabilityDropping bool `json:"allow_capability_dropping" toml:"allow_capability_dropping"` + AllowUnencryptedScratch bool `json:"allow_unencrypted_scratch" toml:"allow_unencrypted_scratch"` + AllowCapabilityDropping bool `json:"allow_capability_dropping" toml:"allow_capability_dropping"` + AllowRegistryChangesDropping bool `json:"allow_registry_changes_dropping" toml:"allow_registry_changes_dropping"` // AllowLogProviderDropping controls how EnforceLogProviderPolicy handles // requested ETW providers that are not on the allow-list. When false // (default, fail-close) any disallowed provider causes the entire @@ -105,6 +106,15 @@ type FragmentConfig struct { Includes []string `json:"includes" toml:"include"` } +// WindowsMappedDirectoryRule describes a single whitelisted VSMB mapped +// directory share for a Windows UVM. Mapped directories are mounted at the +// UVM level (before any container is started), so the rule is keyed only on +// the container-visible path and the read-only flag. +type WindowsMappedDirectoryRule struct { + ContainerPath string `json:"container_path" toml:"container_path"` + ReadOnly bool `json:"read_only" toml:"read_only"` +} + // AuthConfig contains toml or JSON config for registry authentication. type AuthConfig struct { Username string `json:"username" toml:"username"` @@ -341,12 +351,44 @@ type WindowsContainer struct { Layers Layers `json:"layers"` MountedCim []string `json:"mounted_cim"` WorkingDir string `json:"working_dir"` + Mounts Mounts `json:"mounts"` + RegistryChanges WindowsRegistryChanges `json:"registry_changes"` ExecProcesses []WindowsExecProcessConfig `json:"-"` Signals []guestrequest.SignalValueWCOW `json:"-"` AllowStdioAccess bool `json:"-"` User string `json:"-"` } +// WindowsRegistryChanges is the set of registry changes a Windows container is +// allowed to make. Registry changes are a Windows-only concept. +type WindowsRegistryChanges struct { + AddValues []WindowsRegistryValue `json:"add_values"` + DeleteKeys []WindowsRegistryKey `json:"delete_keys"` +} + +// WindowsRegistryKey identifies the registry key that a registry value applies +// to. +type WindowsRegistryKey struct { + Hive string `json:"hive"` + Name string `json:"name"` + Volatile bool `json:"volatile"` +} + +// WindowsRegistryValue is a single registry value a container is allowed to +// write. Type selects which of the value fields is significant, mirroring the +// registry value types understood by the runtime ("String", "ExpandedString", +// "MultiString", "DWord", "QWord", "Binary", "CustomType", "None"). +type WindowsRegistryValue struct { + Key WindowsRegistryKey `json:"key"` + Name string `json:"name"` + Type string `json:"type"` + StringValue string `json:"string_value,omitempty"` + DWordValue int32 `json:"dword_value,omitempty"` + QWordValue int32 `json:"qword_value,omitempty"` + BinaryValue string `json:"binary_value,omitempty"` + CustomType int32 `json:"custom_type,omitempty"` +} + // StringArrayMap wraps an array of strings as a string map. type StringArrayMap struct { Length int `json:"length"` diff --git a/pkg/securitypolicy/securitypolicy_internal.go b/pkg/securitypolicy/securitypolicy_internal.go index 68117d9da0..6457860e9e 100644 --- a/pkg/securitypolicy/securitypolicy_internal.go +++ b/pkg/securitypolicy/securitypolicy_internal.go @@ -20,6 +20,7 @@ type securityPolicyInternal struct { AllowEnvironmentVariableDropping bool AllowUnencryptedScratch bool AllowCapabilityDropping bool + AllowRegistryChangesDropping bool AllowLogProviderDropping bool } @@ -28,6 +29,7 @@ type securityPolicyWindowsInternal struct { Containers []*securityPolicyWindowsContainer ExternalProcesses []*externalProcess Fragments []*fragment + MappedDirectories []WindowsMappedDirectoryRule AllowPropertiesAccess bool AllowDumpStacks bool AllowRuntimeLogging bool @@ -35,6 +37,7 @@ type securityPolicyWindowsInternal struct { AllowEnvironmentVariableDropping bool AllowUnencryptedScratch bool AllowCapabilityDropping bool + AllowRegistryChangesDropping bool AllowLogProviderDropping bool } @@ -103,6 +106,7 @@ func newSecurityPolicyInternal( allowDropEnvironmentVariables bool, allowUnencryptedScratch bool, allowDropCapabilities bool, + allowRegistryChangesDropping bool, allowLogProviderDropping bool, ) (*securityPolicyInternal, error) { containersInternal, err := containersToInternal(containers) @@ -121,6 +125,7 @@ func newSecurityPolicyInternal( AllowEnvironmentVariableDropping: allowDropEnvironmentVariables, AllowUnencryptedScratch: allowUnencryptedScratch, AllowCapabilityDropping: allowDropCapabilities, + AllowRegistryChangesDropping: allowRegistryChangesDropping, AllowLogProviderDropping: allowLogProviderDropping, }, nil } @@ -219,6 +224,12 @@ type securityPolicyWindowsContainer struct { // WorkingDir is a path to container's working directory, which all the processes // will default to. WorkingDir string `json:"working_dir"` + // The set of mount constraints that the container is allowed to be created + // with. Matched against the OCI spec mounts at container creation time. + Mounts []mountInternal `json:"mounts"` + // The set of registry changes the container is allowed to make. Matched + // against the registry changes requested at container creation time. + RegistryChanges registryChangesInternal `json:"registry_changes,omitempty"` // A list of lists of commands that can be used to execute additional // processes within the container ExecProcesses []windowsContainerExecProcess `json:"exec_processes"` @@ -258,6 +269,31 @@ type mountInternal struct { Options []string `json:"options"` } +// Internal version of WindowsRegistryChanges +type registryChangesInternal struct { + AddValues []registryValueInternal `json:"add_values"` + DeleteKeys []registryKeyInternal `json:"delete_keys"` +} + +// Internal version of WindowsRegistryKey +type registryKeyInternal struct { + Hive string `json:"hive"` + Name string `json:"name"` + Volatile bool `json:"volatile"` +} + +// Internal version of WindowsRegistryValue +type registryValueInternal struct { + Key registryKeyInternal `json:"key"` + Name string `json:"name"` + Type string `json:"type"` + StringValue string `json:"string_value,omitempty"` + DWordValue int32 `json:"dword_value,omitempty"` + QWordValue int32 `json:"qword_value,omitempty"` + BinaryValue string `json:"binary_value,omitempty"` + CustomType int32 `json:"custom_type,omitempty"` +} + // Internal version of Capabilities type capabilitiesInternal struct { Bounding []string @@ -347,12 +383,19 @@ func (c *WindowsContainer) toInternal() (*securityPolicyWindowsContainer, error) execProcesses[i] = windowsContainerExecProcess(ep) } + mounts, err := c.Mounts.toInternal() + if err != nil { + return nil, err + } + return &securityPolicyWindowsContainer{ Command: command, EnvRules: envRules, Layers: layers, MountedCim: c.MountedCim, WorkingDir: c.WorkingDir, + Mounts: mounts, + RegistryChanges: c.RegistryChanges.toInternal(), ExecProcesses: execProcesses, Signals: c.Signals, AllowStdioAccess: c.AllowStdioAccess, @@ -360,6 +403,31 @@ func (c *WindowsContainer) toInternal() (*securityPolicyWindowsContainer, error) }, nil } +func (r WindowsRegistryChanges) toInternal() registryChangesInternal { + addValues := make([]registryValueInternal, len(r.AddValues)) + for i, v := range r.AddValues { + addValues[i] = registryValueInternal{ + Key: registryKeyInternal{ + Hive: v.Key.Hive, + Name: v.Key.Name, + Volatile: v.Key.Volatile, + }, + Name: v.Name, + Type: v.Type, + StringValue: v.StringValue, + DWordValue: v.DWordValue, + QWordValue: v.QWordValue, + BinaryValue: v.BinaryValue, + CustomType: v.CustomType, + } + } + deleteKeys := make([]registryKeyInternal, len(r.DeleteKeys)) + for i, k := range r.DeleteKeys { + deleteKeys[i] = registryKeyInternal(k) + } + return registryChangesInternal{AddValues: addValues, DeleteKeys: deleteKeys} +} + func (c CommandArgs) toInternal() ([]string, error) { return stringMapToStringArray(c.Elements) } diff --git a/pkg/securitypolicy/securitypolicy_marshal.go b/pkg/securitypolicy/securitypolicy_marshal.go index 224a873011..92b79c346d 100644 --- a/pkg/securitypolicy/securitypolicy_marshal.go +++ b/pkg/securitypolicy/securitypolicy_marshal.go @@ -50,6 +50,7 @@ func rejectJSONMarshaller( _ bool, _ bool, _ bool, + _ bool, ) (string, error) { return "", fmt.Errorf("JSON policy output is no longer supported; use the %q marshaller", regoMarshaller) } @@ -92,6 +93,7 @@ type OSAwareMarshalFunc func( allowEnvironmentVariableDropping bool, allowUnencryptedScratch bool, allowCapabilityDropping bool, + allowRegistryChangesDropping bool, allowLogProviderDropping bool, ) (string, error) @@ -110,6 +112,7 @@ func osAwareMarshalRego( allowEnvironmentVariableDropping bool, allowUnencryptedScratch bool, allowCapabilityDropping bool, + allowRegistryChangesDropping bool, allowLogProviderDropping bool, ) (string, error) { if allowAll { @@ -127,7 +130,7 @@ func osAwareMarshalRego( return marshalRego(allowAll, linuxContainers, externalProcesses, fragments, allowPropertiesAccess, allowDumpStacks, allowRuntimeLogging, allowHostNetwork, allowEnvironmentVariableDropping, allowUnencryptedScratch, allowCapabilityDropping, - allowLogProviderDropping) + allowRegistryChangesDropping, allowLogProviderDropping) case "windows": if len(linuxContainers) > 0 { @@ -136,7 +139,7 @@ func osAwareMarshalRego( return marshalWindowsRego(allowAll, windowsContainers, externalProcesses, fragments, allowPropertiesAccess, allowDumpStacks, allowRuntimeLogging, allowHostNetwork, allowEnvironmentVariableDropping, allowUnencryptedScratch, allowCapabilityDropping, - allowLogProviderDropping) + allowRegistryChangesDropping, allowLogProviderDropping) default: return "", fmt.Errorf("unsupported OS type: %s", osType) @@ -156,6 +159,7 @@ func marshalWindowsRego( allowEnvironmentVariableDropping bool, allowUnencryptedScratch bool, allowCapabilityDropping bool, + allowRegistryChangesDropping bool, allowLogProviderDropping bool, ) (string, error) { if allowAll { @@ -182,6 +186,7 @@ func marshalWindowsRego( AllowEnvironmentVariableDropping: allowEnvironmentVariableDropping, AllowUnencryptedScratch: allowUnencryptedScratch, AllowCapabilityDropping: allowCapabilityDropping, + AllowRegistryChangesDropping: allowRegistryChangesDropping, AllowLogProviderDropping: allowLogProviderDropping, } @@ -200,6 +205,7 @@ func marshalRego( allowEnvironmentVariableDropping bool, allowUnencryptedScratch bool, allowCapabilityDropping bool, + allowRegistryChangesDropping bool, allowLogProviderDropping bool, ) (string, error) { if allowAll { @@ -221,6 +227,7 @@ func marshalRego( allowEnvironmentVariableDropping, allowUnencryptedScratch, allowCapabilityDropping, + allowRegistryChangesDropping, allowLogProviderDropping, ) if err != nil { @@ -272,6 +279,7 @@ func MarshalPolicy( allowEnvironmentVariableDropping bool, allowUnencryptedScratch bool, allowCapbilitiesDropping bool, + allowRegistryChangesDropping bool, allowLogProviderDropping bool, ) (string, error) { if marshaller == "" { @@ -295,6 +303,7 @@ func MarshalPolicy( allowEnvironmentVariableDropping, allowUnencryptedScratch, allowCapbilitiesDropping, + allowRegistryChangesDropping, allowLogProviderDropping, ) } @@ -314,6 +323,7 @@ func MarshalWindowsPolicy( allowEnvironmentVariableDropping bool, allowUnencryptedScratch bool, allowCapabilitiesDropping bool, + allowRegistryChangesDropping bool, allowLogProviderDropping bool, ) (string, error) { if marshaller == "" { @@ -342,6 +352,7 @@ func MarshalWindowsPolicy( allowEnvironmentVariableDropping, allowUnencryptedScratch, allowCapabilitiesDropping, + allowRegistryChangesDropping, allowLogProviderDropping, ) } @@ -524,6 +535,15 @@ func (m mountInternal) marshalRego() string { }{m.Destination, json.RawMessage(options), m.Source, m.Type}) } +// escapeRegoString escapes a Go string so it is a valid double-quoted Rego +// string literal. This matters for Windows registry keys and values, which +// contain backslashes that would otherwise be interpreted as escape sequences. +func escapeRegoString(s string) string { + s = strings.ReplaceAll(s, `\`, `\\`) + s = strings.ReplaceAll(s, `"`, `\"`) + return s +} + func writeMounts(builder *strings.Builder, mounts []mountInternal, indent string) { values := make([]string, len(mounts)) for i, mount := range mounts { @@ -533,6 +553,51 @@ func writeMounts(builder *strings.Builder, mounts []mountInternal, indent string writeLine(builder, `%s"mounts": [%s],`, indent, strings.Join(values, ",")) } +func (k registryKeyInternal) marshalRego() string { + return fmt.Sprintf(`{"hive": "%s", "name": "%s", "volatile": %t}`, + escapeRegoString(k.Hive), escapeRegoString(k.Name), k.Volatile) +} + +func (v registryValueInternal) marshalRego() string { + fields := []string{ + fmt.Sprintf(`"key": %s`, v.Key.marshalRego()), + fmt.Sprintf(`"name": "%s"`, escapeRegoString(v.Name)), + fmt.Sprintf(`"type": "%s"`, escapeRegoString(v.Type)), + } + // Type selects which value field is significant; emit only that one so the + // policy value matches the shape registry_value_matches compares against. + switch v.Type { + case "String", "ExpandedString", "MultiString": + fields = append(fields, fmt.Sprintf(`"string_value": "%s"`, escapeRegoString(v.StringValue))) + case "DWord": + fields = append(fields, fmt.Sprintf(`"dword_value": %d`, v.DWordValue)) + case "QWord": + fields = append(fields, fmt.Sprintf(`"qword_value": %d`, v.QWordValue)) + case "Binary": + fields = append(fields, fmt.Sprintf(`"binary_value": "%s"`, escapeRegoString(v.BinaryValue))) + case "CustomType": + fields = append(fields, fmt.Sprintf(`"custom_type": %d`, v.CustomType)) + fields = append(fields, fmt.Sprintf(`"binary_value": "%s"`, escapeRegoString(v.BinaryValue))) + case "None": + // No value to compare, just key, name and type. + } + return fmt.Sprintf("{%s}", strings.Join(fields, ", ")) +} + +func writeRegistryChanges(builder *strings.Builder, registryChanges registryChangesInternal, indent string) { + addValues := make([]string, len(registryChanges.AddValues)) + for i, value := range registryChanges.AddValues { + addValues[i] = value.marshalRego() + } + deleteKeys := make([]string, len(registryChanges.DeleteKeys)) + for i, key := range registryChanges.DeleteKeys { + deleteKeys[i] = key.marshalRego() + } + + writeLine(builder, `%s"registry_changes": {"add_values": [%s], "delete_keys": [%s]},`, + indent, strings.Join(addValues, ", "), strings.Join(deleteKeys, ", ")) +} + // Windows-specific marshal functions func writeWindowsSignals(builder *strings.Builder, signals []guestrequest.SignalValueWCOW, indent string) { signalsArray := make([]string, len(signals)) @@ -575,6 +640,10 @@ func writeWindowsContainer(builder *strings.Builder, container *securityPolicyWi writeEnvRules(builder, container.EnvRules, indent+indentUsing) writeLayers(builder, container.Layers, indent+indentUsing) writeMountedCim(builder, container.MountedCim, indent+indentUsing) + writeMounts(builder, container.Mounts, indent+indentUsing) + if len(container.RegistryChanges.AddValues) > 0 || len(container.RegistryChanges.DeleteKeys) > 0 { + writeRegistryChanges(builder, container.RegistryChanges, indent+indentUsing) + } writeWindowsExecProcesses(builder, container.ExecProcesses, indent+indentUsing) writeWindowsSignals(builder, container.Signals, indent+indentUsing) writeWindowsUser(builder, container.User, indent+indentUsing) @@ -726,6 +795,20 @@ func addFragments(builder *strings.Builder, fragments []*fragment) { writeLine(builder, "]") } +func addWindowsMappedDirectories(builder *strings.Builder, rules []WindowsMappedDirectoryRule) { + if len(rules) == 0 { + return + } + + writeLine(builder, "mapped_directories := [") + + for _, rule := range rules { + writeLine(builder, `%s{"container_path": %q, "read_only": %t},`, indentUsing, rule.ContainerPath, rule.ReadOnly) + } + + writeLine(builder, "]") +} + func (p securityPolicyInternal) marshalRego() string { builder := new(strings.Builder) addFragments(builder, p.Fragments) @@ -738,6 +821,7 @@ func (p securityPolicyInternal) marshalRego() string { writeLine(builder, "allow_environment_variable_dropping := %t", p.AllowEnvironmentVariableDropping) writeLine(builder, "allow_unencrypted_scratch := %t", p.AllowUnencryptedScratch) writeLine(builder, "allow_capability_dropping := %t", p.AllowCapabilityDropping) + writeLine(builder, "allow_registry_changes_dropping := %t", p.AllowRegistryChangesDropping) writeLine(builder, "allow_log_provider_dropping := %t", p.AllowLogProviderDropping) result := strings.Replace(policyRegoTemplate, "@@OBJECTS@@", builder.String(), 1) result = strings.Replace(result, "@@API_VERSION@@", apiVersion, 1) @@ -759,6 +843,7 @@ func (p securityPolicyWindowsInternal) marshalWindowsRego() string { addFragments(builder, p.Fragments) addWindowsContainers(builder, p.Containers) addExternalProcesses(builder, p.ExternalProcesses) + addWindowsMappedDirectories(builder, p.MappedDirectories) writeLine(builder, `allow_properties_access := %t`, p.AllowPropertiesAccess) writeLine(builder, `allow_dump_stacks := %t`, p.AllowDumpStacks) writeLine(builder, `allow_runtime_logging := %t`, p.AllowRuntimeLogging) @@ -766,6 +851,7 @@ func (p securityPolicyWindowsInternal) marshalWindowsRego() string { writeLine(builder, "allow_environment_variable_dropping := %t", p.AllowEnvironmentVariableDropping) writeLine(builder, "allow_unencrypted_scratch := %t", p.AllowUnencryptedScratch) writeLine(builder, "allow_capability_dropping := %t", p.AllowCapabilityDropping) + writeLine(builder, "allow_registry_changes_dropping := %t", p.AllowRegistryChangesDropping) writeLine(builder, "allow_log_provider_dropping := %t", p.AllowLogProviderDropping) result := strings.Replace(policyRegoTemplate, "@@OBJECTS@@", builder.String(), 1) result = strings.Replace(result, "@@API_VERSION@@", apiVersion, 1) diff --git a/pkg/securitypolicy/securitypolicyenforcer.go b/pkg/securitypolicy/securitypolicyenforcer.go index 31e1ecd74a..7ac9be4a18 100644 --- a/pkg/securitypolicy/securitypolicyenforcer.go +++ b/pkg/securitypolicy/securitypolicyenforcer.go @@ -162,9 +162,12 @@ type SecurityPolicyEnforcer interface { LoadTransparencyTrustList(ctx context.Context, opts LoadTransparencyTrustListOptions) error EnforceScratchMountPolicy(ctx context.Context, scratchPath string, encrypted bool) (err error) EnforceScratchUnmountPolicy(ctx context.Context, scratchPath string) (err error) + EnforceMappedDirectoryMountPolicy(ctx context.Context, containerPath string, readOnly bool) (err error) + EnforceMappedDirectoryUnmountPolicy(ctx context.Context, containerPath string) (err error) GetUserInfo(spec *oci.Process, rootPath string) (IDName, []IDName, string, error) - EnforceVerifiedCIMsPolicy(ctx context.Context, containerID string, layerHashes []string, mountedCim []string) (err error) - EnforceRegistryChangesPolicy(ctx context.Context, containerID string, registryValues interface{}) error + EnforceVerifiedCIMsPolicy(ctx context.Context, containerID string, layerHashes []string, mountedCim []string, volumeGUID string) (err error) + EnforceCIMUnmountPolicy(ctx context.Context, volumeGUID string) (err error) + EnforceRegistryChangesPolicy(ctx context.Context, containerID string, registryChanges interface{}) (interface{}, error) // EnforceLogProviderPolicy validates a batch of requested ETW provider // names against the policy's allowed_log_providers list. It returns the // subset of provider names that the caller should forward to the inbox @@ -374,18 +377,30 @@ func (OpenDoorSecurityPolicyEnforcer) EnforceScratchUnmountPolicy(context.Contex return nil } +func (OpenDoorSecurityPolicyEnforcer) EnforceMappedDirectoryMountPolicy(context.Context, string, bool) error { + return nil +} + +func (OpenDoorSecurityPolicyEnforcer) EnforceMappedDirectoryUnmountPolicy(context.Context, string) error { + return nil +} + func (OpenDoorSecurityPolicyEnforcer) GetUserInfo(spec *oci.Process, rootPath string) (IDName, []IDName, string, error) { return IDName{}, nil, "", nil } -func (OpenDoorSecurityPolicyEnforcer) EnforceVerifiedCIMsPolicy(ctx context.Context, containerID string, layerHashes []string, mountedCim []string) error { +func (OpenDoorSecurityPolicyEnforcer) EnforceVerifiedCIMsPolicy(ctx context.Context, containerID string, layerHashes []string, mountedCim []string, volumeGUID string) error { return nil } -func (OpenDoorSecurityPolicyEnforcer) EnforceRegistryChangesPolicy(ctx context.Context, containerID string, registryValues interface{}) error { +func (OpenDoorSecurityPolicyEnforcer) EnforceCIMUnmountPolicy(ctx context.Context, volumeGUID string) error { return nil } +func (OpenDoorSecurityPolicyEnforcer) EnforceRegistryChangesPolicy(ctx context.Context, containerID string, registryChanges interface{}) (interface{}, error) { + return registryChanges, nil +} + func (OpenDoorSecurityPolicyEnforcer) EnforceLogProviderPolicy(_ context.Context, providerNames []string) ([]string, error) { return providerNames, nil } @@ -527,16 +542,28 @@ func (ClosedDoorSecurityPolicyEnforcer) EnforceScratchUnmountPolicy(context.Cont return errors.New("unmounting scratch is denied by the policy") } +func (ClosedDoorSecurityPolicyEnforcer) EnforceMappedDirectoryMountPolicy(context.Context, string, bool) error { + return errors.New("mounting mapped directory is denied by the policy") +} + +func (ClosedDoorSecurityPolicyEnforcer) EnforceMappedDirectoryUnmountPolicy(context.Context, string) error { + return errors.New("unmounting mapped directory is denied by the policy") +} + func (ClosedDoorSecurityPolicyEnforcer) GetUserInfo(spec *oci.Process, rootPath string) (IDName, []IDName, string, error) { return IDName{}, nil, "", nil } -func (ClosedDoorSecurityPolicyEnforcer) EnforceVerifiedCIMsPolicy(ctx context.Context, containerID string, layerHashes []string, mountedCim []string) error { +func (ClosedDoorSecurityPolicyEnforcer) EnforceVerifiedCIMsPolicy(ctx context.Context, containerID string, layerHashes []string, mountedCim []string, volumeGUID string) error { return nil } -func (ClosedDoorSecurityPolicyEnforcer) EnforceRegistryChangesPolicy(ctx context.Context, containerID string, registryValues interface{}) error { - return errors.New("registry changes are denied by policy") +func (ClosedDoorSecurityPolicyEnforcer) EnforceCIMUnmountPolicy(ctx context.Context, volumeGUID string) error { + return errors.New("CIM unmounting is denied by policy") +} + +func (ClosedDoorSecurityPolicyEnforcer) EnforceRegistryChangesPolicy(ctx context.Context, containerID string, registryChanges interface{}) (interface{}, error) { + return nil, errors.New("registry changes are denied by policy") } func (ClosedDoorSecurityPolicyEnforcer) EnforceLogProviderPolicy(context.Context, []string) ([]string, error) { diff --git a/pkg/securitypolicy/securitypolicyenforcer_rego.go b/pkg/securitypolicy/securitypolicyenforcer_rego.go index 26763ef7da..d3485bd110 100644 --- a/pkg/securitypolicy/securitypolicyenforcer_rego.go +++ b/pkg/securitypolicy/securitypolicyenforcer_rego.go @@ -846,6 +846,7 @@ func (policy *regoEnforcer) EnforceCreateContainerPolicyV2( } input = inputData{ + "mounts": appendMountData([]interface{}{}, mounts), "containerID": containerID, "argList": argList, "envList": envList, @@ -1524,26 +1525,54 @@ func (policy *regoEnforcer) EnforceScratchUnmountPolicy(ctx context.Context, scr return nil } -func (policy *regoEnforcer) EnforceVerifiedCIMsPolicy(ctx context.Context, containerID string, layerHashes []string, mountedCim []string) error { +func (policy *regoEnforcer) EnforceMappedDirectoryMountPolicy(ctx context.Context, containerPath string, readOnly bool) error { + input := inputData{ + "containerPath": containerPath, + "readOnly": readOnly, + } + _, err := policy.enforce(ctx, "mapped_directory_mount", input) + return err +} + +func (policy *regoEnforcer) EnforceMappedDirectoryUnmountPolicy(ctx context.Context, containerPath string) error { + input := inputData{ + "unmountTarget": containerPath, + } + _, err := policy.enforce(ctx, "mapped_directory_unmount", input) + return err +} + +func (policy *regoEnforcer) EnforceVerifiedCIMsPolicy(ctx context.Context, containerID string, layerHashes []string, mountedCim []string, volumeGUID string) error { log.G(ctx).Tracef("Enforcing verified cims in securitypolicy pkg %+v", layerHashes) input := inputData{ "containerID": containerID, "layerHashes": layerHashes, "mountedCim": mountedCim, + "volumeGUID": volumeGUID, } _, err := policy.enforce(ctx, "mount_cims", input) return err } -func (policy *regoEnforcer) EnforceRegistryChangesPolicy(ctx context.Context, containerID string, registryValues interface{}) error { +func (policy *regoEnforcer) EnforceCIMUnmountPolicy(ctx context.Context, volumeGUID string) error { + log.G(ctx).Trace("Enforcing CIM unmount policy") + input := inputData{ + "volumeGUID": volumeGUID, + } + + _, err := policy.enforce(ctx, "unmount_cims", input) + return err +} + +func (policy *regoEnforcer) EnforceRegistryChangesPolicy(ctx context.Context, containerID string, registryChanges interface{}) (interface{}, error) { log.G(ctx).Trace("Enforcing registry changes policy") // Import the schema type for proper conversion - regChanges, ok := registryValues.(*hcsschema.RegistryChanges) + regChanges, ok := registryChanges.(*hcsschema.RegistryChanges) if !ok { log.G(ctx).Warn("Input registry values are not of expected type") - return errors.New("invalid registry values type") + return nil, errors.New("invalid registry values type") } input := inputData{ @@ -1551,8 +1580,36 @@ func (policy *regoEnforcer) EnforceRegistryChangesPolicy(ctx context.Context, co "registryChanges": regChanges, } - _, err := policy.enforce(ctx, "registry_changes", input) - return err + result, err := policy.enforce(ctx, "registry_changes", input) + if err != nil { + return nil, err + } + + // The policy uses dropping semantics: it authorizes a subset of the + // requested changes and returns the kept add values and delete keys in + // "add_values_to_keep" / "delete_keys_to_keep". Round-trip them back into + // the schema type so the caller applies only the kept changes. + kept := &hcsschema.RegistryChanges{} + if raw, verr := result.Value("add_values_to_keep"); verr == nil && raw != nil { + buf, merr := json.Marshal(raw) + if merr != nil { + return nil, fmt.Errorf("failed to marshal kept registry values: %w", merr) + } + if uerr := json.Unmarshal(buf, &kept.AddValues); uerr != nil { + return nil, fmt.Errorf("failed to unmarshal kept registry values: %w", uerr) + } + } + if raw, verr := result.Value("delete_keys_to_keep"); verr == nil && raw != nil { + buf, merr := json.Marshal(raw) + if merr != nil { + return nil, fmt.Errorf("failed to marshal kept registry delete keys: %w", merr) + } + if uerr := json.Unmarshal(buf, &kept.DeleteKeys); uerr != nil { + return nil, fmt.Errorf("failed to unmarshal kept registry delete keys: %w", uerr) + } + } + + return kept, nil } func (policy *regoEnforcer) EnforceLogProviderPolicy(ctx context.Context, providerNames []string) ([]string, error) { diff --git a/pkg/securitypolicy/windows_tooling_test.go b/pkg/securitypolicy/windows_tooling_test.go index f9d5fd740f..9f4a397b8c 100644 --- a/pkg/securitypolicy/windows_tooling_test.go +++ b/pkg/securitypolicy/windows_tooling_test.go @@ -61,7 +61,7 @@ func TestMarshalWindowsPolicy(t *testing.T) { t.Fatal(err) } - policy, err := MarshalWindowsPolicy("rego", false, []*WindowsContainer{container}, nil, nil, false, false, false, false, false, false, false, false) + policy, err := MarshalWindowsPolicy("rego", false, []*WindowsContainer{container}, nil, nil, false, false, false, false, false, false, false, false, false) if err != nil { t.Fatal(err) } @@ -73,14 +73,14 @@ func TestMarshalWindowsPolicy(t *testing.T) { } func TestMarshalWindowsPolicyRejectsJSON(t *testing.T) { - _, err := MarshalWindowsPolicy("json", false, nil, nil, nil, false, false, false, false, false, false, false, false) + _, err := MarshalWindowsPolicy("json", false, nil, nil, nil, false, false, false, false, false, false, false, false, false) if err == nil { t.Fatal("expected JSON marshalling to be rejected for Windows policies") } } func TestMarshalPolicyRejectsJSON(t *testing.T) { - _, err := MarshalPolicy("json", false, nil, nil, nil, false, false, false, false, false, false, false, false) + _, err := MarshalPolicy("json", false, nil, nil, nil, false, false, false, false, false, false, false, false, false) if err == nil { t.Fatal("expected JSON marshalling to be rejected") } @@ -102,7 +102,7 @@ func TestMarshalWindowsPolicyEscapesBackslashes(t *testing.T) { t.Fatal(err) } - policy, err := MarshalWindowsPolicy("rego", false, []*WindowsContainer{container}, nil, nil, false, false, false, false, false, false, false, false) + policy, err := MarshalWindowsPolicy("rego", false, []*WindowsContainer{container}, nil, nil, false, false, false, false, false, false, false, false, false) if err != nil { t.Fatal(err) } diff --git a/test/pkg/securitypolicy/policy.go b/test/pkg/securitypolicy/policy.go index 32bff9fc68..73fcf85b22 100644 --- a/test/pkg/securitypolicy/policy.go +++ b/test/pkg/securitypolicy/policy.go @@ -66,6 +66,7 @@ func PolicyWithOpts(tb testing.TB, policyType string, pOpts ...securitypolicy.Po config.AllowEnvironmentVariableDropping, config.AllowUnencryptedScratch, config.AllowCapabilityDropping, + config.AllowRegistryChangesDropping, config.AllowLogProviderDropping, ) if err != nil { @@ -123,6 +124,7 @@ func WindowsPolicyWithOpts(tb testing.TB, policyType string, pOpts ...securitypo config.AllowEnvironmentVariableDropping, config.AllowUnencryptedScratch, config.AllowCapabilityDropping, + config.AllowRegistryChangesDropping, config.AllowLogProviderDropping, ) if err != nil {