Skip to content

CWCOW: Enforce registry entries on containers #2611

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
anmaxvl merged 3 commits into from
Mar 18, 2026
Merged

CWCOW: Enforce registry entries on containers #2611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fa1974e
CWCOW: Enforce registry entries on containers
MahatiC Feb 20, 2026
764fdd2
CWCOW: Use builtin go APIs
MahatiC Mar 11, 2026
0d85aa8
CWCOW: Rename registry test annotation
MahatiC Mar 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions internal/annotations/annotations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,3 +67,9 @@ const (
// [HCS RegistryValue]: https://learn.microsoft.com/en-us/virtualization/api/hcs/schemareference#registryvalue
AdditionalRegistryValues = "io.microsoft.virtualmachine.wcow.additional-reg-keys"
)

// WCOW container annotations.
const (
// This is for testing and debugging registry entries that can be set in the WCOW containers
WCOWRegistryAnnotationTest = "io.microsoft.container.wcow.test-registry-annotation"
)
41 changes: 41 additions & 0 deletions internal/gcs-sidecar/default_registry_values.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
//go:build windows
// +build windows

package bridge

import (
"math"
"reflect"
"slices"
"strconv"

hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2"
)

// DefaultRegistryValues contains the registry values that are always allowed
// without requiring policy validation. These are common system settings needed
// for proper UVM operation.
var defaultRegistryValues = []hcsschema.RegistryValue{
{
Key: &hcsschema.RegistryKey{
Hive: hcsschema.RegistryHive_SYSTEM,
Name: "ControlSet001\\Control",
},
Name: "WaitToKillServiceTimeout",
StringValue: strconv.Itoa(math.MaxInt32),
Type_: hcsschema.RegistryValueType_STRING,
},
}

// isDefaultRegistryValue checks if the given registry value matches one of the default allowed values
func isDefaultRegistryValue(value hcsschema.RegistryValue) bool {
return slices.ContainsFunc(defaultRegistryValues, func(rv hcsschema.RegistryValue) bool {
return registryValuesMatch(rv, value)
})
}
Comment thread
MahatiC marked this conversation as resolved.

// registryValuesMatch checks if two registry values are equivalent.
// Assumes registry values are well-formed (only relevant value fields are populated for each Type_).
func registryValuesMatch(a, b hcsschema.RegistryValue) bool {
return reflect.DeepEqual(a, b)
}
39 changes: 39 additions & 0 deletions internal/gcs-sidecar/handlers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,45 @@ func (b *Bridge) createContainer(req *request) (err error) {
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 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)
}
}
}

// 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))

nonDefaultChanges := &hcsschema.RegistryChanges{
AddValues: nonDefaultValues,
}

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")
}

log.G(ctx).Infof("Registry validation complete: %d total values (%d defaults + %d validated)",
len(container.RegistryChanges.AddValues), len(defaultValues), len(nonDefaultValues))
}

user := securitypolicy.IDName{
Name: spec.Process.User.Username,
}
Expand Down
9 changes: 9 additions & 0 deletions internal/hcsoci/hcsdoc_wcow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,15 @@ func createWindowsContainerDocument(ctx context.Context, coi *createOptionsInter
}...)
}

// Parse and add test annotation registry values if present (for testing/debugging)
testAnnotationValues := oci.ParseTestAnnotationRegistryValues(ctx, coi.Spec.Annotations)
if len(testAnnotationValues) > 0 {
log.G(ctx).WithField("count", len(testAnnotationValues)).Info("adding test annotation registry values to container")
registryAdd = append(registryAdd, testAnnotationValues...)
} else {
log.G(ctx).Debug("no test annotation registry values found in container annotations")
}

