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 1cf1ebff39..145e0b20f7 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" @@ -21,15 +22,14 @@ import ( "github.com/Microsoft/hcsshim/internal/gcs/prot" 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" ) @@ -57,6 +57,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 @@ -67,7 +73,6 @@ func (b *Bridge) createContainer(req *request) (err error) { // containerConfig can be of type uvnConfig or hcsschema.HostedSystem or guestresource.CWCOWHostedSystem var ( uvmConfig prot.UvmConfig - hostedSystemConfig hcsschema.HostedSystem cwcowHostedSystemConfig guestresource.CWCOWHostedSystem ) if err = commonutils.UnmarshalJSONWithHresult(containerConfig, &uvmConfig); err == nil && @@ -75,11 +80,6 @@ func (b *Bridge) createContainer(req *request) (err error) { systemType := uvmConfig.SystemType timeZoneInformation := uvmConfig.TimeZoneInformation log.G(ctx).Tracef("createContainer: uvmConfig: {systemType: %v, timeZoneInformation: %v}}", systemType, timeZoneInformation) - } else if err = commonutils.UnmarshalJSONWithHresult(containerConfig, &hostedSystemConfig); err == nil && - hostedSystemConfig.SchemaVersion != nil && hostedSystemConfig.Container != nil { - schemaVersion := hostedSystemConfig.SchemaVersion - container := hostedSystemConfig.Container - log.G(ctx).Tracef("rpcCreate: HostedSystemConfig: {schemaVersion: %v, container: %v}}", schemaVersion, container) } else if err = commonutils.UnmarshalJSONWithHresult(containerConfig, &cwcowHostedSystemConfig); err == nil && cwcowHostedSystemConfig.Spec.Version != "" && cwcowHostedSystemConfig.CWCOWHostedSystem.Container != nil { cwcowHostedSystem := cwcowHostedSystemConfig.CWCOWHostedSystem @@ -87,56 +87,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") - err := b.hostState.securityOptions.PolicyEnforcer.EnforceRegistryChangesPolicy(ctx, containerID, nonDefaultChanges) + // 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) + + // 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) + } + commandLine := len(spec.Process.Args) > 0 c := &Container{ id: containerID, @@ -144,11 +248,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() { @@ -159,25 +264,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 { @@ -219,6 +342,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 @@ -243,6 +564,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. @@ -276,11 +648,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) @@ -344,7 +788,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), @@ -353,6 +797,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 @@ -366,7 +826,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 } @@ -376,7 +839,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, @@ -389,6 +852,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 @@ -558,15 +1079,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 @@ -693,31 +1235,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) @@ -821,46 +1426,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{ @@ -874,9 +1502,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 @@ -933,6 +1572,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)) @@ -946,52 +1598,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 @@ -1014,6 +1696,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 6de3a0a605..f1816fc3c0 100644 --- a/internal/gcs-sidecar/handlers_test.go +++ b/internal/gcs-sidecar/handlers_test.go @@ -6,15 +6,20 @@ package bridge import ( "context" "encoding/json" + "errors" "io" + "reflect" + "strings" "testing" "time" "github.com/Microsoft/go-winio/pkg/guid" "github.com/Microsoft/hcsshim/internal/gcs/prot" + hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2" "github.com/Microsoft/hcsshim/internal/protocol/guestrequest" "github.com/Microsoft/hcsshim/internal/protocol/guestresource" "github.com/Microsoft/hcsshim/pkg/securitypolicy" + oci "github.com/opencontainers/runtime-spec/specs-go" ) // buildModifySettingsRequest creates a serialized ModifySettings request message @@ -48,6 +53,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), @@ -55,6 +61,132 @@ func newTestBridge(enforcer securitypolicy.SecurityPolicyEnforcer) *Bridge { } } +// 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 @@ -327,3 +459,779 @@ func TestModifySettings_PolicyFragment_TypeAssertionFailure(t *testing.T) { t.Fatal("expected error for empty fragment, got nil") } } + +// 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") + } +} 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 0609966a0a..903e98a973 100644 --- a/internal/guest/runtime/hcsv2/uvm.go +++ b/internal/guest/runtime/hcsv2/uvm.go @@ -738,7 +738,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/tools/securitypolicy/main.go b/internal/tools/securitypolicy/main.go index d5ad89e28b..1a8306c575 100644 --- a/internal/tools/securitypolicy/main.go +++ b/internal/tools/securitypolicy/main.go @@ -68,6 +68,7 @@ func main() { config.AllowEnvironmentVariableDropping, config.AllowUnencryptedScratch, config.AllowCapabilityDropping, + config.AllowRegistryChangesDropping, ) } if err != nil { diff --git a/pkg/securitypolicy/api.rego b/pkg/securitypolicy/api.rego index d1fca363be..06eec6592e 100644 --- a/pkg/securitypolicy/api.rego +++ b/pkg/securitypolicy/api.rego @@ -26,5 +26,8 @@ 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}, "load_transparency_trust_list": {"introducedVersion": "0.12.0", "default_results": {"allowed": false}, "use_framework": false}, } diff --git a/pkg/securitypolicy/framework.rego b/pkg/securitypolicy/framework.rego index ef47564d83..dd5b951c42 100644 --- a/pkg/securitypolicy/framework.rego +++ b/pkg/securitypolicy/framework.rego @@ -184,7 +184,7 @@ candidate_containers := containers { default mount_cims := {"allowed": false} -mount_cims := {"metadata": [addMatches], "allowed": true} { +mount_cims := {"metadata": [addMatches, addCimVolume], "allowed": true} { not overlay_exists containers := [container | @@ -200,6 +200,36 @@ mount_cims := {"metadata": [addMatches], "allowed": true} { "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) { + data.metadata.mountedCimVolumes[volumeGUID] +} + +default unmount_cims := {"allowed": false} + +unmount_cims := {"metadata": [removeCimVolume], "allowed": true} { + 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) { constraint == source } -mountConstraint_ok(constraint, mount) { - 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) { count(mount.options) == count(constraint.options) every option in mount.options { some constraintOption in constraint.options @@ -846,6 +873,82 @@ mountConstraint_ok(constraint, mount) { } } +# 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) { + 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) { + mount.type == "" +} + +windows_mount_type_ok(mount) { + mount.type == "bind" +} + +mountConstraint_ok(constraint, mount) { + 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) { + 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) { + 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) { some constraint in mounts mountConstraint_ok(constraint, mount) @@ -896,16 +999,13 @@ mount_ok(mounts, allow_elevated, mount) { "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) { - is_linux every mount in input.mounts { mount_ok(mounts, allow_elevated, mount) } } -mountList_ok(mounts, allow_elevated) { - # no-op for windows - is_windows -} is_linux { data.metadata.operatingsystem[ostype] == "linux" @@ -1685,6 +1785,62 @@ scratch_unmount := {"metadata": [remove_scratch_mount], "allowed": true} { } } +# Mapped directory (VSMB share) validation for Windows containers +default mapped_directory_mount := {"allowed": false} + +mapped_directory_mounted(target) { + 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 { + 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 { + 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} { + 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} { + mapped_directory_mounted(input.unmountTarget) + remove_mapped_dir := { + "name": "mapped_directories", + "action": "remove", + "key": input.unmountTarget, + } +} + # Registry changes validation default registry_changes := {"allowed": false} @@ -1771,27 +1927,123 @@ registry_value_matches(policy_value, input_value) { 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 { + 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} { - 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) { + 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) { + 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 { + 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 { + 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 { + 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) { + 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, +} { + 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, } } @@ -1965,6 +2217,37 @@ errors["no matching containers for overlay"] { not overlay_matches } +default cim_matches := false + +cim_matches { + some container in candidate_containers + layerHashes_ok(container.layers) + input.mountedCim == container.mounted_cim +} + +errors["the container image layers have already been matched by a prior mount_cims"] { + input.rule == "mount_cims" + overlay_exists +} + +errors["no matching containers for CIM mount"] { + 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[cimVersionError] { + 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 { @@ -2169,12 +2452,18 @@ errors["invalid working directory"] { } mount_matches(mount) { + is_linux some container in data.metadata.matches[input.containerID] mount_ok(container.mounts, container.allow_elevated, mount) } +mount_matches(mount) { + is_windows + some container in data.metadata.matches[input.containerID] + mount_ok(container.mounts, false, mount) +} + errors[mountError] { - is_linux input.rule == "create_container" bad_mounts := [mount.destination | mount := input.mounts[_] @@ -2398,6 +2687,31 @@ errors["no scratch at path to unmount"] { not scratch_mounted(input.unmountTarget) } +errors["no CIM volume at GUID to unmount"] { + input.rule == "unmount_cims" + not cim_volume_mounted(input.volumeGUID) +} + +errors["mapped directory already mounted at path"] { + input.rule == "mapped_directory_mount" + mapped_directory_mounted(input.containerPath) +} + +errors["no matching mapped directory in policy"] { + input.rule == "mapped_directory_mount" + not mapped_directory_ok +} + +errors["no mapped directory at path to unmount"] { + input.rule == "mapped_directory_unmount" + not mapped_directory_mounted(input.unmountTarget) +} + +errors["invalid registry changes"] { + input.rule == "registry_changes" + not registry_changes.allowed +} + errors[framework_version_error] { policy_framework_version == null framework_version_error := concat(" ", ["framework_version is missing. Current version:", version]) @@ -2489,6 +2803,7 @@ errors["containers only distinguishable by allow_stdio_access"] { user_ok(container.user) workingDirectory_ok(container.working_dir) command_ok(container.command) + mountList_ok(container.mounts, false) ] count(possible_after_initial_containers) > 0 @@ -2775,6 +3090,7 @@ check_container(raw_container, framework_version) := container { "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), } } @@ -2844,6 +3160,16 @@ check_signals(raw_container, framework_version) := signals { signals := array.concat(raw_container.signals, [9, 15]) } +check_mounted_cim(raw_container, framework_version) := mounted_cim { + 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 { + semver.compare(framework_version, "0.5.0") < 0 + mounted_cim := [] +} + check_external_process(raw_process, framework_version) := process { semver.compare(framework_version, version) == 0 process := raw_process @@ -2909,6 +3235,10 @@ allow_capability_dropping := flag { 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 c2aec0f039..33bab34364 100644 --- a/pkg/securitypolicy/open_door.rego +++ b/pkg/securitypolicy/open_door.rego @@ -25,4 +25,7 @@ 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} load_transparency_trust_list := {"allowed": true} diff --git a/pkg/securitypolicy/opts.go b/pkg/securitypolicy/opts.go index a11685abc4..1b6c46a740 100644 --- a/pkg/securitypolicy/opts.go +++ b/pkg/securitypolicy/opts.go @@ -122,6 +122,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 6bdd1b3aa2..c404686181 100644 --- a/pkg/securitypolicy/policy.rego +++ b/pkg/securitypolicy/policy.rego @@ -28,5 +28,8 @@ 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 load_transparency_trust_list := data.framework.load_transparency_trust_list reason := data.framework.reason diff --git a/pkg/securitypolicy/rego_utils_test.go b/pkg/securitypolicy/rego_utils_test.go index 64f0ff2574..de2b2891f5 100644 --- a/pkg/securitypolicy/rego_utils_test.go +++ b/pkg/securitypolicy/rego_utils_test.go @@ -65,6 +65,8 @@ const ( maxGeneratedMountOptionLength = 32 maxGeneratedExecProcesses = 4 maxGeneratedWorkingDirLength = 128 + maxGeneratedMappedDirectories = 8 + maxGeneratedMappedDirectoryPathLength = 64 maxSignalNumber = 64 maxGeneratedNameLength = 8 maxGeneratedGroupNames = 4 @@ -723,6 +725,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 @@ -1878,6 +1901,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, @@ -2017,6 +2041,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, @@ -2028,6 +2056,7 @@ func (constraints *generatedConstraints) toPolicy() *securityPolicyInternal { AllowEnvironmentVariableDropping: constraints.allowEnvironmentVariableDropping, AllowUnencryptedScratch: constraints.allowUnencryptedScratch, AllowCapabilityDropping: constraints.allowCapabilityDropping, + AllowRegistryChangesDropping: constraints.allowRegistryChangesDropping, } } @@ -2169,6 +2198,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() @@ -2184,7 +2217,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, ctx: context.Background(), } } @@ -2403,6 +2437,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) } @@ -2953,6 +3009,7 @@ type generatedConstraints struct { namespace string svn string allowCapabilityDropping bool + allowRegistryChangesDropping bool ctx context.Context } @@ -2960,6 +3017,7 @@ type generatedWindowsConstraints struct { containers []*securityPolicyWindowsContainer externalProcesses []*externalProcess fragments []*fragment + mappedDirectories []WindowsMappedDirectoryRule allowGetProperties bool allowDumpStacks bool allowRuntimeLogging bool @@ -2968,6 +3026,7 @@ type generatedWindowsConstraints struct { namespace string svn string allowCapabilityDropping bool + allowRegistryChangesDropping bool ctx context.Context } @@ -2976,12 +3035,14 @@ 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, AllowEnvironmentVariableDropping: constraints.allowEnvironmentVariableDropping, AllowUnencryptedScratch: constraints.allowUnencryptedScratch, AllowCapabilityDropping: constraints.allowCapabilityDropping, + AllowRegistryChangesDropping: constraints.allowRegistryChangesDropping, } } @@ -3026,6 +3087,7 @@ func generateWindowsConstraints(r *rand.Rand, maxContainers int32) *generatedWin allowEnvironmentVariableDropping: false, allowUnencryptedScratch: false, allowCapabilityDropping: false, + allowRegistryChangesDropping: false, namespace: generateFragmentNamespace(r), svn: generateSVN(r), ctx: context.Background(), diff --git a/pkg/securitypolicy/regopolicy_linux_test.go b/pkg/securitypolicy/regopolicy_linux_test.go index 967b2076ef..65cc8e24f5 100644 --- a/pkg/securitypolicy/regopolicy_linux_test.go +++ b/pkg/securitypolicy/regopolicy_linux_test.go @@ -82,6 +82,7 @@ func Test_MarshalRego_Policy(t *testing.T) { p.allowEnvironmentVariableDropping, p.allowUnencryptedScratch, p.allowCapabilityDropping, + p.allowRegistryChangesDropping, ) if err != nil { t.Error(err) diff --git a/pkg/securitypolicy/regopolicy_windows_test.go b/pkg/securitypolicy/regopolicy_windows_test.go index 48ce19394c..e071ec3156 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 @@ -1510,3 +2132,207 @@ func substituteUVMPath(sandboxID string, m mountInternal) mountInternal { _ = sandboxID 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) + } +} diff --git a/pkg/securitypolicy/securitypolicy.go b/pkg/securitypolicy/securitypolicy.go index ebc439ba51..8ac7301736 100644 --- a/pkg/securitypolicy/securitypolicy.go +++ b/pkg/securitypolicy/securitypolicy.go @@ -66,8 +66,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"` } func NewPolicyConfig(opts ...PolicyConfigOpt) (*PolicyConfig, error) { @@ -95,6 +96,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"` @@ -322,12 +332,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 da759a4925..1a2d7115ff 100644 --- a/pkg/securitypolicy/securitypolicy_internal.go +++ b/pkg/securitypolicy/securitypolicy_internal.go @@ -19,6 +19,7 @@ type securityPolicyInternal struct { AllowEnvironmentVariableDropping bool AllowUnencryptedScratch bool AllowCapabilityDropping bool + AllowRegistryChangesDropping bool } // Internal version of Windows SecurityPolicy @@ -26,12 +27,14 @@ type securityPolicyWindowsInternal struct { Containers []*securityPolicyWindowsContainer ExternalProcesses []*externalProcess Fragments []*fragment + MappedDirectories []WindowsMappedDirectoryRule AllowPropertiesAccess bool AllowDumpStacks bool AllowRuntimeLogging bool AllowEnvironmentVariableDropping bool AllowUnencryptedScratch bool AllowCapabilityDropping bool + AllowRegistryChangesDropping bool } type securityPolicyFragment struct { @@ -97,6 +100,7 @@ func newSecurityPolicyInternal( allowDropEnvironmentVariables bool, allowUnencryptedScratch bool, allowDropCapabilities bool, + allowRegistryChangesDropping bool, ) (*securityPolicyInternal, error) { containersInternal, err := containersToInternal(containers) if err != nil { @@ -113,6 +117,7 @@ func newSecurityPolicyInternal( AllowEnvironmentVariableDropping: allowDropEnvironmentVariables, AllowUnencryptedScratch: allowUnencryptedScratch, AllowCapabilityDropping: allowDropCapabilities, + AllowRegistryChangesDropping: allowRegistryChangesDropping, }, nil } @@ -189,6 +194,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"` @@ -228,6 +239,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 @@ -317,11 +353,18 @@ 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, WorkingDir: c.WorkingDir, + Mounts: mounts, + RegistryChanges: c.RegistryChanges.toInternal(), ExecProcesses: execProcesses, Signals: c.Signals, AllowStdioAccess: c.AllowStdioAccess, @@ -329,6 +372,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 1cdcb8177f..33d5860b5d 100644 --- a/pkg/securitypolicy/securitypolicy_marshal.go +++ b/pkg/securitypolicy/securitypolicy_marshal.go @@ -67,6 +67,7 @@ type OSAwareMarshalFunc func( allowEnvironmentVariableDropping bool, allowUnencryptedScratch bool, allowCapabilityDropping bool, + allowRegistryChangesDropping bool, ) (string, error) // osAwareMarshalRego handles both Linux and Windows containers @@ -83,6 +84,7 @@ func osAwareMarshalRego( allowEnvironmentVariableDropping bool, allowUnencryptedScratch bool, allowCapabilityDropping bool, + allowRegistryChangesDropping bool, ) (string, error) { if allowAll { if len(linuxContainers) > 0 || len(windowsContainers) > 0 { @@ -98,7 +100,8 @@ func osAwareMarshalRego( } return marshalRego(allowAll, linuxContainers, externalProcesses, fragments, allowPropertiesAccess, allowDumpStacks, allowRuntimeLogging, - allowEnvironmentVariableDropping, allowUnencryptedScratch, allowCapabilityDropping) + allowEnvironmentVariableDropping, allowUnencryptedScratch, allowCapabilityDropping, + allowRegistryChangesDropping) case "windows": if len(linuxContainers) > 0 { @@ -106,7 +109,8 @@ func osAwareMarshalRego( } return marshalWindowsRego(allowAll, windowsContainers, externalProcesses, fragments, allowPropertiesAccess, allowDumpStacks, allowRuntimeLogging, - allowEnvironmentVariableDropping, allowUnencryptedScratch, allowCapabilityDropping) + allowEnvironmentVariableDropping, allowUnencryptedScratch, allowCapabilityDropping, + allowRegistryChangesDropping) default: return "", fmt.Errorf("unsupported OS type: %s", osType) @@ -125,6 +129,7 @@ func marshalWindowsRego( allowEnvironmentVariableDropping bool, allowUnencryptedScratch bool, allowCapabilityDropping bool, + allowRegistryChangesDropping bool, ) (string, error) { if allowAll { if len(containers) > 0 { @@ -149,6 +154,7 @@ func marshalWindowsRego( AllowEnvironmentVariableDropping: allowEnvironmentVariableDropping, AllowUnencryptedScratch: allowUnencryptedScratch, AllowCapabilityDropping: allowCapabilityDropping, + AllowRegistryChangesDropping: allowRegistryChangesDropping, } return policy.marshalWindowsRego(), nil @@ -167,6 +173,7 @@ func marshalJSON( _ bool, _ bool, _ bool, + _ bool, ) (string, error) { var policy *SecurityPolicy if allowAll { @@ -198,6 +205,7 @@ func marshalRego( allowEnvironmentVariableDropping bool, allowUnencryptedScratch bool, allowCapabilityDropping bool, + allowRegistryChangesDropping bool, ) (string, error) { if allowAll { if len(containers) > 0 { @@ -217,6 +225,7 @@ func marshalRego( allowEnvironmentVariableDropping, allowUnencryptedScratch, allowCapabilityDropping, + allowRegistryChangesDropping, ) if err != nil { return "", err @@ -251,6 +260,7 @@ func MarshalPolicy( allowEnvironmentVariableDropping bool, allowUnencryptedScratch bool, allowCapbilitiesDropping bool, + allowRegistryChangesDropping bool, ) (string, error) { if marshaller == "" { marshaller = defaultMarshaller @@ -272,6 +282,7 @@ func MarshalPolicy( allowEnvironmentVariableDropping, allowUnencryptedScratch, allowCapbilitiesDropping, + allowRegistryChangesDropping, ) } } @@ -416,7 +427,17 @@ func writeCapabilities(builder *strings.Builder, capabilities *capabilitiesInter func (m mountInternal) marshalRego() string { options := stringArray(m.Options).marshalRego() - return fmt.Sprintf(`{"destination": "%s", "options": %s, "source": "%s", "type": "%s"}`, m.Destination, options, m.Source, m.Type) + return fmt.Sprintf(`{"destination": "%s", "options": %s, "source": "%s", "type": "%s"}`, + escapeRegoString(m.Destination), options, escapeRegoString(m.Source), escapeRegoString(m.Type)) +} + +// escapeRegoString escapes a Go string so it is a valid double-quoted Rego +// string literal. This matters for Windows mount paths, 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) { @@ -428,6 +449,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)) @@ -467,10 +533,14 @@ 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) - writeLine(builder, `%s"working_dir": "%s",`, indent+indentUsing, container.WorkingDir) + writeLine(builder, `%s"working_dir": "%s",`, indent+indentUsing, escapeRegoString(container.WorkingDir)) writeLine(builder, `%s"allow_stdio_access": %t,`, indent+indentUsing, container.AllowStdioAccess) writeLine(builder, "%s},", indent) } @@ -599,6 +669,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) @@ -610,6 +694,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) result := strings.Replace(policyRegoTemplate, "@@OBJECTS@@", builder.String(), 1) result = strings.Replace(result, "@@API_VERSION@@", apiVersion, 1) result = strings.Replace(result, "@@FRAMEWORK_VERSION@@", frameworkVersion, 1) @@ -629,12 +714,14 @@ 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) 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) result := strings.Replace(policyRegoTemplate, "@@OBJECTS@@", builder.String(), 1) result = strings.Replace(result, "@@API_VERSION@@", apiVersion, 1) result = strings.Replace(result, "@@FRAMEWORK_VERSION@@", frameworkVersion, 1) diff --git a/pkg/securitypolicy/securitypolicyenforcer.go b/pkg/securitypolicy/securitypolicyenforcer.go index 83e7ed4b7c..efd37ea7e0 100644 --- a/pkg/securitypolicy/securitypolicyenforcer.go +++ b/pkg/securitypolicy/securitypolicyenforcer.go @@ -161,9 +161,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) WithMetadataRollback(fn func() error) error } @@ -361,18 +364,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) WithMetadataRollback(fn func() error) error { return fn() } @@ -506,16 +521,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) WithMetadataRollback(fn func() error) error { diff --git a/pkg/securitypolicy/securitypolicyenforcer_rego.go b/pkg/securitypolicy/securitypolicyenforcer_rego.go index af9d59227c..64de9edc42 100644 --- a/pkg/securitypolicy/securitypolicyenforcer_rego.go +++ b/pkg/securitypolicy/securitypolicyenforcer_rego.go @@ -814,6 +814,7 @@ func (policy *regoEnforcer) EnforceCreateContainerPolicyV2( } input = inputData{ + "mounts": appendMountData([]interface{}{}, mounts), "containerID": containerID, "argList": argList, "envList": envList, @@ -1486,26 +1487,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{ @@ -1513,8 +1542,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) GetUserInfo(process *oci.Process, rootPath string) (IDName, []IDName, string, error) { diff --git a/test/pkg/securitypolicy/policy.go b/test/pkg/securitypolicy/policy.go index eeb93f67c8..7671ef9bea 100644 --- a/test/pkg/securitypolicy/policy.go +++ b/test/pkg/securitypolicy/policy.go @@ -64,6 +64,7 @@ func PolicyWithOpts(tb testing.TB, policyType string, pOpts ...securitypolicy.Po config.AllowEnvironmentVariableDropping, config.AllowUnencryptedScratch, config.AllowCapabilityDropping, + config.AllowRegistryChangesDropping, ) if err != nil { tb.Fatal(err)