v2Container.RegistryChanges = &hcsschema.RegistryChanges{
AddValues: registryAdd,
}
Expand Down
15 changes: 13 additions & 2 deletions internal/oci/annotations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,12 +90,17 @@ func ParseAnnotationsDisableGMSA(ctx context.Context, s *specs.Spec) bool {
//
// Like the [parseAnnotation*] functions, this logs errors but does not return them.
func parseAdditionalRegistryValues(ctx context.Context, a map[string]string) []hcsschema.RegistryValue {
return parseRegistryValues(ctx, a, iannotations.AdditionalRegistryValues)
}

// parseRegistryValues is a generic function to parse registry values from annotations.
func parseRegistryValues(ctx context.Context, a map[string]string, annotationKey string) []hcsschema.RegistryValue {
// rather than have users deal with nil vs []hcsschema.RegistryValue as returns, always
// return the latter.
// this is mostly to make testing easier, since its awkward to have to differentiate between
// situations where one is returned vs the other.

k := iannotations.AdditionalRegistryValues
k := annotationKey
v := a[k]
if v == "" {
return []hcsschema.RegistryValue{}
Expand Down Expand Up @@ -204,7 +209,13 @@ func parseAdditionalRegistryValues(ctx context.Context, a map[string]string) []h
return slices.Clip(rvs)
}

// ParseHVSocketServiceTable extracts any additional Hyper-V socket service configurations from annotations.
// ParseTestAnnotationRegistryValues extracts registry values from the WCOW test annotation.
// This is for testing and debugging purposes only.
func ParseTestAnnotationRegistryValues(ctx context.Context, a map[string]string) []hcsschema.RegistryValue {
return parseRegistryValues(ctx, a, iannotations.WCOWRegistryAnnotationTest)
}

// parseHVSocketServiceTable extracts any additional Hyper-V socket service configurations from annotations.
//
// Like the [parseAnnotation*] functions, this logs errors but does not return them.
func ParseHVSocketServiceTable(ctx context.Context, a map[string]string) map[string]hcsschema.HvSocketServiceConfig {
Expand Down
3 changes: 2 additions & 1 deletion pkg/securitypolicy/api.rego
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ version := "@@API_VERSION@@"
enforcement_points := {
"mount_device": {"introducedVersion": "0.1.0", "default_results": {"allowed": false}},
"mount_overlay": {"introducedVersion": "0.1.0", "default_results": {"allowed": false}},
"mount_cims": {"introducedVersion": "0.11.0", "default_results": {"allowed": false}},
"mount_cims": {"introducedVersion": "0.11.0", "default_results": {"allowed": false}},
"registry_changes": {"introducedVersion": "0.10.0", "default_results": {"allowed": false}},
"create_container": {"introducedVersion": "0.1.0", "default_results": {"allowed": false, "env_list": null, "allow_stdio_access": false}},
"unmount_device": {"introducedVersion": "0.2.0", "default_results": {"allowed": true}},
"unmount_overlay": {"introducedVersion": "0.6.0", "default_results": {"allowed": true}},
Expand Down
110 changes: 110 additions & 0 deletions pkg/securitypolicy/framework.rego
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1231,6 +1231,116 @@ scratch_unmount := {"metadata": [remove_scratch_mount], "allowed": true} {
}
}

# Registry changes validation
default registry_changes := {"allowed": false}

# Helper function to compare registry keys
registry_keys_match(policy_key, input_key) {
policy_key.hive == input_key.Hive
policy_key.name == input_key.Name
# Volatile field comparison (default to false if not specified)
policy_volatile := object.get(policy_key, "volatile", false)
input_volatile := object.get(input_key, "Volatile", false)
policy_volatile == input_volatile
}

# Helper function to compare registry values
# STRING type
registry_value_matches(policy_value, input_value) {
registry_keys_match(policy_value.key, input_value.Key)
policy_value.name == input_value.Name
policy_value.type == input_value.Type
policy_value.type == "String"
policy_value.string_value == input_value.StringValue
}

# EXPANDED_STRING type (uses StringValue field)
registry_value_matches(policy_value, input_value) {
registry_keys_match(policy_value.key, input_value.Key)
policy_value.name == input_value.Name
policy_value.type == input_value.Type
policy_value.type == "ExpandedString"
policy_value.string_value == input_value.StringValue
}

# MULTI_STRING type (uses StringValue field)
registry_value_matches(policy_value, input_value) {
registry_keys_match(policy_value.key, input_value.Key)
policy_value.name == input_value.Name
policy_value.type == input_value.Type
policy_value.type == "MultiString"
policy_value.string_value == input_value.StringValue
}

# D_WORD type
registry_value_matches(policy_value, input_value) {
registry_keys_match(policy_value.key, input_value.Key)
policy_value.name == input_value.Name
policy_value.type == input_value.Type
policy_value.type == "DWord"
policy_value.dword_value == input_value.DWordValue
}

# Q_WORD type
registry_value_matches(policy_value, input_value) {
registry_keys_match(policy_value.key, input_value.Key)
policy_value.name == input_value.Name
policy_value.type == input_value.Type
policy_value.type == "QWord"
policy_value.qword_value == input_value.QWordValue
}

# BINARY type
registry_value_matches(policy_value, input_value) {
registry_keys_match(policy_value.key, input_value.Key)
policy_value.name == input_value.Name
policy_value.type == input_value.Type
policy_value.type == "Binary"
policy_value.binary_value == input_value.BinaryValue
}

# CUSTOM_TYPE - both CustomType field and BinaryValue must match
registry_value_matches(policy_value, input_value) {
registry_keys_match(policy_value.key, input_value.Key)
policy_value.name == input_value.Name
policy_value.type == input_value.Type
policy_value.type == "CustomType"
policy_value.custom_type == input_value.CustomType
policy_value.binary_value == input_value.BinaryValue
}

# NONE type - no value to compare, just key and name
registry_value_matches(policy_value, input_value) {
registry_keys_match(policy_value.key, input_value.Key)
policy_value.name == input_value.Name
policy_value.type == input_value.Type
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)
]

registry_changes := {"allowed": true} {
containers := data.metadata.matches[input.containerID]
container := containers[_]

# Check if container has registry_changes defined in policy
container.registry_changes

# 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)

# Build result with filtered AddValues
result := {
"AddValues": matched_values
}
}
Comment on lines +1327 to +1342

@micromaomao micromaomao Mar 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for resurfacing this merged PR, but I have some worries about this bit.

First, I don't think matched_values is actually used in gcs-sidecar/handlers.go (or even passed out from framework.rego? It's not in the := {"allowed": true} bit), and instead, it seems to me that the container creation request is allowed to continue with the original RegistryChanges even if we have filtered out some values. We're fortunately saved by the container.registry_changes bit - if the container policy contains no entries in registry_changes (or if that field isn't there in the first place), the registry_change enforcement will be denied. But still, the result of this is that if the container policy allows some registry changes, then any host-provided key/value would pass enforcement and be applied.

Also, since we bail out if container.registry_changes doesn't exist or is empty (in my testing this seems to be the effect), this creates a slight inconsistency, imo. If the container allows some registry change, but the host passes some different changes, the creation request will still succeed, except (assuming the above bug is fixed) that the key won't be set. However, the same container create request would be denied altogether if container.registry_changes is empty in the policy (rather than filtering out the provided key).

My second concern is whether this check should in fact be part of the create_container enforcement point, rather than its own. It is called before create_container, which means that by this point we have not narrowed down to the exact container definition yet (but we have narrowed down the layers), and in a situation where two containers are using the same image but with different command line, we would allow a registry change intended for one to be set on the other.

I'm not sure how important the second concern is, because registry changes are not part of the OCI spec and so the customer does not have any way to use this feature anyway, but I do feel like the cleanest solution should be checking it as part of create_container. However, the first bug would result in a policy that looks like it only allows a "harmless" registry change to effectively allow any registry change, and so I think need to be fixed.

@micromaomao micromaomao Mar 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this new field should go into check_container. We probably should split check_container into LCOW and WCOW, so that it does not require LCOW specific fields to be in the container object (currently it effectively breaks if the LCOW fields are missing, and gcs is on a newer framework_version than the policy)

(also without adding this to check_containers, as soon as gcs upgrades to a newer framework_version than the policy, any registry_changes field on the container policy would be ignored)

@micromaomao micromaomao Mar 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also create a denial reason for this (and also mount_cims), e.g.

errors["registry changes denied"] {
    input.rule == "registry_changes"

    data.metadata.matches[input.containerID]
}

errors["no matching containers"] {
    input.rule == "registry_changes"

    not data.metadata.matches[input.containerID]
}

(this will be different (simpler) if we move this into create_container)

For mount_cims maybe

errors["No matching containers for CIM layer hashes"] {
    not overlay_exists

    containers := [container |
        container := candidate_containers[_]
        layerHashes_ok(container.layers)
    ]

    count(containers) == 0
}

@MahatiC MahatiC Mar 20, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My second concern is whether this check should in fact be part of the create_container enforcement point, rather than its own. It is called before create_container, which means that by this point we have not narrowed down to the exact container definition yet (but we have narrowed down the layers), and in a situation where two containers are using the same image but with different command line, we would allow a registry change intended for one to be set on the other.

I'm not sure how important the second concern is, because registry changes are not part of the OCI spec and so the customer does not have any way to use this feature anyway, but I do feel like the cleanest solution should be checking it as part of create_container. However, the first bug would result in a policy that looks like it only allows a "harmless" registry change to effectively allow any registry change, and so I think need to be fixed.```

registry changes are not used for container narrowing intentionally. We must allow certain registry entries from the host (like servercore at this point), so it is not used as part of container narrowing. I'll chat offline instead of a wall of text here :)


reason := {
"errors": errors,
"error_objects": error_objects
Expand Down
1 change: 1 addition & 0 deletions pkg/securitypolicy/open_door.rego
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ mount_device := {"allowed": true}
mount_overlay := {"allowed": true}
create_container := {"allowed": true, "env_list": null, "allow_stdio_access": true}
mount_cims := {"allowed": true}
registry_changes := {"allowed": true}
unmount_device := {"allowed": true}
unmount_overlay := {"allowed": true}
exec_in_container := {"allowed": true, "env_list": null}
Expand Down
1 change: 1 addition & 0 deletions pkg/securitypolicy/policy.rego
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ unmount_device := data.framework.unmount_device
mount_overlay := data.framework.mount_overlay
unmount_overlay := data.framework.unmount_overlay
mount_cims:= data.framework.mount_cims
registry_changes := data.framework.registry_changes
create_container := data.framework.create_container
exec_in_container := data.framework.exec_in_container
exec_external := data.framework.exec_external
Expand Down
Loading
Loading